[Mesa-dev] [PATCH 1/9] i965/gen6+: Invalidate constant cache on brw_emit_mi_flush().

[Kenneth Graunke]

On Tuesday, December 13, 2016 1:10:22 PM PST Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > On Friday, December 9, 2016 11:03:24 AM PST Francisco Jerez wrote:
> >> In order to make sure that the constant cache is coherent with
> >> previous rendering when we start using it for pull constant loads.
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_pipe_control.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> index dd426bf..b8f7406 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> @@ -351,6 +351,7 @@ brw_emit_mi_flush(struct brw_context *brw)
> >>        int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
> >>        if (brw->gen >= 6) {
> >>           flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
> >> +                  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
> >>                    PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >>                    PIPE_CONTROL_VF_CACHE_INVALIDATE |
> >>                    PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
> >> 
> >
> > As we've talked about before...brw_emit_mi_flush() basically means
> > "I didn't actually think about what kind of flushing I need, so I threw
> > random bits at it, and hoped the problems would somehow go away."
> >
> > I guess we may as well add this to the hodgepodge, since we haven't
> > replaced all uses of brw_emit_mi_flush() yet...but I sort of wonder
> > whether it does anything useful.
> >
> > Does anything actually call brw_emit_mi_flush between draws?  (It did
> > two years ago, but you and I changed a number of flushes since then...)
> >
> > The use case I can think of is:
> >
> > 1. App binds a buffer object as an SSBO, atomic counter buffer,
> 
> These (and image stores and atomics) will be handled by
> brw_memory_barrier(), which already takes care of invalidating the
> constant cache.

Oh, right, MemoryBarrier() handles this.  Makes sense.  Sounds like we
don't need any additional flushing then.

> >    transform feedback output, pipeline statistics buffer, or whatever
> 
> I'm not convinced that coherency is currently managed correctly for
> these by the driver (e.g. brw_end_transform_feedback() calls
> brw_emit_mi_flush(), and so does hsw_end_transform_feedback(), but only
> *before* writing its result into the XFBO, so it won't necessarily be
> visible to shaders if the XFBO is immediately reused as e.g. UBO).
> Either way the situation should be no worse than using the sampler.

I can totally believe this is broken.  There are some test failures on
Haswell that I can fix with additional flushing.

> >    (but not rendering - you can't render to a buffer object).
> 
> You can indirectly :), by e.g. binding the buffer as PBO and calling
> glReadPixels -- Which will call brw_emit_mi_flush() after rendering,
> eventually hitting the constant cache invalidation introduced above.

Bah...can't bind it via a framebuffer, but...yes, there is that :)
Good catch.

I'm fine with this patch.  We should just kill off mi_flush in the
not-too-distant future.
-------------- 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/20161213/5e262815/attachment-0001.sig>