On 09/12/2012 08:51 PM, Paul Berry wrote:
> This patch is a band-aid fix for a bug in commit 5fd67fa (i965/blorp:
> Reduce alignment restrictions for stencil blits), which causes
> multisampled stencil blits to work incorrectly on Sandy Bridge.
>
> When blitting to or from a normal stencil buffer, we have to use a
> coordinate transformation that swizzles coordinates to account for the
> fact that stencil buffers use W tiling, but the most similar tiling
> format available for textures and render targets is Y tiling. The
> differences between W and Y tiling cause pixels to be scrambled within
> a block of size 8x4 (width x height) as measured relative to a W tile,
> or 16x2 as measured relative to a Y tile. So in order to make sure
> that pixels at the edges of the blit aren't lost, we need to align the
> rendering rectangle (and the buffer sizes) to multiples of the 8x4
> block size. This alignment happens in the brw_blorp_blit_params
> constructor, whereas the determination of how to swizzle the
> coordinates happens during code generation, in the
> brw_blorp_blit_program class.
>
> When blitting to or from a multisampled stencil buffer, the coordinate
> swizzling is more complex, because it has to account for the
> interleaving pattern of samples, which uses 4x4 blocks for 4x MSAA and
> 8x4 blocks for 8x MSAA. The end result is that if multisampling is in
> use, the 16x2 block size (relative so a Y tile) needs to be expanded
> to 16x4, and the corresponding size relative to a W tile expands to
> 8x8.
>
> The problem doesn't affect Ivy Bridge severely enough to crop up in
> Piglit tests because on Ivy Bridge we have to disable multisampling
> when blitting *to* a multisampled stencil buffer (the blorp compiler
> generates code to compensate for the fact that multisampling is
> disabled). However I suspect a bug is still present because we don't
> disable multisampling when blitting *from* a multisampled stencil
> buffer.
>
> This patch fixes the problem by doubling the vertical alignment
> requirement when blitting to or from a multisampled stencil buffer,
> and multisampling has not been disabled.
>
> In the long run I would like to rework the brw_blorp_blit_params
> constructor--it's difficult to follow and has had several subtle bugs
> like this one. However this band-aid fix should be suitable for
> cherry-picking to release branches.
>
> Fixes Piglit tests "unaligned-blit {2,4} stencil {msaa,upsample}" on
> Sandy Bridge.
>
> NOTE: This is a candidate for stable release branches.
> ---
> src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> index 656a497..2207230 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> @@ -1797,6 +1797,11 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct
> brw_context *brw,
> * account for the differences in aspect ratio between the Y and W
> * sub-tiles. We need to modify the layer width and height similarly.
> *
> + * A correction needs to be applied when MSAA is in use: since
> + * INTEL_MSAA_LAYOUT_IMS uses an interleaving pattern whose height is
> 4,
> + * we need to align the Y coordinates to multiples of 8, so that when
> + * they are divided by two they are still multiples of 4.
> + *
> * Note: Since the x/y offset of the surface will be applied using the
> * SURFACE_STATE command packet, it will be invisible to the swizzling
> * code in the shader; therefore it needs to be in a multiple of the
> @@ -1818,7 +1823,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct
> brw_context *brw,
> * TODO: what if this makes the coordinates (or the texture size) too
> * large?
> */
> - const unsigned x_align = 8, y_align = 4;
> + const unsigned x_align = 8, y_align = dst.num_samples != 0 ? 8 : 4;
> x0 = ROUND_DOWN_TO(x0, x_align) * 2;
> y0 = ROUND_DOWN_TO(y0, y_align) / 2;
> x1 = ALIGN(x1, x_align) * 2;
> @@ -1840,7 +1845,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct
> brw_context *brw,
> *
> * TODO: what if this makes the texture size too large?
> */
> - const unsigned x_align = 8, y_align = 4;
> + const unsigned x_align = 8, y_align = src.num_samples != 0 ? 8 : 4;
> src.width = ALIGN(src.width, x_align) * 2;
> src.height = ALIGN(src.height, y_align) / 2;
> src.x_offset *= 2;
This looks reasonable to me. It's a simple fix, seems to make sense,
and fixes a piglit test.
For the series:
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
