diff mbox series

[1/3] drm/i915/cdclk: Fix CDCLK programming order when pipes are active

Message ID 20240326203128.10259-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/cdclk: CDCLK fixes | expand

Commit Message

Ville Syrjälä March 26, 2024, 8:31 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we always reprogram CDCLK from the
intel_set_cdclk_post_plane_update() when using squahs/crawl.
The code only works correctly for the cd2x update or full
modeset cases, and it was simply never updated to deal with
squash/crawl.

If the CDCLK frequency is increasing we must reprogram it
before we do anything else that might depend on the new
higher frequency, and conversely we must not decrease
the frequency until everything that might still depend
on the old higher frequency has been dealt with.

So let us only consider the relationship of the old and
new CDCLK frequencies when determining where to do
the reprogramming, regarless of whether all pipes might
be off or not at the time.

If the CDCLK freqiency remains unchanges we may still have to
do the reprogramming to change the voltage_level. Currently
we do that from intel_set_cdclk_pre_plane_update() which
probably is the right choice most of the time. The only
counterexample is when the voltage_level needs to increase
due to a DDI port, but the CDCLK frequency is decreasing.
The current approach will not bump the voltage level up until
after the port has already been enabled, which is too late.
But we'll take care of that case separately.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Ville Syrjälä March 26, 2024, 9:22 p.m. UTC | #1 
On Tue, Mar 26, 2024 at 10:31:26PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we always reprogram CDCLK from the
> intel_set_cdclk_post_plane_update() when using squahs/crawl.
> The code only works correctly for the cd2x update or full
> modeset cases, and it was simply never updated to deal with
> squash/crawl.
> 
> If the CDCLK frequency is increasing we must reprogram it
> before we do anything else that might depend on the new
> higher frequency, and conversely we must not decrease
> the frequency until everything that might still depend
> on the old higher frequency has been dealt with.
> 
> So let us only consider the relationship of the old and
> new CDCLK frequencies when determining where to do
> the reprogramming, regarless of whether all pipes might
> be off or not at the time.
> 
> If the CDCLK freqiency remains unchanges we may still have to
> do the reprogramming to change the voltage_level. Currently
> we do that from intel_set_cdclk_pre_plane_update() which
> probably is the right choice most of the time. The only
> counterexample is when the voltage_level needs to increase
> due to a DDI port, but the CDCLK frequency is decreasing.
> The current approach will not bump the voltage level up until
> after the port has already been enabled, which is too late.
> But we'll take care of that case separately.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfc..49e2f09a796a 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2600,7 +2600,6 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>  		intel_atomic_get_old_cdclk_state(state);
>  	const struct intel_cdclk_state *new_cdclk_state =
>  		intel_atomic_get_new_cdclk_state(state);
> -	enum pipe pipe = new_cdclk_state->pipe;
>  
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2609,11 +2608,11 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_pre_notify(state);
>  
> -	if (pipe == INVALID_PIPE ||
> -	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>  
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
>  
> @@ -2632,7 +2631,6 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>  		intel_atomic_get_old_cdclk_state(state);
>  	const struct intel_cdclk_state *new_cdclk_state =
>  		intel_atomic_get_new_cdclk_state(state);
> -	enum pipe pipe = new_cdclk_state->pipe;
>  
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2641,11 +2639,11 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_post_notify(state);
>  
> -	if (pipe != INVALID_PIPE &&
> -	    old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> +	if (old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>  
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}

And naturally this will not work for the full modeset case.
We must do the cdclk programming in the pre hook in that
case. I suppose we'll need to actually track whether we're
going to do a cd2x/squash/crawl or a full modeset...
Gustavo Sousa March 26, 2024, 9:42 p.m. UTC | #2 
Quoting Ville Syrjala (2024-03-26 17:31:26-03:00)
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Currently we always reprogram CDCLK from the
>intel_set_cdclk_post_plane_update() when using squahs/crawl.

Hm... I'm not following this sentence. Looks like cdclk_state->pipe will
be INVALID_PIPE unless it is a cd2x update, right?

Did you mean intel_set_cdclk_pre_plane_update() above? That would make
sense, because it seems we would be doing the update there even when
decreasing the cdclk because of the condition pipe == INVALID_PIPE.

--
Gustavo Sousa

>The code only works correctly for the cd2x update or full
>modeset cases, and it was simply never updated to deal with
>squash/crawl.
>
>If the CDCLK frequency is increasing we must reprogram it
>before we do anything else that might depend on the new
>higher frequency, and conversely we must not decrease
>the frequency until everything that might still depend
>on the old higher frequency has been dealt with.
>
>So let us only consider the relationship of the old and
>new CDCLK frequencies when determining where to do
>the reprogramming, regarless of whether all pipes might
>be off or not at the time.
>
>If the CDCLK freqiency remains unchanges we may still have to
>do the reprogramming to change the voltage_level. Currently
>we do that from intel_set_cdclk_pre_plane_update() which
>probably is the right choice most of the time. The only
>counterexample is when the voltage_level needs to increase
>due to a DDI port, but the CDCLK frequency is decreasing.
>The current approach will not bump the voltage level up until
>after the port has already been enabled, which is too late.
>But we'll take care of that case separately.
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>index 31aaa9780dfc..49e2f09a796a 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>@@ -2600,7 +2600,6 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>                 intel_atomic_get_old_cdclk_state(state);
>         const struct intel_cdclk_state *new_cdclk_state =
>                 intel_atomic_get_new_cdclk_state(state);
>-        enum pipe pipe = new_cdclk_state->pipe;
> 
>         if (!intel_cdclk_changed(&old_cdclk_state->actual,
>                                  &new_cdclk_state->actual))
>@@ -2609,11 +2608,11 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>         if (IS_DG2(i915))
>                 intel_cdclk_pcode_pre_notify(state);
> 
>-        if (pipe == INVALID_PIPE ||
>-            old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>+        if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>                 drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> 
>-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>+                intel_set_cdclk(i915, &new_cdclk_state->actual,
>+                                new_cdclk_state->pipe);
>         }
> }
> 
>@@ -2632,7 +2631,6 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>                 intel_atomic_get_old_cdclk_state(state);
>         const struct intel_cdclk_state *new_cdclk_state =
>                 intel_atomic_get_new_cdclk_state(state);
>-        enum pipe pipe = new_cdclk_state->pipe;
> 
>         if (!intel_cdclk_changed(&old_cdclk_state->actual,
>                                  &new_cdclk_state->actual))
>@@ -2641,11 +2639,11 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>         if (IS_DG2(i915))
>                 intel_cdclk_pcode_post_notify(state);
> 
>-        if (pipe != INVALID_PIPE &&
>-            old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>+        if (old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>                 drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> 
>-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>+                intel_set_cdclk(i915, &new_cdclk_state->actual,
>+                                new_cdclk_state->pipe);
>         }
> }
> 
>-- 
>2.43.2
>
Ville Syrjälä March 26, 2024, 9:50 p.m. UTC | #3 
On Tue, Mar 26, 2024 at 06:42:09PM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjala (2024-03-26 17:31:26-03:00)
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Currently we always reprogram CDCLK from the
> >intel_set_cdclk_post_plane_update() when using squahs/crawl.
> 
> Hm... I'm not following this sentence. Looks like cdclk_state->pipe will
> be INVALID_PIPE unless it is a cd2x update, right?
> 
> Did you mean intel_set_cdclk_pre_plane_update() above? That would make
> sense, because it seems we would be doing the update there even when
> decreasing the cdclk because of the condition pipe == INVALID_PIPE.

Yeah, apparently copy pasted in the wrong function name here.

> 
> --
> Gustavo Sousa
> 
> >The code only works correctly for the cd2x update or full
> >modeset cases, and it was simply never updated to deal with
> >squash/crawl.
> >
> >If the CDCLK frequency is increasing we must reprogram it
> >before we do anything else that might depend on the new
> >higher frequency, and conversely we must not decrease
> >the frequency until everything that might still depend
> >on the old higher frequency has been dealt with.
> >
> >So let us only consider the relationship of the old and
> >new CDCLK frequencies when determining where to do
> >the reprogramming, regarless of whether all pipes might
> >be off or not at the time.
> >
> >If the CDCLK freqiency remains unchanges we may still have to
> >do the reprogramming to change the voltage_level. Currently
> >we do that from intel_set_cdclk_pre_plane_update() which
> >probably is the right choice most of the time. The only
> >counterexample is when the voltage_level needs to increase
> >due to a DDI port, but the CDCLK frequency is decreasing.
> >The current approach will not bump the voltage level up until
> >after the port has already been enabled, which is too late.
> >But we'll take care of that case separately.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >index 31aaa9780dfc..49e2f09a796a 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >@@ -2600,7 +2600,6 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >                 intel_atomic_get_old_cdclk_state(state);
> >         const struct intel_cdclk_state *new_cdclk_state =
> >                 intel_atomic_get_new_cdclk_state(state);
> >-        enum pipe pipe = new_cdclk_state->pipe;
> > 
> >         if (!intel_cdclk_changed(&old_cdclk_state->actual,
> >                                  &new_cdclk_state->actual))
> >@@ -2609,11 +2608,11 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >         if (IS_DG2(i915))
> >                 intel_cdclk_pcode_pre_notify(state);
> > 
> >-        if (pipe == INVALID_PIPE ||
> >-            old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> >+        if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> >                 drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > 
> >-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> >+                intel_set_cdclk(i915, &new_cdclk_state->actual,
> >+                                new_cdclk_state->pipe);
> >         }
> > }
> > 
> >@@ -2632,7 +2631,6 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> >                 intel_atomic_get_old_cdclk_state(state);
> >         const struct intel_cdclk_state *new_cdclk_state =
> >                 intel_atomic_get_new_cdclk_state(state);
> >-        enum pipe pipe = new_cdclk_state->pipe;
> > 
> >         if (!intel_cdclk_changed(&old_cdclk_state->actual,
> >                                  &new_cdclk_state->actual))
> >@@ -2641,11 +2639,11 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> >         if (IS_DG2(i915))
> >                 intel_cdclk_pcode_post_notify(state);
> > 
> >-        if (pipe != INVALID_PIPE &&
> >-            old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> >+        if (old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> >                 drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > 
> >-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> >+                intel_set_cdclk(i915, &new_cdclk_state->actual,
> >+                                new_cdclk_state->pipe);
> >         }
> > }
> > 
> >-- 
> >2.43.2
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 31aaa9780dfc..49e2f09a796a 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2600,7 +2600,6 @@  intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
 		intel_atomic_get_old_cdclk_state(state);
 	const struct intel_cdclk_state *new_cdclk_state =
 		intel_atomic_get_new_cdclk_state(state);
-	enum pipe pipe = new_cdclk_state->pipe;
 
 	if (!intel_cdclk_changed(&old_cdclk_state->actual,
 				 &new_cdclk_state->actual))
@@ -2609,11 +2608,11 @@  intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
 	if (IS_DG2(i915))
 		intel_cdclk_pcode_pre_notify(state);
 
-	if (pipe == INVALID_PIPE ||
-	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
+	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
 		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
 
-		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
+		intel_set_cdclk(i915, &new_cdclk_state->actual,
+				new_cdclk_state->pipe);
 	}
 }
 
@@ -2632,7 +2631,6 @@  intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
 		intel_atomic_get_old_cdclk_state(state);
 	const struct intel_cdclk_state *new_cdclk_state =
 		intel_atomic_get_new_cdclk_state(state);
-	enum pipe pipe = new_cdclk_state->pipe;
 
 	if (!intel_cdclk_changed(&old_cdclk_state->actual,
 				 &new_cdclk_state->actual))
@@ -2641,11 +2639,11 @@  intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
 	if (IS_DG2(i915))
 		intel_cdclk_pcode_post_notify(state);
 
-	if (pipe != INVALID_PIPE &&
-	    old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
+	if (old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
 		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
 
-		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
+		intel_set_cdclk(i915, &new_cdclk_state->actual,
+				new_cdclk_state->pipe);
 	}
 }