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@xxxxxxxxxxxxxxxxxx>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> >---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
