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

[Kenneth Graunke]

On Saturday, October 7, 2017 8:03:54 AM PDT Jason Ekstrand wrote:
> On October 6, 2017 10:25:55 PM Kenneth Graunke <kenneth at whitecape.org> wrote:
> 
> > 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?
> 
> Leave it in but for a completely different reason than the one you stated. 
> :-)  I don't care all that much about trying to avoid a loop whose number 
> of iterations was bounded above by 8.

Well, it is nested inside a loop bounded by 32... :)

> If the have a multisampled self dependency, it should work fine because 
> compression is limited to a single pixel and we dispatch all the samples 
> for a given pixel together.

Probably not for 16x MSAA and SIMD8 shaders, but...it's probably really
uncommon...

> We shouldn't force a resolve so we don't want 
> to do this of its MCS.  In the case of CCS_D, I'm not sure if it's safe.  
> We will potentially try to sample from a CCS_D image as CCS_E on gen9+ so 
> it's probably best to disable it for that too.
-------------- 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/20171007/10329d4c/attachment.sig>