Quoting Ville Syrjala (2024-03-27 14:45:32-03:00) >From: Ville Syrjälä <[email protected]> > >Currently we always reprogram CDCLK from the >intel_set_cdclk_pre_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. > >Since cdclk_state->pipe is only relevant when doing a cd2x >update we can't use it to determine the correct sequence >during squash/crawl. To that end introduce cdclk_state->disable_pipes >which simply indicates that we must perform the update >while the pipes are disable (ie. during >intel_set_cdclk_pre_plane_update()). Otherwise we use the >same old vs. new CDCLK frequency comparsiong as for cd2x >updates. > >The only remaining problem case is when the voltage_level >needs to increase due to a DDI port, but the CDCLK frequency >is decreasing (and not all pipes are being disabled). 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.
Yep. Maybe that's another reason to have that logic detached from the cdclk sequence in the future? Another one mentioned in an earlier discussion[1] would be the case where voltage level changes without changes to CDCLK. [1] https://lore.kernel.org/intel-gfx/[email protected]/ > >v2: Don't break the "must disable pipes case" > >Signed-off-by: Ville Syrjälä <[email protected]> Reviewed-by: Gustavo Sousa <[email protected]> >--- > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------ > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > 2 files changed, 12 insertions(+), 6 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >b/drivers/gpu/drm/i915/display/intel_cdclk.c >index 31aaa9780dfc..619529dba095 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,12 @@ 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 || >+ if (new_cdclk_state->disable_pipes || > 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 +2632,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 +2640,12 @@ 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 && >+ if (!new_cdclk_state->disable_pipes && > 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); > } > } > >@@ -3124,6 +3124,7 @@ static struct intel_global_state >*intel_cdclk_duplicate_state(struct intel_globa > return NULL; > > cdclk_state->pipe = INVALID_PIPE; >+ cdclk_state->disable_pipes = false; > > return &cdclk_state->base; > } >@@ -3316,6 +3317,8 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state >*state) > if (ret) > return ret; > >+ new_cdclk_state->disable_pipes = true; >+ > drm_dbg_kms(&dev_priv->drm, > "Modeset required for cdclk change\n"); > } >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h >b/drivers/gpu/drm/i915/display/intel_cdclk.h >index bc8f86e292d8..2843fc091086 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h >@@ -53,6 +53,9 @@ struct intel_cdclk_state { > > /* bitmask of active pipes */ > u8 active_pipes; >+ >+ /* update cdclk with pipes disabled */ >+ bool disable_pipes; > }; > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); >-- >2.43.2 >
