[Intel-gfx] [PATCH] drm/i915: Remove the impedance mismatch around intel_engine_enable_signaling

[Chris Wilson]

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 <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > Cc: Michal Winiarski <michal.winiarski at 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