[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Mar 19 08:57:21 UTC 2024
On Mon, Mar 18, 2024 at 11:52:44PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 18, 2024 at 11:10:27PM +0200, Ville Syrjälä wrote:
> > On Sun, Mar 17, 2024 at 07:24:16PM +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ä)
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 15 ++-
> > > drivers/gpu/drm/i915/display/skl_watermark.c | 130 +++++++++++++++----
> > > drivers/gpu/drm/i915/display/skl_watermark.h | 2 +
> > > 3 files changed, 123 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..6e4b2c941b63e 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,17 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > }
> > > }
> > >
> > > + /*
> > > + * Some MBUS/DBuf update scenarios(mbus join disable) require that
> > > + * changes are done _after_ DDB reallocation, but _before_ crtc enabling.
> > > + * Typically we are disabling resources in post_plane/crtc_enable hooks,
> > > + * however in that case BSpec explicitly states that this should be done
> > > + * before we enable additional displays.
> > > + * FIXME: Should we still call this also there(post_plane hook)
> > > + * for extra-safety? If so, how to make sure, we don't call it twice?
> > > + */
> >
> > The function name seems clear enough to me. tl;dr comments
> > aren't going to help anyone. I'd just drop the whole thing.
> >
> > > + intel_dbuf_mbus_post_ddb_update(state);
> > > +
> > > update_pipes = modeset_pipes;
> > >
> > > /*
> > > @@ -7169,9 +7182,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..783a0f6ca1f71 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -2636,13 +2636,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 +3587,59 @@ 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;
> > > +}
> > > +
> > > +static u32 intel_dbuf_get_pipe_select_mask(struct intel_atomic_state *state, enum pipe sync_pipe)
> >
> > Rather than passing in the pipe, I think you could just pass in
> > the whole dbuf state. Then we don't have to repeat the code to
> > determine the pipe in two places.
> >
> > Also we can then add a bit of sanity checking in the form of:
> > drm_WARN_ON(!dbuf_state->mbus_joined);
> > drm_WARN_ON(!is_power_of_two(dbuf_state->active_pipes));
>
> Was in fact wondering if you are going to say that I need to pass dbuf state
> here, the only reason I was just wondering, if code is going to be less
> readable, because in one case we will have to pass new_dbuf_state and in other
> one old_dbuf_state.
Dunno why it's unreadable. You pass the dbug state that has
mbus_join==true because that is the state that will tell you
which pipe is using the joined mbus.
--
Ville Syrjälä
Intel
More information about the Intel-gfx-trybot
mailing list