[Mesa-dev] [PATCH] i965: Fix vertical alignment for multisampled buffers.

[Kenneth Graunke]

On 11/12/2013 01:29 PM, Paul Berry wrote:
> From the Sandy Bridge PRM, Vol 1 Part 1 7.18.3.4 (Alignment Unit
> Size):
> 
>     j [vertical alignment] = 4 for any render target surface is
>     multisampled (4x)
> 
> From the Ivy Bridge PRM, Vol 4 Part 1 2.12.2.1 (SURFACE_STATE for most
> messages), under the "Surface Vertical Alignment" heading:
> 
>     This field is intended to be set to VALIGN_4 if the surface was
>     rendered as a depth buffer, for a multisampled (4x) render target,
>     or for a multisampled (8x) render target, since these surfaces
>     support only alignment of 4.
> 
> Back in 2012 when we added multisampling support to the i965 driver,
> we forgot to update the logic for computing the vertical alignment, so
> we were often using a vertical alignment of 2 for multisampled
> buffers, leading to subtle rendering errors.
> 
> Note that the specs also require a vertical alignment of 4 for all
> Y-tiled render target surfaces; I plan to address that in a separate
> patch.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53077
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 11 +++++++----
>  1 file changed, 7 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 d912862..d05dbeb 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -86,7 +86,7 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,
>  
>  static unsigned int
>  intel_vertical_texture_alignment_unit(struct brw_context *brw,
> -                                     gl_format format)
> +                                      gl_format format, bool multisampled)
>  {
>     /**
>      * From the "Alignment Unit Size" section of various specs, namely:
> @@ -110,8 +110,6 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>      *
>      * On SNB+, non-special cases can be overridden by setting the SURFACE_STATE
>      * "Surface Vertical Alignment" field to VALIGN_2 or VALIGN_4.
> -    *
> -    * We currently don't support multisampling.
>      */
>     if (_mesa_is_format_compressed(format))
>        return 4;
> @@ -119,6 +117,9 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>     if (format == MESA_FORMAT_S8)
>        return brw->gen >= 7 ? 8 : 4;
>  
> +   if (multisampled)
> +      return 4;
> +
>     GLenum base_format = _mesa_get_format_base_format(format);
>  
>     if (brw->gen >= 6 &&
> @@ -276,8 +277,10 @@ brw_miptree_layout_texture_3d(struct brw_context *brw,
>  void
>  brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt)
>  {
> +   bool multisampled = mt->num_samples > 1;
>     mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt->format);
> -   mt->align_h = intel_vertical_texture_alignment_unit(brw, mt->format);
> +   mt->align_h =
> +      intel_vertical_texture_alignment_unit(brw, mt->format, multisampled);
>  
>     switch (mt->target) {
>     case GL_TEXTURE_CUBE_MAP:
> 

Cc: mesa-stable at lists.freedesktop.org
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

However, you might want to just pass the miptree to the
texture_alignment_unit() functions.  That way, you can check mt->tiling too.