[Intel-gfx] [PATCH 1/1] drm/i915: Don't take execlist spinlock when setting wedged

[Chris Wilson]

On Mon, Nov 07, 2016 at 02:35:38PM +0200, Mika Kuoppala wrote:
> We do take execlist spinlock on thread context when
> we declare gpu to be wedged. Avoid the need to change
> the spinlock type just for the sake of wedging by
> removing the spinlock. Keep irqs disabled during reset
> phase and only enable on success path. Also add explicit
> disable to setting wedged so that we leave irqs off
> if we fail init.

Technically that spinlock already needs irqsafe.
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 20 ++------------------
>  drivers/gpu/drm/i915/i915_drv.h | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c |  6 ++++--
>  3 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0213a30..2ed81f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1725,22 +1725,6 @@ int i915_resume_switcheroo(struct drm_device *dev)
>  	return i915_drm_resume(dev);
>  }
>  
> -static void disable_engines_irq(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -
> -	/* Ensure irq handler finishes, and not run again. */
> -	disable_irq(dev_priv->drm.irq);
> -	for_each_engine(engine, dev_priv, id)
> -		tasklet_kill(&engine->irq_tasklet);
> -}
> -
> -static void enable_engines_irq(struct drm_i915_private *dev_priv)
> -{
> -	enable_irq(dev_priv->drm.irq);
> -}
> -
>  /**
>   * i915_reset - reset chip after a hang
>   * @dev: drm device to reset
> @@ -1775,9 +1759,8 @@ void i915_reset(struct drm_i915_private *dev_priv)
>  
>  	pr_notice("drm/i915: Resetting chip after gpu hang\n");
>  
> -	disable_engines_irq(dev_priv);
> +	i915_disable_engine_irqs(dev_priv);
>  	ret = intel_gpu_reset(dev_priv, ALL_ENGINES);
> -	enable_engines_irq(dev_priv);
>  
>  	if (ret) {
>  		if (ret != -ENODEV)
> @@ -1812,6 +1795,7 @@ void i915_reset(struct drm_i915_private *dev_priv)
>  
>  wakeup:
>  	wake_up_bit(&error->flags, I915_RESET_IN_PROGRESS);
> +	i915_enable_engine_irqs(dev_priv);

To make this symmetric with set-wedged from init, it needs to be before
the wakeup:

Except that would be bad...

i915_disable_engine_irqs() doesn't just disable the GT interrupt, but
GMBUS, HPD, etc.

Since we already are planning to change this to use irqsafe spinlock
here, I think we just go forward with that plan. I thought there was
merit to having disable_engines_irq() for set-wedged, but it is
dangerously named!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre