Re: [Intel-gfx] [PATCH v2 3/4] drm/i915/display/adlp: Fix programing of PIPE_MBUS_DBOX_CTL

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 22, 2022 at 02:46:15PM -0700, José Roberto de Souza wrote:
> PIPE_MBUS_DBOX_CTL was only being programmed when a pipe is being
> enabled but that could potentially cause issues as it could have
> mismatching values while pipes are being enabled.
> 
> So here moving the PIPE_MBUS_DBOX_CTL programming of all pipes to be
> executed before the function that enables all pipes, leaving all pipes
> with a matching A_CREDIT value.
> 
> While at it, also moving it to intel_pm.c as we are trying to reduce
> the gigantic size of it and intel_pm.c have other MBUS programing
> sequences.
> 
> v2:
> - do not program PIPE_MBUS_DBOX_CTL if pipe will not be active or
> when it do not needs modeset
> - remove the checks to wait a vblank
> 
> BSpec: 49213
> BSpec: 50343
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 37 +--------------
>  drivers/gpu/drm/i915/intel_pm.c              | 47 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.h              |  1 +
>  3 files changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 424cd7e9afe60..ef5076b5e7027 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1824,35 +1824,6 @@ static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv,
>  	intel_de_write(dev_priv, CLKGATE_DIS_PSL(pipe), val);
>  }
>  
> -static void icl_pipe_mbus_enable(struct intel_crtc *crtc, bool joined_mbus)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum pipe pipe = crtc->pipe;
> -	u32 val;
> -
> -	val = intel_de_read(dev_priv, PIPE_MBUS_DBOX_CTL(pipe));
> -	val &= ~MBUS_DBOX_A_CREDIT_MASK;
> -	/* Wa_22010947358:adl-p */
> -	if (IS_ALDERLAKE_P(dev_priv))
> -		val |= joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
> -	else
> -		val |= MBUS_DBOX_A_CREDIT(2);
> -
> -	val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
> -	if (IS_ALDERLAKE_P(dev_priv)) {
> -		val |= MBUS_DBOX_BW_CREDIT(2);
> -		val |= MBUS_DBOX_B_CREDIT(8);
> -	} else if (DISPLAY_VER(dev_priv) >= 12) {
> -		val |= MBUS_DBOX_BW_CREDIT(2);
> -		val |= MBUS_DBOX_B_CREDIT(12);
> -	} else {
> -		val |= MBUS_DBOX_BW_CREDIT(1);
> -		val |= MBUS_DBOX_B_CREDIT(8);
> -	}
> -
> -	intel_de_write(dev_priv, PIPE_MBUS_DBOX_CTL(pipe), val);
> -}
> -
>  static void hsw_set_linetime_wm(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1988,13 +1959,6 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  
>  	intel_initial_watermarks(state, crtc);
>  
> -	if (DISPLAY_VER(dev_priv) >= 11) {
> -		const struct intel_dbuf_state *dbuf_state =
> -				intel_atomic_get_new_dbuf_state(state);
> -
> -		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> -	}
> -
>  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>  		intel_crtc_vblank_on(new_crtc_state);
>  
> @@ -8599,6 +8563,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/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e60c02d760ffa..cf290bb704221 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8258,3 +8258,50 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
>  	gen9_dbuf_slices_update(dev_priv,
>  				new_dbuf_state->enabled_slices);
>  }
> +
> +void intel_mbus_dbox_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_dbuf_state *new_dbuf_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	if (DISPLAY_VER(i915) < 11 || !state->modeset)
> +		return;
> +
> +	if (HAS_MBUS_JOINING(i915))
> +		new_dbuf_state = intel_atomic_get_dbuf_state(state);
> +
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		u32 val;
> +
> +		if (!new_crtc_state->hw.active ||
> +		    !intel_crtc_needs_modeset(new_crtc_state))
> +			continue;
> +
> +		val = intel_de_read(i915, PIPE_MBUS_DBOX_CTL(crtc->pipe));
> +		val &= ~MBUS_DBOX_A_CREDIT_MASK;
> +		/* Wa_22010947358:adl-p */
> +		if (IS_ALDERLAKE_P(i915))
> +			val |= new_dbuf_state->joined_mbus ? MBUS_DBOX_A_CREDIT(6) :
> +							     MBUS_DBOX_A_CREDIT(4);

Hmm. I'm not super happy with the assumption that the dbuf state
is there. When reading this it's not immediately obvious why this
works.

Might actually be nice to depend purely on the dbuf_state for this
stuff. So how about something like:

intel_mbus_dbox_update()
{
	if (!new_dbuf_state ||
	    (new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus &&
	     new_dbuf_state->active_pipes == old_dbuf_state->active_pipes))
	     return;

	for_each_pipe_masked(new_dbuf_state->active_pipes)
		write PIPE_MBUS_DBOX_CTL
}

?

> +			val |= MBUS_DBOX_A_CREDIT(2);
> +
> +		if (IS_ALDERLAKE_P(i915)) {
> +			val |= MBUS_DBOX_BW_CREDIT(2);
> +			val |= MBUS_DBOX_B_CREDIT(8);
> +		} else if (DISPLAY_VER(i915) >= 12) {
> +			val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
> +			val |= MBUS_DBOX_BW_CREDIT(2);
> +			val |= MBUS_DBOX_B_CREDIT(12);
> +		} else {
> +			val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
> +			val |= MBUS_DBOX_BW_CREDIT(1);
> +			val |= MBUS_DBOX_B_CREDIT(8);
> +		}
> +
> +		intel_de_write(i915, PIPE_MBUS_DBOX_CTL(crtc->pipe), val);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 51705151b842f..50604cf7398c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -94,5 +94,6 @@ intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
>  int intel_dbuf_init(struct drm_i915_private *dev_priv);
>  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
>  void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
> +void intel_mbus_dbox_update(struct intel_atomic_state *state);
>  
>  #endif /* __INTEL_PM_H__ */
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel