Re: [Intel-gfx] [PATCH 1/3] drm/i915: Allow !join_mbus cases for adlp+ dbuf configuration

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, Feb 04, 2022 at 04:18:16PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reintroduce the !join_mbus single pipe cases for adlp+.
> 
> Due to the mbus relative dbuf offsets in PLANE_BUF_CFG we
> need to know the actual slices used by the pipe when doing
> readout, even when mbus joining isn't enabled. Accurate
> readout will be needed to properly sanitize invalid BIOS
> dbuf configurations.
> 
> This will also make it much easier to play around with the
> !join_mbus configs for testin/workaround purposes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 66 +++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 859be750fb22..2eb70ec38f6e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4695,6 +4695,10 @@ static const struct dbuf_slice_conf_entry dg2_allowed_dbufs[] = {
>  };
>  
>  static const struct dbuf_slice_conf_entry adlp_allowed_dbufs[] = {
> +	/*
> +	 * Keep the join_mbus cases first so check_mbus_joined()
> +	 * will prefer them over the !join_mbus cases.
> +	 */
>  	{
>  		.active_pipes = BIT(PIPE_A),
>  		.dbuf_mask = {
> @@ -4709,6 +4713,20 @@ static const struct dbuf_slice_conf_entry adlp_allowed_dbufs[] = {
>  		},
>  		.join_mbus = true,
>  	},
> +	{
> +		.active_pipes = BIT(PIPE_A),
> +		.dbuf_mask = {
> +			[PIPE_A] = BIT(DBUF_S1) | BIT(DBUF_S2),
> +		},
> +		.join_mbus = false,
> +	},
> +	{
> +		.active_pipes = BIT(PIPE_B),
> +		.dbuf_mask = {
> +			[PIPE_B] = BIT(DBUF_S3) | BIT(DBUF_S4),
> +		},
> +		.join_mbus = false,
> +	},
>  	{
>  		.active_pipes = BIT(PIPE_A) | BIT(PIPE_B),
>  		.dbuf_mask = {
> @@ -4825,13 +4843,14 @@ static bool adlp_check_mbus_joined(u8 active_pipes)
>  	return check_mbus_joined(active_pipes, adlp_allowed_dbufs);
>  }
>  
> -static u8 compute_dbuf_slices(enum pipe pipe, u8 active_pipes,
> +static u8 compute_dbuf_slices(enum pipe pipe, u8 active_pipes, bool join_mbus,
>  			      const struct dbuf_slice_conf_entry *dbuf_slices)
>  {
>  	int i;
>  
>  	for (i = 0; i < dbuf_slices[i].active_pipes; i++) {
> -		if (dbuf_slices[i].active_pipes == active_pipes)
> +		if (dbuf_slices[i].active_pipes == active_pipes &&
> +		    dbuf_slices[i].join_mbus == join_mbus)

We had similar blocker issue in May I remember for ADL, I remember I've sent
a fix, which was however just about checking also join_mbus state besides, the active pipes. 
Nice to see we are finally handling this now even more properly

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  			return dbuf_slices[i].dbuf_mask[pipe];
>  	}
>  	return 0;
> @@ -4842,7 +4861,7 @@ static u8 compute_dbuf_slices(enum pipe pipe, u8 active_pipes,
>   * returns correspondent DBuf slice mask as stated in BSpec for particular
>   * platform.
>   */
> -static u8 icl_compute_dbuf_slices(enum pipe pipe, u8 active_pipes)
> +static u8 icl_compute_dbuf_slices(enum pipe pipe, u8 active_pipes, bool join_mbus)
>  {
>  	/*
>  	 * FIXME: For ICL this is still a bit unclear as prev BSpec revision
> @@ -4856,37 +4875,41 @@ static u8 icl_compute_dbuf_slices(enum pipe pipe, u8 active_pipes)
>  	 * still here - we will need it once those additional constraints
>  	 * pop up.
>  	 */
> -	return compute_dbuf_slices(pipe, active_pipes, icl_allowed_dbufs);
> +	return compute_dbuf_slices(pipe, active_pipes, join_mbus,
> +				   icl_allowed_dbufs);
>  }
>  
> -static u8 tgl_compute_dbuf_slices(enum pipe pipe, u8 active_pipes)
> +static u8 tgl_compute_dbuf_slices(enum pipe pipe, u8 active_pipes, bool join_mbus)
>  {
> -	return compute_dbuf_slices(pipe, active_pipes, tgl_allowed_dbufs);
> +	return compute_dbuf_slices(pipe, active_pipes, join_mbus,
> +				   tgl_allowed_dbufs);
>  }
>  
> -static u32 adlp_compute_dbuf_slices(enum pipe pipe, u32 active_pipes)
> +static u8 adlp_compute_dbuf_slices(enum pipe pipe, u8 active_pipes, bool join_mbus)
>  {
> -	return compute_dbuf_slices(pipe, active_pipes, adlp_allowed_dbufs);
> +	return compute_dbuf_slices(pipe, active_pipes, join_mbus,
> +				   adlp_allowed_dbufs);
>  }
>  
> -static u32 dg2_compute_dbuf_slices(enum pipe pipe, u32 active_pipes)
> +static u8 dg2_compute_dbuf_slices(enum pipe pipe, u8 active_pipes, bool join_mbus)
>  {
> -	return compute_dbuf_slices(pipe, active_pipes, dg2_allowed_dbufs);
> +	return compute_dbuf_slices(pipe, active_pipes, join_mbus,
> +				   dg2_allowed_dbufs);
>  }
>  
> -static u8 skl_compute_dbuf_slices(struct intel_crtc *crtc, u8 active_pipes)
> +static u8 skl_compute_dbuf_slices(struct intel_crtc *crtc, u8 active_pipes, bool join_mbus)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  
>  	if (IS_DG2(dev_priv))
> -		return dg2_compute_dbuf_slices(pipe, active_pipes);
> +		return dg2_compute_dbuf_slices(pipe, active_pipes, join_mbus);
>  	else if (IS_ALDERLAKE_P(dev_priv))
> -		return adlp_compute_dbuf_slices(pipe, active_pipes);
> +		return adlp_compute_dbuf_slices(pipe, active_pipes, join_mbus);
>  	else if (DISPLAY_VER(dev_priv) == 12)
> -		return tgl_compute_dbuf_slices(pipe, active_pipes);
> +		return tgl_compute_dbuf_slices(pipe, active_pipes, join_mbus);
>  	else if (DISPLAY_VER(dev_priv) == 11)
> -		return icl_compute_dbuf_slices(pipe, active_pipes);
> +		return icl_compute_dbuf_slices(pipe, active_pipes, join_mbus);
>  	/*
>  	 * For anything else just return one slice yet.
>  	 * Should be extended for other platforms.
> @@ -6139,11 +6162,16 @@ skl_compute_ddb(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> +	if (IS_ALDERLAKE_P(dev_priv))
> +		new_dbuf_state->joined_mbus =
> +			adlp_check_mbus_joined(new_dbuf_state->active_pipes);
> +
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		enum pipe pipe = crtc->pipe;
>  
>  		new_dbuf_state->slices[pipe] =
> -			skl_compute_dbuf_slices(crtc, new_dbuf_state->active_pipes);
> +			skl_compute_dbuf_slices(crtc, new_dbuf_state->active_pipes,
> +						new_dbuf_state->joined_mbus);
>  
>  		if (old_dbuf_state->slices[pipe] == new_dbuf_state->slices[pipe])
>  			continue;
> @@ -6155,9 +6183,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
>  
>  	new_dbuf_state->enabled_slices = intel_dbuf_enabled_slices(new_dbuf_state);
>  
> -	if (IS_ALDERLAKE_P(dev_priv))
> -		new_dbuf_state->joined_mbus = adlp_check_mbus_joined(new_dbuf_state->active_pipes);
> -
>  	if (old_dbuf_state->enabled_slices != new_dbuf_state->enabled_slices ||
>  	    old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
>  		ret = intel_atomic_serialize_global_state(&new_dbuf_state->base);
> @@ -6658,7 +6683,8 @@ void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
>  		}
>  
>  		dbuf_state->slices[pipe] =
> -			skl_compute_dbuf_slices(crtc, dbuf_state->active_pipes);
> +			skl_compute_dbuf_slices(crtc, dbuf_state->active_pipes,
> +						dbuf_state->joined_mbus);
>  
>  		dbuf_state->weight[pipe] = intel_crtc_ddb_weight(crtc_state);
>  
> -- 
> 2.34.1
>