diff mbox series

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

Message ID 20210825163247.v7.7.I2049e180dca12e0d1b3178bfc7292dcf9e05ac28@changeid
State New
Headers show
Series Refactor MTK MDP driver into core/components | expand

Commit Message

Eizan Miyamoto Aug. 25, 2021, 6:33 a.m. UTC
... 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(-)

Comments

houlong wei Aug. 25, 2021, 8:43 a.m. UTC | #1
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);
Enric Balletbo i Serra Aug. 25, 2021, 9:07 a.m. UTC | #2
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);
>
houlong wei Aug. 30, 2021, 5:14 p.m. UTC | #3
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 mbox series

Patch

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