[Intel-gfx] [PATCH 6/6] drm/i915: Only attempt to signal the request once from the interrupt handler

[Tvrtko Ursulin]

On 15/03/2017 20:05, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 07:47:20PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2017 14:01, Chris Wilson wrote:
>>> Check that signaled bit on the request->fence before acquiring a
>>
>>             "is not set" ^
>>
>> Or "Check that the request has not been signalled before.." ?
>>
>>> reference to the request for signaling later in the interrupt handler.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_irq.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index 31f0d7c8992f..e646c4eba65d 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -1056,7 +1056,9 @@ static void notify_ring(struct intel_engine_cs *engine)
>>> 		 * and many waiters.
>>> 		 */
>>> 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
>>> -				      wait->seqno))
>>> +				      wait->seqno) &&
>>> +		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>> +			      &wait->request->fence.flags))
>>> 			rq = i915_gem_request_get(wait->request);
>>>
>>> 		wake_up_process(wait->tsk);
>>>
>>
>> Is this just to optimize things slightly? Does it happen
>> sufficiently often for it to be worth it?
>
> Even once would be enough. An unlocked read of fence.flags is much
> cheaper than the locked get/put we go through otherwise. Even if the
> check is a dud, the load of the cacheline prior to us making it
> exclusive is "free".
>
> I'm mostly thinking about the situations where we have pile of bare
> breadcrumbs where we may very well get interrupts faster than we can get
> rid of the intel_wait. Or perhaps a mythical slow device.

Makes sense now that you've said it. Maybe c&p into the commit as well. ;)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko