<div dir="ltr">On 12 November 2013 15:15, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On 12 November 2013 15:02, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On 11/12/2013 01:29 PM, Paul Berry wrote:<br>
> From the Sandy Bridge PRM, Vol 1 Part 1 7.18.3.4 (Alignment Unit<br>
> Size):<br>
><br>
>     j [vertical alignment] = 4 for any render target surface is<br>
>     multisampled (4x)<br>
><br>
> From the Ivy Bridge PRM, Vol 4 Part 1 2.12.2.1 (SURFACE_STATE for most<br>
> messages), under the "Surface Vertical Alignment" heading:<br>
><br>
>     This field is intended to be set to VALIGN_4 if the surface was<br>
>     rendered as a depth buffer, for a multisampled (4x) render target,<br>
>     or for a multisampled (8x) render target, since these surfaces<br>
>     support only alignment of 4.<br>
><br>
> Back in 2012 when we added multisampling support to the i965 driver,<br>
> we forgot to update the logic for computing the vertical alignment, so<br>
> we were often using a vertical alignment of 2 for multisampled<br>
> buffers, leading to subtle rendering errors.<br>
><br>
> Note that the specs also require a vertical alignment of 4 for all<br>
> Y-tiled render target surfaces; I plan to address that in a separate<br>
> patch.<br>
><br>
> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=53077" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=53077</a><br>
> ---<br>
>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 11 +++++++----<br>
>  1 file changed, 7 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c<br>
> index d912862..d05dbeb 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c<br>
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c<br>
> @@ -86,7 +86,7 @@ intel_horizontal_texture_alignment_unit(struct brw_context *brw,<br>
><br>
>  static unsigned int<br>
>  intel_vertical_texture_alignment_unit(struct brw_context *brw,<br>
> -                                     gl_format format)<br>
> +                                      gl_format format, bool multisampled)<br>
>  {<br>
>     /**<br>
>      * From the "Alignment Unit Size" section of various specs, namely:<br>
> @@ -110,8 +110,6 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,<br>
>      *<br>
>      * On SNB+, non-special cases can be overridden by setting the SURFACE_STATE<br>
>      * "Surface Vertical Alignment" field to VALIGN_2 or VALIGN_4.<br>
> -    *<br>
> -    * We currently don't support multisampling.<br>
>      */<br>
>     if (_mesa_is_format_compressed(format))<br>
>        return 4;<br>
> @@ -119,6 +117,9 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,<br>
>     if (format == MESA_FORMAT_S8)<br>
>        return brw->gen >= 7 ? 8 : 4;<br>
><br>
> +   if (multisampled)<br>
> +      return 4;<br>
> +<br>
>     GLenum base_format = _mesa_get_format_base_format(format);<br>
><br>
>     if (brw->gen >= 6 &&<br>
> @@ -276,8 +277,10 @@ brw_miptree_layout_texture_3d(struct brw_context *brw,<br>
>  void<br>
>  brw_miptree_layout(struct brw_context *brw, struct intel_mipmap_tree *mt)<br>
>  {<br>
> +   bool multisampled = mt->num_samples > 1;<br>
>     mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt->format);<br>
> -   mt->align_h = intel_vertical_texture_alignment_unit(brw, mt->format);<br>
> +   mt->align_h =<br>
> +      intel_vertical_texture_alignment_unit(brw, mt->format, multisampled);<br>
><br>
>     switch (mt->target) {<br>
>     case GL_TEXTURE_CUBE_MAP:<br>
><br>
<br>
</div></div>Cc: <a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a><br>
Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br>
<br>
However, you might want to just pass the miptree to the<br>
texture_alignment_unit() functions.  That way, you can check mt->tiling too.<br></blockquote><div><br></div></div></div><div>Yeah, that's reasonable.  I'll post a better series.</div><div> </div></div><br></div>
</div>
</blockquote></div><br></div><div class="gmail_extra">After further discussion on IRC, Ken, Eric and I decided that the patch should be landed as is.  I will post a follow-up series to deal with the tiling restrictions.<br>
<br></div><div class="gmail_extra">This should be cherry-picked back to 10.0 and 9.2.  Cc-ing stable.<br></div></div>