Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

Sorry, I fix a link error below.

On Mon, 2021-09-06 at 00:23 +0800, houlong wei wrote:

Hi Ezequiel,

Thank you for your attention to this series of patches. I answer
partial of your questions below.
Regards,
Houlong

On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:

quoted

Hi Eizan,

Sorry for seeing this series so late.

On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto [off-list ref]
wrote:

quoted

Broadly, this patch (1) adds a driver for various MTK MDP
components to
go alongside the main MTK MDP driver, and (2) hooks them all
together
using the component framework.

(1) Up until now, the MTK MDP driver controls 8 devices in the
device
tree on its own. When running tests for the hardware video
decoder,
we
found that the iommus and LARBs were not being properly
configured.

Why were not being properly configured? What was the problem?
Why not fixing that instead?

Does this mean the driver is currently broken and unusable?

This series of patches are supplements to another series, please
refer
to  

https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c

Fix the link address: 
https://patchwork.kernel.org/project/linux-mediatek/list/?series=523771

, which add device link between the mtk-iommu consumer and the mtk-
larb 
devices. Without that series of patches, the mtk-mdp driver can work
well so far.
But with that series, it seems the device link only can be
established
for the device which is registered as a platform driver. That's why
Eizan adds this series of patches to make all mdp components to be
registered as platform drivers.

quoted

quoted

To
configure them, a driver for each be added to mtk_mdp_comp so
that
mtk_iommu_add_device() can (eventually) be called from
dma_configure()
inside really_probe().

(2) The integration into the component framework allows us to
defer
the
registration with the v4l2 subsystem until all the MDP-related
devices
have been probed, so that the relevant device node does not
become
available until initialization of all the components is complete.

Some notes about how the component framework has been integrated:

- The driver for the rdma0 component serves double duty as the
"master"
  (aggregate) driver as well as a component driver. This is a
non-
ideal
  compromise until a better solution is developed. This device is
  differentiated from the rest by checking for a "mediatek,vpu"
property
  in the device node.

As I have stated in Yunfei, I am not convinced you need an async
framework
at all. It seems all these devices could have been linked together
in the device tree, and then have a master device to tie them.

I.e. something like

mdp {
  mdp_rdma0 {
  }
  mdp_rsz0 {
  }
  mdp_rsz1 {
  }
}

The commit message of the patch below explains that " If the mdp_*
nodes are under an mdp sub-node, their corresponding platform device
does not automatically get its iommu assigned properly."
Please refer to 

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.14.1&id=8127881f741dbbf9a1da9e9bc59133820160b217

quoted

All this async games seem like making the driver really obfuscated,
which will mean harder to debug and maintain.
I am not sure we want that burden.

Even if we are all fully convinced that you absolutely need
an async framework, then what's wrong with v4l2-async?

I would start by addressing what is wrong with the IOMMUs
in the current design.

Thanks,
Ezequiel

quoted

- The list of mdp components remains hard-coded as
mtk_mdp_comp_dt_ids[]
  in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
  mtk_mdp_comp.c. This unfortunate duplication of information is
  addressed in a following patch in this series.

- The component driver calls component_add() for each device that
is
  probed.

- In mtk_mdp_probe (the "master" device), we scan the device tree
for
  any matching nodes against mtk_mdp_comp_dt_ids, and add
component
  matches for them. The match criteria is a matching device node
  pointer.

- When the set of components devices that have been probed
corresponds
  with the list that is generated by the "master", the callback
to
  mtk_mdp_master_bind() is made, which then calls the component
bind
  functions.

- Inside mtk_mdp_master_bind(), once all the component bind
functions
  have been called, we can then register our device to the v4l2
  subsystem.

- The call to pm_runtime_enable() in the master device is called
after
  all the components have been registered by their bind()
functions
  called by mtk_mtp_master_bind(). As a result, the list of
components
  will not change while power management callbacks
mtk_mdp_suspend()/
  resume() are accessing the list of components.

Signed-off-by: Eizan Miyamoto <redacted>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com

quoted

Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel