[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Mar 21 13:00:53 UTC 2024
On Thu, Mar 21, 2024 at 02:43:49PM +0200, Lisovskiy, Stanislav wrote:
> On Thu, Mar 21, 2024 at 02:16:59PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 21, 2024 at 10:40:37AM +0200, Lisovskiy, Stanislav wrote:
> > > On Wed, Mar 20, 2024 at 11:08:54PM +0200, Ville Syrjälä wrote:
> > > > 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.
> > >
> > > For vblank wait I need just pipe, which I was getting in previous
> > > revision with intel_dbuf_get_sync_pipe, now it is hidden into
> > > mbus_join_update, in prev revision it was used from ddb hooks, as you suggest now.
> > >
> > > But here we are talking about where pipe_select is constructed.
> > > If I would be returning pipe with mbus_joined_pipe, as you suggest,
> > > then someone must construct pipe_select.
> >
> > Either you can just do
> > if (pipe == INVALID)
> > mbus_ctl |= PIPE_SELECT_NONE;
> > else
> > mbus_ctl |= PIPE_SELECT(pipe);
> >
> > or you put that into a small helper ala. bxt_cdclk_cd2x_pipe().
> >
> > The point is that no one outside the mbus_update() function needs
> > to know about the details of MBUS_CTL, but they do need to know the
> > correct pipe, hence have a single function to determine the pipe and
> > pass it around where needed.
> >
> > > Above you commented that
> > > you would return "just pipe" in leave constructing to caller(i.e mbus_join func).
> > >
> > > Should this be also one more function or doing it inside mbus_join programming
> > > function(what I do now) is fine?
> > > I'm fine with both.
> > >
> > > >
> > > > > 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;
> > > > }
> > >
> > > But that is almost exactly how it was done in previous revision,
> > > it was intel_dbuf_get_sync_pipe.
> >
> > I have not seen any version with a function like that.
>
> intel_dbuf_get_sync_pipe is defined in that revision as well.
> It is lacking both WARNS though and doesn't return invalid
> pipe, but I didn't want to add WARNS here because if you
> ever try to get a sync pipe using non-joined dbuf_state,
> both will trigger.
> Also I thought "needs_modeset" is another level of hierarchy,
> probably we shouldn't try calling that function in that case at all.
The whole point of the warns and such is to make sure we don't use this
in the wrong context. It is only to be used when we are changing
mbus_join.
--
Ville Syrjälä
Intel
More information about the Intel-gfx-trybot
mailing list