[Mesa-dev] [PATCH 2/5] i965/gen9: Plugin the code for selecting YF/YS tiling on skl+

[Ben Widawsky]

On Wed, Jun 10, 2015 at 03:30:47PM -0700, Anuj Phogat wrote:
> Buffers with Yf/Ys tiling end up using meta upload / download
> paths or the blitter for cases where they used tiled_memcpy paths
> in case of Y tiling. This has exposed some bugs in meta path. To
> avoid any piglit regressions on SKL this patch keeps the Yf/Ys
> tiling disabled at the moment.
> 
> V3: Make brw_miptree_choose_tr_mode() actually choose TRMODE. (Ben)
>     Few cosmetic changes.
> V4: Get rid of brw_miptree_choose_tr_mode().
>     Take care of all tile resource modes {Yf, Ys, none} for all
>     generations at one place.
> 
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 97 ++++++++++++++++++++++++------
>  1 file changed, 79 insertions(+), 18 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index b9ac4cf..c0ef5cc 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -807,27 +807,88 @@ brw_miptree_layout(struct brw_context *brw,
>                     enum intel_miptree_tiling_mode requested,
>                     struct intel_mipmap_tree *mt)
>  {
> -   mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
> +   const unsigned bpp = mt->cpp * 8;
> +   const bool is_tr_mode_yf_ys_allowed =
> +      brw->gen >= 9 &&
> +      !for_bo &&
> +      !mt->compressed &&
> +      /* Enable YF/YS tiling only for color surfaces because depth and
> +       * stencil surfaces are not supported in blitter using fast copy
> +       * blit and meta PBO upload, download paths. No other paths
> +       * currently support Yf/Ys tiled surfaces.
> +       * FIXME:  Remove this restriction once we have a tiled_memcpy()
> +       * path to do depth/stencil data upload/download to Yf/Ys tiled
> +       * surfaces.
> +       */

I think it's more readable to move this comment above the variable declaration.
Up to you though. Also I think "FINISHME" is the more appropriate classification
for this type of thing.

> +      _mesa_is_format_color_format(mt->format) &&
> +      (requested == INTEL_MIPTREE_TILING_Y ||
> +       requested == INTEL_MIPTREE_TILING_ANY) &&

This is where my tiling flags would have helped a bit since you should be able
to do flags & Y_TILED :P

> +      (bpp && is_power_of_two(bpp)) &&
> +      /* FIXME: To avoid piglit regressions keep the Yf/Ys tiling
> +       * disabled at the moment.
> +       */
> +      false;

Also, "FINISHME"

>  
> -   intel_miptree_set_alignment(brw, mt);
> -   intel_miptree_set_total_width_height(brw, mt);
> +   /* Lower index (Yf) is the higher priority mode */
> +   const uint32_t tr_mode[3] = {INTEL_MIPTREE_TRMODE_YF,
> +                                INTEL_MIPTREE_TRMODE_YS,
> +                                INTEL_MIPTREE_TRMODE_NONE};
> +   int i = is_tr_mode_yf_ys_allowed ? 0 : ARRAY_SIZE(tr_mode) - 1;
>  
> -   if (!mt->total_width || !mt->total_height) {
> -      intel_miptree_release(&mt);
> -      return;
> -   }
> +   while (i < ARRAY_SIZE(tr_mode)) {
> +      if (brw->gen < 9)
> +         assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
> +      else
> +         assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_YF ||
> +                tr_mode[i] == INTEL_MIPTREE_TRMODE_YS ||
> +                tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
>  
> -   /* 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;
> -   }
> +      mt->tr_mode = tr_mode[i];
> +      intel_miptree_set_alignment(brw, mt);
> +      intel_miptree_set_total_width_height(brw, mt);
>  
> -   if (!for_bo)
> -      mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> +      if (!mt->total_width || !mt->total_height) {
> +         intel_miptree_release(&mt);
> +         return;
> +      }
> +
> +      /* 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;
> +      }

Can we just combine this alignment calculation into
intel_miptree_set_alignment()?

> +
> +      if (!for_bo)
> +         mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);

Perhaps (fwiw, I prefer break instead of returning within a loop, but that's
just me)?
/* If there is already a BO, we cannot effect tiling modes */
if (for_bo)
	break;


mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);;
if (is_tr_mode_yf_ys_allowed) {
	...
}

This sort of reflects how I felt earlier about pushing the YF/YS decision into
choose tiling. The code is heading in that direction though, so I am content.


> +
> +      if (is_tr_mode_yf_ys_allowed) {
> +         unsigned int level = 0;
> +         assert(brw->gen >= 9);

I am assert happy, but this is a bit redundant even more my standards :-)

> +
> +         if (mt->tiling == I915_TILING_Y ||
> +             mt->tiling == (I915_TILING_Y | I915_TILING_X) ||
> +             mt->tr_mode == INTEL_MIPTREE_TRMODE_NONE) {
> +            /* FIXME: Don't allow YS tiling at the moment. Using 64KB tiling
> +             * for small textures might result in to memory wastage. Revisit
> +             * this condition when we have more information about the specific
> +             * cases where using YS over YF will be useful.
> +             */
> +            if (mt->tr_mode != INTEL_MIPTREE_TRMODE_YS)
> +               return;
> +         }
> +         /* Failed to use selected tr_mode. Free up the memory allocated
> +          * for miptree levels in intel_miptree_total_width_height().
> +          */
> +         for (level = mt->first_level; level <= mt->last_level; level++) {
> +            free(mt->level[level].slice);
> +            mt->level[level].slice = NULL;
> +         }
> +      }
> +      i++;
> +   }
>  }
>  

I'm still reviewing, but I don't think you need to change any of what I said
unless you want to.