[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Mar 20 21:08:54 UTC 2024
On Wed, Mar 20, 2024 at 08:54:09PM +0200, Lisovskiy, Stanislav wrote:
> On Wed, Mar 20, 2024 at 06:36:31PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 20, 2024 at 10:28:06AM +0200, Stanislav Lisovskiy wrote:
> > > 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.
> > >
> > > v2: Implement additional changes according to BSpec.
> > > Vblank wait is needed after MBus/Dbuf programming in case if
> > > no modeset is done and we are switching from single to multiple
> > > displays, i.e mbus join state switches from "joined" to "non-joined"
> > > state. Otherwise vblank wait is not needed according to spec.
> > >
> > > v3: Split mbus and dbox programming into to pre/post plane update parts,
> > > how it should be done according to BSpec.
> > >
> > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX programming
> > > after DDB reallocation, but before crtc enabling(that is where is has
> > > to be according to spec).
> > > - Check if crtc is still active, not only the old state.
> > > - Do a vblank wait if MBUX DBOX register was modified.
> > > - And of course do vblank wait only if crtc was active.
> > > - Do vblank wait only if we are not doing a modeset, if we are doing
> > > something before *commit_modeset_enables, because all crtcs might be
> > > disabled at this moment, so we will get WARN if try waiting for vblank
> > > then.
> > > - Still getting FIFO underrun so try waiting for vblank in pre_plane update
> > > as well.
> > > - Write also pipe that we need to sync with to MBUS_CTL register.
> > >
> > > v5: - Do vblank wait only for the first pipe, if mbus is joined
> > > - Check also if new/old_dbuf_state is not NULL, before getting single pipe
> > > and active pipes.
> > >
> > > v6: - Add pre/post ddb call sites for doing mbus/mbox updates for switching
> > > between single/multiple displays(Ville Syrjälä)
> > > - When writing MBUS_CTL check if synced pipe is enabled, otherwise use
> > > PIPE_NONE sync option(Ville Syrjälä)
> > > - Use for_each_crtc_in_pipe_mask to go through all active pipes, not
> > > just ones in state, when updating mbox registers(Ville Syrjälä)
> > > - Improve checks for switching between multiple <=> single displays
> > >
> > > v7: - Extract sync pipe calculation to separate function(Ville Syrjälä)
> > > - Removed redundant loop over crtc in post ddb call site(Ville Syrjälä)
> > > - Use old_dbuf_state to determine synced pipe when switching to multiple
> > > displays in post ddb call site(Ville Syrjälä)
> > > - Removed num_active_pipes checks and use joined_mbus only(Ville Syrjälä)
> > > - Extracted dbox update change to separate patch(Ville Syrjälä)
> > > - Added handling for the case, when mbus_joined state didn't change
> > > (Ville Syrjälä)
> > >
> > > v8: - Removed redundant comments(Ville Syrjälä)
> > > - Pass dbuf_state to intel_dbuf_mbus_join_update instead of pipe(Ville Syrjälä)
> > > - Moved non-changed mbus_join logic from pre_plane to post_ddb hook(Ville Syrjälä)
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 6 +-
> > > drivers/gpu/drm/i915/display/skl_watermark.c | 106 +++++++++++++++----
> > > drivers/gpu/drm/i915/display/skl_watermark.h | 2 +
> > > 3 files changed, 90 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index b88f214e111ae..d5351f6fa2eb4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6895,6 +6895,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) {
> > > @@ -6925,6 +6927,8 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > }
> > > }
> > >
> > > + intel_dbuf_mbus_post_ddb_update(state);
> > > +
> > > update_pipes = modeset_pipes;
> > >
> > > /*
> > > @@ -7169,9 +7173,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> > > }
> > >
> > > intel_encoders_update_prepare(state);
> > > -
> > > intel_dbuf_pre_plane_update(state);
> > > - intel_mbus_dbox_update(state);
> > >
> > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > if (new_crtc_state->do_async_flip)
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index 2207ecd2605c2..4c1c2f558e3e7 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -4,6 +4,7 @@
> > > */
> > >
> > > #include <drm/drm_blend.h>
> > > +#include <drm/drm_print.h>
> > >
> > > #include "i915_drv.h"
> > > #include "i915_fixed.h"
> > > @@ -2636,13 +2637,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> > > if (ret)
> > > return ret;
> > >
> > > - if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
> > > - /* TODO: Implement vblank synchronized MBUS joining changes */
> > > - ret = intel_modeset_all_pipes_late(state, "MBUS joining change");
> > > - if (ret)
> > > - return ret;
> > > - }
> > > -
> > > drm_dbg_kms(&i915->drm,
> > > "Enabled dbuf slices 0x%x -> 0x%x (total dbuf slices 0x%x), mbus joined? %s->%s\n",
> > > old_dbuf_state->enabled_slices,
> > > @@ -3594,30 +3588,49 @@ static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state
> > > new_dbuf_state->joined_mbus);
> > > }
> > >
> > > +static enum pipe intel_dbuf_get_sync_pipe(const struct intel_dbuf_state *dbuf_state)
> > > +{
> > > + return ffs(dbuf_state->active_pipes) - 1;
> > > +}
> > > +
> > > /*
> > > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
> > > * update the request state of all DBUS slices.
> > > */
> > > -static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
> > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state,
> > > + const struct intel_dbuf_state *dbuf_state)
> > > {
> > > struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > u32 mbus_ctl;
> > > const struct intel_dbuf_state *new_dbuf_state =
> > > intel_atomic_get_new_dbuf_state(state);
> > > + struct intel_crtc *crtc;
> > > + struct intel_crtc_state *new_crtc_state;
> > > + u32 pipe_select;
> > > + enum pipe sync_pipe;
> > >
> > > if (!HAS_MBUS_JOINING(i915))
> > > return;
> > >
> > > + sync_pipe = intel_dbuf_get_sync_pipe(dbuf_state);
> > > + crtc = intel_crtc_for_pipe(i915, sync_pipe);
> > > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > > +
> > > + if (new_crtc_state && !intel_crtc_needs_modeset(new_crtc_state))
> > > + pipe_select = MBUS_JOIN_PIPE_SELECT(sync_pipe);
> > > + else
> > > + pipe_select = MBUS_JOIN_PIPE_SELECT_NONE;
> >
> > As I said before, that stuff should be a separate function
> > that the pre/post ddb update things can call with the proper
> > arguments. Trying to shoehorn it into this function whose
> > only job should be to write the MBUS_CTL is not a good idea.
>
> That previous comment from you, I interpreted that I return the pipe
> instead of pipe select.
> In previous revision I already had this as a function:
>
> >> + crtc = intel_crtc_for_pipe(i915, sync_pipe);
> >> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> >> +
> >> + if (new_crtc_state && !intel_crtc_needs_modeset(new_crtc_state))
> >> + pipe_select = MBUS_JOIN_PIPE_SELECT(sync_pipe);
> >> + else
> >> + pipe_select = MBUS_JOIN_PIPE_SELECT_NONE;
> >> +
> >> + return pipe_select;
>
> > I would return an enum pipe from here, and leave it up to the
> > caller to deal with whatever is needed to stuff it into the register.
> > Isolates the implementation from changes in the register layout/etc.
>
> Also I don't get why can't we calculate PIPE_SELECT mask in a function,
> which is the only place where it is calculated.
You also need it for the vblank wait in the caller. Right now
you're calculating it in two or three different places and
a bit differently in each case. We want a single function that
does the right thing every time.
> We are anyway constructing
> what we are writing to mbus_ctl here.
> Ok, I will change it since I probably got no other choice to get this reviewed :)
>
> Is that all what has to be changed or there is something else besides?
> I don't see a point in sending another revision, if that's not the final
> list of things to fix.
The way I see these should look more or less is:
mbus_joined_pipe(dbuf_state)
{
pipe = ffs(dbuf_state->active_pipes) - 1;
drm_WARN_ON(!dbuf_state->join);
drm_WARN_ON(!is_pot(dbuf_state->active_pipes));
if (needs_modeset)
return INVALID;
else
return pipe;
}
pre_ddb_update()
{
if (!old_dbuf_state->join && new_duf_state->join) {
pipe = mbus_joined_pipe(new_dbuf_state);
mbus_ctl_update(new_dbuf_state, pipe);
dbuf_tracker_update_etc();
}
}
post_ddb_update()
{
if (old_dbuf_state->join && !new_duf_state->join) {
pipe = mbus_joined_pipe(old_dbuf_state);
dbuf_tracker_update_etc();
mbus_ctl_update(new_duf_state, pipe);
if (pipe != INVALID)
wait_for_vblank(pipe);
} else {
dbuf_tracker_update_etc();
}
}
>
> Stan
>
> >
> > > +
> > > /*
> > > * TODO: Implement vblank synchronized MBUS joining changes.
> > > * Must be properly coordinated with dbuf reprogramming.
> > > */
> > > if (new_dbuf_state->joined_mbus)
> > > mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
> > > - MBUS_JOIN_PIPE_SELECT_NONE;
> > > + pipe_select;
> > > else
> > > mbus_ctl = MBUS_HASHING_MODE_2x2 |
> > > - MBUS_JOIN_PIPE_SELECT_NONE;
> > > + pipe_select;
> > >
> > > intel_de_rmw(i915, MBUS_CTL,
> > > MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> > > @@ -3633,24 +3646,35 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > > intel_atomic_get_old_dbuf_state(state);
> > >
> > > if (!new_dbuf_state ||
> > > - (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
> > > - new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
> > > + (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices))
> > > return;
> > >
> > > WARN_ON(!new_dbuf_state->base.changed);
> > >
> > > - if ((hweight8(new_dbuf_state->active_pipes) <= hweight8(old_dbuf_state->active_pipes))
> > > - || (old_dbuf_state->active_pipes == 0)) {
> > > - intel_dbuf_mbus_join_update(state);
> > > - intel_dbuf_mdclk_min_tracker_update(state);
> > > - }
> > > -
> > > gen9_dbuf_slices_update(i915,
> > > old_dbuf_state->enabled_slices |
> > > new_dbuf_state->enabled_slices);
> > > }
> > >
> > > void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > + const struct intel_dbuf_state *new_dbuf_state =
> > > + intel_atomic_get_new_dbuf_state(state);
> > > + const struct intel_dbuf_state *old_dbuf_state =
> > > + intel_atomic_get_old_dbuf_state(state);
> > > +
> > > + if (!new_dbuf_state ||
> > > + (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices))
> > > + return;
> > > +
> > > + WARN_ON(!new_dbuf_state->base.changed);
> > > +
> > > + gen9_dbuf_slices_update(i915,
> > > + new_dbuf_state->enabled_slices);
> > > +}
> > > +
> > > +void intel_dbuf_mbus_pre_ddb_update(struct intel_atomic_state *state)
> > > {
> > > struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > const struct intel_dbuf_state *new_dbuf_state =
> > > @@ -3665,13 +3689,51 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> > >
> > > WARN_ON(!new_dbuf_state->base.changed);
> > >
> > > - if (hweight8(new_dbuf_state->active_pipes) > hweight8(old_dbuf_state->active_pipes)) {
> > > - intel_dbuf_mbus_join_update(state);
> > > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> > > + drm_WARN_ON(&i915->drm, !is_power_of_2(new_dbuf_state->active_pipes));
> > > +
> > > + intel_dbuf_mbus_join_update(state, new_dbuf_state);
> > > + intel_mbus_dbox_update(state);
> > > intel_dbuf_mdclk_min_tracker_update(state);
> > > }
> > > +}
> > >
> > > - gen9_dbuf_slices_update(i915,
> > > - new_dbuf_state->enabled_slices);
> > > +void intel_dbuf_mbus_post_ddb_update(struct intel_atomic_state *state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > + const struct intel_dbuf_state *new_dbuf_state =
> > > + intel_atomic_get_new_dbuf_state(state);
> > > + const struct intel_dbuf_state *old_dbuf_state =
> > > + intel_atomic_get_old_dbuf_state(state);
> > > +
> > > + if (new_dbuf_state && old_dbuf_state &&
> > > + new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus) {
> > > + intel_dbuf_mdclk_min_tracker_update(state);
> > > + intel_mbus_dbox_update(state);
> > > + }
> > > +
> > > + if (!new_dbuf_state ||
> > > + (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
> > > + new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
> > > + return;
> > > +
> > > + WARN_ON(!new_dbuf_state->base.changed);
> > > +
> > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> > > + enum pipe sync_pipe = intel_dbuf_get_sync_pipe(old_dbuf_state);
> > > + struct intel_crtc *crtc = intel_crtc_for_pipe(i915, sync_pipe);
> > > + struct intel_crtc_state *new_crtc_state =
> > > + intel_atomic_get_new_crtc_state(state, crtc);
> > > +
> > > + drm_WARN_ON(&i915->drm, is_power_of_2(new_dbuf_state->active_pipes));
> > > +
> > > + intel_dbuf_mdclk_min_tracker_update(state);
> > > + intel_mbus_dbox_update(state);
> > > + intel_dbuf_mbus_join_update(state, old_dbuf_state);
> > > +
> > > + if (new_crtc_state && !intel_crtc_needs_modeset(new_crtc_state))
> > > + intel_crtc_wait_for_next_vblank(crtc);
> > > + }
> > > }
> > >
> > > static bool xelpdp_is_only_pipe_per_dbuf_bank(enum pipe pipe, u8 active_pipes)
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> > > index 3a90741cab06a..f6d38b41e3a6c 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> > > @@ -77,6 +77,8 @@ int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8
> > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
> > > void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
> > > void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus);
> > > +void intel_dbuf_mbus_pre_ddb_update(struct intel_atomic_state *state);
> > > +void intel_dbuf_mbus_post_ddb_update(struct intel_atomic_state *state);
> > > void intel_mbus_dbox_update(struct intel_atomic_state *state);
> > >
> > > #endif /* __SKL_WATERMARK_H__ */
> > > --
> > > 2.37.3
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
More information about the Intel-gfx-trybot
mailing list