Quoting Mika Kuoppala (2017-10-24 10:19:15) > Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> > >> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > >> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > >> Cc: Imre Deak <imre.deak@xxxxxxxxx> > >> --- > >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
