[PATCH v2 0/2] Update mdp clk to max supported value to support higher refresh rates

[PATCH v2 0/2] Update mdp clk to max supported value to support higher refresh rates

[Vinod Polimera]

*** BLURB HERE ***

Vinod Polimera (2):
  arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk
  drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table

 arch/arm64/boot/dts/qcom/sc7280.dtsi    | 9 ++-------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk

[Vinod Polimera]

Kernel clock driver assumes that initial rate is the
max rate for that clock and was not allowing it to scale
beyond the assigned clock value.

drop the assigned clock rate property and set it
during resume sequence with max value in the opp table.

Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")

Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index baf1653..408cf6c 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2856,9 +2856,6 @@
 				      "ahb",
 				      "core";
 
-			assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
-			assigned-clock-rates = <300000000>;
-
 			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
 			#interrupt-cells = <1>;
@@ -2892,11 +2889,9 @@
 					      "lut",
 					      "core",
 					      "vsync";
-				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
-						<&dispcc DISP_CC_MDSS_VSYNC_CLK>,
+				assigned-clocks = <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
 						<&dispcc DISP_CC_MDSS_AHB_CLK>;
-				assigned-clock-rates = <300000000>,
-							<19200000>,
+				assigned-clock-rates = <19200000>,
 							<19200000>;
 				operating-points-v2 = <&mdp_opp_table>;
 				power-domains = <&rpmhpd SC7280_CX>;
-- 
2.7.4

Re: [PATCH v2 1/2] arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk

[Stephen Boyd]

Quoting Vinod Polimera (2022-02-25 07:57:49)
> Kernel clock driver assumes that initial rate is the
> max rate for that clock and was not allowing it to scale
> beyond the assigned clock value.
>
> drop the assigned clock rate property and set it
> during resume sequence with max value in the opp table.
>
> Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")
>

There shouldn't be a newline here. The fixes tag should be directly
attached to the SoB area.

> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 ++-------

Can you also update sc7180.dtsi? Are there others SoCs that need an
update because they're using an OPP table?

Doesn't this need to come after the driver change?

[PATCH v2 2/2] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table

[Vinod Polimera]

use max clock during resume sequence from the opp table.
The clock will be scaled down when framework sends an update.

Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")

Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index d550f90..3288f52 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1319,6 +1319,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 	struct drm_device *ddev;
 	struct dss_module_power *mp = &dpu_kms->mp;
 	int i;
+	unsigned long max_freq = ULONG_MAX;
 
 	ddev = dpu_kms->dev;
 
@@ -1333,6 +1334,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 		return rc;
 	}
 
+	dev_pm_opp_find_freq_floor(dev, &max_freq);
+	dev_pm_opp_set_rate(dev, max_freq);
 	dpu_vbif_init_memtypes(dpu_kms);
 
 	drm_for_each_encoder(encoder, ddev)
-- 
2.7.4

Re: [PATCH v2 2/2] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table

[Stephen Boyd]

Quoting Vinod Polimera (2022-02-25 07:57:50)
> use max clock during resume sequence from the opp table.

s/use/Use/

> The clock will be scaled down when framework sends an update.
>
> Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")

Presumably this is the wrong fixes tag, see below.

>
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index d550f90..3288f52 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1319,6 +1319,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
>         struct drm_device *ddev;
>         struct dss_module_power *mp = &dpu_kms->mp;
>         int i;
> +       unsigned long max_freq = ULONG_MAX;
>
>         ddev = dpu_kms->dev;
>
> @@ -1333,6 +1334,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
>                 return rc;
>         }
>
> +       dev_pm_opp_find_freq_floor(dev, &max_freq);

This isn't exactly a cheap thing to do every runtime resume. Please get
the max frequency during probe and stash it somewhere to avoid making
this call over and over again.

> +       dev_pm_opp_set_rate(dev, max_freq);

This is entirely new. The assigned clock rates from DT aren't set during
runtime resume, only during device probe. My question is, if this is
needed now then does it mean we've been running the clk at low speed
after the first runtime suspend and never been pushing it back up again?

>         dpu_vbif_init_memtypes(dpu_kms);
>
>         drm_for_each_encoder(encoder, ddev)

RE: [PATCH v2 2/2] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table

[Vinod Polimera]

> -----Original Message-----
> From: Stephen Boyd <swboyd@chromium.org>
> Sent: Friday, February 25, 2022 11:43 PM
> To: quic_vpolimer <quic_vpolimer@quicinc.com>;
> devicetree@vger.kernel.org; dri-devel@lists.freedesktop.org;
> freedreno@lists.freedesktop.org; linux-arm-msm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com;
> dianders@chromium.org; quic_kalyant <quic_kalyant@quicinc.com>
> Subject: Re: [PATCH v2 2/2] drm/msm/disp/dpu1: set mdp clk to the
> maximum frequency in opp table
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> Quoting Vinod Polimera (2022-02-25 07:57:50)
> > use max clock during resume sequence from the opp table.
> 
> s/use/Use/
> 
> > The clock will be scaled down when framework sends an update.
> >
> > Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")
> 
> Presumably this is the wrong fixes tag, see below.
> 
> >
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index d550f90..3288f52 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1319,6 +1319,7 @@ static int __maybe_unused
> dpu_runtime_resume(struct device *dev)
> >         struct drm_device *ddev;
> >         struct dss_module_power *mp = &dpu_kms->mp;
> >         int i;
> > +       unsigned long max_freq = ULONG_MAX;
> >
> >         ddev = dpu_kms->dev;
> >
> > @@ -1333,6 +1334,8 @@ static int __maybe_unused
> dpu_runtime_resume(struct device *dev)
> >                 return rc;
> >         }
> >
> > +       dev_pm_opp_find_freq_floor(dev, &max_freq);
> 
> This isn't exactly a cheap thing to do every runtime resume. Please get the
> max frequency during probe and stash it somewhere to avoid making this call
> over and over again.
> 
> > +       dev_pm_opp_set_rate(dev, max_freq);
> 
> This is entirely new. The assigned clock rates from DT aren't set during
> runtime resume, only during device probe. My question is, if this is needed
> now then does it mean we've been running the clk at low speed after the
> first runtime suspend and never been pushing it back up again?
mdp clock rate is set based on the calculated value for each update and does not require to
set the clock to max value in opp table. So, dropping this patch.
> 
> >         dpu_vbif_init_memtypes(dpu_kms);
> >
> >         drm_for_each_encoder(encoder, ddev)