Re: [Intel-gfx] [PATCH v2] drm/i915: Simplify CCS and UV plane alignment handling

[Imre Deak <imre.deak@intel.com>]

On Fri, Apr 23, 2021 at 08:04:35PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 21, 2021 at 08:32:20PM +0300, Imre Deak wrote:
> > We can handle the surface alignment of CCS and UV color planes for all
> > modifiers at one place, so do this. An AUX color plane can be a CCS or a
> > UV plane, use only the more specific query functions and remove
> > is_aux_plane() becoming redundant.
> > 
> > While at it add a TODO for linear UV color plane alignments. The spec
> > requires this to be stride-in-bytes * 64 on all platforms, whereas the
> > driver uses an alignment of 4k for gen<12 and 256k for gen>=12 for
> > linear UV planes.
> > 
> > v2:
> > - Restore previous alignment for linear UV surfaces.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 27 +++++++++++++-------
> >  drivers/gpu/drm/i915/display/intel_fb.c      |  8 ------
> >  drivers/gpu/drm/i915/display/intel_fb.h      |  1 -
> >  3 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a10e26380ef3d..e246e5cf75866 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -973,10 +973,26 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> >  	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> >  
> >  	/* AUX_DIST needs only 4K alignment */
> > -	if ((DISPLAY_VER(dev_priv) < 12 && is_aux_plane(fb, color_plane)) ||
> > -	    is_ccs_plane(fb, color_plane))
> > +	if (is_ccs_plane(fb, color_plane))
> >  		return 4096;
> >  
> > +	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.
> > +		 */
> 
> I think it's just saying that UV should always start at an integer
> multiple of Y stride, whether we're dealing with linear or tiled.

This is what I see for SKL (bspec 18566):
"""
Linear:
The start of the UV surface programmed in the PLANE_AUX_DIST register
should be aligned to Stride in bytes * 64.
"""

The ICL page makes the above clearer that the above stride is the Y
plane stride, but I think it's always the same as the linear UV plane
stride.

Yes, I suppose the intention could have been to align to a multiple of
the stride, and maybe also ensure a page alignment.

> Dunno if that's true or not. I suppose there could be some
> tlb/prefetch related reasons for it.

Yea, not sure either. Simply following the above formula would result in
max_stride*64 = 128k * 64 = 8MB alignment requirement, which hopefully
isn't really needed.

> I think the same tile row/stride alignment requirements are specified
> for all gen9+ platforms actually. So if it's supposedly really needed
> then I guess we should do it on all platforms. And if it's not actually
> needed we shoud just nuke it all and be happy with 4k alignment.

I see the same UV alignment requirement on all platforms, but yes my
guess and hope is that only 4k is needed. I will do more tests to see if
misaligning the UV plane (only ensuring a 4k alignment) causes a problem
anywhere and ask for a clarification in bspec if not.

> What are the chances we can even find a suitbly aligned page boundary?
> Not sure.

The linear stride must be a multiple of 64, so stride*64 would always
result in a 4k alignment. The 64 multiplier in the formula could be
reduced in some cases (for instance when stride%4k==0), but in the worst
cases only *64 could provide an offset aligned both to 4k and to the
multiple of stride.

> Oh and there's some oddball mention of the UV start having to be a
> multiple of four lines. Is it talking about AUX_DIST of AUX_OFFSET.y?
> No idea. What lines? Maybe Y lines? Not sure.

Haven't noticed that. In my reading AUX_DIST has to be always page
aligned, but in case of linear,X-,Y-tiled formats the UV-start can
follow the Y surface end "immediately on the next line", so Y-end and
UV-start can be on the same page.  I suppose in this layout there is
still the restriction that AUX_OFFSET.y should be a multiple of 4 (so
"immediately" in bpsec is not precise). Will test this out as well.

Another oddity I noticed now is the Yf format where the FB Y plane could
be allocated (with a good reason) with a different stride than the UV
plane? If so, bpsec says the driver should ensure that the Y and UV
scanout strides match (padding the allocated Y stride tile rows if
needed).

> > +		if (DISPLAY_VER(dev_priv) >= 12) {
> > +			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > +				return intel_linear_alignment(dev_priv);
> > +
> > +			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 intel_linear_alignment(dev_priv);
> > @@ -985,19 +1001,12 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> >  			return 256 * 1024;
> >  		return 0;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > -		if (is_semiplanar_uv_plane(fb, color_plane))
> > -			return intel_tile_row_size(fb, color_plane);
> > -		fallthrough;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return 16 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED:
> > -		if (DISPLAY_VER(dev_priv) >= 12 &&
> > -		    is_semiplanar_uv_plane(fb, color_plane))
> > -			return intel_tile_row_size(fb, color_plane);
> > -		fallthrough;
> >  	case I915_FORMAT_MOD_Yf_TILED:
> >  		return 1 * 1024 * 1024;
> 
> As for these IIRC TGL+ should not need any extra alignment anymore.
> But that's material for a separate patch.

Ok, can check this later.

> Anyways patch seems ok.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks.

> >  	default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 0ec9ad7220a14..c8aaca3e79e97 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -30,14 +30,6 @@ bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane)
> >  	       plane == 2;
> >  }
> >  
> > -bool is_aux_plane(const struct drm_framebuffer *fb, int plane)
> > -{
> > -	if (is_ccs_modifier(fb->modifier))
> > -		return is_ccs_plane(fb, plane);
> > -
> > -	return plane == 1;
> > -}
> > -
> >  bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
> >  {
> >  	return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> > index 6acf792a8c44a..13244ec1ad214 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> > @@ -19,7 +19,6 @@ struct intel_plane_state;
> >  bool is_ccs_plane(const struct drm_framebuffer *fb, int plane);
> >  bool is_gen12_ccs_plane(const struct drm_framebuffer *fb, int plane);
> >  bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane);
> > -bool is_aux_plane(const struct drm_framebuffer *fb, int plane);
> >  bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
> >  
> >  bool is_surface_linear(const struct drm_framebuffer *fb, int color_plane);
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx