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