| Message ID | 20240326221026.9012-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915/cdclk: Fix CDCLK programming order when pipes are active | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > Syrjala > Sent: Wednesday, March 27, 2024 3:40 AM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH v2 1/3] drm/i915/cdclk: Fix CDCLK programming order when > pipes are active > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we always reprogram CDCLK from the > intel_set_cdclk_pre_plane_update() when using squahs/crawl. Nitpick: Typo in squash Change Looks Good to me. Reviewed-by: Uma Shankar <uma.shankar@intel.com> > 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. > > v2: Don't break the "must disable pipes case" > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > 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
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);