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

[Ben Widawsky]

On Fri, May 01, 2015 at 04:54:36PM +0100, Neil Roberts wrote:
> 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

Thanks for the explanations. lgtm
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>