[Intel-gfx] [PATCH 1/2] drm/i915: Recheck breadcrumb seqno after an interrupt

[Chris Wilson]

On Thu, Feb 16, 2017 at 11:19:41AM +0000, Tvrtko Ursulin wrote:
> 
> On 16/02/2017 10:50, Chris Wilson wrote:
> >On Thu, Feb 16, 2017 at 10:45:56AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 16/02/2017 10:36, Chris Wilson wrote:
> >>>On Thu, Feb 16, 2017 at 10:21:17AM +0000, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 16/02/2017 09:29, Chris Wilson wrote:
> >>>>>If an interrupt arrives whilst we are performing the irq-seqno barrier,
> >>>>>recheck the seqno again before returning.
> >>>>>
> >>>>>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>>---
> >>>>>drivers/gpu/drm/i915/i915_drv.h | 6 ++++--
> >>>>>1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>>index c1b400f1ede4..ecb8b414bdd2 100644
> >>>>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>>@@ -4102,6 +4102,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
> >>>>>	if (__i915_gem_request_completed(req, seqno))
> >>>>>		return true;
> >>>>>
> >>>>>+	if (!engine->irq_seqno_barrier)
> >>>>>+		return false;
> >>>>>+
> >>>>>	/* Ensure our read of the seqno is coherent so that we
> >>>>>	 * do not "miss an interrupt" (i.e. if this is the last
> >>>>>	 * request and the seqno write from the GPU is not visible
> >>>>>@@ -4113,8 +4116,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
> >>>>>	 * but it is easier and safer to do it every time the waiter
> >>>>>	 * is woken.
> >>>>>	 */
> >>>>>-	if (engine->irq_seqno_barrier &&
> >>>>>-	    test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
> >>>>>+	while (test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
> >>>>>		unsigned long flags;
> >>>>>
> >>>>>		/* The ordering of irq_posted versus applying the barrier
> >>>>>
> >>>>
> >>>>Hmmm.. if this helps it feels that there is a race somewhere.
> >>>>Because we have processed one interrupt, but it wasn't for us.
> >>>
> >>>There may be many interrupts between now and the one we actually want.
> >>>The caller of this function is (or was at the time) has the first seqno
> >>>to be signaled, it is just not necessarily the next interrupt.
> >>>
> >>>>That
> >>>>means there will be another one coming. Why handle that at this
> >>>>level? Why check twice and not three, four, five times? :)
> >>>
> >>>Because they are all for us! This only saves a trip through schedule. If
> >>>we don't loop here, we just loop at the next level.
> >>
> >>Yes, which looks more correct to me. Because the caller then do what
> >>they want. Signaller just sleeps and i915_wait_request potentially
> >>busy waits for a bit.
> >
> >That's incorrect.
> 
> Which part is incorrect? I double checked and as far as I can see it
> is how I described it.

I had removed the spin part, so for both parties this was just in the
{ check; sleep; } loop. Note that in the case where we do need the
seqno-barrier, the barrier is more effective than spinning. :|

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 7760d7481f85..9e42b2687cae 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -993,6 +993,9 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
                                      seqno))
                        return true;
 
+               if (test_bit(ENGINE_IRQ_BREADCRUMB, &req->engine->irq_posted))
+                       break;
+
                if (signal_pending_state(state, current))
                        break;

which basically tells us that interrupt has arrived without the seqno
becoming visible, abort.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre