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

[Jason Ekstrand]

On Fri, Oct 6, 2017 at 11:13 AM, Nanley Chery <nanleychery at gmail.com> wrote:

> On Thu, Oct 05, 2017 at 10:02:40PM -0700, Kenneth Graunke wrote:
> > Jason and I investigated several OpenGL CTS failures where the tests
> > bind the same texture for rendering and texturing, at the same time.
> > This has defined results as long as the reads happen before writes,
> > or the regions are non-overlapping.  Normally, this just works out.
> >
> > However, CCS can cause problems.  If the shader is reading one set of
> > pixels, and writing to different pixels that are adjacent, they may end
> > up being covered by the same CCS block.  So rendering may be writing a
> > CCS block, while the sampler is trying to read it.  Corruption ensues.
> >
> > Disabling CCS is unfortunate, but safe.
> >
> > Fixes several KHR-GL45.texture_barrier.* subtests.
> >
> > Cc: nanleychery at gmail.com
> > Cc: jason at jlekstrand.net
> > ---
> >  src/mesa/drivers/dri/i965/brw_draw.c          |  8 +++++++-
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 +++++++------
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
> >  3 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index cab3758d7b5..c13fa8c367a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -383,7 +383,13 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
> >           translate_tex_format(brw, tex_obj->_Format,
> sampler->sRGBDecode);
> >        int drawbuf_idx = get_drawbuffer_index(brw, tex_obj->mt->bo);
> >
> > -      bool aux_supported;
> > +      /* Disable auxiliary buffers if there's a self-dependency, where
> > +       * we're both texturing from and rendering to the same buffer.
> > +       * It's not necessarily safe - concurrent reads and writes to the
> > +       * CCS buffer can result in incorrect pixels.
> > +       */
> > +      bool aux_supported = drawbuf_idx == -1;
> > +
> >        intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
> >                                      &aux_supported);
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 5b7cde82f65..29b93dd656c 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -2651,9 +2651,10 @@ intel_miptree_prepare_texture_slices(struct
> brw_context *brw,
> >                                       enum isl_format view_format,
> >                                       uint32_t start_level, uint32_t
> num_levels,
> >                                       uint32_t start_layer, uint32_t
> num_layers,
> > -                                     bool *aux_supported_out)
> > +                                     bool *aux_supported)
>
> I find the name of this new parameter a little misleading. It suggests
> that the caller can determine that the aux surface is usable during the
> texture operation, when in fact it is the
> intel_miptree_texture_aux_usage function in the callee which does this
> determination. Since the caller can determine that the aux surface
> cannot be used, perhaps something like disable_aux would be a better
> fit?
>

I was thinking the same thing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171006/9c7e2e30/attachment-0001.html>