Re: [Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size
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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size
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
Re: [Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size
On Fri, Feb 20, 2015 at 10:31:07PM +, Neil Roberts wrote:
On Skylake it is possible to choose your own alignment values for
compressed textures but they are expressed as a multiple of the block
size. The minimum alignment value we can use is 4 so we effectively
have to align to 4 times the block size. This patch makes it initially
set mt-align_[wh] to the large alignment value and then later divides
it by the block size so that it can be uploaded as part of the surface
state.
---
src/mesa/drivers/dri/i965/brw_tex_layout.c | 31
++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
b/src/mesa/drivers/dri/i965/brw_tex_layout.c
index d89a04e..1fe2c26 100644
--- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
+++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
@@ -73,7 +73,16 @@ intel_horizontal_texture_alignment_unit(struct brw_context
*brw,
*/
unsigned int i, j;
_mesa_get_format_block_size(format, i, j);
- return i;
+
+ /* 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;
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)...
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. Also, doesn't this make FXT1 have an alignment of 32?
}
if (format == MESA_FORMAT_S_UINT8)
@@ -113,7 +122,8 @@ intel_vertical_texture_alignment_unit(struct brw_context
*brw,
* the SURFACE_STATE Surface Vertical Alignment field.
*/
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.
if (format == MESA_FORMAT_S_UINT8)
return brw-gen = 7 ? 8 : 4;
@@ -195,6 +205,9 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
unsigned width = mt-physical_width0;
unsigned height = mt-physical_height0;
unsigned depth = mt-physical_depth0; /* number of array layers. */
+ unsigned int bw, bh;
+
+ _mesa_get_format_block_size(mt-format, bw, bh);
mt-total_width = mt-physical_width0;
@@ -212,7 +225,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), mt-align_w);
+ ALIGN(minify(mt-physical_width0, 2), bw);
} else {
mip1_width = ALIGN(minify(mt-physical_width0, 1), mt-align_w) +
minify(mt-physical_width0, 2);
@@ -232,7 +245,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
img_height = ALIGN(height, mt-align_h);
if (mt-compressed)
- img_height /= mt-align_h;
+ img_height /= bh;
if (mt-array_layout == ALL_SLICES_AT_EACH_LOD) {
/* Compact arrays with separated miplevels */
@@ -481,5 +494,15 @@ brw_miptree_layout(struct brw_context *brw, struct
intel_mipmap_tree *mt)
}
DBG(%s: %dx%dx%d\n, __FUNCTION__,
mt-total_width, mt-total_height, mt-cpp);
+
+ /* On Gen9+ the alignment values are expressed in multiples of the block
+* size
+*/
+ if (brw-gen = 9) {
+ unsigned int i, j;
+ _mesa_get_format_block_size(mt-format, i, j);
+ mt-align_w /= i;
+ mt-align_h /= j;
+ }
}
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
--
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 5/6] i965/skl: Align compressed textures to four times the block size
On Skylake it is possible to choose your own alignment values for
compressed textures but they are expressed as a multiple of the block
size. The minimum alignment value we can use is 4 so we effectively
have to align to 4 times the block size. This patch makes it initially
set mt-align_[wh] to the large alignment value and then later divides
it by the block size so that it can be uploaded as part of the surface
state.
---
src/mesa/drivers/dri/i965/brw_tex_layout.c | 31 ++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
b/src/mesa/drivers/dri/i965/brw_tex_layout.c
index d89a04e..1fe2c26 100644
--- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
+++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
@@ -73,7 +73,16 @@ intel_horizontal_texture_alignment_unit(struct brw_context
*brw,
*/
unsigned int i, j;
_mesa_get_format_block_size(format, i, j);
- return i;
+
+ /* 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;
}
if (format == MESA_FORMAT_S_UINT8)
@@ -113,7 +122,8 @@ intel_vertical_texture_alignment_unit(struct brw_context
*brw,
* the SURFACE_STATE Surface Vertical Alignment field.
*/
if (_mesa_is_format_compressed(format))
- return 4;
+ /* See comment above for the horizontal alignment */
+ return brw-gen = 9 ? 16 : 4;
if (format == MESA_FORMAT_S_UINT8)
return brw-gen = 7 ? 8 : 4;
@@ -195,6 +205,9 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
unsigned width = mt-physical_width0;
unsigned height = mt-physical_height0;
unsigned depth = mt-physical_depth0; /* number of array layers. */
+ unsigned int bw, bh;
+
+ _mesa_get_format_block_size(mt-format, bw, bh);
mt-total_width = mt-physical_width0;
@@ -212,7 +225,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), mt-align_w);
+ ALIGN(minify(mt-physical_width0, 2), bw);
} else {
mip1_width = ALIGN(minify(mt-physical_width0, 1), mt-align_w) +
minify(mt-physical_width0, 2);
@@ -232,7 +245,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
img_height = ALIGN(height, mt-align_h);
if (mt-compressed)
-img_height /= mt-align_h;
+img_height /= bh;
if (mt-array_layout == ALL_SLICES_AT_EACH_LOD) {
/* Compact arrays with separated miplevels */
@@ -481,5 +494,15 @@ brw_miptree_layout(struct brw_context *brw, struct
intel_mipmap_tree *mt)
}
DBG(%s: %dx%dx%d\n, __FUNCTION__,
mt-total_width, mt-total_height, mt-cpp);
+
+ /* On Gen9+ the alignment values are expressed in multiples of the block
+* size
+*/
+ if (brw-gen = 9) {
+ unsigned int i, j;
+ _mesa_get_format_block_size(mt-format, i, j);
+ mt-align_w /= i;
+ mt-align_h /= j;
+ }
}
--
1.9.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
