* [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
@ 2018-02-06 9:46 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06 9:46 UTC (permalink / raw)
To: intel-gfx; +Cc: stable
When a request is preempted, it is unsubmitted from the HW queue and
removed from the active list of breadcrumbs. In the process, this
however triggers the signaler and it may see the clear rbtree with the
old, and still valid, seqno. This confuses the signaler into action and
signaling the fence.
Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.12+
---
drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index efbc627a2a25..b955f7d7bd0f 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
spin_unlock_irq(&b->rb_lock);
}
-static bool signal_valid(const struct drm_i915_gem_request *request)
-{
- return intel_wait_check_request(&request->signaling.wait, request);
-}
-
static bool signal_complete(const struct drm_i915_gem_request *request)
{
if (!request)
return false;
- /* If another process served as the bottom-half it may have already
- * signalled that this wait is already completed.
- */
- if (intel_wait_complete(&request->signaling.wait))
- return signal_valid(request);
-
- /* Carefully check if the request is complete, giving time for the
+ /*
+ * Carefully check if the request is complete, giving time for the
* seqno to be visible or if the GPU hung.
*/
- if (__i915_request_irq_complete(request))
- return true;
-
- return false;
+ return __i915_request_irq_complete(request);
}
static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
@@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
&request->fence.flags)) {
local_bh_disable();
dma_fence_signal(&request->fence);
+ GEM_BUG_ON(!i915_gem_request_completed(request));
local_bh_enable(); /* kick start the tasklets */
}
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-06 9:46 ` Chris Wilson
(?)
@ 2018-02-06 9:52 ` Chris Wilson
-1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06 9:52 UTC (permalink / raw)
To: intel-gfx; +Cc: Tvrtko Ursulin, Joonas Lahtinen, stable
Quoting Chris Wilson (2018-02-06 09:46:33)
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
>
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Testcase: igt/gem_exec_whisper/*-priority # reproducibility ~10%
-Chris
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-06 9:46 ` Chris Wilson
(?)
(?)
@ 2018-02-06 10:29 ` Patchwork
-1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-06 10:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/breadcrumbs: Ignore unsubmitted signalers
URL : https://patchwork.freedesktop.org/series/37724/
State : success
== Summary ==
Series 37724v1 drm/i915/breadcrumbs: Ignore unsubmitted signalers
https://patchwork.freedesktop.org/api/1.0/series/37724/revisions/1/mbox/
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:423s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:421s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:374s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:490s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:287s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:484s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:470s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:459s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:575s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:414s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:281s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:516s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:390s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:404s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:460s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:416s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:493s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:502s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:593s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:428s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:515s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:488s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:484s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:415s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:427s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:529s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:402s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:472s
5883e1383ed69b4bd7a537ceafeeabbc61cdf55e drm-tip: 2018y-02m-06d-09h-04m-49s UTC integration manifest
2eb59c50f0ca drm/i915/breadcrumbs: Ignore unsubmitted signalers
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7900/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ Fi.CI.IGT: warning for drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-06 9:46 ` Chris Wilson
` (2 preceding siblings ...)
(?)
@ 2018-02-06 12:12 ` Patchwork
-1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-06 12:12 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/breadcrumbs: Ignore unsubmitted signalers
URL : https://patchwork.freedesktop.org/series/37724/
State : warning
== Summary ==
Test gem_eio:
Subgroup in-flight:
dmesg-warn -> PASS (shard-snb) fdo#104058
Test kms_flip:
Subgroup modeset-vs-vblank-race:
fail -> PASS (shard-apl) fdo#103060
Subgroup flip-vs-expired-vblank:
pass -> FAIL (shard-apl) fdo#102887
Subgroup 2x-flip-vs-wf_vblank:
fail -> PASS (shard-hsw) fdo#100368
Test kms_vblank:
Subgroup pipe-b-ts-continuation-dpms-suspend:
pass -> SKIP (shard-hsw)
Test kms_cursor_crc:
Subgroup cursor-128x128-suspend:
pass -> SKIP (shard-snb) fdo#103880
Test kms_sysfs_edid_timing:
pass -> WARN (shard-apl) fdo#100047
Test kms_frontbuffer_tracking:
Subgroup fbc-2p-scndscrn-cur-indfb-move:
skip -> PASS (shard-hsw)
Test kms_atomic_interruptible:
Subgroup legacy-setmode:
pass -> SKIP (shard-snb)
fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
shard-apl total:3379 pass:1744 dwarn:1 dfail:0 fail:22 skip:1610 time:12444s
shard-hsw total:3442 pass:1757 dwarn:1 dfail:0 fail:11 skip:1672 time:11707s
shard-snb total:3442 pass:1349 dwarn:1 dfail:0 fail:10 skip:2082 time:6608s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7900/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-06 9:46 ` Chris Wilson
` (3 preceding siblings ...)
(?)
@ 2018-02-06 17:13 ` Chris Wilson
-1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06 17:13 UTC (permalink / raw)
To: intel-gfx; +Cc: Tvrtko Ursulin, Joonas Lahtinen, stable
Quoting Chris Wilson (2018-02-06 09:46:33)
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
>
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
Any takers for this brown paper bug?
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
> 1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index efbc627a2a25..b955f7d7bd0f 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> spin_unlock_irq(&b->rb_lock);
> }
>
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> - return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
> static bool signal_complete(const struct drm_i915_gem_request *request)
> {
> if (!request)
> return false;
>
> - /* If another process served as the bottom-half it may have already
> - * signalled that this wait is already completed.
> - */
> - if (intel_wait_complete(&request->signaling.wait))
> - return signal_valid(request);
> -
> - /* Carefully check if the request is complete, giving time for the
> + /*
> + * Carefully check if the request is complete, giving time for the
> * seqno to be visible or if the GPU hung.
> */
> - if (__i915_request_irq_complete(request))
> - return true;
> -
> - return false;
> + return __i915_request_irq_complete(request);
> }
>
> static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
> &request->fence.flags)) {
> local_bh_disable();
> dma_fence_signal(&request->fence);
> + GEM_BUG_ON(!i915_gem_request_completed(request));
> local_bh_enable(); /* kick start the tasklets */
> }
>
> --
> 2.15.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-06 9:46 ` Chris Wilson
` (4 preceding siblings ...)
(?)
@ 2018-02-07 10:40 ` Tvrtko Ursulin
2018-02-07 11:04 ` Chris Wilson
2018-02-07 11:10 ` Chris Wilson
-1 siblings, 2 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-07 10:40 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Joonas Lahtinen, stable
On 06/02/2018 09:46, Chris Wilson wrote:
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
>
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
> 1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index efbc627a2a25..b955f7d7bd0f 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> spin_unlock_irq(&b->rb_lock);
> }
>
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> - return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
> static bool signal_complete(const struct drm_i915_gem_request *request)
> {
> if (!request)
> return false;
>
> - /* If another process served as the bottom-half it may have already
> - * signalled that this wait is already completed.
> - */
> - if (intel_wait_complete(&request->signaling.wait))
> - return signal_valid(request);
Okay so this can return true for unsubmitted requests since rb node will
be empty and global_seqno == wait.seqno == 0.
I just panic when thinking about races and ordering, since these checks
used to run unlocked. So even better that they are gone.
> -
> - /* Carefully check if the request is complete, giving time for the
> + /*
> + * Carefully check if the request is complete, giving time for the
> * seqno to be visible or if the GPU hung.
> */
> - if (__i915_request_irq_complete(request))
> - return true;
> -
> - return false;
> + return __i915_request_irq_complete(request); > }
>
> static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
> &request->fence.flags)) {
> local_bh_disable();
> dma_fence_signal(&request->fence);
> + GEM_BUG_ON(!i915_gem_request_completed(request));
> local_bh_enable(); /* kick start the tasklets */
> }
>
>
Looks OK. But I can't say it's straightforward to understand it.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-07 10:40 ` Tvrtko Ursulin
@ 2018-02-07 11:04 ` Chris Wilson
2018-02-07 11:10 ` Chris Wilson
1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-07 11:04 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Joonas Lahtinen, stable
Quoting Tvrtko Ursulin (2018-02-07 10:40:46)
>
> On 06/02/2018 09:46, Chris Wilson wrote:
> > When a request is preempted, it is unsubmitted from the HW queue and
> > removed from the active list of breadcrumbs. In the process, this
> > however triggers the signaler and it may see the clear rbtree with the
> > old, and still valid, seqno. This confuses the signaler into action and
> > signaling the fence.
> >
> > Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.12+
> > ---
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
> > 1 file changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index efbc627a2a25..b955f7d7bd0f 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> > spin_unlock_irq(&b->rb_lock);
> > }
> >
> > -static bool signal_valid(const struct drm_i915_gem_request *request)
> > -{
> > - return intel_wait_check_request(&request->signaling.wait, request);
> > -}
> > -
> > static bool signal_complete(const struct drm_i915_gem_request *request)
> > {
> > if (!request)
> > return false;
> >
> > - /* If another process served as the bottom-half it may have already
> > - * signalled that this wait is already completed.
> > - */
> > - if (intel_wait_complete(&request->signaling.wait))
> > - return signal_valid(request);
>
> Okay so this can return true for unsubmitted requests since rb node will
> be empty and global_seqno == wait.seqno == 0.
Hmm, ah, signal_valid() operated under the belief that its wait.seqno
was untouched. That makes a bit more sense now. I was having to concoct
some scary data races to try and explain how global_seqno and wait.seqno
had the same non-zero value.
-Chris
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
2018-02-07 10:40 ` Tvrtko Ursulin
2018-02-07 11:04 ` Chris Wilson
@ 2018-02-07 11:10 ` Chris Wilson
1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-07 11:10 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Joonas Lahtinen, stable
Quoting Tvrtko Ursulin (2018-02-07 10:40:46)
>
> On 06/02/2018 09:46, Chris Wilson wrote:
> > When a request is preempted, it is unsubmitted from the HW queue and
> > removed from the active list of breadcrumbs. In the process, this
> > however triggers the signaler and it may see the clear rbtree with the
> > old, and still valid, seqno. This confuses the signaler into action and
> > signaling the fence.
> >
> > Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.12+
> > ---
> Looks OK. But I can't say it's straightforward to understand it.
But I hope the GEM_BUG_ON() was nice icing on the cake :)
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Thanks, added your explanation to the changelog and pushed.
-Chris
^ permalink raw reply [flat|nested] 12+ messages in thread