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

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

[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 vote on the mdp clock as per
calculated value during the usecase.

Changes in v2:
- Remove assigned-clock-rate property and set mdp clk during resume sequence.
- Add fixes tag.

Changes in v3:
- Remove extra line after fixes tag.(Stephen Boyd)
- Add similar changes for sc7180, sdm845 which uses opp table for voting mdp clk.(Stephen Boyd)
- Drop patch: "drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table"

Changes in v4:
- Add similar change for sm8250.(Dmitry)

Vinod Polimera (4):
  arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk
  arm64/dts/qcom/sc7180: remove assigned-clock-rate property for mdp clk
  arm64/dts/qcom/sdm845: remove assigned-clock-rate property for mdp clk
  arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++-------
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 ++-------
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 ++-------
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 9 ++-------
 4 files changed, 8 insertions(+), 28 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] 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 vote on the mdp clock as per
calculated value during the usecase.

Changes in v2:
- Remove assigned-clock-rate property and set mdp clk during resume sequence.
- Add fixes tag.

Changes in v3:
- Remove extra line after fixes tag.(Stephen Boyd)

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

[PATCH v4 2/4] arm64/dts/qcom/sc7180: 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 vote on the mdp clock as per
calculated value during the usecase.

Fixes: a3db7ad1af("arm64: dts: qcom: sc7180: add display dt nodes")
Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index e1c46b8..eaab746 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -2900,9 +2900,6 @@
 				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
 			clock-names = "iface", "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>;
@@ -2932,12 +2929,10 @@
 					 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
 				clock-names = "bus", "iface", "rot", "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_ROT_CLK>,
 						  <&dispcc DISP_CC_MDSS_AHB_CLK>;
-				assigned-clock-rates = <300000000>,
-						       <19200000>,
+				assigned-clock-rates = <19200000>,
 						       <19200000>,
 						       <19200000>;
 				operating-points-v2 = <&mdp_opp_table>;
-- 
2.7.4

[PATCH v4 3/4] arm64/dts/qcom/sdm845: 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 vote on the mdp clock as per
calculated value during the usecase.

Fixes: 08c2a076d1("arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file")
Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0d6286d..80dc486 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4181,9 +4181,6 @@
 				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
 			clock-names = "iface", "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>;
@@ -4214,10 +4211,8 @@
 					 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
 				clock-names = "gcc-bus", "iface", "bus", "core", "vsync";
 
-				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
-						  <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
-				assigned-clock-rates = <300000000>,
-						       <19200000>;
+				assigned-clocks = <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
+				assigned-clock-rates = <19200000>;
 				operating-points-v2 = <&mdp_opp_table>;
 				power-domains = <&rpmhpd SDM845_CX>;
 
-- 
2.7.4

[PATCH v4 4/4] arm64/dts/qcom/sm8250: 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 vote on the mdp clock as per
calculated value during the usecase.

Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index fdaf303..2105eb7 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -3164,9 +3164,6 @@
 				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
 			clock-names = "iface", "bus", "nrt_bus", "core";
 
-			assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
-			assigned-clock-rates = <460000000>;
-
 			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
 			#interrupt-cells = <1>;
@@ -3191,10 +3188,8 @@
 					 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
 				clock-names = "iface", "bus", "core", "vsync";
 
-				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
-						  <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
-				assigned-clock-rates = <460000000>,
-						       <19200000>;
+				assigned-clocks = <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
+				assigned-clock-rates = <19200000>;
 
 				operating-points-v2 = <&mdp_opp_table>;
 				power-domains = <&rpmhpd SM8250_MMCX>;
-- 
2.7.4

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

[Stephen Boyd]

Quoting Vinod Polimera (2022-03-03 01:39:58)
> 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 vote on the mdp clock as per
> calculated value during the usecase.
>
> Changes in v2:
> - Remove assigned-clock-rate property and set mdp clk during resume sequence.
> - Add fixes tag.
>
> Changes in v3:
> - Remove extra line after fixes tag.(Stephen Boyd)
>

This changelog goes below triple dash when they aren't intended for drm.

> Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 2/4] arm64/dts/qcom/sc7180: remove assigned-clock-rate property for mdp clk

[Stephen Boyd]

Quoting Vinod Polimera (2022-03-03 01:39:59)
> 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 vote on the mdp clock as per
> calculated value during the usecase.
>
> Fixes: a3db7ad1af("arm64: dts: qcom: sc7180: add display dt nodes")
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 3/4] arm64/dts/qcom/sdm845: remove assigned-clock-rate property for mdp clk

