Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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

> Quoting Chris Wilson (2017-10-23 22:32:35)
>> When we park the engine (upon idling), we kill the irq tasklet. However,
>> to be sure that it is not restarted by a final interrupt after doing so,
>> flush the interrupt handler before parking. As we only park the engines
>> when we believe the system is idle, there should not be any spurious
>> interrupts later to distrub us; so flushing the final in-flight
>> interrupt should be sufficient.
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index bb0e85043e01..b547a6327d34 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
>>         if (new_requests_since_last_retire(dev_priv))
>>                 goto out_unlock;
>>  
>> +       /*
>> +        * Be paranoid and flush a concurrent interrupt to make sure
>> +        * we don't reactivate any irq tasklets after parking.
>
> * FIXME: Note that even though we have waited for execlists to be idle,
> * there may still be an in-flight interrupt even though the CSB
> * is now empty. synchronize_irq() makes sure that the residual
> * interrupt is completed before we continue, but it doesn't prevent
> * the HW from raising that residual interrupt later. To complete
> * the shield we should coordinate disabling the CS irq with
> * flushing the residual interrupt.

Apparently tasklet_kill is 'broken' in a sence that
yeah new schedule reanimates the dead one. This
seems weird guarantee for an api of such name and
someone has even tried to fix it:

https://patchwork.kernel.org/patch/9298717/

I am pondering that can we combine the tasklet_disable
into the mix. We would just defer the tasklet until
we are ready and willing to process again?

-Mika

>
>> +        */
>> +       synchronize_irq(dev_priv->drm.irq);
>> +
>>         /*
>>          * We are committed now to parking the engines, make sure there
>>          * will be no more interrupts arriving later.
>> -- 
>> 2.15.0.rc1
>> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx