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

[Anuj Phogat]

On Tue, Jul 7, 2015 at 2:47 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, June 10, 2015 03:30:47 PM 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.
>> +       */
>> +      _mesa_is_format_color_format(mt->format) &&
>> +      (requested == INTEL_MIPTREE_TILING_Y ||
>> +       requested == INTEL_MIPTREE_TILING_ANY) &&
>> +      (bpp && is_power_of_two(bpp)) &&
>> +      /* FIXME: To avoid piglit regressions keep the Yf/Ys tiling
>> +       * disabled at the moment.
>> +       */
>> +      false;
>
> I must say, I was a bit surprised to see this land as is.  You've got a
> lot of conditions there, only to finish them up with && false - with a
> comment saying that your code isn't passing Piglit yet.  That doesn't
> really meet our usual qualifications for merging.
>
> Coverity also pointed out that your if (is_tr_mode_yf_ys_allowed) block
> below is dead code, issuing new warnings.
>
I agree I should have waited more before landing this patch and
especially this part of code you're pointing to doesn't look nice
upstream :(.
I'm happy to revert the changes related to 'is_tr_mode_yf_ys_allowed'
to avoid coverity warning or the whole patch if you think that's better?

As Ben suggested, I can use an env variable to enable Yf/Ys while
it's in development. Any thoughts?

> Forgive my ignorance, but what's the purpose of Yf/Ys tiling?  My
> understanding was that Ys is primarily in support of a new OpenGL
> feature - GL_ARB_spare_texture(*) - which isn't yet enabled:
>
> https://www.opengl.org/registry/specs/ARB/sparse_texture.txt
>
> Is Yf tiling supposed to be more efficient than legacy Y-tiling?  If so,
> then switching to it is an optimization, isn't it?  We usually require
> data indicating some kind of performance improvement (any kind!) before
> landing a bunch of code for optimizations.  Obviously that's pretty
> tricky with pre-release hardware, so I'd settle for "it's complete
> and functions correctly."
>
Looking at the Yf tiling details in the hw specs, It looks like a better
memory layout with possibility of performance improvement.
Considering that we're already using 4KB tiling patterns in our driver,
it made sense to try using Yf (4KB). I don't have much information
about the 64KB tiling use cases except that they might be useful in
cases of high resolution textures.

> At any rate, it's merged, and hopefully you're able to get it working...
>
>>
>> -   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;
>> +      }
>> +
>> +      if (!for_bo)
>> +         mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
>> +
>> +      if (is_tr_mode_yf_ys_allowed) {
>> +         unsigned int level = 0;
>> +         assert(brw->gen >= 9);
>> +
>> +         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++;
>> +   }
>>  }
>>
>>