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

[Nanley Chery]

On Thu, Jun 11, 2015 at 8:56 AM, Neil Roberts <neil at linux.intel.com> wrote:
> Hi Nanley,
>
> Could you explain the reasoning behind this patch? I can't find any
> mention of needing to align to the square of the block size in the docs.
>
Sure. I came to the conclusion that alignment must be squared due to 3
parts of the Bspec. The "Alignment Unit Size" table lists the i and j
values for each texture. The compressed textures have their alignment
values listed here as constants and "all others" are set by Surface
Horizontal Alignment in the RENDER_SURFACE_STATE object. In the
section concerning Calculating Texel Location for 2D Surfaces SKL+,
the formula to cache align the width of an LOD is "ALIGN_NPOT(w, i*p)
/ i*p" where i is the alignment width from the table (either a
constant or a variable set by RENDER_SURFACE_STATE) and p is the width
of the block (1 for non compressed textures). The formula is the same
for cache aligning the height. Since i and p, j and q are equal this
means that alignment units for compressed textures are effectively
squared.

Because the compressed texture alignments are constants in the table,
I thought that the HALIGN and VALIGN values in the
RENDER_SURFACE_STATE object were ignored. This was explicitly stated
to be true for IVB, but for SKL, there is no mention of this.

> I think how it works is that on Skylake you can pick any alignment value
> you want out of 4, 8 or 16 but for compressed textures that is counted
> in units of blocks rather than pixels. Currently we effectively always
> pick 4 for compressed textures because I don't think there's any reason
> to align to a higher value. The mt->align_w/h values are used for two
> things; in intel_miptree_set_total_width_height they are used to choose
> the positions for the mipmap images and after that they are only used to
> upload the alignment values as part of the surface state. On Skylake
> mt->align_w/h is temporarily set to be in units of pixels during
> brw_miptree_layout but at the end of the function it divides the values
> by the block size so that they will be in units of blocks ready for
> uploading as part of the surface state.
>
I agree with your explanation on what the mt->align_w/h values are
used for. I thought that the RENDER_SURFACE_STATE values 4, 8, and 16
would only used if the alignment value is not a constant defined in
the "Alignment Unit Size" table. This is mentioned in the section
concerning Calculating Texel Location for 2D Surfaces SKL+. My comment
below points towards this not being the case.

> Your patch modifies how it picks the alignment value but it doesn't
> modify the bit at the end which divides the chosen alignment by the
> block size. For FXT1 I think that would end up making it pick a
> horizontal alignment value of ALIGN_8 (ie, the pixel alignment value is
> now 8*8=64 which when divided by the block width ends up as
> 64/8=ALIGN_8).
>
> Before my patches for Skylake bits of the code were lazily assuming that
> mt->align_w/h is always the same as the block size because that
> previously happened to be always correct. I fixed some of the cases
> within the layouting code to instead directly query the block size.
> However it looks like I missed one in intel_miptree_copy_slice. I think
> your patch would make this particular case work because now the
> alignment value is ALIGN_8 which happens to match the block width for
> FXT1.
>
You're right, it does work because the align_w is set to ALIGN_8.
Hardcoding the alignment to ALIGN_4 causes the test to fail. Perhaps
the i and j must match the RENDER_SURFACE_STATE values. I can't find
this explicitly stated in the documentation unfortunately.

> I'm about to post an alternative patch which fixes this case to use the
> block size instead and it does seem to fix the broken FXT1 test cases as
> well.
>

Thanks,
Nanley