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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel