[RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Dmitry Baryshkov]

The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
components") changed the MDP5 driver to look for the interconnect paths
in the MDSS device rather than in the MDP5 device itself. This was left
unnoticed since on my testing devices the interconnects probably didn't
reach the sync state.

Rather than just using the MDP5 device for ICC path lookups for the MDP5
devices, introduce an additional helper to check both MDP5/DPU and MDSS
nodes. This will be helpful for the MDP5->DPU conversion, since the
driver will have to check both nodes.

Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
Reported-by: Yassine Oudjana <y.oudjana@protonmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  7 ++-----
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  9 +++------
 drivers/gpu/drm/msm/msm_drv.h            |  2 ++
 drivers/gpu/drm/msm/msm_io_utils.c       | 22 ++++++++++++++++++++++
 4 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e23e2552e802..9eff6c2b1917 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -383,12 +383,9 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
 	struct icc_path *path1;
 	struct drm_device *dev = dpu_kms->dev;
 	struct device *dpu_dev = dev->dev;
-	struct device *mdss_dev = dpu_dev->parent;
 
-	/* Interconnects are a part of MDSS device tree binding, not the
-	 * MDP/DPU device. */
-	path0 = of_icc_get(mdss_dev, "mdp0-mem");
-	path1 = of_icc_get(mdss_dev, "mdp1-mem");
+	path0 = msm_icc_get(dpu_dev, "mdp0-mem");
+	path1 = msm_icc_get(dpu_dev, "mdp1-mem");
 
 	if (IS_ERR_OR_NULL(path0))
 		return PTR_ERR_OR_ZERO(path0);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 3d5621a68f85..b0c372fef5d5 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -921,12 +921,9 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
 
 static int mdp5_setup_interconnect(struct platform_device *pdev)
 {
-	/* Interconnects are a part of MDSS device tree binding, not the
-	 * MDP5 device. */
-	struct device *mdss_dev = pdev->dev.parent;
-	struct icc_path *path0 = of_icc_get(mdss_dev, "mdp0-mem");
-	struct icc_path *path1 = of_icc_get(mdss_dev, "mdp1-mem");
-	struct icc_path *path_rot = of_icc_get(mdss_dev, "rotator-mem");
+	struct icc_path *path0 = msm_icc_get(&pdev->dev, "mdp0-mem");
+	struct icc_path *path1 = msm_icc_get(&pdev->dev, "mdp1-mem");
+	struct icc_path *path_rot = msm_icc_get(&pdev->dev, "rotator-mem");
 
 	if (IS_ERR(path0))
 		return PTR_ERR(path0);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 08388d742d65..d38510f6dbf5 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -441,6 +441,8 @@ void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
 		phys_addr_t *size);
 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
 
+struct icc_path *msm_icc_get(struct device *dev, const char *name);
+
 #define msm_writel(data, addr) writel((data), (addr))
 #define msm_readl(addr) readl((addr))
 
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
index 7b504617833a..d02cd29ce829 100644
--- a/drivers/gpu/drm/msm/msm_io_utils.c
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -5,6 +5,8 @@
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
+#include <linux/interconnect.h>
+
 #include "msm_drv.h"
 
 /*
@@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
 	work->worker = worker;
 	kthread_init_work(&work->work, fn);
 }
+
+struct icc_path *msm_icc_get(struct device *dev, const char *name)
+{
+	struct device *mdss_dev = dev->parent;
+	struct icc_path *path;
+
+	path = of_icc_get(dev, name);
+	if (path)
+		return path;
+
+	/*
+	 * If there are no interconnects attached to the corresponding device
+	 * node, of_icc_get() will return NULL.
+	 *
+	 * If the MDP5/DPU device node doesn't have interconnects, lookup the
+	 * path in the parent (MDSS) device.
+	 */
+	return of_icc_get(mdss_dev, name);
+
+}
-- 
2.35.1

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Marijn Suijten]

