[Mesa-dev] [PATCH] i965: correct alignment units for 2D compressed textures on Skylake

[Nanley Chery]

On Thu, Jun 11, 2015 at 9:04 AM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Wed, Jun 10, 2015 at 05:01:44PM -0700, Nanley Chery wrote:
>> From: Nanley Chery <nanley.g.chery at intel.com>
>>
>> On Gen9+, vertical and horizontal alignment values for compressed textures are
>> equal to the pre-Gen9 value squared. Each miplevel must be aligned to this
>> value.
>>
>> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
>
> While not a requirement, for future reference you should add Cc on patches which
> are touching something someone recently changed/and or was working on. In this
> case Ccing Neil would have been great.
>
I thought about this after sending out the patch unfortunately, thanks
for the reminder.

>> ---
>>
>> This fixes an FXT1 Piglit test regression and shows no failures on Jenkins.
>
> You ran full piglit on SKL? Jenkins won't and it's pretty important for this
> patch.
>
I recently ran piglit with:
./piglit run gpu -x glx -gx glean -x fbo-depth-array. There are no
regressions and 9 additional fixes. Unfortunately, my Piglit tests
haven't been consistent, so please take the results with a grain of
salt.

>>
>>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index 312a887..dffc699 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -149,15 +149,8 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,
>>        unsigned int i, j;
>>        _mesa_get_format_block_size(mt->format, &i, &j);
>>
>> -      /* On Gen9+ we can pick our own alignment for compressed textures but it
>> -       * has to be a multiple of the block size. The minimum alignment we can
>> -       * pick is 4 so we effectively have to align to 4 times the block
>> -       * size
>> -       */
>> -      if (brw->gen >= 9)
>> -         return i * 4;
>> -      else
>> -         return i;
>> +      /* On Gen9+ the alignment value is squared. */
>> +      return brw->gen >= 9 ? i * i : i;
>
> I don't think this is right. Isn't this going to push non compressed textures to
> an invalid HALIGN when we divide later ie. don't you get 1?
>
The divide only occurs for compressed textures.

>>      }
>>
>>     if (mt->format == MESA_FORMAT_S_UINT8)
>> @@ -269,9 +262,12 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>>      * Where "*" means either VALIGN_2 or VALIGN_4 depending on the setting of
>>      * the SURFACE_STATE "Surface Vertical Alignment" field.
>>      */
>> -   if (_mesa_is_format_compressed(mt->format))
>> +   if (_mesa_is_format_compressed(mt->format)) {
>> +      unsigned int i, j;
>> +      _mesa_get_format_block_size(mt->format, &i, &j);
>>        /* See comment above for the horizontal alignment */
>
> I think you need to kill this comment now. Doesn't it refer to what you killed
> above?
>
I replaced the comment with another, so it's still valid. I can just
replace it with the same one-liner though.

>> -      return brw->gen >= 9 ? 16 : 4;
>> +      return brw->gen >= 9 ? j * j : j;
>> +   }
>
> It kind of looks like this is just working around the way Neil implemented the
> divide later on. There's probably a nicer way to do the right thing, but I
> haven't tried it myself. Also, I believe this doesn't actually fix anything, it
> just uses slightly more optimal aligns - ie. maybe do a separate patch. No big
> deal though.
>
The logic is derived from the Bspec. After doing more testing and
getting Neil's feedback, I'm not completely sure the alignment is
correct. I'm looking into it more.

>>
>>     if (mt->format == MESA_FORMAT_S_UINT8)
>>        return brw->gen >= 7 ? 8 : 4;
>> @@ -379,7 +375,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>>
>>         if (mt->compressed) {
>>            mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>> -             ALIGN(minify(mt->physical_width0, 2), bw);
>> +             ALIGN(minify(mt->physical_width0, 2), mt->align_w);
>>         } else {
>>            mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>>               minify(mt->physical_width0, 2);
>