[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Mar 18 21:10:27 UTC 2024
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));
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *new_crtc_state;
> + u32 pipe_select;
> +
> + if (sync_pipe == INVALID_PIPE)
> + return MBUS_JOIN_PIPE_SELECT_NONE;
> +
> + 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.
> +}
> +
> /*
> * 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, enum pipe sync_pipe)
> {
> 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);
> + u32 pipe_select;
>
> if (!HAS_MBUS_JOINING(i915))
> return;
>
> + pipe_select = intel_dbuf_get_pipe_select_mask(state, sync_pipe);
> +
> /*
> * 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 |
> @@ -3632,19 +3654,18 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_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);
> + }
I think all of that stuff should be in the post ddb hook.
No reason that I can see to spread it around like this.
> +
> 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);
> @@ -3658,6 +3679,23 @@ void intel_dbuf_post_plane_update(struct intel_atomic_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)
> +{
> + 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 &&
> new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
> @@ -3665,13 +3703,61 @@ void intel_dbuf_post_plane_up/date(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);
> + /*
> + * Switching to single display scenario(enable mbus join).
> + */
This comment doesn't appear to provide anything
the code isn't already saying.
> + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> + enum pipe sync_pipe;
> +
> + if (new_dbuf_state->active_pipes)
> + sync_pipe = intel_dbuf_get_sync_pipe(new_dbuf_state);
> + else
> + sync_pipe = INVALID_PIPE;
> +
> + intel_dbuf_mbus_join_update(state, sync_pipe);
> intel_dbuf_mdclk_min_tracker_update(state);
> + intel_mbus_dbox_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 ||
> + (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);
> +
> + /*
> + * Switching to multiple display scenario
> + */
ditto
> + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> + enum pipe sync_pipe;
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *new_crtc_state = NULL;
> +
> + if (old_dbuf_state->active_pipes) {
> + sync_pipe = intel_dbuf_get_sync_pipe(old_dbuf_state);
> + crtc = intel_crtc_for_pipe(i915, sync_pipe);
> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> + } else {
> + sync_pipe = INVALID_PIPE;
> + }
> +
> + intel_dbuf_mbus_join_update(state, sync_pipe);
> + intel_dbuf_mdclk_min_tracker_update(state);
> + intel_mbus_dbox_update(state);
Either this or the other guy must be doing things on the wrong order.
Can't recall anymore which was the correct order for which case.
> +
> + 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
More information about the Intel-gfx-trybot
mailing list