On 2022-08-05 14:56:30, Dmitry Baryshkov wrote:
> The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
> components") changed the MDP5 driver to look for the interconnect paths
> in the MDSS device rather than in the MDP5 device itself. This was left
> unnoticed since on my testing devices the interconnects probably didn't
> reach the sync state.
> 
> Rather than just using the MDP5 device for ICC path lookups for the MDP5
> devices, introduce an additional helper to check both MDP5/DPU and MDSS
> nodes. This will be helpful for the MDP5->DPU conversion, since the
> driver will have to check both nodes.
> 
> Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
> Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reported-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Tested-by: Marijn Suijten <marijn.suijten@somainline.org> # On sdm630

But I'm not sure about giving my Reviewed-by to this, as I'd rather
*correct* the DT bindings for sdm630 and msm8996 to provide
interconnects in the MDSS node unless there are strong reasons not to
(and I don't consider "backwards compatibility" to be one, this binding
"never even existed" if mdp5.txt is to be believed).

- Marijn

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  7 ++-----
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  9 +++------
>  drivers/gpu/drm/msm/msm_drv.h            |  2 ++
>  drivers/gpu/drm/msm/msm_io_utils.c       | 22 ++++++++++++++++++++++
>  4 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e23e2552e802..9eff6c2b1917 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -383,12 +383,9 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
>  	struct icc_path *path1;
>  	struct drm_device *dev = dpu_kms->dev;
>  	struct device *dpu_dev = dev->dev;
> -	struct device *mdss_dev = dpu_dev->parent;
>  
> -	/* Interconnects are a part of MDSS device tree binding, not the
> -	 * MDP/DPU device. */
> -	path0 = of_icc_get(mdss_dev, "mdp0-mem");
> -	path1 = of_icc_get(mdss_dev, "mdp1-mem");
> +	path0 = msm_icc_get(dpu_dev, "mdp0-mem");
> +	path1 = msm_icc_get(dpu_dev, "mdp1-mem");
>  
>  	if (IS_ERR_OR_NULL(path0))
>  		return PTR_ERR_OR_ZERO(path0);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 3d5621a68f85..b0c372fef5d5 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -921,12 +921,9 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
>  
>  static int mdp5_setup_interconnect(struct platform_device *pdev)
>  {
> -	/* Interconnects are a part of MDSS device tree binding, not the
> -	 * MDP5 device. */
> -	struct device *mdss_dev = pdev->dev.parent;
> -	struct icc_path *path0 = of_icc_get(mdss_dev, "mdp0-mem");
> -	struct icc_path *path1 = of_icc_get(mdss_dev, "mdp1-mem");
> -	struct icc_path *path_rot = of_icc_get(mdss_dev, "rotator-mem");
> +	struct icc_path *path0 = msm_icc_get(&pdev->dev, "mdp0-mem");
> +	struct icc_path *path1 = msm_icc_get(&pdev->dev, "mdp1-mem");
> +	struct icc_path *path_rot = msm_icc_get(&pdev->dev, "rotator-mem");
>  
>  	if (IS_ERR(path0))
>  		return PTR_ERR(path0);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 08388d742d65..d38510f6dbf5 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -441,6 +441,8 @@ void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
>  		phys_addr_t *size);
>  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
>  
> +struct icc_path *msm_icc_get(struct device *dev, const char *name);
> +
>  #define msm_writel(data, addr) writel((data), (addr))
>  #define msm_readl(addr) readl((addr))
>  
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> index 7b504617833a..d02cd29ce829 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -5,6 +5,8 @@
>   * Author: Rob Clark <robdclark@gmail.com>
>   */
>  
> +#include <linux/interconnect.h>
> +
>  #include "msm_drv.h"
>  
>  /*
> @@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
>  	work->worker = worker;
>  	kthread_init_work(&work->work, fn);
>  }
> +
> +struct icc_path *msm_icc_get(struct device *dev, const char *name)
> +{
> +	struct device *mdss_dev = dev->parent;
> +	struct icc_path *path;
> +
> +	path = of_icc_get(dev, name);
> +	if (path)
> +		return path;
> +
> +	/*
> +	 * If there are no interconnects attached to the corresponding device
> +	 * node, of_icc_get() will return NULL.
> +	 *
> +	 * If the MDP5/DPU device node doesn't have interconnects, lookup the
> +	 * path in the parent (MDSS) device.
> +	 */
> +	return of_icc_get(mdss_dev, name);
> +
> +}
> -- 
> 2.35.1
> 

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Yassine Oudjana]

