Quoting Chris Wilson (2018-03-31 00:08:47)
> Quoting Chris Wilson (2018-03-22 07:35:32)
> > Using engine->irq_posted for execlists, we are not always serialised by
> > the tasklet as we supposed. On the reset paths, the tasklet is disabled
> > and ignored. Instead, we manipulate the engine->irq_posted directly to
> > account for the reset, but if an interrupt fired before the reset and so
> > wrote to engine->irq_posted, that write may not be flushed from the
> > local CPU's cacheline until much later as the tasklet is already active
> > and so does not generate a mb(). To correctly serialise the interrupt
> > with reset, we need serialisation on the set_bit() itself.
> >
> > And at last Mika can be happy.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > CC: Michel Thierry <michel.thierry@xxxxxxxxx>
> > Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index fa7310766217..27aee25429b7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> > bool tasklet = false;
> >
> > if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> > - if (READ_ONCE(engine->execlists.active)) {
> > - __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> > - tasklet = true;
> > - }
> > + if (READ_ONCE(engine->execlists.active))
> > + tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> > + &engine->irq_posted);
>
> This is driving me mad. A very rare missed interrupt unless we
> unconditionally kick tasklet:
>
> if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> - if (READ_ONCE(engine->execlists.active))
> - tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> - &engine->irq_posted);
> + if (READ_ONCE(engine->execlists.active)) {
> + set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> + tasklet = true;
> + }
> }
>
> I can't see why.
>
> Hmm, I wonder if we are seeing READ_ONCE(execlsts->active) false
> negatives.
Fortunately, doesn't appear to be that.
@@ -1405,9 +1405,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
bool tasklet = false;
if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
- if (READ_ONCE(engine->execlists.active))
- tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
- &engine->irq_posted);
+ GEM_BUG_ON(!READ_ONCE(execlists->tasklet.state) &&
+ test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted));
+ tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
+ &engine->irq_posted);
}
Hasn't even hit a BUG, which is a little disconcerting.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
