[Intel-gfx] [PATCH 1/2] drm/i915: Recheck breadcrumb seqno after an interrupt

[Tvrtko Ursulin]

On 16/02/2017 10:36, Chris Wilson wrote:
> On Thu, Feb 16, 2017 at 10:21:17AM +0000, Tvrtko Ursulin wrote:
>>
>> On 16/02/2017 09:29, Chris Wilson wrote:
>>> If an interrupt arrives whilst we are performing the irq-seqno barrier,
>>> recheck the seqno again before returning.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index c1b400f1ede4..ecb8b414bdd2 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -4102,6 +4102,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>>> 	if (__i915_gem_request_completed(req, seqno))
>>> 		return true;
>>>
>>> +	if (!engine->irq_seqno_barrier)
>>> +		return false;
>>> +
>>> 	/* Ensure our read of the seqno is coherent so that we
>>> 	 * do not "miss an interrupt" (i.e. if this is the last
>>> 	 * request and the seqno write from the GPU is not visible
>>> @@ -4113,8 +4116,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>>> 	 * but it is easier and safer to do it every time the waiter
>>> 	 * is woken.
>>> 	 */
>>> -	if (engine->irq_seqno_barrier &&
>>> -	    test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
>>> +	while (test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
>>> 		unsigned long flags;
>>>
>>> 		/* The ordering of irq_posted versus applying the barrier
>>>
>>
>> Hmmm.. if this helps it feels that there is a race somewhere.
>> Because we have processed one interrupt, but it wasn't for us.
>
> There may be many interrupts between now and the one we actually want.
> The caller of this function is (or was at the time) has the first seqno
> to be signaled, it is just not necessarily the next interrupt.
>
>> That
>> means there will be another one coming. Why handle that at this
>> level? Why check twice and not three, four, five times? :)
>
> Because they are all for us! This only saves a trip through schedule. If
> we don't loop here, we just loop at the next level.

Yes, which looks more correct to me. Because the caller then do what 
they want. Signaller just sleeps and i915_wait_request potentially busy 
waits for a bit.

So essentially this is just a performance optimisation for some cases, 
but we have to be sure it is not the opposite for some other ones.

Regards,

Tvrtko