<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Jul 1, 2016 at 5:43 PM, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > 3 and 4 are > > Cc: "12.0 11.1 11.2" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> Hmm, I'll cc PATCH 2 to mesa-stable too since technically it also fixes a bug. PATCH 1 shouldn't make much of a difference though. > Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > I did look over 3 fairly carefully. > Thanks. > It's worth noting that I think we have some double-pipe-controls that could > probably be put together now. Not sure that we actually want to do that > though. I don't think it matters much, usually it's nearly the same amount of lines of code to do it as one call to brw_emit_pipe_control_flush() or as two, both approaches are functionally equivalent, and doing it explicitly as two brw_emit_pipe_control_flush() calls makes it clear to the reader that the read cache invalidations are supposed to happen after the write caches are flushed. In some places though it's really convenient to be able to put all cache bits for which coherency is required into a single bitfield and have brw_emit_pipe_control_flush() figure out whether the command needs to be split (e.g. brw_memory_barrier()). </blockquote><div> </div><div>That sounds reasonable. I wasn't even really sure about it myself but thought it would be worth a few words of discussion. Sounds like we're on the same page. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > --Jason > > On Thu, Jun 30, 2016 at 10:07 PM, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> > wrote: > >> This hardware race condition has caused problems several times already >> (see "i965: Fix cache pollution race during L3 partitioning set-up.", >> "i965: Fix brw_render_cache_set_check_flush's PIPE_CONTROLs." and >> "i965: intel_texture_barrier reimplemented"). The problem is that >> whenever we attempt to both flush and invalidate multiple caches with >> a single pipe control command the flush and invalidation happen in >> reverse order, so the contents flushed from the R/W caches aren't >> guaranteed to become visible from the invalidated caches after the >> PIPE_CONTROL command completes execution if some concurrent rendering >> workload happened to pollute any of the invalidated R/O caches in the >> short window of time between the invalidation and flush. >> >> This makes sure that brw_emit_pipe_control_flush() has the effect >> expected by most callers of making the contents flushed from any R/W >> caches visible from the invalidated R/O caches. >> --- >> src/mesa/drivers/dri/i965/brw_pipe_control.c | 18 ++++++++++++++++++ >> src/mesa/drivers/dri/i965/intel_reg.h | 9 +++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c >> b/src/mesa/drivers/dri/i965/brw_pipe_control.c >> index 14a8f7c..05e8c05 100644 >> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c >> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c >> @@ -96,6 +96,24 @@ gen7_cs_stall_every_four_pipe_controls(struct >> brw_context *brw, uint32_t flags) >> void >> brw_emit_pipe_control_flush(struct brw_context *brw, uint32_t flags) >> { >> + if (brw->gen >= 6 && >> + (flags & PIPE_CONTROL_CACHE_FLUSH_BITS) && >> + (flags & PIPE_CONTROL_CACHE_INVALIDATE_BITS)) { >> + /* A pipe control command with flush and invalidate bits set >> + * simultaneously is an inherently racy operation on Gen6+ if the >> + * contents of the flushed caches were intended to become visible >> from >> + * any of the invalidated caches. Split it in two PIPE_CONTROLs, >> the >> + * first one should stall the pipeline to make sure that the >> flushed R/W >> + * caches are coherent with memory once the specified R/O caches are >> + * invalidated. On pre-Gen6 hardware the (implicit) R/O cache >> + * invalidation seems to happen at the bottom of the pipeline >> together >> + * with any write cache flush, so this shouldn't be a concern. >> + */ >> + brw_emit_pipe_control_flush(brw, (flags & >> PIPE_CONTROL_CACHE_FLUSH_BITS) | >> + PIPE_CONTROL_CS_STALL); >> + flags &= ~(PIPE_CONTROL_CACHE_FLUSH_BITS | PIPE_CONTROL_CS_STALL); >> + } >> + >> if (brw->gen >= 8) { >> if (brw->gen == 8) >> gen8_add_cs_stall_workaround_bits(&flags); >> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h >> b/src/mesa/drivers/dri/i965/intel_reg.h >> index 95365fe..7a82be4 100644 >> --- a/src/mesa/drivers/dri/i965/intel_reg.h >> +++ b/src/mesa/drivers/dri/i965/intel_reg.h >> @@ -134,6 +134,15 @@ >> #define PIPE_CONTROL_PPGTT_WRITE (0 << 2) >> #define PIPE_CONTROL_GLOBAL_GTT_WRITE (1 << 2) >> >> +#define PIPE_CONTROL_CACHE_FLUSH_BITS \ >> + (PIPE_CONTROL_DEPTH_CACHE_FLUSH | PIPE_CONTROL_DATA_CACHE_FLUSH | \ >> + PIPE_CONTROL_RENDER_TARGET_FLUSH) >> + >> +#define PIPE_CONTROL_CACHE_INVALIDATE_BITS \ >> + (PIPE_CONTROL_STATE_CACHE_INVALIDATE | >> PIPE_CONTROL_CONST_CACHE_INVALIDATE | \ >> + PIPE_CONTROL_VF_CACHE_INVALIDATE | >> PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE | \ >> + PIPE_CONTROL_INSTRUCTION_INVALIDATE) >> + >> /** @} */ >> >> #define XY_SETUP_BLT_CMD (CMD_2D | (0x01 << 22)) >> -- >> 2.9.0 >> >> </div></div></blockquote></div> </div></div>