[Mesa-dev] [PATCH v3] i965: intel_texture_barrier reimplemented
Francisco Jerez
currojerez at riseup.net
Fri Jul 1 04:36:45 UTC 2016
Jason Ekstrand <jason at jlekstrand.net> writes:
> On Jun 30, 2016 9:25 PM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>>
>> Jason Ekstrand <jason at jlekstrand.net> 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.
Nope, we always want the the flush with CS stall followed by the R/O
cache invalidation as far as I'm aware.
>
>> 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" <apinheiro at igalia.com>
> 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
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160630/adde17a5/attachment.sig>
More information about the mesa-dev
mailing list