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

[Neil Roberts]

Hi Nanley,

Could you explain the reasoning behind this patch? I can't find any
mention of needing to align to the square of the block size in the docs.

I think how it works is that on Skylake you can pick any alignment value
you want out of 4, 8 or 16 but for compressed textures that is counted
in units of blocks rather than pixels. Currently we effectively always
pick 4 for compressed textures because I don't think there's any reason
to align to a higher value. The mt->align_w/h values are used for two
things; in intel_miptree_set_total_width_height they are used to choose
the positions for the mipmap images and after that they are only used to
upload the alignment values as part of the surface state. On Skylake
mt->align_w/h is temporarily set to be in units of pixels during
brw_miptree_layout but at the end of the function it divides the values
by the block size so that they will be in units of blocks ready for
uploading as part of the surface state.

Your patch modifies how it picks the alignment value but it doesn't
modify the bit at the end which divides the chosen alignment by the
block size. For FXT1 I think that would end up making it pick a
horizontal alignment value of ALIGN_8 (ie, the pixel alignment value is
now 8*8=64 which when divided by the block width ends up as
64/8=ALIGN_8).

Before my patches for Skylake bits of the code were lazily assuming that
mt->align_w/h is always the same as the block size because that
previously happened to be always correct. I fixed some of the cases
within the layouting code to instead directly query the block size.
However it looks like I missed one in intel_miptree_copy_slice. I think
your patch would make this particular case work because now the
alignment value is ALIGN_8 which happens to match the block width for
FXT1.

I'm about to post an alternative patch which fixes this case to use the
block size instead and it does seem to fix the broken FXT1 test cases as
well.

Regards,
- Neil

Nanley Chery <nanleychery at gmail.com> writes:

> 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>
> ---
>
> This fixes an FXT1 Piglit test regression and shows no failures on Jenkins. 
>
>  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;
>      }
>  
>     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 */
> -      return brw->gen >= 9 ? 16 : 4;
> +      return brw->gen >= 9 ? j * j : j;
> +   }
>  
>     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);
> -- 
> 2.4.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev