[Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.

[Kenneth Graunke]

On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
> On October 6, 2017 8:00:04 PM Kenneth Graunke <kenneth at whitecape.org> wrote:
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c 
> > b/src/mesa/drivers/dri/i965/brw_draw.c
> > index c7ed7284501..fcb194dbe86 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> >     }
> >  }
> >
> > +/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
> > + * or shader image.  This causes a self-dependency, where both rendering
> > + * and sampling may concurrently read or write the CCS buffer, causing
> > + * incorrect pixels.
> > + *
> > + * We don't support sampling from CCS_D, so this only matters for CCS_E.
> > + */
> >  static bool
> > -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo *bo)
> > +intel_disable_rb_aux_buffer(struct brw_context *brw,
> > +                            struct intel_mipmap_tree *tex_mt,
> > +                            const char *usage)
> >  {
> >     const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> >     bool found = false;
> >
> > +   /* Nothing to disable, don't bother looking */
> > +   if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> > +      return false;
> 
> As I mentioned in the office today, I'm not convinced this is actually 
> needed.  When it isn't CCS_E, we'll resolve anyway so passing disable_aux = 
> true won't hurt anything.

Yeah, that's true - but I figured we could avoid the overhead of the
loop in other cases, since it doesn't really matter either way.

Make sense?  Should we leave it?  Or would you rather drop it?

> > +
> >     for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
> >        const struct intel_renderbuffer *irb =
> >           intel_renderbuffer(fb->_ColorDrawBuffers[i]);
> >
> > -      if (irb && irb->mt->bo == bo) {
> > +      if (irb && irb->mt->bo == tex_mt->bo) {
> >           found = brw->draw_aux_buffer_disabled[i] = true;
> >        }
> >     }
> >
> > +   if (found) {
> > +      perf_debug("Disabling CCS because a renderbuffer is also bound %s.\n",
> > +                 usage);
> > +   }
> > +
> >     return found;
> >  }
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 670a92c1168..48392e7494a 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -2678,7 +2676,7 @@ intel_miptree_prepare_fb_fetch(struct brw_context *brw,
> >                                 uint32_t start_layer, uint32_t num_layers)
> >  {
> >     intel_miptree_prepare_texture_slices(brw, mt, mt->surf.format, level, 1,
> > -                                        start_layer, num_layers, NULL);
> > +                                        start_layer, num_layers, false);
> 
> I think we probably want true here.  It is for fb_fetch after all.  :-)  
> Also, we probably want to do disable_rb_aux_buffer for framebuffer fetch as 
> well.

Sure, I can do that.  It won't actually do anything, however, as this is
the code for non-coherent framebuffer fetch...and on Gen9+, we always do
a coherent fetch using the Render Target Read messages (even if the app
says that a non-coherent fetch would be fine).  And, Gen9+ is the only
platform where CCS_E exists.  So, the only platforms where this matters
don't use this code :)

Still, I think your suggestion would make the code clearer, and would
future-proof it in case we ever decide that non-coherent fetches are
useful on modern hardware (say, if they're cheaper someday).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171006/475ff6f5/attachment-0001.sig>