[Intel-gfx] [PATCH] drm/i915/execlists: Use a locked clear_bit() for synchronisation with interrupt

[Jani Nikula]

On Wed, 21 Mar 2018, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Quoting Michel Thierry (2018-03-21 17:01:12)
>> On 3/21/2018 3:46 AM, Mika Kuoppala wrote:
>> > Chris Wilson <chris at chris-wilson.co.uk> writes:
>> >> -            /* The write will be ordered by the uncached read (itself
>> >> -             * a memory barrier), so we do not need another in the form
>> >> -             * of a locked instruction. The race between the interrupt
>> >> -             * handler and the split test/clear is harmless as we order
>> >> -             * our clear before the CSB read. If the interrupt arrived
>> >> -             * first between the test and the clear, we read the updated
>> >> -             * CSB and clear the bit. If the interrupt arrives as we read
>> >> -             * the CSB or later (i.e. after we had cleared the bit) the bit
>> >> -             * is set and we do a new loop.
>> >> -             */
>> >> -            __clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>> >> +            /* Clear before reading to catch new interrupts */
>> >> +            clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>> >> +            smp_mb__after_atomic();
>> 
>> Checkpatch wants a comment for the memory barrier... Are we being strict 
>> about it? (https://patchwork.freedesktop.org/series/40359/)
>
> There's a comment for it not two lines above! Silly perl script.

Sure, it's nowhere near perfect. But I do like to get the reminder about
this, "hey don't forget to document your memory barriers, locks,
etc.". It does mean we can't use checkpatch for gating, but I think it
can make the reviewer's life easier to be able to just point at the
results, and ask the author to fix the relevant stuff. I think it's less
tedious and less offensive than the reviewer doing the job manually.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center