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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
