Hi Enric, My answers are as follows.Thanks for deep thinking about mtk-mdp. Regards, Houlong On Thu, 2021-08-26 at 00:54 +0800, Enric Balletbo i Serra wrote: > Hi Houlong, > > Thank you for your answer, some more questions below :-) > > On 25/8/21 17:18, houlong wei wrote: > > On Wed, 2021-08-25 at 17:07 +0800, Enric Balletbo i Serra wrote: > >> Hi Houlong, > >> > >> Thank you for following up this patchset. I have some questions to try to > >> understand better the hardware though. > >> > >> On 25/8/21 10:43, houlong wei wrote: > >>> Hi Eizan, > >>> > >>> Thanks for you patch. I have inline comment below. > >>> > >>> Regards, > >>> Houlong > >>> > >>> On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote: > >>>> ... Instead of depending on the presence of a mediatek,vpu property in > >>>> the device node. > >>>> > >>>> That property was originally added to link to the vpu node so that the > >>>> mtk_mdp_core driver could pass the right device to > >>>> vpu_wdt_reg_handler(). However in a previous patch in this series, > >>>> the driver has been modified to search the device tree for that node > >>>> instead. > >>>> > >>>> That property was also used to indicate the primary MDP device, so that > >>>> it can be passed to the V4L2 subsystem as well as register it to be > >>>> used when setting up queues in the open() callback for the filesystem > >>>> device node that is created. In this case, assuming that the primary > >>>> MDP device is the one with a specific alias seems useable because the > >>>> alternative is to add a property to the device tree which doesn't > >>>> actually represent any facet of hardware (i.e., this being the primary > >>>> MDP device is a software decision). In other words, this solution is > >>>> equally as arbitrary, but at least it doesn't add a property to a > >>>> device node where said property is unrelated to the hardware present. > >>>> > >>>> Signed-off-by: Eizan Miyamoto <eizan@xxxxxxxxxxxx> > >>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> > >>>> --- > >>>> > >>>> (no changes since v1) > >>>> > >>>> drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------ > >>>> drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++---- > >>>> 2 files changed, 64 insertions(+), 28 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > >>>> index 85ef274841a3..9527649de98e 100644 > >>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > >>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > >>>> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp) > >>>> mtk_smi_larb_put(comp->larb_dev); > >>>> } > >>>> > >>>> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data) > >>>> +/* > >>>> + * The MDP master device node is identified by the device tree alias > >>>> + * "mdp-rdma0". > >>>> + */ > >>>> +static bool is_mdp_master(struct device *dev) > >>>> +{ > >>>> + struct device_node *aliases, *mdp_rdma0_node; > >>>> + const char *mdp_rdma0_path; > >>>> + > >>>> + if (!dev->of_node) > >>>> + return false; > >>>> + > >>>> + aliases = of_find_node_by_path("/aliases"); > >>>> + if (!aliases) { > >>>> + dev_err(dev, "no aliases found for mdp-rdma0"); > >>>> + return false; > >>>> + } > >>>> + > >>>> + mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL); > >>>> + if (!mdp_rdma0_path) { > >>>> + dev_err(dev, "get mdp-rdma0 property of /aliases failed"); > >>>> + return false; > >>>> + } > >>>> + > >>>> + mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path); > >>>> + if (!mdp_rdma0_node) { > >>>> + dev_err(dev, "path resolution failed for %s", mdp_rdma0_path); > >>>> + return false; > >>>> + } > >>>> + > >>>> + return dev->of_node == mdp_rdma0_node; > >>> > >>> > >>> About how to determine the master mdp driver, we also can > >>> judge the component type. The component type can be gotten by calling > >>> of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is > >>> the master driver. No matter it is mdp_rdma0 or mdp_rdma1. > >> > >> > >> I'm confused, you mean that the component type, aka MTK_MDP_RDMA is only set for > >> mtk_mdp_rdma0? isn't mtk_dmp_rdma1 also a MTK_MDP_RDMA type? Because looks weird > >> to me that two rdma have diffent types. > >> > > Hi Enric, > > > > I can understand your doubts. For mtk-mdp dirver, it is really difficult > > to understand the relationship among the components. Because the MDP > > hardware path is configured and connected in the VPU firmware. > > So is fixed by the VPU firmware and can't be changed via, i.e writing some > registers? Can't be platform data then? > > If that's the case, can you draw the mdp hardware path for i.e MT8173? (so I > have a better understanding) > I reviewed the mt8173 VPU firmware code and found it only use 1 MDP path.Perhaps one MDP path could meet the project team's requirement when the VPU firmware was implemented. A MDP path is a data path which consists of several MDP hardware engines, e.g. mdp_rdma1->mdp_rsz1->mdp_wrot1, mdp_rdma2->mdp_rsz2->mdp_wrot2. Ideally,if there are multiple MDP paths, the users(e.g. applications) can use them concurrently. > > > For MT8173, the component type of mtk_mdp_rdma0 and mtk_mdp_rdma1 are > > both MTK_MDP_RDMA. Even though the mtk-mdp driver's component list > > contains both of them, only one corresponding hardware component takes > > effect during the processing of a frame, the VPU decides it. > > > > So IIUC the VPU firmware decides which rdma uses on every frame, right? From hw > point of view, why one of them need to be master? > For mt8173, the VPU firmware only use one rdma because another MDP path is not implemented.If we implement multiple MDP paths in certain project, each rdma can be master. > > Regards, > > Houlong > >> > >>> http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html > >>> > >>> IMO, judging it by component type is more flexible because it does not > >>> limit to 'mdp_rdma0'. > >>> > >> > >> Using an alias like Eizan did is also flexible, you only need to set mdp-rdma0 > >> to point the mdp_rdma1 node. > >> > >> What I am really interested to know is the differences between the platforms to > >> understand if this is really a platform hardware property or a software > >> configurable thing, so would help if you can give use the different use cases > >> for different SoCs. > >> > >> For MT8173 which is the master device? is _always_ rdma0 or can also be rdma1? > >> > >> Or maybe what is not clear here is what exactly means be a master device? > >> > >> What about MT8183 and other SoCs? > >> > >> Thanks, > >> Enric > >> > > Hi Enric, > > > > Before answer your questions, let's unify the concept of the master > > device.If an MDP device-tree node can not only generate a /dev/video > > device node, but also can generate a mtk component device node, then > > this component device is the master device. > > Sorry, what you mean with a mtk component device node? My meaning is that the device-tree node can generate a platform device, it is what the mtk_mdp_comp.c does. > > > From the perspective of the device tree node, the compatible of a MDP > > device tree node contains both "mediatek,mt8173-mdp-rdma" and > > "mediatek,mt8173-mdp", then its corresponding mtk component device is > > the master device. > > So that's what's wrong, and Eizan patches get rid of this. In this case you > can't use device-tree to specify such master device. Device-tree is only about > hardware description, can't be used, in this case, as entry point to instantiate > the mdp driver. Hence Eizan patches change this, and mdp is instantiated by the > mmsys driver. Similar to what we do for the drm driver. > > For the two hw rdma blocks the compatible should be just > "mediatek,mt8173-mdp-rdma" in this case. > > Thanks, > Enric My consideration is how the driver supports it when there are multiple MDP paths. Assume there are 2 MDP paths, as a V4L2 driver, mtk-mdp can generates two device nodes, e.g. /dev/video0 supports one of MDP path, /dev/video1 supports another MDP path. > > Since this concept comes from Eizan's patches, if my understanding is > > different from yours, please let me know, thanks. > > > > If there is only one master device, I fully agree with you. For MT8173, > > both mdp_rdma0 and mdp_rdma1 can be master device at the same time. > > But now only mdp_rdma0 takes this role, perhaps because one MDP hardware > > path could meet the project's the requirement six years ago. > > In some project, we have two or more MDP hardware paths, we can > > configure all the mdp_rdma components as master devices, and there will > > be several /dev/video device nodes. The user can control them > > concurrently. > > > > Regards, > > Houlong > > > >> > >>>> +} > >>>> + > >>>> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master, > >>>> + void *data) > >>>> { > >>>> struct mtk_mdp_comp *comp = dev_get_drvdata(dev); > >>>> struct mtk_mdp_dev *mdp = data; > >>>> - struct device_node *vpu_node; > >>>> > >>>> mtk_mdp_register_component(mdp, comp); > >>>> > >>>> - /* > >>>> - * If this component has a "mediatek-vpu" property, it is responsible for > >>>> - * notifying the mdp master driver about it so it can be further initialized > >>>> - * later. > >>>> - */ > >>>> - vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0); > >>>> - if (vpu_node) { > >>>> + if (is_mdp_master(dev)) { > >>>> int ret; > >>>> > >>>> - mdp->vpu_dev = of_find_device_by_node(vpu_node); > >>>> - if (WARN_ON(!mdp->vpu_dev)) { > >>>> - dev_err(dev, "vpu pdev failed\n"); > >>>> - of_node_put(vpu_node); > >>>> - } > >>>> - > >>>> ret = v4l2_device_register(dev, &mdp->v4l2_dev); > >>>> if (ret) { > >>>> dev_err(dev, "Failed to register v4l2 device\n"); > >>>> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da > >>>> } > >>>> > >>>> /* > >>>> - * presence of the "mediatek,vpu" property in a device node > >>>> - * indicates that it is the primary MDP rdma device and MDP DMA > >>>> - * ops should be handled by its DMA callbacks. > >>>> + * MDP DMA ops will be handled by the DMA callbacks associated with this > >>>> + * device; > >>>> */ > >>>> mdp->rdma_dev = dev; > >>>> } > >>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > >>>> index 50eafcc9993d..6a775463399c 100644 > >>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > >>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > >>>> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data) > >>>> > >>>> static int mtk_mdp_master_bind(struct device *dev) > >>>> { > >>>> - int status; > >>>> struct mtk_mdp_dev *mdp = dev_get_drvdata(dev); > >>>> + struct device_node *vpu_node; > >>>> + int status; > >>>> > >>>> status = component_bind_all(dev, mdp); > >>>> if (status) { > >>>> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev) > >>>> goto err_component_bind_all; > >>>> } > >>>> > >>>> - if (mdp->vpu_dev) { > >>>> - int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, > >>>> - VPU_RST_MDP); > >>>> - if (ret) { > >>>> - dev_err(dev, "Failed to register reset handler\n"); > >>>> - goto err_wdt_reg; > >>>> - } > >>>> - } else { > >>>> - dev_err(dev, "no vpu_dev found\n"); > >>>> + if (mdp->rdma_dev == NULL) { > >>>> + dev_err(dev, "Primary MDP device not found"); > >>>> + status = -ENODEV; > >>>> + goto err_component_bind_all; > >>>> + } > >>>> + > >>>> + vpu_node = of_find_node_by_name(NULL, "vpu"); > >>>> + if (!vpu_node) { > >>>> + dev_err(dev, "unable to find vpu node"); > >>>> + status = -ENODEV; > >>>> + goto err_wdt_reg; > >>>> + } > >>>> + > >>>> + mdp->vpu_dev = of_find_device_by_node(vpu_node); > >>> > >>> The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it > >>> is not used. > >>> > >>>> + if (!mdp->vpu_dev) { > >>>> + dev_err(dev, "unable to find vpu device"); > >>>> + status = -ENODEV; > >>>> + goto err_wdt_reg; > >>>> + } > >>>> + > >>>> + status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP); > >>>> + if (status) { > >>>> + dev_err(dev, "Failed to register reset handler\n"); > >>>> + goto err_wdt_reg; > >>>> } > >>>> > >>>> status = mtk_mdp_register_m2m_device(mdp); > >>> > >