Re: [PATCH 11/13] drm/i915: Implement vblank synchronized MBUS join changes
On Fri, Mar 29, 2024 at 03:15:02PM -0300, Gustavo Sousa wrote: > Quoting Ville Syrjala (2024-03-27 14:45:42-03:00) > >@@ -3663,24 +3659,42 @@ static void > >intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state > > intel_atomic_get_old_dbuf_state(state); > > const struct intel_dbuf_state *new_dbuf_state = > > intel_atomic_get_new_dbuf_state(state); > >+int mdclk_cdclk_ratio; > > > >-if (DISPLAY_VER(i915) >= 20 && > >-old_dbuf_state->mdclk_cdclk_ratio != > >new_dbuf_state->mdclk_cdclk_ratio) { > >-/* > >- * For Xe2LPD and beyond, when there is a change in the > >ratio > >- * between MDCLK and CDCLK, updates to related registers > >need to > >- * happen at a specific point in the CDCLK change sequence. > >In > >- * that case, we defer to the call to > >- * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > >- */ > >-return; > >+if (intel_cdclk_is_decreasing_later(state)) { > >+/* cdclk/mdclk will be changed later by > >intel_set_cdclk_post_plane_update() */ > >+mdclk_cdclk_ratio = old_dbuf_state->mdclk_cdclk_ratio; > >+} else { > >+/* cdclk/mdclk already changed by > >intel_set_cdclk_pre_plane_update() */ > >+mdclk_cdclk_ratio = new_dbuf_state->mdclk_cdclk_ratio; > > } > > > >-intel_dbuf_mdclk_cdclk_ratio_update(i915, > >new_dbuf_state->mdclk_cdclk_ratio, > >+intel_dbuf_mdclk_cdclk_ratio_update(i915, mdclk_cdclk_ratio, > > new_dbuf_state->joined_mbus); > > I get the feeling that this part actually belongs to the previous patch. Hmm, right. In fact I think it can just be its own patch. I'll carve it out. -- Ville Syrjälä Intel
Re: [PATCH 11/13] drm/i915: Implement vblank synchronized MBUS join changes
Quoting Ville Syrjala (2024-03-27 14:45:42-03:00) >From: Stanislav Lisovskiy > >Currently we can't change MBUS join status without doing a modeset, >because we are lacking mechanism to synchronize those with vblank. >However then this means that we can't do a fastset, if there is a need >to change MBUS join state. Fix that by implementing such change. >We already call correspondent check and update at pre_plane dbuf update, >so the only thing left is to have a non-modeset version of that. >If active pipes stay the same then fastset is possible and only MBUS >join state/ddb allocation updates would be committed. > >The full mbus/cdclk sequence will look as follows: >1. disable pipes >2. increase cdclk if necessary > 2.1 reprogram cdclk > 2.2 update dbuf tracker value >3. enable mbus joining if necessary > 3.1 update mbus_ctl > 3.2 update dbuf tracker value >4. reallocate dbuf for planes on active pipes >5. disable mbus joining if necessary > 5.1 update dbuf tracker value > 5.2 update mbus_ctl >6. enable pipes >7. decrease cdclk if necessary > 7.1 update dbuf tracker value > 7.2 reprogram cdclk > >And in order to keep things in sync we need: >Step 2: >- mbus_join == old >- mdclk/cdclk ratio == new >Step 3: >- mbus_join == new >- mdclk/cdclk ratio == old when cdclk is changing in step 7 >- mdclk/cdclk ratio == new when cdclk is changing in step 2 >Step 5: >- mbus_join == new >- mdclk/cdclk ratio == old when cdclk is changing in step 7 >- mdclk/cdclk ratio == new when cdclk is changing in step 2 >Step 7: >- mbus_join == new >- mdclk/cdclk ratio == new > >v2: - Removed redundant parentheses(Ville Syrjälä) >- Constified new_crtc_state in intel_mbus_joined_pipe(Ville Syrjälä) >- Removed pipe_select variable(Ville Syrjälä) >[v3: vsyrjala: Correctly sequence vs. cdclk updates, > properly describe the full sequence, > shuffle code around to make the diff more legible, > streamline a few things] > >Signed-off-by: Stanislav Lisovskiy >Co-developed-by: Ville Syrjälä >Signed-off-by: Ville Syrjälä >--- > drivers/gpu/drm/i915/display/intel_cdclk.c | 11 ++ > drivers/gpu/drm/i915/display/intel_cdclk.h | 1 + > drivers/gpu/drm/i915/display/intel_display.c | 5 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 141 --- > drivers/gpu/drm/i915/display/skl_watermark.h | 3 +- > 5 files changed, 112 insertions(+), 49 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >b/drivers/gpu/drm/i915/display/intel_cdclk.c >index 4024118a7ffb..66c161d7b485 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >@@ -2576,6 +2576,17 @@ static void intel_cdclk_pcode_post_notify(struct >intel_atomic_state *state) >update_cdclk, update_pipe_count); > } > >+bool intel_cdclk_is_decreasing_later(struct intel_atomic_state *state) >+{ >+const struct intel_cdclk_state *old_cdclk_state = >+intel_atomic_get_old_cdclk_state(state); >+const struct intel_cdclk_state *new_cdclk_state = >+intel_atomic_get_new_cdclk_state(state); >+ >+return new_cdclk_state && !new_cdclk_state->disable_pipes && >+new_cdclk_state->actual.cdclk < old_cdclk_state->actual.cdclk; >+} >+ > /** > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware > * @state: intel atomic state >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h >b/drivers/gpu/drm/i915/display/intel_cdclk.h >index 2843fc091086..5d4faf401774 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h >@@ -69,6 +69,7 @@ bool intel_cdclk_clock_changed(const struct >intel_cdclk_config *a, >const struct intel_cdclk_config *b); > u8 intel_mdclk_cdclk_ratio(struct drm_i915_private *i915, >const struct intel_cdclk_config *cdclk_config); >+bool intel_cdclk_is_decreasing_later(struct intel_atomic_state *state); > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state); > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state); > void intel_cdclk_dump_config(struct drm_i915_private *i915, >diff --git a/drivers/gpu/drm/i915/display/intel_display.c >b/drivers/gpu/drm/i915/display/intel_display.c >index 4d6668a5f1ab..023cf4a77e6f 100644 >--- a/drivers/gpu/drm/i915/display/intel_display.c >+++ b/drivers/gpu/drm/i915/display/intel_display.c >@@ -6915,6 +6915,8 @@ static void skl_commit_modeset_enables(struct >intel_atomic_state *state) > intel_pre_update_crtc(state, crtc); > } > >+intel_dbuf_mbus_pre_ddb_update(state); >+ > while (update_pipes) { > for_each_oldnew_intel_crtc_in_state(state, crtc, > old_crtc_state, > new_crtc_state, i) { >@@ -6945,6 +6947,8 @@ static void
RE: [PATCH 11/13] drm/i915: Implement vblank synchronized MBUS join changes
> -Original Message- > From: Intel-gfx On Behalf Of Ville > Syrjala > Sent: Wednesday, March 27, 2024 11:16 PM > To: intel-gfx@lists.freedesktop.org > Cc: Ville Syrjälä > Subject: [PATCH 11/13] drm/i915: Implement vblank synchronized MBUS join > changes > > From: Stanislav Lisovskiy > > Currently we can't change MBUS join status without doing a modeset, because > we are lacking mechanism to synchronize those with vblank. > However then this means that we can't do a fastset, if there is a need to > change > MBUS join state. Fix that by implementing such change. > We already call correspondent check and update at pre_plane dbuf update, so > the > only thing left is to have a non-modeset version of that. > If active pipes stay the same then fastset is possible and only MBUS join > state/ddb allocation updates would be committed. > > The full mbus/cdclk sequence will look as follows: > 1. disable pipes > 2. increase cdclk if necessary > 2.1 reprogram cdclk > 2.2 update dbuf tracker value > 3. enable mbus joining if necessary > 3.1 update mbus_ctl > 3.2 update dbuf tracker value > 4. reallocate dbuf for planes on active pipes 5. disable mbus joining if > necessary > 5.1 update dbuf tracker value > 5.2 update mbus_ctl > 6. enable pipes > 7. decrease cdclk if necessary > 7.1 update dbuf tracker value > 7.2 reprogram cdclk > > And in order to keep things in sync we need: > Step 2: > - mbus_join == old > - mdclk/cdclk ratio == new > Step 3: > - mbus_join == new > - mdclk/cdclk ratio == old when cdclk is changing in step 7 > - mdclk/cdclk ratio == new when cdclk is changing in step 2 Step 5: > - mbus_join == new > - mdclk/cdclk ratio == old when cdclk is changing in step 7 > - mdclk/cdclk ratio == new when cdclk is changing in step 2 Step 7: > - mbus_join == new > - mdclk/cdclk ratio == new > > v2: - Removed redundant parentheses(Ville Syrjälä) > - Constified new_crtc_state in intel_mbus_joined_pipe(Ville Syrjälä) > - Removed pipe_select variable(Ville Syrjälä) > [v3: vsyrjala: Correctly sequence vs. cdclk updates, >properly describe the full sequence, > shuffle code around to make the diff more legible, > streamline a few things] Looks Good to me. Reviewed-by: Uma Shankar > Signed-off-by: Stanislav Lisovskiy > Co-developed-by: Ville Syrjälä > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 11 ++ > drivers/gpu/drm/i915/display/intel_cdclk.h | 1 + > drivers/gpu/drm/i915/display/intel_display.c | 5 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 141 --- > drivers/gpu/drm/i915/display/skl_watermark.h | 3 +- > 5 files changed, 112 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 4024118a7ffb..66c161d7b485 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2576,6 +2576,17 @@ static void intel_cdclk_pcode_post_notify(struct > intel_atomic_state *state) > update_cdclk, update_pipe_count); } > > +bool intel_cdclk_is_decreasing_later(struct intel_atomic_state *state) > +{ > + const struct intel_cdclk_state *old_cdclk_state = > + intel_atomic_get_old_cdclk_state(state); > + const struct intel_cdclk_state *new_cdclk_state = > + intel_atomic_get_new_cdclk_state(state); > + > + return new_cdclk_state && !new_cdclk_state->disable_pipes && > + new_cdclk_state->actual.cdclk < old_cdclk_state->actual.cdclk; > } > + > /** > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware > * @state: intel atomic state > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > b/drivers/gpu/drm/i915/display/intel_cdclk.h > index 2843fc091086..5d4faf401774 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > @@ -69,6 +69,7 @@ bool intel_cdclk_clock_changed(const struct > intel_cdclk_config *a, > const struct intel_cdclk_config *b); > u8 intel_mdclk_cdclk_ratio(struct drm_i915_private *i915, > const struct intel_cdclk_config *cdclk_config); > +bool intel_cdclk_is_decreasing_later(struct intel_atomic_state *state); > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state); > void > intel_set_cdclk_post_plane_update(struct intel_atomic_state *state); void > intel_cdclk_dump_config(struct drm_i915_private *i915, diff --git > a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 4d6668a5f1ab..023cf4a77e6f 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6915,6 +6915,8 @@ static void skl_commit_modeset_enables(struct > intel_atomic_state *state) > intel_pre_update_crtc(state,