Re: [PATCH] drm/i915: Only start with the fake-irq timer if interrupts are dead

Tvrtko Ursulin
2017-02-17T14:59:46Z

On 17/02/2017 11:55, Chris Wilson wrote:
> As a backup to waiting on a user-interrupt from the GPU, we use a heavy
> and frequent timer to wake up the waiting process should we detect an
> inconsistency whilst waiting. After seeing a "missed interrupt", the
> next time we wait, we restart the heavy timer. This patch is more
> reluctant to restart the timer and will only do so if we have not see any
> interrupts since when we started the fake irq timer. If we are seeing
> interrupts, then the waiters are being woken normally and we had an
> incoherency that caused to miss last time - that is unlikely to reoccur
> and so taking the risk of stalling again seems pragmatic.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 83a8b67d6427..1719f9da13b8 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -107,6 +107,23 @@ static void irq_disable(struct intel_engine_cs *engine)
> spin_unlock(&engine->i915->irq_lock);
> }
>
> +static bool use_fake_irq(const struct intel_breadcrumbs *b)
> +{
> + const struct intel_engine_cs *engine =
> + container_of(b, struct intel_engine_cs, breadcrumbs);
> +
> + if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
> + return false;
> +
> + /* Only start with the heavy weight fake irq timer if we have not
> + * seen any interrupts since enabling it the first time. If the
> + * interrupts are still arriving, it means we made a mistake in our
> + * engine->seqno_barrier(), a timing error that should be transient
> + * and unlikely to reoccur.
> + */
> + return atomic_read(&engine->irq_count) == b->hangcheck_interrupts;
> +}
> +
> static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> {
> struct intel_engine_cs *engine =
> @@ -145,8 +162,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> b->irq_enabled = true;
> }
>
> - if (!b->irq_enabled ||
> - test_bit(engine->id, &i915->gpu_error.missed_irq_rings)) {
> + if (!b->irq_enabled || use_fake_irq(b)) {
> mod_timer(&b->fake_irq, jiffies + 1);
> i915_queue_hangcheck(i915);
> } else {
>

Very soothing now that I have discovered the pre-existing behaviour. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko