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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-03-05 12:25:21)
> 
> 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.

i915_request_signal() isn't a bad suggestion. We don't want many
dma_fence_signal() callsites but on all occasions the assertion should
hold true.

I'll try to remember for next time I'm passing.

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

Thanks and pushed,
-Chris