On Thu, Jun 11, 2015 at 8:56 AM, Neil Roberts <[email protected]> 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 _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
