[Intel-gfx] [PATCH] drm/i915/guc: keep breadcrumb irq always enabled

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-08-12 23:10:16)
> We rely on the tasklet to update the GT PM refcount, so we can't disable
> it even if we've processed all the requests for the engine because we
> might have detected the request completion before the interrupt arrived.
> 
> Since on all platforms on which we plan to support guc submission we
> don't allow disabling the breadcrumb interrupts, we can further siplify
> the park/unpark flow by removing the interrupt pin/unpin. A BUG_ON has
> been added to catch changes to this flow that would require us to
> restore some kind of pinning.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 22 ----------------
>  drivers/gpu/drm/i915/gt/intel_engine.h        |  3 ---
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 ++++++++-----------
>  3 files changed, 11 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index ceba1da61967..15bbdd8c7552 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -212,28 +212,6 @@ static void signal_irq_work(struct irq_work *work)
>         intel_engine_breadcrumbs_irq(engine);
>  }
>  
> -void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine)
> -{
> -       struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -
> -       spin_lock_irq(&b->irq_lock);
> -       if (!b->irq_enabled++)
> -               irq_enable(engine);
> -       GEM_BUG_ON(!b->irq_enabled); /* no overflow! */
> -       spin_unlock_irq(&b->irq_lock);
> -}
> -
> -void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine)
> -{
> -       struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -
> -       spin_lock_irq(&b->irq_lock);
> -       GEM_BUG_ON(!b->irq_enabled); /* no underflow! */
> -       if (!--b->irq_enabled)
> -               irq_disable(engine);
> -       spin_unlock_irq(&b->irq_lock);
> -}

Could you split this to a second patch? The last draft of the pv-engine
was still using this pin_irq.

> -
>  static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>  {
>         struct intel_engine_cs *engine =
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index e1228b0e577f..bc694adcd9ea 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -338,9 +338,6 @@ void intel_engine_init_execlists(struct intel_engine_cs *engine);
>  void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>  
> -void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine);
> -void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine);
> -
>  void intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 449ca6357018..deb054eeb37c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1059,18 +1059,6 @@ static void guc_interrupts_release(struct intel_gt *gt)
>         rps->pm_intrmsk_mbz &= ~ARAT_EXPIRED_INTRMSK;
>  }
>  
> -static void guc_submission_park(struct intel_engine_cs *engine)
> -{
> -       intel_engine_unpin_breadcrumbs_irq(engine);
> -       engine->flags &= ~I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> -}
> -
> -static void guc_submission_unpark(struct intel_engine_cs *engine)
> -{
> -       engine->flags |= I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> -       intel_engine_pin_breadcrumbs_irq(engine);
> -}
> -
>  static void guc_set_default_submission(struct intel_engine_cs *engine)
>  {
>         /*
> @@ -1088,8 +1076,8 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
>  
>         engine->execlists.tasklet.func = guc_submission_tasklet;
>  
> -       engine->park = guc_submission_park;
> -       engine->unpark = guc_submission_unpark;
> +       /* do not use execlists park/unpark */
> +       engine->park = engine->unpark = NULL;
>  
>         engine->reset.prepare = guc_reset_prepare;
>         engine->reset.reset = guc_reset;
> @@ -1098,6 +1086,15 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
>         engine->cancel_requests = guc_cancel_requests;
>  
>         engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
> +       engine->flags |= I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> +
> +       /*
> +        * For the breadcrumb irq to work we need the interrupts to stay
> +        * enabled. However, on all platforms on which we'll have support for
> +        * GuC submission we don't allow disabling the interrupts at runtime, so
> +        * we're always safe with the current flow.
> +        */
> +       GEM_BUG_ON(engine->irq_enable || engine->irq_disable);

After a momentary panic, yes.
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris