drm/msm/disp/dpu1: fix warn stack reported during dpu resume

PATCH In review Kalyan Thota 4 participants 8 messages Mar 31, 2021 – Apr 2, 2021

Subsystems (3)

DRM drivers Arm Qualcomm mailing list DRM driver for Qualcomm display hardware

lore

Share & tools

lore

Kalyan Thota

author

2021-03-31 11:27 UTC #0 lore raw

PATCH 1/1 drm/msm/disp/dpu1: fix warn stack reported during dpu resume

WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.

"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"

For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.

Reported-by Steev Klimaszewski <[email protected]>

Signed-off-by Kalyan Thota <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
 3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+	if (!dpu_supports_bw_scaling(dev))
+		return 0;
+
 	path0 = of_icc_get(dev->dev, "mdp0-mem");
 	path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 		DPU_DEBUG("REG_DMA is not defined");
 	}
 
-	if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
-		dpu_kms_parse_data_bus_icc_path(dpu_kms);
+	dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 
 	ddev = dpu_kms->dev;
 
-	WARN_ON(!(dpu_kms->num_paths));
+	WARN_ON((dpu_supports_bw_scaling(ddev) && !dpu_kms->num_paths));
 	/* Min vote of BW is required before turning on AXI clk */
 	for (i = 0; i < dpu_kms->num_paths; i++)
 		icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +9 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
 #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+	return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");
+}
+
 /* Global private object state for tracking resources that are shared across
  * multiple kms objects (planes/crtcs/etc).
  */

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct drm_device *dev,
 	struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
 	struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+	if (dpu_supports_bw_scaling(dev))
+		return 0;
+
 	if (IS_ERR_OR_NULL(path0))
 		return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
 	DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
 
-	if (!of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss")) {
-		ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-		if (ret)
-			return ret;
-	}
+	ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+	if (ret)
+		return ret;
 
 	mp = &dpu_mdss->mp;
 	ret = msm_dss_parse_clock(pdev, mp);

Signature · 6 lines

2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Doug Anderson

2021-03-31 15:56 UTC #1 lore raw

Hi,

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) · line 302

Quoted 11 lines · Kalyan Thota

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

@@ -294,6 +294,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;
 
+       if (!dpu_supports_bw_scaling(dev))
+               return 0;
+
        path0 = of_icc_get(dev->dev, "mdp0-mem");
        path1 = of_icc_get(dev->dev, "mdp1-mem");

Instead of hard coding a check for specific SoC compatible strings, why not just check to see if path0 and/or path1 are ERR_PTR(-ENODEV)? Then change dpu_supports_bw_scaling() to just return:

!IS_ERR(dpu_kms->path[0])

It also seems like it would be nice if you did something if you got an error other than -ENODEV. Right now this function returns it but the caller ignores it? At least spit an error message out?

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h struct vsync_info { · line 163

Quoted 11 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +7 -0

@@ -154,6 +154,15 @@ struct vsync_info {
 
 #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+       return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");

See above, but I think this would be better as:

  return !IS_ERR(dpu_kms->path[0]);

Specifically, I don't think of_device_is_compatible() is really designed as something to call a lot. It's doing a whole bunch of data structure parsing / string comparisons. It's OK-ish during probe (though better to use the of_match_table), but you don't want to call it on every runtime suspend / runtime resume.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Dmitry Baryshkov

2021-03-31 16:03 UTC #2 lore raw

In reply to Kalyan Thota

Quoted 7 lines · Kalyan Thota

WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.

Should we change all DPU targets to use bw scaling to the mdp from the mdss nodes? The limitation to sc7180 looks artificial.

Quoted 92 lines

Reported-by Steev Klimaszewski <[email protected]>

Signed-off-by Kalyan Thota <[email protected]>

---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
  3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+	if (!dpu_supports_bw_scaling(dev))
+		return 0;
+
  	path0 = of_icc_get(dev->dev, "mdp0-mem");
  	path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
  		DPU_DEBUG("REG_DMA is not defined");
  	}
 
-	if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
-		dpu_kms_parse_data_bus_icc_path(dpu_kms);
+	dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
  	pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 
  	ddev = dpu_kms->dev;
 
-	WARN_ON(!(dpu_kms->num_paths));
+	WARN_ON((dpu_supports_bw_scaling(ddev) && !dpu_kms->num_paths));
  	/* Min vote of BW is required before turning on AXI clk */
  	for (i = 0; i < dpu_kms->num_paths; i++)
  		icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +9 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
  #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+	return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");
+}
+
  /* Global private object state for tracking resources that are shared across
   * multiple kms objects (planes/crtcs/etc).
   */

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct drm_device *dev,
  	struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
  	struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+	if (dpu_supports_bw_scaling(dev))
+		return 0;
+
  	if (IS_ERR_OR_NULL(path0))
  		return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
  	DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
 
-	if (!of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss")) {
-		ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-		if (ret)
-			return ret;
-	}
+	ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+	if (ret)
+		return ret;
 
  	mp = &dpu_mdss->mp;
  	ret = msm_dss_parse_clock(pdev, mp);

Signature · 6 lines

With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Rob Clark

2021-03-31 22:43 UTC #3 lore raw

Quoted 5 lines · Dmitry Baryshkov

Quoted 3 lines · Kalyan Thota
WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.

Quoted 7 lines

Quoted 4 lines
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.
Should we change all DPU targets to use bw scaling to the mdp from the mdss nodes? The limitation to sc7180 looks artificial.

yes, we should, this keeps biting us on 845

Quoted 2 lines

Quoted 2 lines
Reported-by Steev Klimaszewski <[email protected]>

Please add Fixes: tag as well

Quoted 87 lines

Signed-off-by Kalyan Thota <[email protected]>

---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
  3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+     if (!dpu_supports_bw_scaling(dev))
+             return 0;
+
      path0 = of_icc_get(dev->dev, "mdp0-mem");
      path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
              DPU_DEBUG("REG_DMA is not defined");
      }
 
-     if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
-             dpu_kms_parse_data_bus_icc_path(dpu_kms);
+     dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
      pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 
      ddev = dpu_kms->dev;
 
-     WARN_ON(!(dpu_kms->num_paths));
+     WARN_ON((dpu_supports_bw_scaling(ddev) && !dpu_kms->num_paths));
      /* Min vote of BW is required before turning on AXI clk */
      for (i = 0; i < dpu_kms->num_paths; i++)
              icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +9 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
  #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+     return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");
+}
+
  /* Global private object state for tracking resources that are shared across
   * multiple kms objects (planes/crtcs/etc).
   */

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct drm_device *dev,
      struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
      struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+     if (dpu_supports_bw_scaling(dev))
+             return 0;
+
      if (IS_ERR_OR_NULL(path0))
              return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
      DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
 
-     if (!of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss")) {
-             ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-             if (ret)
-                     return ret;
-     }
+     ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+     if (ret)
+             return ret;

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c int dpu_mdss_init(struct drm_device *dev) · line 287
Quoted 3 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +0 -0
      mp = &dpu_mdss->mp;
      ret = msm_dss_parse_clock(pdev, mp);
--
With best wishes
Dmitry

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Dmitry Baryshkov

2021-04-01 02:07 UTC #4 lore raw

Quoted 9 lines · Rob Clark

Quoted 7 lines · Dmitry Baryshkov
Quoted 5 lines · Kalyan Thota
WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.
Should we change all DPU targets to use bw scaling to the mdp from the mdss nodes? The limitation to sc7180 looks artificial.
yes, we should, this keeps biting us on 845

Done, https://lore.kernel.org/linux-arm-msm/[email protected]/

Quoted 101 lines

Quoted 3 lines

Quoted 3 lines
Reported-by Steev Klimaszewski <[email protected]>

Please add Fixes: tag as well

Quoted 95 lines

Quoted 80 lines

Signed-off-by Kalyan Thota <[email protected]>

---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
   3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+     if (!dpu_supports_bw_scaling(dev))
+             return 0;
+
       path0 = of_icc_get(dev->dev, "mdp0-mem");
       path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
               DPU_DEBUG("REG_DMA is not defined");
       }
 
-     if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
-             dpu_kms_parse_data_bus_icc_path(dpu_kms);
+     dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
       pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 
       ddev = dpu_kms->dev;
 
-     WARN_ON(!(dpu_kms->num_paths));
+     WARN_ON((dpu_supports_bw_scaling(ddev) && !dpu_kms->num_paths));
       /* Min vote of BW is required before turning on AXI clk */
       for (i = 0; i < dpu_kms->num_paths; i++)
               icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +9 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
   #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+     return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");
+}
+
   /* Global private object state for tracking resources that are shared across
    * multiple kms objects (planes/crtcs/etc).
    */

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +3 -1

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct drm_device *dev,
       struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
       struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+     if (dpu_supports_bw_scaling(dev))
+             return 0;
+
       if (IS_ERR_OR_NULL(path0))
               return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
       DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
 
-     if (!of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss")) {

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c int dpu_mdss_init(struct drm_device *dev) · line 287

Quoted 10 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +3 -4

-             ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-             if (ret)
-                     return ret;
-     }
+     ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+     if (ret)
+             return ret;
 
       mp = &dpu_mdss->mp;
       ret = msm_dss_parse_clock(pdev, mp);

--
With best wishes
Dmitry

Signature · 6 lines

With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[email protected]

author

2021-04-01 13:19 UTC #5 lore raw

Quoted 13 lines · Dmitry Baryshkov

Quoted 12 lines · Rob Clark
Quoted 10 lines · Dmitry Baryshkov
Quoted 7 lines · Kalyan Thota
WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Should we change all DPU targets to use bw scaling to the mdp from the mdss nodes? The limitation to sc7180 looks artificial.
yes, we should, this keeps biting us on 845
Done,
https://lore.kernel.org/linux-arm-msm/[email protected]/

Hi Dmitry,

https://lore.kernel.org/linux-arm-msm/[email protected]/

you need to add clk_inefficiency_factor, bw_inefficiency_factor in the catalogue for the new targets where bw scaling is being enabled. please reuse sc7180 values.

secondly, the AXI clock needs to be moved from mdss to mdp device like as in sc7180 dt if its not done already.

lastly, if you are planning to remove the static votes from dpu_mdss, do you also want to move the interconnect paths from mdss device to mdp device in the dt ?

Thanks,
Kalyan

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Reported-by Steev Klimaszewski <[email protected]>
Please add Fixes: tag as well

Adding Fixes tag above my sign-off, should i push another version or can it be picked from here ?

Fixes Id252b9c2887 ("drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume")

Quoted 100 lines

Signed-off-by Kalyan Thota <[email protected]>

---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
   3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+     if (!dpu_supports_bw_scaling(dev))
+             return 0;
+
       path0 = of_icc_get(dev->dev, "mdp0-mem");
       path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
               DPU_DEBUG("REG_DMA is not defined");
       }
 
-     if (of_device_is_compatible(dev->dev->of_node, 
 "qcom,sc7180-mdss"))
-             dpu_kms_parse_data_bus_icc_path(dpu_kms);
+     dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
       pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused 
 dpu_runtime_resume(struct device *dev)
 
       ddev = dpu_kms->dev;
 
-     WARN_ON(!(dpu_kms->num_paths));
+     WARN_ON((dpu_supports_bw_scaling(ddev) && 
 !dpu_kms->num_paths));
       /* Min vote of BW is required before turning on AXI clk */

       for (i = 0; i < dpu_kms->num_paths; i++)
               icc_set_bw(dpu_kms->path[i], 0,

Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
   #define to_dpu_global_state(x) container_of(x, struct 
 dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support

bw scaling.
+ * @dev: Pointer to drm_device structure
+ */

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +3 -0

+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+     return of_device_is_compatible(dev->dev->of_node,

"qcom,sc7180-mdss");

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

+}
+
   /* Global private object state for tracking resources that are

shared across
* multiple kms objects (planes/crtcs/etc).
*/

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct 
 drm_device *dev,
       struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
       struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+     if (dpu_supports_bw_scaling(dev))
+             return 0;
+
       if (IS_ERR_OR_NULL(path0))
               return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
       DRM_DEBUG("mapped mdss address space @%pK\n", 
 dpu_mdss->mmio);
 
-     if (!of_device_is_compatible(dev->dev->of_node, 
 "qcom,sc7180-mdss")) {
-             ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-             if (ret)
-                     return ret;
-     }
+     ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+     if (ret)
+             return ret;

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
       mp = &dpu_mdss->mp;
       ret = msm_dss_parse_clock(pdev, mp);
--
With best wishes
Dmitry

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Dmitry Baryshkov

2021-04-01 13:31 UTC #6 lore raw

Quoted 25 lines · On Thu, 1 Apr 2021 at 16:19, <[email protected]> wrote:

Quoted 14 lines · Dmitry Baryshkov
Quoted 13 lines · Rob Clark
Quoted 11 lines · Dmitry Baryshkov
Quoted 8 lines · Kalyan Thota
WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Should we change all DPU targets to use bw scaling to the mdp from the
mdss nodes? The limitation to sc7180 looks artificial.
yes, we should, this keeps biting us on 845
Done,
https://lore.kernel.org/linux-arm-msm/[email protected]/

Quoted 7 lines

Hi Dmitry,
https://lore.kernel.org/linux-arm-msm/[email protected]/
you need to add clk_inefficiency_factor, bw_inefficiency_factor in the catalogue for the new
targets where bw scaling is being enabled. please reuse sc7180 values.

Done in patch 1 in that series.

Quoted 3 lines

secondly, the AXI clock needs to be moved from mdss to mdp device like as in sc7180 dt if its not done already.

Is this enough:
sm8250 has <&gcc GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes sdm845 has <&gcc GCC_DISP_AXI_CLK> in mdss node and <&dispcc DISP_CC_MDSS_AXI_CLK> in the mdp node.

Quoted 4 lines

lastly, if you are planning to remove the static votes from dpu_mdss, do you also want to move the
interconnect paths from mdss device to mdp device in the dt ?

I have no strong opinion on this. So far I did not change dt to be compatible with the current device trees.

Quoted 127 lines

Thanks,
Kalyan

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Reported-by Steev Klimaszewski <[email protected]>
Please add Fixes: tag as well

Adding Fixes tag above my sign-off, should i push another version or can it be picked from here ?

Fixes: Id252b9c2887 ("drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume")

Quoted 111 lines

Quoted 103 lines

Signed-off-by Kalyan Thota <[email protected]>

---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
   3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+     if (!dpu_supports_bw_scaling(dev))
+             return 0;
+
       path0 = of_icc_get(dev->dev, "mdp0-mem");
       path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
               DPU_DEBUG("REG_DMA is not defined");
       }
 
-     if (of_device_is_compatible(dev->dev->of_node,
 "qcom,sc7180-mdss"))
-             dpu_kms_parse_data_bus_icc_path(dpu_kms);
+     dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
       pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused
 dpu_runtime_resume(struct device *dev)
 
       ddev = dpu_kms->dev;
 
-     WARN_ON(!(dpu_kms->num_paths));
+     WARN_ON((dpu_supports_bw_scaling(ddev) &&
 !dpu_kms->num_paths));
       /* Min vote of BW is required before turning on AXI clk */

       for (i = 0; i < dpu_kms->num_paths; i++)
               icc_set_bw(dpu_kms->path[i], 0,

Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
   #define to_dpu_global_state(x) container_of(x, struct
 dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support

bw scaling.
+ * @dev: Pointer to drm_device structure
+ */

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +3 -0

+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+     return of_device_is_compatible(dev->dev->of_node,

"qcom,sc7180-mdss");

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

+}
+
   /* Global private object state for tracking resources that are

shared across
* multiple kms objects (planes/crtcs/etc).
*/

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct
 drm_device *dev,
       struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
       struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+     if (dpu_supports_bw_scaling(dev))
+             return 0;
+
       if (IS_ERR_OR_NULL(path0))
               return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
       DRM_DEBUG("mapped mdss address space @%pK\n",
 dpu_mdss->mmio);
 
-     if (!of_device_is_compatible(dev->dev->of_node,
 "qcom,sc7180-mdss")) {
-             ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-             if (ret)
-                     return ret;
-     }
+     ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+     if (ret)
+             return ret;

Quoted 3 lines

       mp = &dpu_mdss->mp;
       ret = msm_dss_parse_clock(pdev, mp);

--
With best wishes
Dmitry

Signature · 6 lines

With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[email protected]

author

2021-04-02 11:42 UTC #7 lore raw

Quoted 28 lines · Dmitry Baryshkov

Quoted 27 lines · On Thu, 1 Apr 2021 at 16:19, <[email protected]> wrote:
Quoted 14 lines · Dmitry Baryshkov
Quoted 13 lines · Rob Clark
Quoted 11 lines · Dmitry Baryshkov
Quoted 8 lines · Kalyan Thota
WARN_ON was introduced by the below commit to catch runtime resumes that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume"
For the targets where the bw scaling is not enabled, this WARN_ON is a false alarm. Fix the WARN condition appropriately.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Should we change all DPU targets to use bw scaling to the mdp from the
mdss nodes? The limitation to sc7180 looks artificial.
yes, we should, this keeps biting us on 845
Done,
https://lore.kernel.org/linux-arm-msm/[email protected]/
Hi Dmitry,

Quoted 8 lines

Quoted 5 lines
https://lore.kernel.org/linux-arm-msm/[email protected]/
you need to add clk_inefficiency_factor, bw_inefficiency_factor in the catalogue for the new
targets where bw scaling is being enabled. please reuse sc7180 values.
Done in patch 1 in that series.

got it.

Quoted 10 lines

Quoted 3 lines
secondly, the AXI clock needs to be moved from mdss to mdp device like as in sc7180 dt if its not done already.
Is this enough:
sm8250 has <&gcc GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes sdm845 has <&gcc GCC_DISP_AXI_CLK> in mdss node and <&dispcc DISP_CC_MDSS_AXI_CLK> in the mdp node.
Since there is no BW vote in mdss, we need to move the AXI clock ON to mdp node.

for sm8250
remove GCC_DISP_HF_AXI_CLK from mdss device and add it in mdp device.

for sdm845
remove GCC_DISP_AXI_CLK from mdss device and add it in mdp device before we turn on DISP_CC_MDSS_AXI_CLK, i.e as first item.

Quoted 135 lines

Quoted 4 lines

lastly, if you are planning to remove the static votes from dpu_mdss, do you also want to move the
interconnect paths from mdss device to mdp device in the dt ?

I have no strong opinion on this. So far I did not change dt to be compatible with the current device trees.

Quoted 127 lines

Thanks,
Kalyan

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Reported-by Steev Klimaszewski <[email protected]>
Please add Fixes: tag as well

Adding Fixes tag above my sign-off, should i push another version or can it be picked from here ?

Fixes 627dc55c273d ("drm/msm/disp/dpu1: icc path needs to be set before

dpu runtime resume")

Quoted 111 lines

Quoted 103 lines

Signed-off-by Kalyan Thota <[email protected]>

---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |  9 +++++++++
   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++-----
   3 files changed, 20 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +5 -3

index cab387f..0071a4d 100644
@@ -294,6 +294,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;
 
+     if (!dpu_supports_bw_scaling(dev))
+             return 0;
+
       path0 = of_icc_get(dev->dev, "mdp0-mem");
       path1 = of_icc_get(dev->dev, "mdp1-mem");

@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
               DPU_DEBUG("REG_DMA is not defined");
       }
 
-     if (of_device_is_compatible(dev->dev->of_node,
 "qcom,sc7180-mdss"))
-             dpu_kms_parse_data_bus_icc_path(dpu_kms);
+     dpu_kms_parse_data_bus_icc_path(dpu_kms);
 
       pm_runtime_get_sync(&dpu_kms->pdev->dev);

@@ -1198,7 +1200,7 @@ static int __maybe_unused
 dpu_runtime_resume(struct device *dev)
 
       ddev = dpu_kms->dev;
 
-     WARN_ON(!(dpu_kms->num_paths));
+     WARN_ON((dpu_supports_bw_scaling(ddev) &&
 !dpu_kms->num_paths));
       /* Min vote of BW is required before turning on AXI clk */

       for (i = 0; i < dpu_kms->num_paths; i++)
               icc_set_bw(dpu_kms->path[i], 0,

Bps_to_icc(MIN_IB_BW));

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

index d6717d6..f7bcc0a 100644
@@ -154,6 +154,15 @@ struct vsync_info {
 
   #define to_dpu_global_state(x) container_of(x, struct
 dpu_global_state, base)
 
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support

bw scaling.
+ * @dev: Pointer to drm_device structure
+ */

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +3 -0

+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+     return of_device_is_compatible(dev->dev->of_node,

"qcom,sc7180-mdss");

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +2 -0

+}
+
   /* Global private object state for tracking resources that are

shared across
* multiple kms objects (planes/crtcs/etc).
*/

drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +6 -5

index cd40788..8cd712c 100644
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct
 drm_device *dev,
       struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
       struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
 
+     if (dpu_supports_bw_scaling(dev))
+             return 0;
+
       if (IS_ERR_OR_NULL(path0))
               return PTR_ERR_OR_ZERO(path0);

@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
 
       DRM_DEBUG("mapped mdss address space @%pK\n",
 dpu_mdss->mmio);
 
-     if (!of_device_is_compatible(dev->dev->of_node,
 "qcom,sc7180-mdss")) {
-             ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
-             if (ret)
-                     return ret;
-     }
+     ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+     if (ret)
+             return ret;

Quoted 3 lines

       mp = &dpu_mdss->mp;
       ret = msm_dss_parse_clock(pdev, mp);

--
With best wishes
Dmitry