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.
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. 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. 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. Regards, - Neil Nanley Chery <nanleych...@gmail.com> writes: > From: Nanley Chery <nanley.g.ch...@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.ch...@intel.com> > --- > > This fixes an FXT1 Piglit test regression and shows no failures on Jenkins. > > 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; > } > > 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 */ > - return brw->gen >= 9 ? 16 : 4; > + return brw->gen >= 9 ? j * j : j; > + } > > 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); > -- > 2.4.2 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev