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

[Ben Widawsky]

On Fri, Apr 17, 2015 at 04:51:42PM -0700, Anuj Phogat wrote:
> Note: Yf/Ys tiling stays disabled to avoid any piglit regressions. I'm
> working on fixing the remaining piglit failures.
> 
> We need to do some benchmarking to come up with conditions to choose
> Ys (64 KB) over Yf (4 KB). Any thoughts on how big a texture should
> be so that 64 KB tiling is preferred over 4KB?
> 
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 81 ++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index 28927e9..b2a2dac 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -684,6 +684,65 @@ intel_miptree_total_width_height(struct brw_context *brw,
>     }
>  }
>  
> +static bool
> +intel_miptree_choose_tr_mode(struct brw_context *brw,

Either rename this function, or make it actually choose the tr_mode.

> +                             mesa_format format,
> +                             uint32_t width0,
> +                             uint32_t num_samples,
> +                             enum intel_miptree_tiling_mode requested,
> +                             struct intel_mipmap_tree *mt,
> +                             uint32_t tr_mode)

As I state above and below, I don't think tr_mode should be passed in here. This
function is supposed to "choose" the mode.

> +{
> +   const unsigned bpp = mt->cpp * 8;
> +
> +   /* bpp must be power of 2. */
> +   if (!mt->compressed &&
> +       _mesa_is_format_color_format(mt->format) &&

I am not finding the reason for the color format restriction. I believe it, just
not seeing it...

> +       (requested == INTEL_MIPTREE_TILING_Y ||
> +        requested == INTEL_MIPTREE_TILING_ANY) &&
> +       (bpp && (bpp & (bpp - 1)) == 0)) {
> +

is_power_of_2


How about something like...

/* Low bits are the highest priority modes */
modes = INTEL_MIPTREE_TRMODE_YS | INTEL_MIPTREE_TRMODE_YF;
while ((modes >>= 1) & 1) {
	... try to allocate
}

> +      mt->tr_mode = tr_mode;

you're leaking tr_mode state here if you fail below.

> +      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);
> +
> +      /* pitch == 0 || height == 0  indicates the null texture */

What is the null texture?

> +      if (!mt || !mt->total_width || !mt->total_height) {
> +         intel_miptree_release(&mt);
> +         return false;
> +      }
> +
> +      mt->tiling = intel_miptree_choose_tiling(brw, format, width0,
> +                                               num_samples,
> +                                               requested, mt);
> +
> +      if (mt->tiling == I915_TILING_Y ||
> +          mt->tiling == (I915_TILING_Y | I915_TILING_X)) {
> +
> +         /* Don't allow YS tiling at the moment. Using 64KB tiling for small
> +          * textures might result in to wastage of memory.
> +          * FIXME: 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;
> +      }
> +
> +      /* Can't use requested 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;
> +      }

intel_miptree_release(&mt); ? Either that or remove it above. Seems like
returning false should always do the same thing.

> +   }
> +
> +   return false;
> +}
> +
>  void
>  brw_miptree_layout(struct brw_context *brw,
>                     mesa_format format,
> @@ -695,6 +754,28 @@ brw_miptree_layout(struct brw_context *brw,
>  {
>     bool gen6_hiz_or_stencil = false;
>  
> +   /* Check in advance if we can do Y tiling with Yf or 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 and missing support of
> +    * few formats in meta path. To avoid piglit regressions disable the
> +    * Yf/Ys tiling at the moment.
> +    */
> +   if (brw->gen >= 9 && !for_bo && false /* disable Yf/Ys */) {
> +      /* Prefer YF over YS at the moment. */
> +      if (intel_miptree_choose_tr_mode(brw, format, width0, num_samples,
> +                                       requested, mt,
> +                                       INTEL_MIPTREE_TRMODE_YF) ||
> +          intel_miptree_choose_tr_mode(brw, format, width0, num_samples,
> +                                       requested, mt,
> +                                       INTEL_MIPTREE_TRMODE_YS))
> +         return;
> +   }

I'm not a huge fan of the way you wrote this. It seems like the determination of
the TILING would be better off in choose (and making the verb choose correct in
the process).


if ((brw->gen >= 9 && !for_bo && false /* disable Yf/Ys */) {
    if (intel_miptree_choose_tr_mode(brw, format...))
    	return;
}

> +
> +   if (!mt)
> +      return;

This can't be right for gen9. You just tried to deref mt above for the tr_mode
determination. I'm guessing you rebased this and originally you had an
allocation function for the mt - either way, this seems wrong.

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

Looking forward to the next rev :-)

-- 
Ben Widawsky, Intel Open Source Technology Center