[Intel-gfx] [PATCH 2/4] drm/i915: Synchronize irq before parking each engine

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-24 10:19:15)
> Chris Wilson <chris at 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 at chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> >> Cc: Imre Deak <imre.deak at 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?

Not for an extended period of time. tasklet_disable() has the
side-effect of spinning if a tasklet is scheduled between the
disable/enable calls. (It doesn't prevent the disabled tasklet from
being queued and doesn't remove it from the queue if it runs whilst
disabled.)
-Chris