[Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size

[Neil Roberts]

Sorry for the really long delay in replying! This patch is still needed
in order to fix a number of Piglit tests so it would be good to get it
landed.

Ben Widawsky <ben at bwidawsk.net> writes:

> Sorry for the delay, but I put this off initially because I wasn't
> sure which part of the docs this was addressing. I see that the
> Surface Horizontal Alignment field is now used for compressed formats.
> Assuming that's what this is referring to (if not, please correct
> me)...

Yes, that's right.

> The only thing that appears to be missing is the handling of the MCS
> case (must always be 16) which may or may not be relevant. AFAICT,
> it's a possible scenario.

The MCS buffer is only used when rendering right? I don't think it is
possible or would make sense to render to a compressed buffer. There is
already a section at the bottom of the function to handle the alignment
for uncompressed buffers when there is an MCS buffer. If it were
possible to use a compressed buffer with an MCS buffer then it would be
broken anyway even without this patch because it would just return the
block size which wouldn't be 16.

> Also, doesn't this make FXT1 have an alignment of 32?

Yes, but bear in mind that a row of 32 pixels only takes up 4 blocks so
that is only 64 bytes. I guess that is still quite a lot but if that's
what the hardware requires then that's what we have to do.

>>     if (_mesa_is_format_compressed(format))
>> -      return 4;
>> +      /* See comment above for the horizontal alignment */
>> +      return brw->gen >= 9 ? 16 : 4;
>
> This seems okay since the max height we support is 4.

What do you mean by the max height? I think previously this was just
hardcoding the block height to 4 so I am just bumping the alignment to
16 to represent 4 blocks.

> These hunks look okay to me. Any particular reason not to update
> brw_miptree_layout_texture_array as well? I am pretty certain we don't
> use ALL_SLICES_AT_EACH_LOD in Gen9 today, but presumably we could,
> right? /me shrugs

We actually can't use ALL_SLICES_AT_EACH_LOD on Gen9 (see the earlier
patches) but yes, brw_miptree_layout_texture_array needs to work for 2D
array textures. However I don't think anything needs changing there. The
only changes I made to brw_miptree_layout_2d were because it was
assuming that mt->align_w/h is always equal to the compressed block size
but with this patch that is no longer the case. I only changed the
places that wanted the block size so that they would use a separate
variable instead of conflating it with the alignment. I don't think
anything in brw_miptree_layout_texture_array needs to refer to the block
size so it should be ok.

- Neil