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 <b...@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 <b...@bwidawsk.net> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
