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@xxxxxxxxxxxxxxxxxx>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxx>
Thanks and pushed,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
