[Intel-gfx] [PATCH 3/3] drm/i915/gt: Eliminate the trylock for reading a timeline's hwsp

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-12-16 16:40:15)
> 
> On 12/12/2019 01:46, Chris Wilson wrote:
> > As we stash a pointer to the HWSP cacheline on the request, when reading
> > it we only need confirm that the cacheline is still valid by checking
> > that the request and timeline are still intact.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_timeline.c | 38 ++++++++----------------
> >   1 file changed, 13 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > index 728da39e8ace..566ce19bb0ea 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > @@ -515,6 +515,7 @@ int intel_timeline_read_hwsp(struct i915_request *from,
> >                            struct i915_request *to,
> >                            u32 *hwsp)
> >   {
> > +     struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
> >       struct intel_timeline *tl;
> >       int err;
> >   
> > @@ -527,33 +528,20 @@ int intel_timeline_read_hwsp(struct i915_request *from,
> >               return 1;
> >   
> >       GEM_BUG_ON(rcu_access_pointer(to->timeline) == tl);
> > -
> > -     err = -EAGAIN;
> > -     if (mutex_trylock(&tl->mutex)) {
> > -             struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
> > -
> > -             if (i915_request_completed(from)) {
> > -                     err = 1;
> > -                     goto unlock;
> > -             }
> > -
> > -             err = cacheline_ref(cl, to);
> > -             if (err)
> > -                     goto unlock;
> > -
> > -             if (likely(cl == tl->hwsp_cacheline)) {
> > -                     *hwsp = tl->hwsp_offset;
> > -             } else { /* across a seqno wrap, recover the original offset */
> > -                     *hwsp = i915_ggtt_offset(cl->hwsp->vma) +
> > -                             ptr_unmask_bits(cl->vaddr, CACHELINE_BITS) *
> > -                             CACHELINE_BYTES;
> > -             }
> > -
> > -unlock:
> > -             mutex_unlock(&tl->mutex);
> > +     err = cacheline_ref(cl, to);
> > +     if (err)
> > +             goto out;
> > +
> > +     *hwsp = tl->hwsp_offset;
> > +     if (unlikely(cl != READ_ONCE(tl->hwsp_cacheline))) {
> > +             /* across a seqno wrap, recover the original offset */
> > +             *hwsp = i915_ggtt_offset(cl->hwsp->vma) +
> > +                     ptr_unmask_bits(cl->vaddr, CACHELINE_BITS) *
> > +                     CACHELINE_BYTES;
> 
> There is some confusion here (for me) which timeline is which. "From" 
> timeline is which is unlocked now and cl and tl come from it. And that 
> is the signaling request.
> 
> It is just RCU which guarantees it is safe to dereference the timeline 
> on this request?

from->timeline looks reasonable. It's cacheline_ref(cl) that is hairy. I
was thinking that cacheline_ref() was actually a
i915_active_acquire_if_busy(), but it is not. And even if it were, we
need RCU protection on the cl.

Hmm. 
-Chris