| Message ID | 20191212014629.854076-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Use EAGAIN for trylock failures | expand |
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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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? Regards, Tvrtko > } > - intel_timeline_put(tl); > > +out: > + intel_timeline_put(tl); > return err; > } > >
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@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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
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; } - intel_timeline_put(tl); +out: + intel_timeline_put(tl); return err; }
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@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gt/intel_timeline.c | 38 ++++++++---------------- 1 file changed, 13 insertions(+), 25 deletions(-)