* [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq
@ 2018-03-05 10:41 Chris Wilson
2018-03-05 11:12 ` Tvrtko Ursulin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Chris Wilson @ 2018-03-05 10:41 UTC (permalink / raw)
To: intel-gfx
After we call dma_fence_signal(), confirm that the request was indeed
complete.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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);
}
--
2.16.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 10:41 [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq Chris Wilson
@ 2018-03-05 11:12 ` Tvrtko Ursulin
2018-03-05 11:21 ` Chris Wilson
2018-03-05 12:08 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-03-05 15:18 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2018-03-05 11:12 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 11:12 ` Tvrtko Ursulin
@ 2018-03-05 11:21 ` Chris Wilson
2018-03-05 12:25 ` Tvrtko Ursulin
0 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2018-03-05 11:21 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 10:41 [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq Chris Wilson
2018-03-05 11:12 ` Tvrtko Ursulin
@ 2018-03-05 12:08 ` Patchwork
2018-03-05 15:18 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-03-05 12:08 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Assert that the request is indeed complete when signaled from irq
URL : https://patchwork.freedesktop.org/series/39369/
State : success
== Summary ==
Series 39369v1 drm/i915: Assert that the request is indeed complete when signaled from irq
https://patchwork.freedesktop.org/api/1.0/series/39369/revisions/1/mbox/
---- Known issues:
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:417s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:370s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:484s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:278s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:478s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:483s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:463s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:458s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:393s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:569s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:574s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:416s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:289s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:504s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:385s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:407s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:455s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:413s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:450s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:489s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:451s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:494s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:583s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:426s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:498s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:521s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:485s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:479s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:407s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:428s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:392s
2501aa775c7abc194aa2fbf93475cd1898aaa710 drm-tip: 2018y-03m-05d-10h-55m-37s UTC integration manifest
dae77e6c5a88 drm/i915: Assert that the request is indeed complete when signaled from irq
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8228/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 11:21 ` Chris Wilson
@ 2018-03-05 12:25 ` Tvrtko Ursulin
2018-03-07 12:59 ` Chris Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2018-03-05 12:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 10:41 [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq Chris Wilson
2018-03-05 11:12 ` Tvrtko Ursulin
2018-03-05 12:08 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-03-05 15:18 ` Patchwork
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-03-05 15:18 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Assert that the request is indeed complete when signaled from irq
URL : https://patchwork.freedesktop.org/series/39369/
State : success
== Summary ==
---- Known issues:
Test kms_chv_cursor_fail:
Subgroup pipe-b-128x128-top-edge:
dmesg-warn -> PASS (shard-snb) fdo#105185 +1
Subgroup pipe-c-128x128-bottom-edge:
pass -> FAIL (shard-apl) fdo#104671
Test kms_flip:
Subgroup 2x-plain-flip-fb-recreate:
pass -> FAIL (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
Subgroup basic:
fail -> PASS (shard-apl) fdo#103167
Subgroup fbc-suspend:
skip -> PASS (shard-hsw) fdo#103540
Test kms_vblank:
Subgroup pipe-b-ts-continuation-dpms-suspend:
pass -> INCOMPLETE (shard-hsw) fdo#105054
Subgroup pipe-c-accuracy-idle:
pass -> FAIL (shard-hsw) fdo#102583
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#105054 https://bugs.freedesktop.org/show_bug.cgi?id=105054
fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
shard-apl total:3452 pass:1813 dwarn:1 dfail:0 fail:8 skip:1629 time:11714s
shard-hsw total:3417 pass:1745 dwarn:1 dfail:0 fail:3 skip:1666 time:11409s
shard-snb total:3468 pass:1365 dwarn:1 dfail:0 fail:1 skip:2101 time:7115s
Blacklisted hosts:
shard-kbl total:3270 pass:1810 dwarn:33 dfail:0 fail:7 skip:1416 time:8184s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8228/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq
2018-03-05 12:25 ` Tvrtko Ursulin
@ 2018-03-07 12:59 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2018-03-07 12:59 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
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@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Thanks and pushed,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-07 12:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 10:41 [PATCH] drm/i915: Assert that the request is indeed complete when signaled from irq Chris Wilson
2018-03-05 11:12 ` Tvrtko Ursulin
2018-03-05 11:21 ` Chris Wilson
2018-03-05 12:25 ` Tvrtko Ursulin
2018-03-07 12:59 ` Chris Wilson
2018-03-05 12:08 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-03-05 15:18 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.