[Mesa-dev] [PATCH 1/2] i965/blorp: Increase Y alignment for multisampled stencil blits.

Kenneth Graunke kenneth at whitecape.org
Fri Sep 21 16:51:21 PDT 2012


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 <kenneth at whitecape.org>


More information about the mesa-dev mailing list