[Intel-gfx] [PATCH 2/2] drm/i915: Check waiter->seqno carefully in case of preemption

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-09-19 10:28:42)
> 
> On 18/09/2017 17:27, Chris Wilson wrote:
> > If preemption occurs at precisely the right moment, we may decide that
> > the wait is complete even though the wait's request is no longer
> > executing (having been preempted). We handle this situation by double
> > checking that request following deciding whether the wait is complete.
> > 
> > Reported-by: Michał Winiarski <michal.winiarski at intel.com
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bb69c5b0efc4..7a53d94b7e61 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1053,10 +1053,13 @@ static void notify_ring(struct intel_engine_cs *engine)
> >                */
> >               if (i915_seqno_passed(intel_engine_get_seqno(engine),
> >                                     wait->seqno)) {
> > +                     struct drm_i915_gem_request *waiter = wait->request;
> > +
> >                       wakeup = true;
> >                       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > -                                   &wait->request->fence.flags))
> > -                             rq = i915_gem_request_get(wait->request);
> > +                                   &waiter->fence.flags) &&
> > +                         intel_wait_check_request(wait, waiter))
> > +                             rq = i915_gem_request_get(waiter);
> >               }
> >   
> >               if (wakeup)
> > 
> 
> Hm but as the user interrupt is nor serialized to exelists, what 
> prevents the preemption to happen after the intel_wait_check_request and 
> before dma_fence_signal?

The preemption is serialized to the breadcrumb, so that our
understanding of i915_gem_request_completed() is accurate. It would be
a nasty bug if we flagged DMA_FENCE_FLAG_SIGNALED_BIT prior to
i915_gem_request_completed().

Here, wait_check_request just confirms that the wait->seqno still
matches the rq->global_seqno. So as we have already found that the
breadcrumb has passed wait->seqno, that means that the request is ready
to wake up. We only do the wake avoidance if we believe that we can
trust the order of MI_USER_INTERRUPT with the breadcrumb write, so that
then we know that if the request is not yet ready, we will receive
another interrupt in the future. (On the other side, if the wait queue
is manipulated, it ensures that the new waiter is woken to do the
breadcrumb check to avoid missed interrupts.)

So... If the preemption happens after intel_wait_check_request, it does
not affect the prior completed request.
-Chris