| Message ID | 20210825163247.v7.7.I2049e180dca12e0d1b3178bfc7292dcf9e05ac28@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor MTK MDP driver into core/components | expand |
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@chromium.org> > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > (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. 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'. > +} > + > +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);
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@chromium.org> >> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> >> (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. > 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 >> +} >> + >> +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); >
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@chromium.org> > >>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >>>> --- > >>>> > >>>> (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); > >>> > >
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; +} + +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); + 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);