Quoting Tvrtko Ursulin (2018-03-09 17:24:33)
>
> On 08/03/2018 14:07, Chris Wilson wrote:
> > There is some redundancy between dma_fence->ops->enable_signaling (via
> > i915_fence_enable_signaling) and our backend,
> > intel_engine_enable_signaling() in that both levels recheck the fence
> > status multiple times. If we convert intel_engine_enable_signaling() to
> > return the information desired by dma_fence->ops->enable_signaling, we
> > can reduce i915_fence_enable_signaling to a simple stub and avoid
> > trying to reinterpret the same information.
> >
> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> > Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> > Cc: Michal Winiarski <michal.winiar...@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_request.c | 6 +-----
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 21 +++++++++++++--------
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
> > 3 files changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c
> > b/drivers/gpu/drm/i915/i915_request.c
> > index d437beac3969..2a841800d4cf 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -59,11 +59,7 @@ static bool i915_fence_signaled(struct dma_fence *fence)
> >
> > static bool i915_fence_enable_signaling(struct dma_fence *fence)
> > {
> > - if (i915_fence_signaled(fence))
> > - return false;
>
> This was based on hws seqno check...
>
> > -
> > - intel_engine_enable_signaling(to_request(fence), true);
> > - return !i915_fence_signaled(fence);
> > + return intel_engine_enable_signaling(to_request(fence), true);
> > }
> > @@ -768,11 +769,15 @@ void intel_engine_enable_signaling(struct
> > i915_request *request, bool wakeup)
> > */
> > spin_lock(&b->rb_lock);
> > insert_signal(b, request, seqno);
> > - wakeup &= __intel_engine_add_wait(engine, &request->signaling.wait);
> > + wakeup &= __intel_engine_add_wait(engine, wait);
> > spin_unlock(&b->rb_lock);
> >
> > - if (wakeup)
> > + if (wakeup) {
> > wake_up_process(b->signaler);
> > + return !intel_wait_complete(wait);
>
> ... and would now be based on breadcrumb waiter waking up and removing
> itself from the tree.
And don't forget the same HWS check before the waiter is inserted. So we
have the same guards as before, just inside yet another spinlock.
> So some potential latency where it wasn't before, well, enable-disable
> signalling cycles actually.
The extra steps would be the insert_signal(). Fwiw, we could reorder the
insert_signal() but frankly this, dma_fence enable signaling of an
inflight request, is not a fast path. More commonly we will be enabling
signaling on the request as it is submitted (where wakeup is false and
we know that the HWS cannot be complete).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
