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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 22 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-03-22 09:34:18)
>> On Wed, 21 Mar 2018, Chris Wilson <chris@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@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.
>
> The complaint was only in jest. The reminder to document locks and mb is
> indeed invaluable, just sometimes the limitation of being a "dumb" perl
> script show through.

Oh, I didn't misread you. I just switched to serious mode because we do
need to evaluate whether the checkpatch reports from CI are net positive
or negative, and, either way, what can we do to further improve the S/N.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx