[Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size

[Ben Widawsky]

On Fri, Feb 20, 2015 at 10:31:07PM +0000, Neil Roberts wrote:
> On Skylake it is possible to choose your own alignment values for
> compressed textures but they are expressed as a multiple of the block
> size. The minimum alignment value we can use is 4 so we effectively
> have to align to 4 times the block size. This patch makes it initially
> set mt->align_[wh] to the large alignment value and then later divides
> it by the block size so that it can be uploaded as part of the surface
> state.
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 31 ++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index d89a04e..1fe2c26 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -73,7 +73,16 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,
>          */
>        unsigned int i, j;
>        _mesa_get_format_block_size(format, &i, &j);
> -      return i;
> +
> +      /* 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;

Sorry for the delay, but I put this off initially because I wasn't sure which
part of the docs this was addressing. I see that the Surface Horizontal
Alignment field is now used for compressed formats.  Assuming that's what this
is referring to (if not, please correct me)...

The only thing that appears to be missing is the handling of the MCS case (must
always be 16) which may or may not be relevant. AFAICT, it's a possible
scenario. Also, doesn't this make FXT1 have an alignment of 32?

>      }
>  
>     if (format == MESA_FORMAT_S_UINT8)
> @@ -113,7 +122,8 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>      * the SURFACE_STATE "Surface Vertical Alignment" field.
>      */
>     if (_mesa_is_format_compressed(format))
> -      return 4;
> +      /* See comment above for the horizontal alignment */
> +      return brw->gen >= 9 ? 16 : 4;

This seems okay since the max height we support is 4.

>  
>     if (format == MESA_FORMAT_S_UINT8)
>        return brw->gen >= 7 ? 8 : 4;
> @@ -195,6 +205,9 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>     unsigned width = mt->physical_width0;
>     unsigned height = mt->physical_height0;
>     unsigned depth = mt->physical_depth0; /* number of array layers. */
> +   unsigned int bw, bh;
> +
> +   _mesa_get_format_block_size(mt->format, &bw, &bh);
>  
>     mt->total_width = mt->physical_width0;
>  
> @@ -212,7 +225,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), mt->align_w);
> +             ALIGN(minify(mt->physical_width0, 2), bw);
>         } else {
>            mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>               minify(mt->physical_width0, 2);
> @@ -232,7 +245,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>  
>        img_height = ALIGN(height, mt->align_h);
>        if (mt->compressed)
> -	 img_height /= mt->align_h;
> +	 img_height /= bh;
>  
>        if (mt->array_layout == ALL_SLICES_AT_EACH_LOD) {
>           /* Compact arrays with separated miplevels */
> @@ -481,5 +494,15 @@ brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt)
>     }
>     DBG("%s: %dx%dx%d\n", __FUNCTION__,
>         mt->total_width, mt->total_height, mt->cpp);
> +
> +   /* On Gen9+ the alignment values are expressed in multiples of the block
> +    * size
> +    */
> +   if (brw->gen >= 9) {
> +      unsigned int i, j;
> +      _mesa_get_format_block_size(mt->format, &i, &j);
> +      mt->align_w /= i;
> +      mt->align_h /= j;
> +   }
>  }
>  

These hunks look okay to me. Any particular reason not to update
brw_miptree_layout_texture_array as well? I am pretty certain we don't use
ALL_SLICES_AT_EACH_LOD in Gen9 today, but presumably we could, right?
/me shrugs

-- 
Ben Widawsky, Intel Open Source Technology Center