[Intel-gfx] [PATCH] drm/i915: Do a quick check on whether the fence is already signaled first

[Tvrtko Ursulin]

On 26/04/2017 11:48, Chris Wilson wrote:
> On Wed, Apr 26, 2017 at 11:43:03AM +0100, Tvrtko Ursulin wrote:
>>
>> On 26/04/2017 11:15, Chris Wilson wrote:
>>> Now that we try to signal the fence from inside the interrupt handler,
>>> when we reach the signaler thread, the fence is most likely already
>>> signaled. Skip manipulating the bottom-half locks if this is so.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 8f52fd5f6102..5f79c8135b3f 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -616,9 +616,12 @@ static int intel_breadcrumbs_signaler(void *arg)
>>> 			request = i915_gem_request_get_rcu(request);
>>> 		rcu_read_unlock();
>>> 		if (signal_complete(request)) {
>>> -			local_bh_disable();
>>> -			dma_fence_signal(&request->fence);
>>> -			local_bh_enable(); /* kick start the tasklets */
>>> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>> +				      &request->fence.flags)) {
>>> +				local_bh_disable();
>>> +				dma_fence_signal(&request->fence);
>>> +				local_bh_enable(); /* kick start the tasklet */
>>> +			}
>>>
>>> 			spin_lock_irq(&b->rb_lock);
>>>
>>>
>>
>> What are you referring to by "bottom-half locks" in the commit msg?
>> AFAICS it would only skip kicking the tasklets with this change.
>> That may be worth it, I haven't measured, but I don't see a
>> difference wrt any locks.
>
> It's just the preempt enable/disable pair plus the function call

Okay then, with a corrected commit message:

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

>> In fact we could change this to:
>>
>> 	if (!dma_fence_signal(...)) {
>> 		local_bh_disable();
>> 		local_bh_enable();
>> 	}
>>
>> If we wanted to avoid touching the flags directly, but then would
>> have a function call..
>
> Yeah, plus that looks ridiculous ;)

Hey, we can always hide it in a wrapper! :))

Regards,

Tvrtko

> The biggest cost in the signaler thread is going to sleep.
> -Chris
>