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

[Paul Berry]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131112/cd09fe91/attachment.html>