[Stephen Boyd]

Quoting Vinod Polimera (2022-03-03 01:40:00)
> 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 vote on the mdp clock as per
> calculated value during the usecase.
>
> Fixes: 08c2a076d1("arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file")
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Stephen Boyd]

Quoting Vinod Polimera (2022-03-03 01:40:01)
> 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 vote on the mdp clock as per
> calculated value during the usecase.
>
> Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

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

[Doug Anderson]

Hi,

On Thu, Mar 3, 2022 at 1:40 AM Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>
> 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 vote on the mdp clock as per
> calculated value during the usecase.

I see the "Drop the assigned clock rate property" part, but where is
the "and vote on the mdp clock" part? Did it already land or
something? I definitely see that commit 5752c921d267 ("drm/msm/dpu:
simplify clocks handling") changed a bunch of this but it looks like
dpu_core_perf_init() still sets "max_core_clk_rate" to whatever the
clock was at bootup. I assume you need to modify that function to call
into the OPP layer to find the max frequency?


> Changes in v2:
> - Remove assigned-clock-rate property and set mdp clk during resume sequence.
> - Add fixes tag.
>
> Changes in v3:
> - Remove extra line after fixes tag.(Stephen Boyd)
>
> Fixes: 62fbdce91("arm64: dts: qcom: sc7280: add display dt nodes")

Having a "Fixes" is good, but presumably you need a code change along
with this, right? Otherwise if someone picks this back to stable then
they'll end up breaking, right? We need to tag / note that _somehow_.

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Dmitry Baryshkov]

On Thu, 3 Mar 2022 at 12:40, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>
> 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 vote on the mdp clock as per
> calculated value during the usecase.
>
> Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")

Please remove the Fixes tags from all commits. Otherwise the patches
might be picked up into earlier kernels, which do not have a patch
adding a vote on the MDP clock.

> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index fdaf303..2105eb7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -3164,9 +3164,6 @@
>                                  <&dispcc DISP_CC_MDSS_MDP_CLK>;
>                         clock-names = "iface", "bus", "nrt_bus", "core";
>
> -                       assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> -                       assigned-clock-rates = <460000000>;
> -
>                         interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
>                         interrupt-controller;
>                         #interrupt-cells = <1>;
> @@ -3191,10 +3188,8 @@
>                                          <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
>                                 clock-names = "iface", "bus", "core", "vsync";
>
> -                               assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> -                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> -                               assigned-clock-rates = <460000000>,
> -                                                      <19200000>;
> +                               assigned-clocks = <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> +                               assigned-clock-rates = <19200000>;
>
>                                 operating-points-v2 = <&mdp_opp_table>;
>                                 power-domains = <&rpmhpd SM8250_MMCX>;
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> On Thu, 3 Mar 2022 at 12:40, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
> >
> > 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 vote on the mdp clock as per
> > calculated value during the usecase.
> >
> > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
>
> Please remove the Fixes tags from all commits. Otherwise the patches
> might be picked up into earlier kernels, which do not have a patch
> adding a vote on the MDP clock.

What patch is that? The Fixes tag could point to that commit.

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Dmitry Baryshkov]

On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
> > >
> > > 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 vote on the mdp clock as per
> > > calculated value during the usecase.
> > >
> > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
> >
> > Please remove the Fixes tags from all commits. Otherwise the patches
> > might be picked up into earlier kernels, which do not have a patch
> > adding a vote on the MDP clock.
>
> What patch is that? The Fixes tag could point to that commit.

Please correct me if I'm wrong.
Currently the dtsi enforces bumping the MDP clock when the mdss device
is being probed and when the dpu device is being probed.
Later during the DPU lifetime the core_perf would change the clock's
rate as it sees fit according to the CRTC requirements.

However it would happen only when the during the
dpu_crtc_atomic_flush(), before we call this function, the MDP clock
is left in the undetermined state. The power rails controlled by the
opp table are left in the undetermined state.

I suppose that during the dpu_bind we should bump the clock to the max
possible freq and let dpu_core_perf handle it afterwards.


--
With best wishes
Dmitry

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Doug Anderson]

Hi,

