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

[Ben Widawsky]

On Tue, Jun 02, 2015 at 04:42:50PM -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.

I think this sentence could be a bit clearer since it confused me. I think you
can just say, Yf/Ys tiled buffers will use meta, or blitter in order to avoid
piglit regressions....

> 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.
> 
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> 
> ---
> I think we need some benchmarking to come up with conditions to
> choose Ys (64 KB) over Yf (4 KB). Any thoughts on minimum texture
> size so that 64 KB tiling is preferred over 4KB?

Given that this should alleviate some TLB pressure, it might not hurt to
actually try at 64K

> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 97 ++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index 6a99101..cdef7bb 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -758,6 +758,87 @@ intel_miptree_total_width_height(struct brw_context *brw,
>     }
>  }
>  
> +static bool
> +brw_miptree_choose_tr_mode(struct brw_context *brw,
> +                           enum intel_miptree_tiling_mode requested,
> +                           struct intel_mipmap_tree *mt)
> +{
> +   const unsigned bpp = mt->cpp * 8;
> +
> +   if (!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.
> +        */
> +       _mesa_is_format_color_format(mt->format) &&
> +       (requested == INTEL_MIPTREE_TILING_Y ||
> +        requested == INTEL_MIPTREE_TILING_ANY) &&
> +       (bpp && is_power_of_two(bpp))) {
> +      /* Low bits are the higher priority modes */
> +      int modes = INTEL_MIPTREE_TRMODE_YS | INTEL_MIPTREE_TRMODE_YF;

I guess since you are moving i instead of modes you could make modes a const.
Initially when I recommended a loop, aside from reducing code duplication, I was
thinking it might be possible to put the regular Y-tiled here too. At the time I
was thinking we could have a list of all tile modes in the order we want to try
them. That's probably more work than I initially though, and I don't know if you
don't think the check will ever be more than the two modes. you can go back to
the simple if/then/else and duplicated code if you think it will always be these
two modes.


If you're keeping it as a loop though... some comments below. Maybe it wasn't
clear that I meant to use the modes as bitfields.

> +      int i = 0;
> +
> +      do {
> +            uint32_t tr_mode = modes & ++i;

I think you mean 1 << i++? Also you should probably be explicit when you define
the tr modes that they're bitfields.

> +
> +            if (!tr_mode)
> +               continue;
> +            else
> +               mt->tr_mode = tr_mode;
> +
> +            assert(mt->tr_mode == INTEL_MIPTREE_TRMODE_YF ||
> +                   mt->tr_mode == INTEL_MIPTREE_TRMODE_YS);
> +
> +            mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt);
> +            mt->align_h = intel_vertical_texture_alignment_unit(brw, mt);
> +
> +            intel_miptree_total_width_height(brw, mt);

I think I mentioned in another review that the name of this function is pretty
poor. At the very least there should be a verb in the name since it is setting
something.

> +
> +            DBG("%s: %dx%dx%d\n", __func__,
> +                mt->total_width, mt->total_height, mt->cpp);
> +
> +            if (!mt || !mt->total_width || !mt->total_height) {
> +               intel_miptree_release(&mt);
> +               return false;
> +            }
> +
> +            mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> +
> +            if (mt->tiling == I915_TILING_Y ||
> +                mt->tiling == (I915_TILING_Y | I915_TILING_X)) {
> +
> +               /* FIXME: Don't allow YS tiling at the moment. Using 64KB
> +                * tiling for small textures might result in to wastage of
> +                * memory. 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_YF)
> +                  return true;
> +            }
> +

It really seems like INTEL_MIPTREE_TRMODE_Y[FS] should just be another tiling
mode and a lot of this logic should be in brw_miptree_choose_tiling. I realize
to some extent that's what you're doing in wrapping that function, but it just
looks strange to me.

> +            /* Failed to use tr_mode. Free up the memory allocated for miptree
> +             * levels in intel_miptree_total_width_height().
> +             */
> +            unsigned level;
> +            for (level = mt->first_level; level <= mt->last_level; level++) {
> +               free(mt->level[level].slice);
> +               mt->level[level].slice = NULL;
> +            }
> +
> +            modes &= ~mt->tr_mode;
> +      } while (i < _mesa_fls(modes));


Since you're clearing the bits as you go, it's just:
	...
	modes &= ~(1 << i);
} while(modes);

> +   }
> +
> +   mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
> +   return false;
> +}
> +
>  void
>  brw_miptree_layout(struct brw_context *brw,
>                     bool for_bo,
> @@ -766,6 +847,22 @@ brw_miptree_layout(struct brw_context *brw,
>  {
>     bool gen6_hiz_or_stencil = false;
>  
> +   /* Check if we can use Yf/Ys tiled resource modes. Fall back to using
> +    * INTEL_MIPTREE_TRMODE_NONE.
> +    *
> +    * FIXME: 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
> +    * piglit regressions keep the Yf/Ys tiling disabled at the moment.
> +    */
> +   if (brw->gen >= 9 && !for_bo && false /* disable Yf/Ys */) {
> +      if (brw_miptree_choose_tr_mode(brw, requested, mt))
> +         return;
> +   }
> +
> +   if (!mt)
> +      return;
> +

So miptree layout calls choose tr mode which calls a miptree layout function to
pick a tiling mode so we can determine if it's valid for the tr mode... Can
choose_tiling call choose_tr mode instead of the other way around? Clearly I am
not good at predicting what looks better.

>     mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
>  
>     if (brw->gen == 6 && mt->array_layout == ALL_SLICES_AT_EACH_LOD) {


I apologize for the churn. I thought the looping over the modes would look a bit
better than it turned out. Other than the couple of coding suggestions, I found
nothing wrong. I really am curious to hear what you think of this.