[Intel-gfx] [PATCH 1/3] drm/i915: Add support for non-power-of-2 FB plane alignment

[Chris Wilson]

Quoting Imre Deak (2019-12-27 23:51:45)
> At least one framebuffer plane on TGL - the UV plane of YUV semiplanar
> FBs - requires a non-power-of-2 alignment, so add support for this. This
> new alignment restriction applies only to an offset within an FB, so the
> GEM buffer itself containing the FB must still be power-of-2 aligned.

It's worth talking about virtual memory alignment (in the GGTT) here and
not the physical alignment of the backing store. The buffer itself plays
no part here.

> Add a check for this (in practice plane 0, since the plane 0 offset must
> be 0).
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 22 +++++++++++++-------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 624ba9be7293..d8970198c77e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2194,6 +2194,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>                 return ERR_PTR(-EINVAL);
>  
>         alignment = intel_surf_alignment(fb, 0);
> +       WARN_ON(!is_power_of_2(alignment));

Handle the error, if you are going to the trouble to warn, add the
return as well.

>  
>         /* Note that the w/a also requires 64 PTE of padding following the
>          * bo. We currently fill all unused PTE with the shadow page and so
> @@ -2432,9 +2433,6 @@ static u32 intel_compute_aligned_offset(struct drm_i915_private *dev_priv,
>         unsigned int cpp = fb->format->cpp[color_plane];
>         u32 offset, offset_aligned;
>  
> -       if (alignment)
> -               alignment--;
> -
>         if (!is_surface_linear(fb, color_plane)) {
>                 unsigned int tile_size, tile_width, tile_height;
>                 unsigned int tile_rows, tiles, pitch_tiles;
> @@ -2456,17 +2454,24 @@ static u32 intel_compute_aligned_offset(struct drm_i915_private *dev_priv,
>                 *x %= tile_width;
>  
>                 offset = (tile_rows * pitch_tiles + tiles) * tile_size;
> -               offset_aligned = offset & ~alignment;
> +
> +               offset_aligned = offset;
> +               if (alignment)
> +                       offset_aligned = rounddown(offset_aligned, alignment);
>  
>                 intel_adjust_tile_offset(x, y, tile_width, tile_height,
>                                          tile_size, pitch_tiles,
>                                          offset, offset_aligned);
>         } else {
>                 offset = *y * pitch + *x * cpp;
> -               offset_aligned = offset & ~alignment;
> -
> -               *y = (offset & alignment) / pitch;
> -               *x = ((offset & alignment) - *y * pitch) / cpp;
> +               offset_aligned = offset;
> +               if (alignment) {
> +                       offset_aligned = rounddown(offset_aligned, alignment);
> +                       *y = (offset % alignment) / pitch;
> +                       *x = ((offset % alignment) - *y * pitch) / cpp;
> +               } else {
> +                       *y = *x = 0;
> +               }
>         }
>  
>         return offset_aligned;
> @@ -3738,6 +3743,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>         intel_add_fb_offsets(&x, &y, plane_state, 0);
>         offset = intel_plane_compute_aligned_offset(&x, &y, plane_state, 0);
>         alignment = intel_surf_alignment(fb, 0);
> +       WARN_ON(!is_power_of_2(alignment));

The other two are expected to handle !is_pot...

I would strongly suggest handling the WARNs, or else you may as well bug
out for the programming error.
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris