[Mesa-dev] [PATCH 4/4] i965: Fix remaining flush vs invalidate race conditions in brw_emit_pipe_control_flush.

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> 3 and 4 are
>
> Cc: "12.0 11.1 11.2" <mesa-stable at lists.freedesktop.org>

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 <jason at jlekstrand.net>
>
> 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()).

> --Jason
>
> On Thu, Jun 30, 2016 at 10:07 PM, Francisco Jerez <currojerez at riseup.net>
> 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
>>
>>
-------------- 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/20160701/4f9fd259/attachment-0001.sig>