On Thu, Mar 3, 2022 at 4:16 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
> > > >
> > > > 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 vote on the mdp clock as per
> > > > calculated value during the usecase.
> > > >
> > > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
> > >
> > > Please remove the Fixes tags from all commits. Otherwise the patches
> > > might be picked up into earlier kernels, which do not have a patch
> > > adding a vote on the MDP clock.
> >
> > What patch is that? The Fixes tag could point to that commit.
>
> Please correct me if I'm wrong.
> Currently the dtsi enforces bumping the MDP clock when the mdss device
> is being probed and when the dpu device is being probed.
> Later during the DPU lifetime the core_perf would change the clock's
> rate as it sees fit according to the CRTC requirements.

"Currently" means _before_ ${SUBJECT} patch lands, right? Since
${SUBJECT} patch is removing the bump to max.


> However it would happen only when the during the
> dpu_crtc_atomic_flush(), before we call this function, the MDP clock
> is left in the undetermined state. The power rails controlled by the
> opp table are left in the undetermined state.
>
> I suppose that during the dpu_bind we should bump the clock to the max
> possible freq and let dpu_core_perf handle it afterwards.

Definitely feels like seeing the clock to something predictable during
the initial probe makes sense. If it's just for the initial probe then
setting it to max (based on the opp table) seems fine. I think an
earlier version of this series set it to max every time we did runtime
resume. We'd have to have a good reason to do that.

-Doug

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Dmitry Baryshkov]

On Sat, 5 Mar 2022 at 00:49, Doug Anderson <dianders@chromium.org> wrote:
> On Thu, Mar 3, 2022 at 4:16 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > > > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
> > > > >
> > > > > 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 vote on the mdp clock as per
> > > > > calculated value during the usecase.
> > > > >
> > > > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display system nodes")
> > > >
> > > > Please remove the Fixes tags from all commits. Otherwise the patches
> > > > might be picked up into earlier kernels, which do not have a patch
> > > > adding a vote on the MDP clock.
> > >
> > > What patch is that? The Fixes tag could point to that commit.
> >
> > Please correct me if I'm wrong.
> > Currently the dtsi enforces bumping the MDP clock when the mdss device
> > is being probed and when the dpu device is being probed.
> > Later during the DPU lifetime the core_perf would change the clock's
> > rate as it sees fit according to the CRTC requirements.
>
> "Currently" means _before_ ${SUBJECT} patch lands, right? Since
> ${SUBJECT} patch is removing the bump to max.

Yes. 'Before this patch'.

>
>
> > However it would happen only when the during the
> > dpu_crtc_atomic_flush(), before we call this function, the MDP clock
> > is left in the undetermined state. The power rails controlled by the
> > opp table are left in the undetermined state.
> >
> > I suppose that during the dpu_bind we should bump the clock to the max
> > possible freq and let dpu_core_perf handle it afterwards.
>
> Definitely feels like seeing the clock to something predictable during
> the initial probe makes sense. If it's just for the initial probe then
> setting it to max (based on the opp table) seems fine.

Vinod, could you please implement it?

> I think an
> earlier version of this series set it to max every time we did runtime
> resume. We'd have to have a good reason to do that.

Yes, this is correct. Based on the comments I had the impression that
there was a suggestion that the place for the calls was wrong. Most
probably I was instead projecting my own thoughts.

-- 
With best wishes
Dmitry

RE: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Vinod Polimera]

> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Sat, 5 Mar 2022 at 00:49, Doug Anderson <dianders@chromium.org>
> wrote:
> > On Thu, Mar 3, 2022 at 4:16 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org>
> wrote:
> > > >
> > > > Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > > > > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera
> <quic_vpolimer@quicinc.com> wrote:
> > > > > >
> > > > > > 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 vote on the mdp clock as
> per
> > > > > > calculated value during the usecase.
> > > > > >
> > > > > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display
> system nodes")
> > > > >
> > > > > Please remove the Fixes tags from all commits. Otherwise the
> patches
> > > > > might be picked up into earlier kernels, which do not have a patch
> > > > > adding a vote on the MDP clock.
> > > >
> > > > What patch is that? The Fixes tag could point to that commit.
> > >
> > > Please correct me if I'm wrong.
> > > Currently the dtsi enforces bumping the MDP clock when the mdss
> device
> > > is being probed and when the dpu device is being probed.
> > > Later during the DPU lifetime the core_perf would change the clock's
> > > rate as it sees fit according to the CRTC requirements.
> >
> > "Currently" means _before_ ${SUBJECT} patch lands, right? Since
> > ${SUBJECT} patch is removing the bump to max.
> 
> Yes. 'Before this patch'.
> 
> >
> >
> > > However it would happen only when the during the
> > > dpu_crtc_atomic_flush(), before we call this function, the MDP clock
> > > is left in the undetermined state. The power rails controlled by the
> > > opp table are left in the undetermined state.
> > >
> > > I suppose that during the dpu_bind we should bump the clock to the max
> > > possible freq and let dpu_core_perf handle it afterwards.
> >
> > Definitely feels like seeing the clock to something predictable during
> > the initial probe makes sense. If it's just for the initial probe then
> > setting it to max (based on the opp table) seems fine.
> 
> Vinod, could you please implement it?
> 
> > I think an
> > earlier version of this series set it to max every time we did runtime
> > resume. We'd have to have a good reason to do that.
> 
> Yes, this is correct. Based on the comments I had the impression that
> there was a suggestion that the place for the calls was wrong. Most
> probably I was instead projecting my own thoughts.
> 
I had discussed internally with the team. Traditionally, mdp clk vote during
probe/bind is required when display is turned on in bootloader and persists
till first update in kernel. As in chromebook, timing engine will be turned 
off during depthcharge exit and as there is no display transition from 
bootloader to kernel, mdp clk can be voted based on the calculated value 
during framework update and does not required vote during probe/bind.

