[Mesa-dev] [PATCH] i965: correct alignment units for 2D compressed textures on Skylake

[Ben Widawsky]

On Wed, Jun 10, 2015 at 05:01:44PM -0700, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.chery at intel.com>
> 
> On Gen9+, vertical and horizontal alignment values for compressed textures are
> equal to the pre-Gen9 value squared. Each miplevel must be aligned to this
> value.
> 
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>

While not a requirement, for future reference you should add Cc on patches which
are touching something someone recently changed/and or was working on. In this
case Ccing Neil would have been great.

> ---
> 
> This fixes an FXT1 Piglit test regression and shows no failures on Jenkins. 

You ran full piglit on SKL? Jenkins won't and it's pretty important for this
patch.

> 
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index 312a887..dffc699 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -149,15 +149,8 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,
>        unsigned int i, j;
>        _mesa_get_format_block_size(mt->format, &i, &j);
>  
> -      /* On Gen9+ we can pick our own alignment for compressed textures but it
> -       * has to be a multiple of the block size. The minimum alignment we can
> -       * pick is 4 so we effectively have to align to 4 times the block
> -       * size
> -       */
> -      if (brw->gen >= 9)
> -         return i * 4;
> -      else
> -         return i;
> +      /* On Gen9+ the alignment value is squared. */
> +      return brw->gen >= 9 ? i * i : i;

I don't think this is right. Isn't this going to push non compressed textures to
an invalid HALIGN when we divide later ie. don't you get 1?

>      }
>  
>     if (mt->format == MESA_FORMAT_S_UINT8)
> @@ -269,9 +262,12 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>      * Where "*" means either VALIGN_2 or VALIGN_4 depending on the setting of
>      * the SURFACE_STATE "Surface Vertical Alignment" field.
>      */
> -   if (_mesa_is_format_compressed(mt->format))
> +   if (_mesa_is_format_compressed(mt->format)) {
> +      unsigned int i, j;
> +      _mesa_get_format_block_size(mt->format, &i, &j);
>        /* See comment above for the horizontal alignment */

I think you need to kill this comment now. Doesn't it refer to what you killed
above?

> -      return brw->gen >= 9 ? 16 : 4;
> +      return brw->gen >= 9 ? j * j : j;
> +   }

It kind of looks like this is just working around the way Neil implemented the
divide later on. There's probably a nicer way to do the right thing, but I
haven't tried it myself. Also, I believe this doesn't actually fix anything, it
just uses slightly more optimal aligns - ie. maybe do a separate patch. No big
deal though.

>  
>     if (mt->format == MESA_FORMAT_S_UINT8)
>        return brw->gen >= 7 ? 8 : 4;
> @@ -379,7 +375,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>  
>         if (mt->compressed) {
>            mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
> -             ALIGN(minify(mt->physical_width0, 2), bw);
> +             ALIGN(minify(mt->physical_width0, 2), mt->align_w);
>         } else {
>            mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>               minify(mt->physical_width0, 2);