[Intel-gfx] [PATCH 2/3] drm/i915/gt: Wait for CSB entries on Tigerlake

[Chris Wilson]

Quoting Chang, Bruce (2020-08-15 03:16:58)
> On 8/14/2020 5:36 PM, Chang, Bruce wrote:
> >
> >>>> @@ -2498,9 +2498,22 @@ invalidate_csb_entries(const u64 *first, 
> >>>> const u64 *last)
> >>>>     */
> >>>>    static inline bool gen12_csb_parse(const u64 *csb)
> >>>>    {
> >>>> -     u64 entry = READ_ONCE(*csb);
> >>>> -     bool ctx_away_valid = GEN12_CSB_CTX_VALID(upper_32_bits(entry));
> >>>> -     bool new_queue =
> >>>> +     bool ctx_away_valid;
> >>>> +     bool new_queue;
> >>>> +     u64 entry;
> >>>> +
> >>>> +     /* XXX HSD */
> >>>> +     entry = READ_ONCE(*csb);
> >>>> +     if (unlikely(entry == -1)) {
> >>>> +             preempt_disable();
> >>>> +             if (wait_for_atomic_us((entry = READ_ONCE(*csb)) != 
> >>>> -1, 50))
> >>>> +                     GEM_WARN_ON("50us CSB timeout");
> >>> Out tests showed that 10us is not long enough, but 20us worked well. So
> >>> 50us should be good enough.
> >
> > Just realized this may not fully work, as one of the common issue we 
> > run into is that higher 32bit is updated from the HW, but lower 32bit 
> > update at a later time: meaning the csb will read like 
> > 0xFFFFFFFF:xxxxxxxx (low:high) . So this check (!= -1) can still pass 
> > but with a partial invalid csb status. So, we may need to check each 
> > 32bit separately.
> >
> After tested, with the new 64bit read, the above issue never happened so 
> far. So, it seems this only applicable to 32bit read (CSB updated 
> between the two lower and high 32bit reads). Assuming the HW 64bit CSB 
> update is also atomic, the above code should be fine.

Fortunately for all the platforms we care about here, READ_ONCE(u64)
will be a single 64b read and so both lower/upper dwords will be pulled
from the same bus transfer. We really need a compiler warning for when
READ_ONCE() is not a singular atomic operation. atomic64_t has too much
connotation with cross-core atomicity for my liking when dealing with
[cacheable] mmio semantics.
-Chris