Thanks,
Vinod.
> --
> With best wishes
> Dmitry

Re: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Dmitry Baryshkov]

On Mon, 7 Mar 2022 at 19:05, Vinod Polimera <vpolimer@qti.qualcomm.com> wrote:
>
> > WARNING: This email originated from outside of Qualcomm. Please be wary
> > of any links or attachments, and do not enable macros.
> >
> > On Sat, 5 Mar 2022 at 00:49, Doug Anderson <dianders@chromium.org>
> > wrote:
> > > On Thu, Mar 3, 2022 at 4:16 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org>
> > wrote:
> > > > >
> > > > > Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > > > > > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera
> > <quic_vpolimer@quicinc.com> wrote:
> > > > > > >
> > > > > > > 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 vote on the mdp clock as
> > per
> > > > > > > calculated value during the usecase.
> > > > > > >
> > > > > > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display
> > system nodes")
> > > > > >
> > > > > > Please remove the Fixes tags from all commits. Otherwise the
> > patches
> > > > > > might be picked up into earlier kernels, which do not have a patch
> > > > > > adding a vote on the MDP clock.
> > > > >
> > > > > What patch is that? The Fixes tag could point to that commit.
> > > >
> > > > Please correct me if I'm wrong.
> > > > Currently the dtsi enforces bumping the MDP clock when the mdss
> > device
> > > > is being probed and when the dpu device is being probed.
> > > > Later during the DPU lifetime the core_perf would change the clock's
> > > > rate as it sees fit according to the CRTC requirements.
> > >
> > > "Currently" means _before_ ${SUBJECT} patch lands, right? Since
> > > ${SUBJECT} patch is removing the bump to max.
> >
> > Yes. 'Before this patch'.
> >
> > >
> > >
> > > > However it would happen only when the during the
> > > > dpu_crtc_atomic_flush(), before we call this function, the MDP clock
> > > > is left in the undetermined state. The power rails controlled by the
> > > > opp table are left in the undetermined state.
> > > >
> > > > I suppose that during the dpu_bind we should bump the clock to the max
> > > > possible freq and let dpu_core_perf handle it afterwards.
> > >
> > > Definitely feels like seeing the clock to something predictable during
> > > the initial probe makes sense. If it's just for the initial probe then
> > > setting it to max (based on the opp table) seems fine.
> >
> > Vinod, could you please implement it?
> >
> > > I think an
> > > earlier version of this series set it to max every time we did runtime
> > > resume. We'd have to have a good reason to do that.
> >
> > Yes, this is correct. Based on the comments I had the impression that
> > there was a suggestion that the place for the calls was wrong. Most
> > probably I was instead projecting my own thoughts.
> >
> I had discussed internally with the team. Traditionally, mdp clk vote during
> probe/bind is required when display is turned on in bootloader and persists
> till first update in kernel.

Not each and every board has a display setup in the bootloader. For
example the RB5 I have here doesn't support setting up the display.
Not to mention that we should tell Linux, which vote is cast,
otherwise the .sync_state can turn respective votes off.

> As in chromebook, timing engine will be turned
> off during depthcharge exit and as there is no display transition from
> bootloader to kernel, mdp clk can be voted based on the calculated value
> during framework update and does not required vote during probe/bind.

Generally Linux should not depend on the bootloader setup. You can not
be sure. What if we kexec next kernel?

-- 
With best wishes
Dmitry