[PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes

[Ville Syrjälä]

On Mon, Aug 21, 2023 at 03:16:51PM +0300, 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.

Hmm. Once you put the mbus stuff into skl_commit_modeset_enables()
it'd perhaps be just as easy to go all the way and push it into eg.
skl_crtc_planes_update_arm() (or possibly commit_pipe_pre/post_plane())
so that it will really happen atomically with the DDB update for the
planes.

For a bit of extra safety we should perhaps still use the pre/post split
for the atomic mbus update, just in case our vblank evasion fails. The
pre call would enable joining, post call would disable it, as needed.

You're also totally missing the setup of the MBUS_CTL pipe
selection bits so you're not actually synchronizing the MBUS
changes with the correct pipe.

> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  8 ++-
>  drivers/gpu/drm/i915/display/skl_watermark.c | 56 ++++++++++++++++----
>  drivers/gpu/drm/i915/display/skl_watermark.h |  1 +
>  3 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 8c81206ce90d7..e5476beaaaf07 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6782,6 +6782,12 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  		}
>  	}
>  
> +	/*
> +	 * Some MBUS/DBuf update scenarios require that changes are done _after_ DDB
> +	 * reallocation, but _before_ crtc enabling.
> +	 */
> +	intel_dbuf_mbus_post_ddb_update(state);
> +
>  	update_pipes = modeset_pipes;
>  
>  	/*
> @@ -7041,9 +7047,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 af99f2abd8446..c30528bdf133e 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2614,13 +2614,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(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,
> @@ -3524,8 +3517,15 @@ void intel_dbuf_pre_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))
> +	/*
> +	 * Switching from multiple to single display scenario.
> +	 * Also we put here "<=" instead of "<" for suboptimal cases, when
> +	 * we switch from single => single display, enabling mbus join.
> +	 */
> +	if (hweight8(new_dbuf_state->active_pipes) <= hweight8(old_dbuf_state->active_pipes)) {
>  		intel_dbuf_mbus_update(state);
> +		intel_mbus_dbox_update(state);
> +	}
>  
>  	gen9_dbuf_slices_update(i915,
>  				old_dbuf_state->enabled_slices |
> @@ -3547,13 +3547,47 @@ 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_update(state);
> -
>  	gen9_dbuf_slices_update(i915,
>  				new_dbuf_state->enabled_slices);
>  }
>  
> +void intel_dbuf_mbus_post_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))
> +		return;
> +
> +	WARN_ON(!new_dbuf_state->base.changed);
> +
> +	/*
> +	 * Switching from single to multiple display scenario
> +	 */
> +	if (hweight8(new_dbuf_state->active_pipes) > hweight8(old_dbuf_state->active_pipes)) {
> +		struct intel_crtc *crtc;
> +		struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> +		int i;
> +		intel_dbuf_mbus_update(state);
> +		intel_mbus_dbox_update(state);
> +
> +		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +						    new_crtc_state, i) {
> +			/*
> +			 * According to BSpec we should wait vblank on previously single display
> +			 */
> +			if (!old_crtc_state->hw.active || !new_crtc_state->hw.active)
> +				continue;
> +
> +			intel_crtc_wait_for_next_vblank(crtc);
> +		}
> +	}
> +}
> +
>  static bool xelpdp_is_only_pipe_per_dbuf_bank(enum pipe pipe, u8 active_pipes)
>  {
>  	switch (pipe) {
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> index f91a3d4ddc075..96a4eed1daceb 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> @@ -71,6 +71,7 @@ intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
>  int intel_dbuf_init(struct drm_i915_private *i915);
>  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_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