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

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/1] drm/i915: Don't take execlist spinlock when setting wedged
Date: Mon, 07 Nov 2016 15:01:05 +0200	[thread overview]
Message-ID: <87zilb5tri.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <20161107124750.GM30925@nuc-i3427.alporthouse.com>

Chris Wilson <chris@chris-wilson.co.uk> writes:

> 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@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@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!

Yeah it disables more than engines. Lets wait for irqsave flavour to land
and rethink. This patch can be ignored.

-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx