linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
@ 2021-03-31 11:27 Kalyan Thota
  2021-03-31 15:55 ` Doug Anderson
  2021-03-31 16:03 ` Dmitry Baryshkov
  0 siblings, 2 replies; 8+ messages in thread
From: Kalyan Thota @ 2021-03-31 11:27 UTC (permalink / raw)
  To: y, dri-devel, linux-arm-msm, freedreno, devicetree
  Cc: Kalyan Thota, linux-kernel, robdclark, dianders, mkrishn, hywu,
	mka, midean, steev

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 <steev@kali.org>
Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
---
 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(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index cab387f..0071a4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -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));
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index d6717d6..f7bcc0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -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).
  */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index cd40788..8cd712c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -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);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-03-31 11:27 [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Kalyan Thota
@ 2021-03-31 15:55 ` Doug Anderson
  2021-03-31 16:03 ` Dmitry Baryshkov
  1 sibling, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2021-03-31 15:55 UTC (permalink / raw)
  To: Kalyan Thota
  Cc: y, dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Rob Clark, mkrishn, Daniel Hung-yu Wu, Matthias Kaehlcke,
	Michelle Dean, Steev Klimaszewski

Hi,

On Wed, Mar 31, 2021 at 4:27 AM Kalyan Thota <kalyan_t@codeaurora.org> wrote:
>
> @@ -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?


> @@ -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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-03-31 11:27 [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Kalyan Thota
  2021-03-31 15:55 ` Doug Anderson
@ 2021-03-31 16:03 ` Dmitry Baryshkov
  2021-03-31 22:47   ` Rob Clark
  1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-03-31 16:03 UTC (permalink / raw)
  To: Kalyan Thota, y, dri-devel, linux-arm-msm, freedreno, devicetree
  Cc: linux-kernel, robdclark, dianders, mkrishn, hywu, mka, midean, steev

On 31/03/2021 14:27, Kalyan Thota wrote:
> 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.

> 
> Reported-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
> ---
>   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(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index cab387f..0071a4d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -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));
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index d6717d6..f7bcc0a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -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).
>    */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> index cd40788..8cd712c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> @@ -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);
> 


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-03-31 16:03 ` Dmitry Baryshkov
@ 2021-03-31 22:47   ` Rob Clark
  2021-04-01  2:07     ` Dmitry Baryshkov
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2021-03-31 22:47 UTC (permalink / raw)
  To: Dmitry Baryshkov, Kalyan Thota
  Cc: y, dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Douglas Anderson, Krishna Manikandan,
	Daniel Hung-yu Wu, mka, Michelle Dean, Steev Klimaszewski

On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 31/03/2021 14:27, Kalyan Thota wrote:
> > 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.

yes, we should, this keeps biting us on 845

> >
> > Reported-by: Steev Klimaszewski <steev@kali.org>

Please add Fixes: tag as well

> > Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
> > ---
> >   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(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index cab387f..0071a4d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -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));
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index d6717d6..f7bcc0a 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -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).
> >    */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > index cd40788..8cd712c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > @@ -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);
> >
>
>
> --
> With best wishes
> Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-03-31 22:47   ` Rob Clark
@ 2021-04-01  2:07     ` Dmitry Baryshkov
  2021-04-01 13:18       ` [Freedreno] " kalyan_t
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-01  2:07 UTC (permalink / raw)
  To: Rob Clark, Kalyan Thota
  Cc: y, dri-devel, linux-arm-msm, freedreno,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Douglas Anderson, Krishna Manikandan,
	Daniel Hung-yu Wu, mka, Michelle Dean, Steev Klimaszewski

On 01/04/2021 01:47, Rob Clark wrote:
> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 31/03/2021 14:27, Kalyan Thota wrote:
>>> 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.
> 
> yes, we should, this keeps biting us on 845

Done, 
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/

> 
>>>
>>> Reported-by: Steev Klimaszewski <steev@kali.org>
> 
> Please add Fixes: tag as well
> 
>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
>>> ---
>>>    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(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index cab387f..0071a4d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -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));
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> index d6717d6..f7bcc0a 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> @@ -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).
>>>     */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>> index cd40788..8cd712c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>> @@ -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);
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-04-01  2:07     ` Dmitry Baryshkov
@ 2021-04-01 13:18       ` kalyan_t
  2021-04-01 13:31         ` Dmitry Baryshkov
  0 siblings, 1 reply; 8+ messages in thread
From: kalyan_t @ 2021-04-01 13:18 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rob Clark,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Krishna Manikandan, linux-arm-msm, Daniel Hung-yu Wu,
	Linux Kernel Mailing List, dri-devel, Douglas Anderson, mka,
	Michelle Dean, Steev Klimaszewski, freedreno, y