On Friday, August 5th, 2022 at 12:56 PM, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
> components") changed the MDP5 driver to look for the interconnect paths
> in the MDSS device rather than in the MDP5 device itself. This was left
> unnoticed since on my testing devices the interconnects probably didn't
> reach the sync state.
>
> Rather than just using the MDP5 device for ICC path lookups for the MDP5
> devices, introduce an additional helper to check both MDP5/DPU and MDSS
> nodes. This will be helpful for the MDP5->DPU conversion, since the
>
> driver will have to check both nodes.
>
> Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
> Reported-by: Marijn Suijten marijn.suijten@somainline.org
>
> Reported-by: Yassine Oudjana y.oudjana@protonmail.com
>
> Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 ++-----
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 9 +++------
> drivers/gpu/drm/msm/msm_drv.h | 2 ++
> drivers/gpu/drm/msm/msm_io_utils.c | 22 ++++++++++++++++++++++
> 4 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e23e2552e802..9eff6c2b1917 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -383,12 +383,9 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
> struct icc_path *path1;
> struct drm_device *dev = dpu_kms->dev;
>
> struct device *dpu_dev = dev->dev;
>
> - struct device *mdss_dev = dpu_dev->parent;
>
>
> - /* Interconnects are a part of MDSS device tree binding, not the
> - * MDP/DPU device. */
> - path0 = of_icc_get(mdss_dev, "mdp0-mem");
> - path1 = of_icc_get(mdss_dev, "mdp1-mem");
> + path0 = msm_icc_get(dpu_dev, "mdp0-mem");
> + path1 = msm_icc_get(dpu_dev, "mdp1-mem");
>
> if (IS_ERR_OR_NULL(path0))
> return PTR_ERR_OR_ZERO(path0);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 3d5621a68f85..b0c372fef5d5 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -921,12 +921,9 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
>
> static int mdp5_setup_interconnect(struct platform_device pdev)
> {
> - / Interconnects are a part of MDSS device tree binding, not the
> - * MDP5 device. */
> - struct device *mdss_dev = pdev->dev.parent;
>
> - struct icc_path *path0 = of_icc_get(mdss_dev, "mdp0-mem");
> - struct icc_path *path1 = of_icc_get(mdss_dev, "mdp1-mem");
> - struct icc_path *path_rot = of_icc_get(mdss_dev, "rotator-mem");
> + struct icc_path *path0 = msm_icc_get(&pdev->dev, "mdp0-mem");
>
> + struct icc_path *path1 = msm_icc_get(&pdev->dev, "mdp1-mem");
>
> + struct icc_path *path_rot = msm_icc_get(&pdev->dev, "rotator-mem");
>
>
> if (IS_ERR(path0))
> return PTR_ERR(path0);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 08388d742d65..d38510f6dbf5 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -441,6 +441,8 @@ void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
> phys_addr_t *size);
> void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
>
> +struct icc_path *msm_icc_get(struct device *dev, const char *name);
> +
> #define msm_writel(data, addr) writel((data), (addr))
> #define msm_readl(addr) readl((addr))
>
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> index 7b504617833a..d02cd29ce829 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -5,6 +5,8 @@
> * Author: Rob Clark robdclark@gmail.com
>
> */
>
> +#include <linux/interconnect.h>
>
> +
> #include "msm_drv.h"
>
> /*
> @@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
> work->worker = worker;
>
> kthread_init_work(&work->work, fn);
>
> }
> +
> +struct icc_path *msm_icc_get(struct device *dev, const char *name)
> +{
> + struct device *mdss_dev = dev->parent;
>
> + struct icc_path path;
> +
> + path = of_icc_get(dev, name);
> + if (path)
> + return path;
> +
> + /
> + * If there are no interconnects attached to the corresponding device
> + * node, of_icc_get() will return NULL.
> + *
> + * If the MDP5/DPU device node doesn't have interconnects, lookup the
> + * path in the parent (MDSS) device.
> + */
> + return of_icc_get(mdss_dev, name);
> +
> +}
> --
> 2.35.1

