LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
To: houlong wei <houlong.wei@mediatek.com>
Cc: "Eizan Miyamoto" <eizan@chromium.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"chunkuang.hu@kernel.org" <chunkuang.hu@kernel.org>,
	"Yong Wu (吴勇)" <Yong.Wu@mediatek.com>,
	"wenst@chromium.org" <wenst@chromium.org>,
	"CK Hu (胡俊光)" <ck.hu@mediatek.com>,
	"Yongqiang Niu (牛永强)" <yongqiang.niu@mediatek.com>,
	"Andrew-CT Chen (陳智迪)" <Andrew-CT.Chen@mediatek.com>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Minghsiu Tsai (蔡明修)" <Minghsiu.Tsai@mediatek.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
Date: Wed, 25 Aug 2021 18:54:54 +0200	[thread overview]
Message-ID: <e9020b68-5d20-8e4f-4883-273fe4697d60@collabora.com> (raw)
In-Reply-To: <1629904697.15752.11.camel@mhfsdcap03>

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)


> 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?

> 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?

> 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

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

  reply	other threads:[~2021-08-25 16:55 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  6:33 [PATCH v7 0/7] Refactor MTK MDP driver into core/components Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 1/7] mtk-mdp: propagate errors from clock_on Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components Eizan Miyamoto
2021-09-04 12:34   ` Ezequiel Garcia
2021-09-05 16:23     ` houlong wei
2021-09-05 16:39       ` houlong wei
2021-12-07 12:37       ` Hans Verkuil
2021-12-07 12:46         ` Ezequiel Garcia
2021-12-07 13:02           ` Hans Verkuil
2021-12-08  2:00       ` Yong Wu
2021-08-25  6:33 ` [PATCH v7 3/7] mtk-mdp: use pm_runtime in MDP component driver Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 4/7] media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 5/7] mtk-mdp: make mdp driver to be loadable by platform_device_register*() Eizan Miyamoto
2021-08-25  8:18   ` houlong wei
2021-08-25  6:33 ` [PATCH v7 6/7] soc: mediatek: mmsys: instantiate mdp virtual device from mmsys Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master Eizan Miyamoto
2021-08-25  8:43   ` houlong wei
2021-08-25  9:07     ` Enric Balletbo i Serra
2021-08-25 15:18       ` houlong wei
2021-08-25 16:54         ` Enric Balletbo i Serra [this message]
2021-08-30 17:14           ` houlong wei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e9020b68-5d20-8e4f-4883-273fe4697d60@collabora.com \
    --to=enric.balletbo@collabora.com \
    --cc=Andrew-CT.Chen@mediatek.com \
    --cc=Minghsiu.Tsai@mediatek.com \
    --cc=Yong.Wu@mediatek.com \
    --cc=chunkuang.hu@kernel.org \
    --cc=ck.hu@mediatek.com \
    --cc=eizan@chromium.org \
    --cc=houlong.wei@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=wenst@chromium.org \
    --cc=yongqiang.niu@mediatek.com \
    --subject='Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).