On 9/5/21 6:23 PM, 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: >> 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. Hold on, so this means that if that iommu device-link series is merged, then the mtk-mdp driver breaks? I posted a PR for that iommu series, but I've just withdrawn that PR until this issue is clarified. Is it only mtk-mdp that is affected by this iommu device-link series, or others as well? Regards, Hans > >> >>> 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> >>> --- >>> >
