Message-ID: <1630343686.7403.44.camel@mhfsdcap03>
Date:   Tue, 31 Aug 2021 01:14:46 +0800
From:   houlong wei <houlong.wei@...iatek.com>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
CC:     Eizan Miyamoto <eizan@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
        Yong Wu (吴勇) 
        <Yong.Wu@...iatek.com>, "wenst@...omium.org" <wenst@...omium.org>,
        "CK Hu (胡俊光)" 
        <ck.hu@...iatek.com>,
        "Yongqiang Niu (牛永强)" 
        <yongqiang.niu@...iatek.com>,
        "Andrew-CT Chen (陳智迪)" 
        <Andrew-CT.Chen@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Minghsiu Tsai (蔡明修)" 
        <Minghsiu.Tsai@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>, <houlong.wei@...iatek.com>
Subject: Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to
 MDP master

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@...omium.org>
> >>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@...labora.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);
> >>>
> > 

Powered by blists - more mailing lists