[Mesa-dev] [PATCH] i965/gen7: workaround alpha blending problems with CMS MSAA buffers.
Paul Berry
stereotype441 at gmail.com
Mon Nov 4 15:19:04 PST 2013
On 4 November 2013 14:44, Chris Forbes <chrisf at ijw.co.nz> wrote:
> Is this broken the same way on all of IVB|HSW|BYT?
>
Good question. I'd tested IVB and HSW but not BYT. I've now tested all
three and the bug is present on all three platforms.
>
> On Tue, Nov 5, 2013 at 11:24 AM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > i965/gen7 hardware doesn't perform alpha blending correctly when
> > compressed (CMS) multisampled buffers are in use. Therefore, we need
> > to detect when alpha blending is used on a compressed multisampled
> > buffer, and convert the buffer to uncompressed (UMS) layout.
> >
> > Once a given buffer has been converted from CMS to UMS, we leave it in
> > UMS mode forever after, because (a) the conversion is expensive, and
> > (b) it seems likely that if alpha blending is performed on a buffer
> > once, it will probably be performed again on subsequent frames.
> >
> > Fixes glitches in GTA San Andreas under Wine.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53077
> > ---
> > src/mesa/drivers/dri/i965/brw_blorp.h | 14 ++++
> > src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 51 +++++++++++++
> > src/mesa/drivers/dri/i965/brw_context.h | 1 +
> > src/mesa/drivers/dri/i965/brw_draw.c | 5 +-
> > src/mesa/drivers/dri/i965/brw_misc_state.c | 104
> +++++++++++++++++++++++++++
> > src/mesa/drivers/dri/i965/gen6_cc.c | 2 +
> > src/mesa/drivers/dri/i965/gen7_blorp.cpp | 2 +-
> > 7 files changed, 176 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h
> b/src/mesa/drivers/dri/i965/brw_blorp.h
> > index 07ab805..bbc610d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp.h
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
> > @@ -54,6 +54,9 @@ void
> > brw_blorp_resolve_color(struct brw_context *brw,
> > struct intel_mipmap_tree *mt);
> >
> > +void
> > +brw_blorp_cms_to_ums(struct brw_context *brw, struct intel_mipmap_tree
> *mt);
> > +
> > #ifdef __cplusplus
> > } /* end extern "C" */
> >
> > @@ -356,6 +359,17 @@ public:
> > virtual uint32_t get_wm_prog(struct brw_context *brw,
> > brw_blorp_prog_data **prog_data) const;
> >
> > + /**
> > + * Force the destination to use uncompressed multisampling
> > + * (INTEL_MSAA_LAYOUT_UMS) instead of compressed multisampling.
> This is
> > + * used when we are converting a framebuffer from an uncompressed to
> a
> > + * compressed layout.
> > + */
> > + void force_dst_to_ums()
> > + {
> > + this->dst.msaa_layout = INTEL_MSAA_LAYOUT_UMS;
> > + }
> > +
> > private:
> > brw_blorp_blit_prog_key wm_prog_key;
> > };
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > index 7e436f7..6d558d8 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > @@ -167,6 +167,57 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> > intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level,
> dst_layer);
> > }
> >
> > +
> > +/**
> > + * Convert a multisampled framebuffer from compressed layout
> > + * (INTEL_MSAA_LAYOUT_CMS) to uncompressed layout
> (INTEL_MSAA_LAYOUT_UMS).
> > + *
> > + * The conversion happens in place--no extra buffer needs to be
> allocated.
> > + */
> > +void
> > +brw_blorp_cms_to_ums(struct brw_context *brw, struct intel_mipmap_tree
> *mt)
> > +{
> > + assert(mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS);
> > +
> > + /* Multisampled buffers aren't mipmapped, so the code below only has
> to fix
> > + * up miplevel 0.
> > + */
> > + assert(mt->first_level == 0 && mt->last_level == 0);
> > +
> > + /* Cubemaps can't be multisampled, so the code below doesn't have to
> > + * correct by a factor of 6 when counting layers.
> > + */
> > + assert(mt->target != GL_TEXTURE_CUBE_MAP);
> > +
> > + for (unsigned layer = 0; layer < mt->logical_depth0; layer++) {
> > + /* Note: normally blitting from one miptree to itself is
> undefined if
> > + * the source and destination rectangles overlap, because (a) the
> 3D
> > + * pipeline doesn't make any guarantee as to what order the
> pixels are
> > + * processed in, and (b) the source and destination rectangles are
> > + * accessed through different (non-coherent) caches. However, in
> this
> > + * case it's ok, since the source and destination rectangles are
> > + * identical, so (a) order doesn't matter because every pixel is
> copied
> > + * to itself, and (b) cache coherency isn't a problem because
> every
> > + * pixel is read exactly once and then written exactly once.
> > + */
> > + brw_blorp_blit_params params(brw,
> > + mt, 0, layer,
> > + mt, 0, layer,
> > + 0, 0,
> > + mt->logical_width0,
> mt->logical_height0,
> > + 0, 0,
> > + mt->logical_width0,
> mt->logical_height0,
> > + GL_NEAREST, false, false);
> > + params.force_dst_to_ums();
> > + brw_blorp_exec(brw, ¶ms);
> > + }
> > +
> > + intel_miptree_release(&mt->mcs_mt);
> > + mt->msaa_layout = INTEL_MSAA_LAYOUT_UMS;
> > + mt->mcs_state = INTEL_MCS_STATE_NONE;
> > +}
> > +
> > +
> > static void
> > do_blorp_blit(struct brw_context *brw, GLbitfield buffer_bit,
> > struct intel_renderbuffer *src_irb,
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> > index 7aacf4f..85cb567 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -1466,6 +1466,7 @@ void brw_get_depthstencil_tile_masks(struct
> intel_mipmap_tree *depth_mt,
> > uint32_t *out_tile_mask_y);
> > void brw_workaround_depthstencil_alignment(struct brw_context *brw,
> > GLbitfield clear_mask);
> > +void brw_workaround_cms_blending(struct brw_context *brw);
> >
> > /* brw_object_purgeable.c */
> > void brw_init_object_purgeable_functions(struct dd_function_table
> *functions);
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 0acd089..50c0ddc 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -355,10 +355,11 @@ static bool brw_try_draw_prims( struct gl_context
> *ctx,
> >
> > intel_prepare_render(brw);
> >
> > - /* This workaround has to happen outside of brw_upload_state()
> because it
> > - * may flush the batchbuffer for a blit, affecting the state flags.
> > + /* These workarounds have to happen outside of brw_upload_state()
> because
> > + * they may flush the batchbuffer for a blit, affecting the state
> flags.
> > */
> > brw_workaround_depthstencil_alignment(brw, 0);
> > + brw_workaround_cms_blending(brw);
> >
> > /* Resolves must occur after updating renderbuffers, updating
> context state,
> > * and finalizing textures but before setting up any hardware state
> for
> > diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c
> b/src/mesa/drivers/dri/i965/brw_misc_state.c
> > index 70b0dbd..95836fa 100644
> > --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> > @@ -36,6 +36,7 @@
> > #include "intel_mipmap_tree.h"
> > #include "intel_regions.h"
> >
> > +#include "brw_blorp.h"
> > #include "brw_context.h"
> > #include "brw_state.h"
> > #include "brw_defines.h"
> > @@ -1081,3 +1082,106 @@ const struct brw_tracked_state
> brw_state_base_address = {
> > },
> > .emit = upload_state_base_address
> > };
> > +
> > +
> > +/**
> > + * Return true if the color buffer indicated by \c b needs the CMS
> blending
> > + * workaround applied to it.
> > + *
> > + * When the CMS MSAA layout is in use, the hardware's pixel backend
> always
> > + * seems to write new color values into the buffer correctly, but it
> doesn't
> > + * always read old values from the buffer correctly. Therefore,
> whenever the
> > + * blending/logic-op mode causes the final value of covered samples to
> depend
> > + * on the old values of those samples, we need to do a workaround.
> > + */
> > +static inline bool
> > +cms_blending_workaround_needed(struct brw_context *brw, int b)
> > +{
> > + struct gl_context *ctx = &brw->ctx;
> > + struct gl_renderbuffer *rb = ctx->DrawBuffer->_ColorDrawBuffers[b];
> > + struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> > + if (irb->mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)
> > + return false;
> > +
> > + /* _NEW_COLOR */
> > + if (ctx->Color.ColorLogicOpEnabled) {
> > + switch (ctx->Color.LogicOp) {
> > + case GL_CLEAR:
> > + case GL_COPY:
> > + case GL_COPY_INVERTED:
> > + case GL_SET:
> > + /* In these modes, the final sample value depends only on the
> > + * "source" value from the shader; they don't depend on the
> > + * destination sample value.
> > + */
> > + return false;
> > + default:
> > + return true;
> > + }
> > + }
> > +
> > + if (!(ctx->Color.BlendEnabled & (1 << b)))
> > + return false;
> > +
> > + GLenum eqRGB = ctx->Color.Blend[b].EquationRGB;
> > + if (eqRGB == GL_MIN || eqRGB == GL_MAX)
> > + return true;
> > + switch (ctx->Color.Blend[b].DstRGB) {
> > + case GL_ZERO:
> > + break;
> > + case GL_ONE_MINUS_DST_ALPHA:
> > + /* If the buffer is missing an alpha channel, alpha is effectively
> > + * always 1.0, so GL_ONE_MINUS_DST_ALPHA mode reduces to GL_ZERO
> mode.
> > + */
> > + return _mesa_base_format_has_channel(rb->_BaseFormat,
> > + GL_TEXTURE_ALPHA_TYPE);
> > + default:
> > + return true;
> > + }
> > +
> > + if (_mesa_base_format_has_channel(rb->_BaseFormat,
> GL_TEXTURE_ALPHA_TYPE)) {
> > + GLenum eqA = ctx->Color.Blend[b].EquationA;
> > + if (eqA == GL_MIN || eqA == GL_MAX)
> > + return true;
> > + if (ctx->Color.Blend[b].DstA != GL_ZERO)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +
> > +/**
> > + * When the CMS MSAA layout is in use, the hardware's pixel backend
> always
> > + * seems to write new color values into the buffer correctly, but it
> doesn't
> > + * always read old values from the buffer correctly.
> > + *
> > + * Therefore, if we're rendering in a way that causes the pixel backend
> to
> > + * read color values from the buffer, we need to first convert from the
> CMS to
> > + * the UMS layout.
> > + */
> > +void
> > +brw_workaround_cms_blending(struct brw_context *brw)
> > +{
> > + struct gl_context *ctx = &brw->ctx;
> > +
> > + if (!(brw->NewGLState & (_NEW_BUFFERS | _NEW_COLOR)))
> > + return;
> > +
> > + /* Prior to Gen7, compressed multisampling wasn't supported.
> Hopefully
> > + * this bug is fixed in Gen8.
> > + */
> > + if (brw->gen != 7)
> > + return;
> > +
> > + /* _NEW_BUFFERS */
> > + int nr_draw_buffers = ctx->DrawBuffer->_NumColorDrawBuffers;
> > + for (int b = 0; b < nr_draw_buffers; b++) {
> > + if (!cms_blending_workaround_needed(brw, b))
> > + continue;
> > +
> > + struct gl_renderbuffer *rb =
> ctx->DrawBuffer->_ColorDrawBuffers[b];
> > + struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> > + brw_blorp_cms_to_ums(brw, irb->mt);
> > + }
> > +}
> > diff --git a/src/mesa/drivers/dri/i965/gen6_cc.c
> b/src/mesa/drivers/dri/i965/gen6_cc.c
> > index 45c926c..f1222a3 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_cc.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_cc.c
> > @@ -30,11 +30,13 @@
> > #include "brw_defines.h"
> > #include "brw_util.h"
> > #include "intel_batchbuffer.h"
> > +#include "intel_fbo.h"
> > #include "main/macros.h"
> > #include "main/enums.h"
> > #include "main/glformats.h"
> > #include "main/stencil.h"
> >
> > +
> > static void
> > gen6_upload_blend_state(struct brw_context *brw)
> > {
> > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > index 71f31b7..d66e5cc 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > @@ -196,7 +196,7 @@ gen7_blorp_emit_surface_state(struct brw_context
> *brw,
> > surf[3] = pitch_bytes - 1;
> >
> > surf[4] = gen7_surface_msaa_bits(surface->num_samples,
> surface->msaa_layout);
> > - if (surface->mt->mcs_mt) {
> > + if (surface->mt->mcs_mt && surface->msaa_layout !=
> INTEL_MSAA_LAYOUT_UMS) {
> > gen7_set_surface_mcs_info(brw, surf, wm_surf_offset,
> surface->mt->mcs_mt,
> > is_render_target);
> > }
> > --
> > 1.8.4.2
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131104/771ea23a/attachment-0001.html>
More information about the mesa-dev
mailing list