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

[Jason Ekstrand]

On Sat, Oct 7, 2017 at 11:42 AM, Kenneth Graunke <kenneth at whitecape.org>
wrote:

> 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... :)
>

Ok, fair enough.  But intel_miptree_prepare can do some nasty looping too.
In any case checking for certain aux_usages is a better plan than I thought.


> > 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...
>

And given that we don't have MSAA resolve code, it's probably a good idea
to not ask for a full resolve.  That would end badly. :-)


> > 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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171007/be789689/attachment-0001.html>