[Intel-gfx] [PATCH v2 2/2] drm/i915: Delay disabling the user interrupt for breadcrumbs
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Feb 24 10:34:07 UTC 2017
On 23/02/2017 15:42, 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
>
> 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 | 5 +-
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 95 +++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-
> 4 files changed, 65 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a8167003c10b..3ec8c948fe45 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 3c79753edf0e..ba0bb33e12ed 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1036,9 +1036,6 @@ static void notify_ring(struct intel_engine_cs *engine)
> struct drm_i915_gem_request *rq = NULL;
> struct intel_wait *wait;
>
> - if (!intel_engine_has_waiter(engine))
> - return;
> -
> atomic_inc(&engine->irq_count);
> set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
>
> @@ -1061,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 5f2665aa817c..bf14022f6adb 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -35,8 +35,10 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> {
> struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + struct intel_wait *wait;
> + unsigned long flags;
>
> - if (!b->irq_enabled)
> + if (!b->irq_armed)
> return;
>
> if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
> @@ -49,10 +51,15 @@ 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)) {
> + spin_lock_irqsave(&b->lock, flags);
> + wait = b->first_wait;
> + if (!wait || !wake_up_process(wait->tsk)) {
> mod_timer(&b->hangcheck, wait_timeout());
> - return;
> + wait = NULL;
> }
> + spin_unlock_irqrestore(&b->lock, flags);
> + if (!wait)
> + return;
I don't see a difference versus the old code:
if (!intel_engine_wakeup(engine)) {
mod_timer(...);
return;
}
There is only one instance of the timer at a time so we don't need to
extend the lock to mod_timer. Or you were thinking
intel_engine_reset_breadcrumbs? But there we do del_timer_sync before
proceeding to touch the timer.
>
> DRM_DEBUG("Hangcheck timer elapsed... %s idle\n", engine->name);
> set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> @@ -110,6 +117,34 @@ static void irq_disable(struct intel_engine_cs *engine)
> spin_unlock(&engine->i915->irq_lock);
> }
>
> +void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> +{
> + struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> + assert_spin_locked(&b->lock);
> +
> + if (b->irq_enabled) {
> + irq_disable(engine);
> + b->irq_enabled = false;
> + }
> +
> + b->irq_armed = false;
> +}
> +
> +void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> +{
> + struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + unsigned long flags;
> +
> + if (!b->irq_armed)
> + return;
Sounds safer to put this check under the spinlock so reader can think
less. :) Or in fact do we even need the check? It's only the idle work
handler so could just have the below block I think.
> +
> + spin_lock_irqsave(&b->lock, flags);
> + if (!intel_engine_has_waiter(engine))
> + __intel_engine_disarm_breadcrumbs(engine);
> + spin_unlock_irqrestore(&b->lock, flags);
> +}
> +
> static bool use_fake_irq(const struct intel_breadcrumbs *b)
> {
> const struct intel_engine_cs *engine =
> @@ -134,9 +169,17 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> struct drm_i915_private *i915 = engine->i915;
>
> assert_spin_locked(&b->lock);
> - if (b->rpm_wakelock)
> + if (b->irq_armed)
> return;
>
> + /* The breadcrumb irq will be disarmed on the interrupt after the
> + * waiters are signaled. This gives us a single interrupt window in
> + * which we can add a new waiter and avoid the cost of re-enabling
> + * the irq.
> + */
> + b->irq_armed = true;
> + GEM_BUG_ON(b->irq_enabled);
> +
> if (I915_SELFTEST_ONLY(b->mock)) {
> /* For our mock objects we want to avoid interaction
> * with the real hardware (which is not set up). So
> @@ -145,17 +188,15 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> * implementation to call intel_engine_wakeup()
> * itself when it wants to simulate a user interrupt,
> */
> - b->rpm_wakelock = true;
> return;
> }
>
> /* Since we are waiting on a request, the GPU should be busy
> - * and should have its own rpm reference. For completeness,
> - * record an rpm reference for ourselves to cover the
> - * interrupt we unmask.
> + * and should have its own rpm reference. This is tracked
> + * by i915->gt.awake, we can forgo holding our own wakref
> + * for the interrupt as before i915->gt.awake is released (when
> + * the driver is idle) we disarm the breadcrumbs.
> */
> - intel_runtime_pm_get_noresume(i915);
> - b->rpm_wakelock = true;
>
> /* No interrupts? Kick the waiter every jiffie! */
> if (intel_irqs_enabled(i915)) {
> @@ -173,29 +214,6 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> }
> }
>
> -static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
> -{
> - struct intel_engine_cs *engine =
> - container_of(b, struct intel_engine_cs, breadcrumbs);
> -
> - assert_spin_locked(&b->lock);
> - if (!b->rpm_wakelock)
> - return;
> -
> - if (I915_SELFTEST_ONLY(b->mock)) {
> - b->rpm_wakelock = false;
> - return;
> - }
> -
> - if (b->irq_enabled) {
> - irq_disable(engine);
> - b->irq_enabled = false;
> - }
> -
> - intel_runtime_pm_put(engine->i915);
> - b->rpm_wakelock = false;
> -}
> -
> static inline struct intel_wait *to_wait(struct rb_node *node)
> {
> return rb_entry(node, struct intel_wait, node);
> @@ -415,7 +433,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
> wake_up_process(b->first_wait->tsk);
> } else {
> b->first_wait = NULL;
> - __intel_breadcrumbs_disable_irq(b);
> }
> } else {
> GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
> @@ -707,14 +724,16 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> cancel_fake_irq(engine);
> spin_lock_irq(&b->lock);
>
> - __intel_breadcrumbs_disable_irq(b);
> + if (b->irq_enabled)
> + irq_enable(engine);
> + else
> + irq_disable(engine);
> +
> if (intel_engine_has_waiter(engine)) {
> - __intel_breadcrumbs_enable_irq(b);
> if (test_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted))
> wake_up_process(b->first_wait->tsk);
> - } else {
> + mod_timer(&b->hangcheck, wait_timeout());
> /* sanitize the IMR and unmask any auxiliary interrupts */
> - irq_disable(engine);
> }
>
> spin_unlock_irq(&b->lock);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c90b60f47f25..9f89160c7de9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -246,8 +246,8 @@ struct intel_engine_cs {
>
> unsigned int hangcheck_interrupts;
>
> + bool irq_armed : 1;
> bool irq_enabled : 1;
> - bool rpm_wakelock : 1;
> I915_SELFTEST_DECLARE(bool mock : 1);
> } breadcrumbs;
>
> @@ -657,6 +657,8 @@ static inline bool intel_engine_wakeup(struct intel_engine_cs *engine)
> return wakeup;
> }
>
> +void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
> +void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
> void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
> void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
> bool intel_breadcrumbs_busy(struct intel_engine_cs *engine);
>
The rest looks OK, but I wonder if I missed something since the CI
wasn't happy with it?
Regards,
Tvrtko
More information about the Intel-gfx
mailing list