[Mesa-dev] [PATCH 3/6 v2] i965/skl: Fix the qpitch value

[Ben Widawsky]

On Mon, Feb 23, 2015 at 05:27:29PM +0000, Neil Roberts wrote:
> On Skylake the qpitch value is uploaded as part of the surface state
> so we don't need to add the extra rows that are done for other
> generations. However for 3D textures it needs to be aligned to the
> tile height and for depth/stencil textures it needs to be a multiple
> of 8. Unlike previous generations the qpitch is measured as a multiple
> of the block size for compressed surfaces. When the horizontal mipmap
> layout is used for 1D textures then the qpitch is measured in pixels
> instead of rows.
> 
> v2: Align the depth/stencil textures to a multiple of 8
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c    | 52 +++++++++++++++++++++------
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 10 ++++--
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index 851742b..834d4ec 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -297,24 +297,56 @@ static void
>  brw_miptree_layout_texture_array(struct brw_context *brw,
>  				 struct intel_mipmap_tree *mt)
>  {
> -   int h0, h1;
>     unsigned height = mt->physical_height0;
>     bool layout_1d = use_linear_1d_layout(brw, mt);
> -
> -   h0 = ALIGN(mt->physical_height0, mt->align_h);
> -   h1 = ALIGN(minify(mt->physical_height0, 1), mt->align_h);
> -   if (mt->array_layout == ALL_SLICES_AT_EACH_LOD)
> -      mt->qpitch = h0;
> -   else
> -      mt->qpitch = (h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->align_h);
> -
> -   int physical_qpitch = mt->compressed ? mt->qpitch / 4 : mt->qpitch;
> +   int physical_qpitch;
>  
>     if (layout_1d)
>        gen9_miptree_layout_1d(mt);
>     else
>        brw_miptree_layout_2d(mt);
>  
> +   if (layout_1d) {
> +      physical_qpitch = mt->align_h;
> +      /* When using the horizontal layout the qpitch is measured in pixels */

I think the docs words are a bit clearer:
"Surface QPitch specifies the distance in pixels between array slices."

> +      mt->qpitch = physical_qpitch * mt->total_width;

talking to myself - this seems more complicated than it ought to be.

> +   } else if (brw->gen >= 9) {
> +      GLenum base_format;
> +
> +      /* On Gen9 we can pick whatever qpitch we like as long as it's aligned
> +       * to the vertical alignment so we don't need to add any extra rows.
> +       */
> +      mt->qpitch = mt->total_height;
> +
> +      /* If the surface might be used as a stencil buffer or HiZ buffer then
> +       * it needs to be a multiple of 8.
> +       */
> +      base_format = _mesa_get_format_base_format(mt->format);
> +      if (_mesa_is_depth_or_stencil_format(base_format))
> +         mt->qpitch = ALIGN(mt->qpitch, 8);
> +
> +      /* 3D textures need to be aligned to the tile height. At this point we
> +       * don't know which tiling will be used so let's just align it to 32
> +       */
> +      if (mt->target == GL_TEXTURE_3D)
> +         mt->qpitch = ALIGN(mt->qpitch, 32);
> +
> +      /* Unlike previous generations the qpitch is now a multiple of the
> +       * compressed block size so physical_qpitch matches mt->qpitch.
> +       */
> +      physical_qpitch = mt->qpitch;
> +   } else {
> +      int h0 = ALIGN(mt->physical_height0, mt->align_h);
> +      int h1 = ALIGN(minify(mt->physical_height0, 1), mt->align_h);
> +
> +      if (mt->array_layout == ALL_SLICES_AT_EACH_LOD)
> +         mt->qpitch = h0;
> +      else
> +         mt->qpitch = (h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->align_h);
> +
> +      physical_qpitch = mt->compressed ? mt->qpitch / 4 : mt->qpitch;
> +   }
> +
>     for (unsigned level = mt->first_level; level <= mt->last_level; level++) {
>        unsigned img_height;
>        img_height = ALIGN(height, mt->align_h);
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index ee9cf1e..247e5b8 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -380,10 +380,14 @@ struct intel_mipmap_tree
>     enum miptree_array_layout array_layout;
>  
>     /**
> -    * The distance in rows between array slices in an uncompressed surface.
> +    * The distance in between array slices.
>      *
> -    * For compressed surfaces, slices are stored closer together physically;
> -    * the real distance is (qpitch / block height).
> +    * The value is the one that is sent in the surface state. The actual
> +    * meaning depends on certain criteria. Usually it is simply the number of
> +    * uncompressed rows between each slice. However on Gen9+ for compressed
> +    * surfaces it is the number of blocks. For 1D surfaces that have the
						   ^ I suppose array is implied?
> +    * mipmap tree stored horizontally it is the number of pixels between each
> +    * slice.
>      */
>     uint32_t qpitch;
>  

This patch is a bit trickier than the previous, and I got lazy with fact
checking in bspec, but it lgtm:
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>