[Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

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 at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> CC: Michel Thierry <michel.thierry at intel.com>
> Cc: Jeff McGee <jeff.mcgee at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  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.

Getting close to admitting defeat :(
-Chris