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

[Paul Berry]

On 12 November 2013 15:15, Paul Berry <stereotype441 at gmail.com> wrote:

> On 12 November 2013 15:02, Kenneth Graunke <kenneth at whitecape.org> wrote:
>
>> 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.
>>
>
> Yeah, that's reasonable.  I'll post a better series.
>
>
>
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.

This should be cherry-picked back to 10.0 and 9.2.  Cc-ing stable.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131115/7d6a68a9/attachment-0001.html>