Tested-by: Yassine Oudjana <y.oudjana@protonmail.com> # msm8996

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-08-05 04:56:30)
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> index 7b504617833a..d02cd29ce829 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
>         work->worker = worker;
>         kthread_init_work(&work->work, fn);
>  }
> +
> +struct icc_path *msm_icc_get(struct device *dev, const char *name)
> +{
> +       struct device *mdss_dev = dev->parent;
> +       struct icc_path *path;
> +
> +       path = of_icc_get(dev, name);
> +       if (path)
> +               return path;
> +
> +       /*
> +        * If there are no interconnects attached to the corresponding device
> +        * node, of_icc_get() will return NULL.
> +        *
> +        * If the MDP5/DPU device node doesn't have interconnects, lookup the
> +        * path in the parent (MDSS) device.
> +        */
> +       return of_icc_get(mdss_dev, name);

Perhaps this would be better served by having another icc_get() API that
looks in the device and also the parent? Or maybe there should be
interconnect-ranges (similar to clock-ranges) so that interconnects can
be mapped to child nodes in DT.

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Dmitry Baryshkov]

On 05/08/2022 15:24, Marijn Suijten wrote:
> On 2022-08-05 14:56:30, Dmitry Baryshkov wrote:
>> The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
>> components") changed the MDP5 driver to look for the interconnect paths
>> in the MDSS device rather than in the MDP5 device itself. This was left
>> unnoticed since on my testing devices the interconnects probably didn't
>> reach the sync state.
>>
>> Rather than just using the MDP5 device for ICC path lookups for the MDP5
>> devices, introduce an additional helper to check both MDP5/DPU and MDSS
>> nodes. This will be helpful for the MDP5->DPU conversion, since the
>> driver will have to check both nodes.
>>
>> Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
>> Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
>> Reported-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Tested-by: Marijn Suijten <marijn.suijten@somainline.org> # On sdm630
> 
> But I'm not sure about giving my Reviewed-by to this, as I'd rather
> *correct* the DT bindings for sdm630 and msm8996 to provide
> interconnects in the MDSS node unless there are strong reasons not to
> (and I don't consider "backwards compatibility" to be one, this binding
> "never even existed" if mdp5.txt is to be believed).

As a kind of a joke, I'd prefer to have interconnects in the mdp/dpu 
device node. In the end, the interconnects describe the path between the 
display controller and the DDR, not the path between the whole MDSS and DDR.

So, for next chipsets I'd vote to move icc to dpu/mdp node (and maybe 
even move existing inerconnects to the dpu node).

-- 
With best wishes
Dmitry

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Dmitry Baryshkov]

On 24/08/2022 00:31, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-08-05 04:56:30)
>> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
>> index 7b504617833a..d02cd29ce829 100644
>> --- a/drivers/gpu/drm/msm/msm_io_utils.c
>> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
>> @@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
>>          work->worker = worker;
>>          kthread_init_work(&work->work, fn);
>>   }
>> +
>> +struct icc_path *msm_icc_get(struct device *dev, const char *name)
>> +{
>> +       struct device *mdss_dev = dev->parent;
>> +       struct icc_path *path;
>> +
>> +       path = of_icc_get(dev, name);
>> +       if (path)
>> +               return path;
>> +
>> +       /*
>> +        * If there are no interconnects attached to the corresponding device
>> +        * node, of_icc_get() will return NULL.
>> +        *
>> +        * If the MDP5/DPU device node doesn't have interconnects, lookup the
>> +        * path in the parent (MDSS) device.
>> +        */
>> +       return of_icc_get(mdss_dev, name);
> 
> Perhaps this would be better served by having another icc_get() API that
> looks in the device and also the parent? Or maybe there should be
> interconnect-ranges (similar to clock-ranges) so that interconnects can
> be mapped to child nodes in DT.

I was not sure how common this situation is, so I did not add the 
helper/API. Typically the driver knows exactly, which node has the 
interconnects. In our case this is complicated because we are 
effectively merging two different driver generations and two different 
bindings. Thus I suppose this situation is quite unique.


-- 
With best wishes
Dmitry

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Marijn Suijten]

