On Jun 30, 2016 9:25 PM, "Francisco Jerez" <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > Fwiw, I very much like the way I did this in the Vulkan driver where it > > splits it into two pipe controls automatically based on the input bits. > > (Look at genX_cmd_buffer.c cmd_buffer_apply_pipe_flushes.) I very much > > doubt that this is the only place we have this problem in the GL driver. > > Yes, it definitely isn't. > > > We should probably fix it in brw_emit_pipe_control. > > I had been poking around with making brw_emit_pipe_control_flush (I > think that's what you meant by brw_emit_pipe_control?) catch the racy > invalidate+flush combinations and emit two pipe controls instead. I'm > not terribly attached to the idea of hard-coding this much policy into > the rather low-level brw_emit_pipe_control_flush(), but it is sure less > code (and more idiot-proof) than fixing all of its users individually. > > In case other people don't like it I have another series that fixes the > remaining PIPE_CONTROL races manually and just adds an assertion to > brw_emit_pipe_control_flush() checking that the caller is not trying to > flush and invalidate at the same time in a single PIPE_CONTROL command, > I don't really care doing it one way or another. Since you've already written all that code, you should be in top form for answering this question: Is there anywhere in the code where we emit a pipe control containing both flushes and invalidates but *don't* want the stall? If not, then I'm a big fan of idiot-proof. > I'll reply to this thread with the fixes I've written which apply on top > of Alejandro's patch -- I believe we want to land v3 of his fix anyway > because it replaces a call to brw_emit_mi_flush [AKA "I don't know how > to use pipe control, just flush everything for me" ;)] with the right > sequence of cache flushes and invalidations, which is always a good > thing. > > > On Jun 30, 2016 12:00 AM, "Alejandro Piñeiro" <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote: > > > > Fixes: > > GL44-CTS.texture_barrier_ARB.same-texel-rw-multipass > > > > On Haswell, Broadwell and Skylake (note that in order to execute that > > test, it is needed to override GL and GLSL versions). > > > > On gen6 this test was already working without this change. It keeps > > working after it. > > > > This commit replaces the call to brw_emit_mi_flush for gen6+ with two > > calls to brw_emit_pipe_control_flush: > > > > * The first one with RENDER_TARGET_FLUSH and CS_STALL set to initiate > > a render cache flush after any concurrent rendering completes and > > cause the CS to stop parsing commands until the render cache > > becomes coherent with memory. > > > > * The second one have TEXTURE_CACHE_INVALIDATE set (and no CS stall) > > to clean up any stale data from the sampler caches before rendering > > continues. > > > > Didn't touch gen4-5, basically because I don't have a way to test > > them. > > > > More info on commits: > > 0aa4f99f562a05880a779707cbcd46be459863bf > > 72473658c51d5e074ce219c1e6385a4cce29f467 > > > > Thanks to Curro to help to tracking this down, as the root case was a > > hw race condition. > > > > v2: use two calls to pipe_control_flush instead of a combination of > > gen7_emit_cs_stall_flush and brw_emit_mi_flush calls (Curro) > > v3: no need to const cache invalidation (Curro) > > --- > > > > FWIW: checked with the CTS tests, and the piglit series, and confirmed > > that the const cache invalidation is not needed. > > > > src/mesa/drivers/dri/i965/intel_tex.c | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/mesa/drivers/dri/i965/intel_tex.c > > b/src/mesa/drivers/dri/i965/intel_tex.c > > index cac33ac..a802d5a 100644 > > --- a/src/mesa/drivers/dri/i965/intel_tex.c > > +++ b/src/mesa/drivers/dri/i965/intel_tex.c > > @@ -9,6 +9,7 @@ > > #include "intel_mipmap_tree.h" > > #include "intel_tex.h" > > #include "intel_fbo.h" > > +#include "intel_reg.h" > > > > #define FILE_DEBUG_FLAG DEBUG_TEXTURE > > > > @@ -362,7 +363,25 @@ intel_texture_barrier(struct gl_context *ctx) > > { > > struct brw_context *brw = brw_context(ctx); > > > > - brw_emit_mi_flush(brw); > > + if (brw->gen >= 6) { > > + if (brw->gen == 6) { > > + /* [Dev-SNB{W/A}]: Before a PIPE_CONTROL with Write Cache > > + * Flush Enable = 1, a PIPE_CONTROL with any non-zero > > + * post-sync-op is required. > > + */ > > + brw_emit_post_sync_nonzero_flush(brw); > > + } > > + > > + brw_emit_pipe_control_flush(brw, > > + PIPE_CONTROL_DEPTH_CACHE_FLUSH | > > + PIPE_CONTROL_RENDER_TARGET_FLUSH | > > + PIPE_CONTROL_CS_STALL); > > + > > + brw_emit_pipe_control_flush(brw, > > + PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE); > > + } else { > > + brw_emit_mi_flush(brw); > > + } > > } > > > > void > > -- > > 2.7.4 > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>