[Intel-gfx] [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq

[Tvrtko Ursulin]

On 05/03/2018 11:21, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-03-05 11:12:45)
>>
>> On 05/03/2018 10:41, Chris Wilson wrote:
>>> After we call dma_fence_signal(), confirm that the request was indeed
>>> complete.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_irq.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index ce16003ef048..633c18785c1e 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -1123,6 +1123,7 @@ static void notify_ring(struct intel_engine_cs *engine)
>>>    
>>>        if (rq) {
>>>                dma_fence_signal(&rq->fence);
>>> +             GEM_BUG_ON(!i915_request_completed(rq));
>>>                i915_request_put(rq);
>>>        }
>>>    
>>>
>>
>> What's the motivation? There is a i915_seqno_passed check a few lines
> 
> The seqno check is on wait.seqno, this is to confirm it all ties
> together with the request and our preemption avoidance is solid. The
> motivation was the bug in the signaler along the same lines.
> 
>> above. So there would have to be a confusion in internal breadcrumbs
>> state for this to be possible. In which case I'd rather put the assert
>> in breadcrumbs code. For instance in intel_wait_check_request, asserting
>> that the seqno in wait matches the seqno in wait->request.
> 
> The entire point of that check is to say when they don't match so that
> we know when the request was unsubmitted during the wait.

Ok my suggesting wasn't really appropriate. I just disliked a bit open 
coding the assert. No smart and worthwhile suggestions to improve it. 
i915_request_signal came to mind to wrap the assert and dma_fence_signal 
but I dont see sufficient call sites.

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

Regards,

Tvrtko