On 2022-08-26 12:16:40, Dmitry Baryshkov wrote:
> On 05/08/2022 15:24, Marijn Suijten wrote:
> > On 2022-08-05 14:56:30, Dmitry Baryshkov wrote:
> >> The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
> >> components") changed the MDP5 driver to look for the interconnect paths
> >> in the MDSS device rather than in the MDP5 device itself. This was left
> >> unnoticed since on my testing devices the interconnects probably didn't
> >> reach the sync state.
> >>
> >> Rather than just using the MDP5 device for ICC path lookups for the MDP5
> >> devices, introduce an additional helper to check both MDP5/DPU and MDSS
> >> nodes. This will be helpful for the MDP5->DPU conversion, since the
> >> driver will have to check both nodes.
> >>
> >> Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
> >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> Reported-by: Yassine Oudjana <y.oudjana@protonmail.com>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > Tested-by: Marijn Suijten <marijn.suijten@somainline.org> # On sdm630
> > 
> > But I'm not sure about giving my Reviewed-by to this, as I'd rather
> > *correct* the DT bindings for sdm630 and msm8996 to provide
> > interconnects in the MDSS node unless there are strong reasons not to
> > (and I don't consider "backwards compatibility" to be one, this binding
> > "never even existed" if mdp5.txt is to be believed).
> 
> As a kind of a joke, I'd prefer to have interconnects in the mdp/dpu 
> device node. In the end, the interconnects describe the path between the 
> display controller and the DDR, not the path between the whole MDSS and DDR.
> 
> So, for next chipsets I'd vote to move icc to dpu/mdp node (and maybe 
> even move existing inerconnects to the dpu node).

Sure.  In that case, do you want to rework this patch / code again to
only look in the DPU/MDP, and not at MDSS at all?  (Or is that another
DT API break we'd rather not make?)

- Marijn

Re: [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices

[Dmitry Baryshkov]

On 19/10/2022 12:13, Marijn Suijten wrote:
> On 2022-08-26 12:16:40, Dmitry Baryshkov wrote:
>> On 05/08/2022 15:24, Marijn Suijten wrote:
>>> On 2022-08-05 14:56:30, Dmitry Baryshkov wrote:
>>>> The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
>>>> components") changed the MDP5 driver to look for the interconnect paths
>>>> in the MDSS device rather than in the MDP5 device itself. This was left
>>>> unnoticed since on my testing devices the interconnects probably didn't
>>>> reach the sync state.
>>>>
>>>> Rather than just using the MDP5 device for ICC path lookups for the MDP5
>>>> devices, introduce an additional helper to check both MDP5/DPU and MDSS
>>>> nodes. This will be helpful for the MDP5->DPU conversion, since the
>>>> driver will have to check both nodes.
>>>>
>>>> Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
>>>> Reported-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>> Reported-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> Tested-by: Marijn Suijten <marijn.suijten@somainline.org> # On sdm630
>>>
>>> But I'm not sure about giving my Reviewed-by to this, as I'd rather
>>> *correct* the DT bindings for sdm630 and msm8996 to provide
>>> interconnects in the MDSS node unless there are strong reasons not to
>>> (and I don't consider "backwards compatibility" to be one, this binding
>>> "never even existed" if mdp5.txt is to be believed).
>>
>> As a kind of a joke, I'd prefer to have interconnects in the mdp/dpu
>> device node. In the end, the interconnects describe the path between the
>> display controller and the DDR, not the path between the whole MDSS and DDR.
>>
>> So, for next chipsets I'd vote to move icc to dpu/mdp node (and maybe
>> even move existing inerconnects to the dpu node).
> 
> Sure.  In that case, do you want to rework this patch / code again to
> only look in the DPU/MDP, and not at MDSS at all?  (Or is that another
> DT API break we'd rather not make?)

I'd rather not make this break. Let's keep backwards compatibility at 
least for now.

-- 
With best wishes
Dmitry