[Intel-gfx] [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs

[Chris Wilson]

On Mon, Feb 27, 2017 at 10:31:36AM +0000, Tvrtko Ursulin wrote:
> 
> On 24/02/2017 18:01, Chris Wilson wrote:
> >A significant cost in setting up a wait is the overhead of enabling the
> >interrupt. As we disable the interrupt whenever the queue of waiters is
> >empty, if we are frequently waiting on alternating batches, we end up
> >re-enabling the interrupt on a frequent basis. We do want to disable the
> >interrupt during normal operations as under high load it may add several
> >thousand interrupts/s - we have been known in the past to occupy whole
> >cores with our interrupt handler after accidentally leaving user
> >interrupts enabled. As a compromise, leave the interrupt enabled until
> >the next IRQ, or the system is idle. This gives a small window for a
> >waiter to keep the interrupt active and not be delayed by having to
> >re-enable the interrupt.
> >
> >v2: Restore hangcheck/missed-irq detection for continuations
> >v3: Be more careful restoring the hangcheck timer after reset
> >v4: Be more careful restoring the fake irq after reset (if required!)
> >v5: Redo changes to intel_engine_wakeup()
> >
> >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 linux.intel.com>
> >---
> > drivers/gpu/drm/i915/i915_gem.c          |   4 +-
> > drivers/gpu/drm/i915/i915_irq.c          |   2 +
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 137 ++++++++++++++++++++-----------
> > drivers/gpu/drm/i915/intel_ringbuffer.h  |   7 +-
> > 4 files changed, 97 insertions(+), 53 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 01dbba3813c7..0ad080984877 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2988,8 +2988,10 @@ i915_gem_idle_work_handler(struct work_struct *work)
> > 	if (wait_for(intel_execlists_idle(dev_priv), 10))
> > 		DRM_ERROR("Timeout waiting for engines to idle\n");
> >
> >-	for_each_engine(engine, dev_priv, id)
> >+	for_each_engine(engine, dev_priv, id) {
> >+		intel_engine_disarm_breadcrumbs(engine);
> > 		i915_gem_batch_pool_fini(&engine->batch_pool);
> >+	}
> >
> > 	GEM_BUG_ON(!dev_priv->gt.awake);
> > 	dev_priv->gt.awake = false;
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >index 7607aba35f26..ba0bb33e12ed 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -1058,6 +1058,8 @@ static void notify_ring(struct intel_engine_cs *engine)
> > 			rq = wait->request;
> >
> > 		wake_up_process(wait->tsk);
> >+	} else {
> >+		__intel_engine_disarm_breadcrumbs(engine);
> > 	}
> > 	spin_unlock(&engine->breadcrumbs.lock);
> >
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index d7511e89c8ab..8e83be343057 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -36,8 +36,8 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> > 	wait = engine->breadcrumbs.first_wait;
> > 	if (wait) {
> > 		ret = ENGINE_WAKEUP_WAITER;
> >-		if (!wake_up_process(wait->tsk))
> >-			ret |= ENGINE_WAKEUP_ACTIVE;
> >+		if (wake_up_process(wait->tsk))
> >+			ret |= ENGINE_WAKEUP_ASLEEP;
> 
> Why did you want to reverse the bit and all?

Because we need to keep the hangcheck alive whilst there is no waiter -
only when we declare the GT as idle do we disarm.

> > 	}
> > 	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
> >
> >@@ -54,7 +54,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> > 	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
> > 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >
> >-	if (!b->irq_enabled)
> >+	if (!b->irq_armed)
> > 		return;
> >
> > 	if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
> >@@ -67,7 +67,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> > 	 * to process the pending interrupt (e.g, low priority task on a loaded
> > 	 * system) and wait until it sleeps before declaring a missed interrupt.
> > 	 */
> >-	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
> >+	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
> > 		mod_timer(&b->hangcheck, wait_timeout());
> > 		return;
> > 	}

-- 
Chris Wilson, Intel Open Source Technology Centre