[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Mar 19 18:21:37 UTC 2024
On Tue, Mar 19, 2024 at 11:39:49AM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Mar 19, 2024 at 10:57:21AM +0200, Ville Syrjälä wrote:
> > 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.
>
> Because I wanted that sync_pipe is calculated explicitly by the caller,
> now considering that we move pipe_select calculation logic back to mbus join
> update function, based on your comment,
No, it should be calculated by the caller, by calling
this function and passing in the correct dbuf state.
> I will have to pass either old
> or new dbuf state to intel_mbus_join_update depending whether it is pre or post ddb hook.
>
> However in the post ddb hook, we still need to know the sync_pipe, because
> we will have to wait for vblank there - i.e this means I will have to call intel_dbuf_get_sync_pipe
> twice - once in intel_mbus_join_update to calculate pipe_select and another time in
> post ddb hook which is calling it.
>
> In above solution, we would have to call it only once, using it both for intel_mbus_join_update
> and also for getting the crtc to wait for vblank.
>
> But in fact cost and difference is so small that I guess it doesn't matter.
>
> We could also may be wait for vblank also in intel_mbus_join_update itself, however I don't
> like that - it would mean intel_mbus_join_update is doing something besides, what it was meant
> to do.
>
> Stan
>
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
More information about the Intel-gfx-trybot
mailing list