Re: [PATCH 8/9] drm/i915: Update plane alignment requirements for TGL+

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

On Tue, May 28, 2024 at 04:22:59PM +0300, Imre Deak wrote:
> On Mon, May 13, 2024 at 08:59:41PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we still use the SKL+ PLANE_SURF alignment even
> > for TGL+ even though the hardware no longer needs it.
> > Introduce a separate tgl_plane_min_alignment() and update
> > it to more accurately reflect the hardware requirements.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/skl_universal_plane.c    | 103 ++++++++++--------
> >  1 file changed, 55 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 1ecd7c691317..ca7fc9fae990 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -502,75 +502,79 @@ skl_plane_max_stride(struct intel_plane *plane,
> >  				max_pixels, max_bytes);
> >  }
> >  
> > -static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
> > -					    const struct drm_framebuffer *fb,
> > -					    int color_plane)
> > +static u32 tgl_plane_min_alignment(struct intel_plane *plane,
> > +				   const struct drm_framebuffer *fb,
> > +				   int color_plane)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -
> > -	if (intel_fb_uses_dpt(fb)) {
> > -		/* AUX_DIST needs only 4K alignment */
> > -		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> > -			return 512 * 4096;
> > -
> > -		/*
> > -		 * FIXME ADL sees GGTT/DMAR faults with async
> > -		 * flips unless we align to 16k at least.
> > -		 * Figure out what's going on here...
> > -		 */
> > -		if (IS_ALDERLAKE_P(dev_priv) &&
> > -		    !intel_fb_is_ccs_modifier(fb->modifier) &&
> > -		    HAS_ASYNC_FLIPS(dev_priv))
> > -			return 512 * 16 * 1024;
> > -
> > -		return 512 * 4096;
> > -	}
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +	/* PLANE_SURF GGTT -> DPT alignment */
> > +	int mult = intel_fb_uses_dpt(fb) ? 512 : 1;
> >  
> >  	/* AUX_DIST needs only 4K alignment */
> >  	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> > -		return 4096;
> > +		return mult * 4 * 1024;
> >  
> >  	if (is_semiplanar_uv_plane(fb, color_plane)) {
> >  		/*
> >  		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> >  		 * alignment for linear UV planes on all platforms.
> >  		 */
> > -		if (DISPLAY_VER(dev_priv) >= 12) {
> > -			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > -				return 256 * 1024;
> > -
> > -			return intel_tile_row_size(fb, color_plane);
> > -		}
> > -
> > -		return 4096;
> > -	}
> > -
> > -	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> > -
> > -	switch (fb->modifier) {
> > -	case DRM_FORMAT_MOD_LINEAR:
> > -		return 256 * 1024;
> > -	case I915_FORMAT_MOD_X_TILED:
> > -		if (HAS_ASYNC_FLIPS(dev_priv))
> > +		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> >  			return 256 * 1024;
> > -		return 0;
> > +
> > +		return intel_tile_row_size(fb, color_plane);
> > +	}
> > +
> > +	switch (fb->modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +	case I915_FORMAT_MOD_X_TILED:
> > +	case I915_FORMAT_MOD_Y_TILED:
> > +	case I915_FORMAT_MOD_4_TILED:
> > +		/*
> > +		 * FIXME ADL sees GGTT/DMAR faults with async
> > +		 * flips unless we align to 16k at least.
> > +		 * Figure out what's going on here...
> > +		 */
> > +		if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))
> 
> On ADL HAS_ASYNC_FLIPS() is always true, otherwise looks ok:

I've been using HAS_ASYNC_FLIPS() to just flag the async flip
specific restrictions. So mainly an aide memoire, but it can
technically be used to also test with less alignment
by just neutering HAS_ASYNC_FLIPS(), without having go trawl
the specs for the specific number again.

Though I'm not super happy how this looks when combine
with the async flip modifier restrictions. Haven't yet
figured out how it actually should look in the though.

> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > +			return mult * 16 * 1024;
> > +		return mult * 4 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
> > -		return 16 * 1024;
> > +		/* 4x1 main surface tiles (16K) match 64B of AUX */
> > +		return max(mult * 4 * 1024, 16 * 1024);
> > +	default:
> > +		MISSING_CASE(fb->modifier);
> > +		return 0;
> > +	}
> > +}
> > +
> > +static u32 skl_plane_min_alignment(struct intel_plane *plane,
> > +				   const struct drm_framebuffer *fb,
> > +				   int color_plane)
> > +{
> > +	/*
> > +	 * AUX_DIST needs only 4K alignment,
> > +	 * as does ICL UV PLANE_SURF.
> > +	 */
> > +	if (color_plane != 0)
> > +		return 4 * 1024;
> > +
> > +	switch (fb->modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +	case I915_FORMAT_MOD_X_TILED:
> > +		return 256 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED:
> > -	case I915_FORMAT_MOD_4_TILED:
> >  	case I915_FORMAT_MOD_Yf_TILED:
> >  		return 1 * 1024 * 1024;
> > -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> > -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> > -	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> > -		return 16 * 1024;
> >  	default:
> >  		MISSING_CASE(fb->modifier);
> >  		return 0;
> > @@ -2442,7 +2446,10 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
> >  	else
> >  		plane->max_stride = skl_plane_max_stride;
> >  
> > -	plane->min_alignment = skl_plane_min_alignment;
> > +	if (DISPLAY_VER(dev_priv) >= 12)
> > +		plane->min_alignment = tgl_plane_min_alignment;
> > +	else
> > +		plane->min_alignment = skl_plane_min_alignment;
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11) {
> >  		plane->update_noarm = icl_plane_update_noarm;
> > -- 
> > 2.43.2
> > 

-- 
Ville Syrjälä
Intel