On 2021-04-01 07:37, Dmitry Baryshkov wrote:
> On 01/04/2021 01:47, Rob Clark wrote:
>> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>>> 
>>> On 31/03/2021 14:27, Kalyan Thota wrote:
>>>> 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.
>> 
>> yes, we should, this keeps biting us on 845
> 
> Done,
> https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/

Hi Dmitry,

https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/

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

> 
>> 
>>>> 
>>>> Reported-by: Steev Klimaszewski <steev@kali.org>
>> 
>> 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")
>> 
>>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
>>>> ---
>>>>    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(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index cab387f..0071a4d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -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));
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> index d6717d6..f7bcc0a 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> @@ -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).
>>>>     */
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>>> index cd40788..8cd712c 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>>>> @@ -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);
>>>> 
>>> 
>>> 
>>> --
>>> With best wishes
>>> Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-04-01 13:18       ` [Freedreno] " kalyan_t
@ 2021-04-01 13:31         ` Dmitry Baryshkov
  2021-04-02 11:42           ` kalyan_t
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-01 13:31 UTC (permalink / raw)
  To: Kalyan Thota
  Cc: Rob Clark,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Krishna Manikandan, linux-arm-msm, Daniel Hung-yu Wu,
	Linux Kernel Mailing List, dri-devel, Douglas Anderson,
	Matthias Kaehlcke, Michelle Dean, Steev Klimaszewski, freedreno,
	y

On Thu, 1 Apr 2021 at 16:19, <kalyan_t@codeaurora.org> wrote:
>
> On 2021-04-01 07:37, Dmitry Baryshkov wrote:
> > On 01/04/2021 01:47, Rob Clark wrote:
> >> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
> >> <dmitry.baryshkov@linaro.org> wrote:
> >>>
> >>> On 31/03/2021 14:27, Kalyan Thota wrote:
> >>>> 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.
> >>
> >> yes, we should, this keeps biting us on 845
> >
> > Done,
> > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/
>
> Hi Dmitry,
>
> https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/
>
> 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.

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


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

>
>
> Thanks,
> Kalyan
>
> >
> >>
> >>>>
> >>>> Reported-by: Steev Klimaszewski <steev@kali.org>
> >>
> >> 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")
> >>
> >>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
> >>>> ---
> >>>>    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(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> index cab387f..0071a4d 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> @@ -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));
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> >>>> index d6717d6..f7bcc0a 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> >>>> @@ -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).
> >>>>     */
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> >>>> index cd40788..8cd712c 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> >>>> @@ -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);
> >>>>
> >>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
  2021-04-01 13:31         ` Dmitry Baryshkov
@ 2021-04-02 11:42           ` kalyan_t
  0 siblings, 0 replies; 8+ messages in thread
From: kalyan_t @ 2021-04-02 11:42 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Rob Clark,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Krishna Manikandan, linux-arm-msm, Daniel Hung-yu Wu,
	Linux Kernel Mailing List, dri-devel, Douglas Anderson,
	Matthias Kaehlcke, Michelle Dean, Steev Klimaszewski, freedreno,
	y

On 2021-04-01 19:01, Dmitry Baryshkov wrote:
> On Thu, 1 Apr 2021 at 16:19, <kalyan_t@codeaurora.org> wrote:
>> 
>> On 2021-04-01 07:37, Dmitry Baryshkov wrote:
>> > On 01/04/2021 01:47, Rob Clark wrote:
>> >> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
>> >> <dmitry.baryshkov@linaro.org> wrote:
>> >>>
>> >>> On 31/03/2021 14:27, Kalyan Thota wrote:
>> >>>> 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.
>> >>
>> >> yes, we should, this keeps biting us on 845
>> >
>> > Done,
>> > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/
>> 
>> Hi Dmitry,
>> 
>> https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/
>> 
>> 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.

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

>> 
>> 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.
> 
>> 
>> 
>> Thanks,
>> Kalyan
>> 
>> >
>> >>
>> >>>>
>> >>>> Reported-by: Steev Klimaszewski <steev@kali.org>
>> >>
>> >> 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")
>> >>
>> >>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
>> >>>> ---
>> >>>>    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(-)
>> >>>>
>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> >>>> index cab387f..0071a4d 100644
>> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> >>>> @@ -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));
>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> >>>> index d6717d6..f7bcc0a 100644
>> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> >>>> @@ -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).
>> >>>>     */
>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> >>>> index cd40788..8cd712c 100644
>> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> >>>> @@ -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);
>> >>>>
>> >>>
>> >>>
>> >>> --
>> >>> With best wishes
>> >>> Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-02 11:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-31 11:27 [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Kalyan Thota
2021-03-31 15:55 ` Doug Anderson
2021-03-31 16:03 ` Dmitry Baryshkov
2021-03-31 22:47   ` Rob Clark
2021-04-01  2:07     ` Dmitry Baryshkov
2021-04-01 13:18       ` [Freedreno] " kalyan_t
2021-04-01 13:31         ` Dmitry Baryshkov
2021-04-02 11:42           ` kalyan_t

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