[Intel-gfx] [PATCH] drm/i915: Remove the impedance mismatch around intel_engine_enable_signaling

[Tvrtko Ursulin]

On 09/03/2018 17:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-03-09 17:24:33)
>>
>> On 08/03/2018 14:07, Chris Wilson wrote:
>>> There is some redundancy between dma_fence->ops->enable_signaling (via
>>> i915_fence_enable_signaling) and our backend,
>>> intel_engine_enable_signaling() in that both levels recheck the fence
>>> status multiple times. If we convert intel_engine_enable_signaling() to
>>> return the information desired by dma_fence->ops->enable_signaling, we
>>> can reduce i915_fence_enable_signaling to a simple stub and avoid
>>> trying to reinterpret the same information.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Michal Winiarski <michal.winiarski at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_request.c      |  6 +-----
>>>    drivers/gpu/drm/i915/intel_breadcrumbs.c | 21 +++++++++++++--------
>>>    drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>>>    3 files changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index d437beac3969..2a841800d4cf 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -59,11 +59,7 @@ static bool i915_fence_signaled(struct dma_fence *fence)
>>>    
>>>    static bool i915_fence_enable_signaling(struct dma_fence *fence)
>>>    {
>>> -     if (i915_fence_signaled(fence))
>>> -             return false;
>>
>> This was based on hws seqno check...
>>
>>> -
>>> -     intel_engine_enable_signaling(to_request(fence), true);
>>> -     return !i915_fence_signaled(fence);
>>> +     return intel_engine_enable_signaling(to_request(fence), true);
>>>    }
> 
>>> @@ -768,11 +769,15 @@ void intel_engine_enable_signaling(struct i915_request *request, bool wakeup)
>>>         */
>>>        spin_lock(&b->rb_lock);
>>>        insert_signal(b, request, seqno);
>>> -     wakeup &= __intel_engine_add_wait(engine, &request->signaling.wait);
>>> +     wakeup &= __intel_engine_add_wait(engine, wait);
>>>        spin_unlock(&b->rb_lock);
>>>    
>>> -     if (wakeup)
>>> +     if (wakeup) {
>>>                wake_up_process(b->signaler);
>>> +             return !intel_wait_complete(wait);
>>
>> ... and would now be based on breadcrumb waiter waking up and removing
>> itself from the tree.
> 
> And don't forget the same HWS check before the waiter is inserted. So we
> have the same guards as before, just inside yet another spinlock.

True, did not think of this. So it may proceed a bit deeper in the call 
chain but there is no actual behaviour change as I described it.

>> So some potential latency where it wasn't before, well, enable-disable
>> signalling cycles actually.
> 
> The extra steps would be the insert_signal(). Fwiw, we could reorder the
> insert_signal() but frankly this, dma_fence enable signaling of an
> inflight request, is not a fast path. More commonly we will be enabling
> signaling on the request as it is submitted (where wakeup is false and
> we know that the HWS cannot be complete).

Yes, no complaints after realizing the above.

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

Regards,

Tvrtko