Re: [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 
> >>> Cc: Tvrtko Ursulin 
> >>> ---
> >>>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(>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 

Thanks and pushed,
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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 
Cc: Tvrtko Ursulin 
---
   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(>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 

Regards,

Tvrtko


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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 
> > Cc: Tvrtko Ursulin 
> > ---
> >   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(>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.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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 
Cc: Tvrtko Ursulin 
---
  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(>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 
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.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx