Quoting Mika Kuoppala (2018-05-08 14:21:34)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>
> > We assume that the CSB is written using the normal ringbuffer
> > coherency protocols, as outlined in kernel/events/ring_buffer.c:
> >
> > * (HW) (DRIVER)
> > *
> > * if (LOAD ->data_tail) { LOAD ->data_head
> > * (A) smp_rmb() (C)
> > * STORE $data LOAD $data
> > * smp_wmb() (B) smp_mb() (D)
> > * STORE ->data_head STORE ->data_tail
> > * }
> >
> > So we assume that the HW fulfils it's ordering requirements (B), and so
> > we should use a complimentary rmb (C) to ensure that our read of its
> > WRITE pointer is completed before we start accessing the data.
> >
> > The final mb (D) is implied by the uncached mmio we perform to inform
> > the HW of our READ pointer.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=105064
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=105888
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106185
> > References: 61bf9719fa17 ("drm/i915/cnl: Use mmio access to context status buffer")
> > Suggested-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Rafael Antognolli <rafael.antognolli@xxxxxxxxx>
> > Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> > Cc: Timo Aaltonen <tjaalton@xxxxxxxxxx>
> > Tested-by: Timo Aaltonen <tjaalton@xxxxxxxxxx>
> > ---
> >
> > Just tweaked the commitmsg to cross reference the mb against the
> > diagram.
> > -Chris
> >
> > ---
> > drivers/gpu/drm/i915/intel_engine_cs.c | 3 ---
> > drivers/gpu/drm/i915/intel_lrc.c | 1 +
> > 2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 70325e0824e3..8303e05b0c7d 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -470,9 +470,6 @@ static bool csb_force_mmio(struct drm_i915_private *i915)
> > if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
> > return true;
> >
> > - if (IS_CANNONLAKE(i915))
> > - return true;
> > -
> > return false;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 911f288f78aa..8977600f0d81 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -992,6 +992,7 @@ static void execlists_submission_tasklet(unsigned long data)
> >
> > head = execlists->csb_head;
> > tail = READ_ONCE(buf[write_idx]);
> > + rmb(); /* Hopefully paired with a wmb() in HW */
>
> If the gpu does ordered writes (with write buffer in between), this is ok.
>
> If the gpu does not order writes, we would still need the rmb()
> to prevent cpu from loading an stale csb entry ahead of tail read?
>
> Quoting memory-barries.txt:
> " (*) loads may be done speculatively, leading to the result having been fetched
> at the wrong time in the expected sequence of events;
> "
>
> So I would change the comment to /* Enforce tail vs csb entry read order */
Barriers are always paired, and the comment should tell the reader where
the other barrier is. Otherwise we would just need read_barrier_depends()
to enforce the read order (which is just READ_ONCE).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
