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: > Hi Eizan, > > Sorry for seeing this series so late. > > On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@xxxxxxxxxxxx> > wrote: > > > > 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 , 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. > > > 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 > 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 > > > - 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 <eizan@xxxxxxxxxxxx> > > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> > > Reviewed-by: Houlong Wei <houlong.wei@xxxxxxxxxxxx> > > --- > >
