Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

From: houlong wei
Date: Mon Aug 30 2021 - 13:14:54 EST

Next message: Jakub Kicinski: "Re: [PATCH V2] fix array-index-out-of-bounds in taprio_change"
Previous message: Hannes Reinecke: "Re: [PATCH v1] nvme: avoid race in shutdown namespace removal"
In reply to: Enric Balletbo i Serra: "Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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);
> >>>
> >

Next message: Jakub Kicinski: "Re: [PATCH V2] fix array-index-out-of-bounds in taprio_change"
Previous message: Hannes Reinecke: "Re: [PATCH v1] nvme: avoid race in shutdown namespace removal"
In reply to: Enric Balletbo i Serra: "Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]