| Message ID | 20210802220943.v6.7.I2049e180dca12e0d1b3178bfc7292dcf9e05ac28@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor MTK MDP driver into core/components | expand |
Hi Eizan, Thank you for your patch. Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag. 2021 a les 14:14: > > ... Instead of depending on the presence of a mediatek,vpu property in Looks like there is something missing in the commit message? > 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> Other than the above, 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; > +} > + > +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); > -- > 2.32.0.554.ge1b32706d8-goog >
Hi Enric, thanks for your comment. > > ... Instead of depending on the presence of a mediatek,vpu property in > > Looks like there is something missing in the commit message? That line is a continuation of the commit message header, I.e., it's meant to read: "use mdp-rdma0 alias to point to MDP master Instead of depending on the presence of a mediatek,vpu property in the device node" Eizan
On Mon, 2021-08-02 at 20:12 +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> > --- > > (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; > + } > + Hi Eizan, "mdp-rdma0" may be not the only one master device node. In fact, there are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016 If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will have one more V4L2 video devie node. mdp_rdma1: rdma@14002000 { compatible = "mediatek,mt8173-mdp-rdma", "mediatek,mt8173-mdp"; ... } We should consider the case that there are more than one "MDP_RDMA" components. Regards, Houlong > + 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); > -- > 2.32.0.554.ge1b32706d8-goog >
On Mon, 2021-08-16 at 11:00 +0800, houlong wei wrote: > On Mon, 2021-08-02 at 20:12 +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> > > --- > > > > (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; > > + } > > + > > Hi Eizan, > > "mdp-rdma0" may be not the only one master device node. In fact, > there > are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link. > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016 > If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will > have > one more V4L2 video devie node. > > mdp_rdma1: rdma@14002000 { > compatible = "mediatek,mt8173-mdp-rdma", > "mediatek,mt8173-mdp"; > ... > } > > We should consider the case that there are more than one "MDP_RDMA" > components. > > Regards, > Houlong > > > + 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; > > + } Hi Eizan, Is your removing "mediatek,vpu" necessary for this series "Refactor MTK MDP driver into core/components" ? In some MediaTek projects (not upstream yet), the device-tree node name "vpu" may be changed to something like "vpu0", "vpu1" or other name, which depends on the implementation of "mtk-vpu" driver. We can specify the different phandle to "mediatek,vpu" in .dtsi file. If we use of_find_node_by_name() to get the vpu_node, we have to modify the name string in different project. If the answer of my previous questions is "No", I prefer to slow down the modification of removing "mediatek,vpu". Sorry for late reply. Regards, Houlong > > + > > + 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); > > -- > > 2.32.0.554.ge1b32706d8-goog > > > >
Hi Houlong, On Mon, Aug 16, 2021 at 1:00 PM houlong wei <houlong.wei@mediatek.com> wrote: > Hi Eizan, > > "mdp-rdma0" may be not the only one master device node. In fact, there > are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link. > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016 > If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will have > one more V4L2 video devie node. > > mdp_rdma1: rdma@14002000 { > compatible = "mediatek,mt8173-mdp-rdma", > "mediatek,mt8173-mdp"; > ... > } > > We should consider the case that there are more than one "MDP_RDMA" > components. Would it be okay with you if we added support for multiple MDP master device nodes in follow-up changes? My rationale is this: - As far as I can tell, the mediatek integration with V4L2 currently only handles a single MDP master device node. It's not clear to me the scope of changes that will be needed to make things work properly with multiple nodes. - The patch series makes video decode work (admittedly, in light of your comments not optimally) upstream, which is better than not landing these changes at all. I'd like to say that I'm very open to (and excited about) discussing further work to support multiple MDP master nodes, perhaps we can work together on this so I can understand what needs to be done. Please let me know your thoughts, Eizan
Hi Houlong, Thank you for your review on this series, it is much appreciated. On Mon, Aug 16, 2021 at 2:53 PM houlong wei <houlong.wei@mediatek.com> wrote: > Is your removing "mediatek,vpu" necessary for this series "Refactor MTK > MDP driver into core/components" ? Removing it is not strictly necessary for the series to function, I will re-upload the series and omit the following changes: - [PATCH v6 8/9] dts: mtk-mdp: remove mediatek,vpu property from primary MDP device - [PATCH v6 9/9] dt-bindings: mediatek: remove vpu requirement from mtk-mdp Do please let me know if you meant something different. Thanks, Eizan > > In some MediaTek projects (not upstream yet), the device-tree node name > "vpu" may be changed to something like "vpu0", "vpu1" or other name, > which depends on the implementation of "mtk-vpu" driver. We can specify > the different phandle to "mediatek,vpu" in .dtsi file. If we use > of_find_node_by_name() to get the vpu_node, we have to modify the name > string in different project. > If the answer of my previous questions is "No", I prefer to slow down > the modification of removing "mediatek,vpu". > > Sorry for late reply. > > Regards, > Houlong
Hi Eizan, Firstly, 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. Secondly, about supporting the multiple MDP master device nodes, you can try my advice in my previous comment after the completion of this series of patches. Thanks a lot. Regards, Houlong On Wed, 2021-08-18 at 15:43 +0800, Eizan Miyamoto wrote: > Hi Houlong, > > On Mon, Aug 16, 2021 at 1:00 PM houlong wei <houlong.wei@mediatek.com > > wrote: > > Hi Eizan, > > > > "mdp-rdma0" may be not the only one master device node. In fact, > > there > > are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016 > > If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will > > have > > one more V4L2 video devie node. > > > > mdp_rdma1: rdma@14002000 { > > compatible = "mediatek,mt8173-mdp-rdma", > > "mediatek,mt8173-mdp"; > > ... > > } > > > > We should consider the case that there are more than one "MDP_RDMA" > > components. > > Would it be okay with you if we added support for multiple MDP master > device nodes in follow-up changes? My rationale is this: > - As far as I can tell, the mediatek integration with V4L2 currently > only handles a single MDP master device node. It's not clear to me > the > scope of changes that will be needed to make things work properly > with > multiple nodes. > - The patch series makes video decode work (admittedly, in light of > your comments not optimally) upstream, which is better than not > landing these changes at all. > > I'd like to say that I'm very open to (and excited about) discussing > further work to support multiple MDP master nodes, perhaps we can > work together on this so I can understand what needs to be done. > > Please let me know your thoughts, > > Eizan
On Wed, 2021-08-18 at 15:50 +0800, Eizan Miyamoto wrote: > Hi Houlong, > > Thank you for your review on this series, it is much appreciated. > > On Mon, Aug 16, 2021 at 2:53 PM houlong wei <houlong.wei@mediatek.com > > wrote: > > Is your removing "mediatek,vpu" necessary for this series "Refactor > > MTK > > MDP driver into core/components" ? > > Removing it is not strictly necessary for the series to function, I > will re-upload the series and omit the following changes: > - [PATCH v6 8/9] dts: mtk-mdp: remove mediatek,vpu property from > primary MDP device > - [PATCH v6 9/9] dt-bindings: mediatek: remove vpu requirement from > mtk-mdp > > Do please let me know if you meant something different. > > Thanks, > > Eizan > > Hi Eizan, Thanks for your help and effort. It's exactly what I expressed. Regards, Houlong > > > > > > In some MediaTek projects (not upstream yet), the device-tree node > > name > > "vpu" may be changed to something like "vpu0", "vpu1" or other > > name, > > which depends on the implementation of "mtk-vpu" driver. We can > > specify > > the different phandle to "mediatek,vpu" in .dtsi file. If we use > > of_find_node_by_name() to get the vpu_node, we have to modify the > > name > > string in different project. > > If the answer of my previous questions is "No", I prefer to slow > > down > > the modification of removing "mediatek,vpu". > > > > Sorry for late reply. > > > > Regards, > > Houlong
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);
... 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> --- (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(-)