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

[Ville Syrjälä]

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 at 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.
Dunno if that's true or not. I suppose there could be some
tlb/prefetch related reasons for it.

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.

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

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.


> +		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.

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

>  	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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel