[PATCH] drm/i915: Stop listening to request resubmission from the signaler kthread

[PATCH] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

The intent here was that we would be listening to
i915_gem_request_unsubmit in order to cancel the signaler quickly and
release the reference on the request. Cancelling the signaler is done
directly via intel_engine_cancel_signaling (called from unsubmit), but
that does not directly wake up the signaling thread, and neither does
setting the request->global_seqno back to zero wake up listeners to the
request->execute waitqueue. So the only time that listening to the
request->execute waitqueue would wake up the signaling kthread would be
on the request resubmission, during which time we would already receive
wake ups from rejoining the global breadcrumbs wait rbtree.

Trying to wake up to release the request remains an issue. If the
signaling was cancelled and no other request required signaling, then it
is possible for us to shutdown with the reference on the request still
held. To ensure that we do not try and shutdown leaking that request, we
kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
parking the engine when idle.

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: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 5ae2d276f7f3..1612efc54f15 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	struct intel_wait *wait, *n, *first;
 
 	if (!b->irq_armed)
-		return;
+		goto wakeup_signaler;
 
 	/* We only disarm the irq when we are idle (all requests completed),
 	 * so if the bottom-half remains asleep, it missed the request
@@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	b->waiters = RB_ROOT;
 
 	spin_unlock_irq(&b->rb_lock);
+
+	/*
+	 * The signaling thread may be asleep holding a reference to a request,
+	 * that had its signaling cancel prior to being preempted, we need to
+	 * kick the signaler to release any such reference.
+	 */
+wakeup_signaler:
+	wake_up_process(b->signaler);
 }
 
 static bool use_fake_irq(const struct intel_breadcrumbs *b)
@@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
 		}
 
 		if (unlikely(do_schedule)) {
-			DEFINE_WAIT(exec);
-
 			if (kthread_should_park())
 				kthread_parkme();
 
@@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 				break;
 			}
 
-			if (request)
-				add_wait_queue(&request->execute, &exec);
-
 			schedule();
-
-			if (request)
-				remove_wait_queue(&request->execute, &exec);
 		}
 		i915_gem_request_put(request);
 	} while (1);
-- 
2.15.1

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

✓ Fi.CI.BAT: success for drm/i915: Stop listening to request resubmission from the signaler kthread

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread
URL   : https://patchwork.freedesktop.org/series/35084/
State : success

== Summary ==

Series 35084v1 drm/i915: Stop listening to request resubmission from the signaler kthread
https://patchwork.freedesktop.org/api/1.0/series/35084/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989
                dmesg-warn -> PASS       (fi-bdw-gvtdvm) fdo#103938 +1
Test gem_exec_reloc:
        Subgroup basic-write-gtt-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938
fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:439s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:382s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:531s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:280s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:506s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:492s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:476s
fi-elk-e7500     total:224  pass:163  dwarn:14  dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:177  dwarn:1   dfail:0   fail:2   skip:108 time:272s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:357s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:261s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:392s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:480s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:445s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:478s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:527s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:538s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:591s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:541s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:570s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:516s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:498s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:547s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:415s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:609s
fi-cnl-y         total:252  pass:226  dwarn:0   dfail:0   fail:0   skip:25 
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:490s

7f090d915d4f2f0f5788d40eba402ae9bf878d43 drm-tip: 2017y-12m-08d-10h-31m-00s UTC integration manifest
cafc1eb2c0d1 drm/i915: Stop listening to request resubmission from the signaler kthread

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7452/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

The intent here was that we would be listening to
i915_gem_request_unsubmit in order to cancel the signaler quickly and
release the reference on the request. Cancelling the signaler is done
directly via intel_engine_cancel_signaling (called from unsubmit), but
that does not directly wake up the signaling thread, and neither does
setting the request->global_seqno back to zero wake up listeners to the
request->execute waitqueue. So the only time that listening to the
request->execute waitqueue would wake up the signaling kthread would be
on the request resubmission, during which time we would already receive
wake ups from rejoining the global breadcrumbs wait rbtree.

Trying to wake up to release the request remains an issue. If the
signaling was cancelled and no other request required signaling, then it
is possible for us to shutdown with the reference on the request still
held. To ensure that we do not try to shutdown, leaking that request, we
kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
parking the engine when idle.

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: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 5ae2d276f7f3..b826725989b1 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	struct intel_wait *wait, *n, *first;
 
 	if (!b->irq_armed)
-		return;
+		goto wakeup_signaler;
 
 	/* We only disarm the irq when we are idle (all requests completed),
 	 * so if the bottom-half remains asleep, it missed the request
@@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	b->waiters = RB_ROOT;
 
 	spin_unlock_irq(&b->rb_lock);
+
+	/*
+	 * The signaling thread may be asleep holding a reference to a request,
+	 * that had its signaling cancelled prior to being preempted. We need
+	 * to kick the signaler, just in case, to release any such reference.
+	 */
+wakeup_signaler:
+	wake_up_process(b->signaler);
 }
 
 static bool use_fake_irq(const struct intel_breadcrumbs *b)
@@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
 		}
 
 		if (unlikely(do_schedule)) {
-			DEFINE_WAIT(exec);
-
 			if (kthread_should_park())
 				kthread_parkme();
 
@@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 				break;
 			}
 
-			if (request)
-				add_wait_queue(&request->execute, &exec);
-
 			schedule();
-
-			if (request)
-				remove_wait_queue(&request->execute, &exec);
 		}
 		i915_gem_request_put(request);
 	} while (1);
-- 
2.15.1

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

✓ Fi.CI.BAT: success for drm/i915: Stop listening to request resubmission from the signaler kthread (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread (rev2)
URL   : https://patchwork.freedesktop.org/series/35084/
State : success

== Summary ==

Series 35084v2 drm/i915: Stop listening to request resubmission from the signaler kthread
https://patchwork.freedesktop.org/api/1.0/series/35084/revisions/2/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:385s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:516s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:516s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:494s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:477s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:1   dfail:0   fail:0   skip:108 time:268s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:399s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:259s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:394s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:448s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:529s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:535s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:594s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:542s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:574s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:515s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:554s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:415s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:602s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s
fi-cnl-y failed to connect after reboot

3078ee832585564b71cd626b2e3f4ac613fcc790 drm-tip: 2017y-12m-08d-11h-43m-03s UTC integration manifest
af43aa14485d drm/i915: Stop listening to request resubmission from the signaler kthread

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7453/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Stop listening to request resubmission from the signaler kthread (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread (rev2)
URL   : https://patchwork.freedesktop.org/series/35084/
State : warning

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (shard-snb) fdo#102848
Test kms_flip:
        Subgroup modeset-vs-vblank-race-interruptible:
                fail       -> PASS       (shard-hsw) fdo#103060
Test kms_draw_crc:
        Subgroup draw-method-xrgb2101010-mmap-wc-xtiled:
                pass       -> SKIP       (shard-hsw) fdo#103184
Test kms_plane_multiple:
        Subgroup atomic-pipe-b-tiling-none:
                pass       -> SKIP       (shard-hsw)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540

shard-hsw        total:2654 pass:1517 dwarn:2   dfail:0   fail:10  skip:1124 time:9259s
shard-snb        total:2679 pass:1307 dwarn:2   dfail:0   fail:12  skip:1358 time:8108s
Blacklisted hosts:
shard-apl        total:2679 pass:1679 dwarn:1   dfail:0   fail:22  skip:977 time:13676s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7453/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Daniel Vetter]

On Fri, Dec 08, 2017 at 12:10:33PM +0000, Chris Wilson wrote:
> The intent here was that we would be listening to
> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> release the reference on the request. Cancelling the signaler is done
> directly via intel_engine_cancel_signaling (called from unsubmit), but
> that does not directly wake up the signaling thread, and neither does
> setting the request->global_seqno back to zero wake up listeners to the
> request->execute waitqueue. So the only time that listening to the
> request->execute waitqueue would wake up the signaling kthread would be
> on the request resubmission, during which time we would already receive
> wake ups from rejoining the global breadcrumbs wait rbtree.
> 
> Trying to wake up to release the request remains an issue. If the
> signaling was cancelled and no other request required signaling, then it
> is possible for us to shutdown with the reference on the request still
> held. To ensure that we do not try to shutdown, leaking that request, we
> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> parking the engine when idle.
> 
> 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: Michał Winiarski <michal.winiarski@intel.com>

I don't really understand the details, but with this patch the signaler
kthread looks like it follows a race-free pattern again:

- tsk->state = UNITERRUPTIBLE
- implicitly added to the wait key - since we always use wake_up_process
  b->signaller that step isn't skipped]
- check the condition we're waiting on
- schedule()

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But probably better some real experts also pour over this :-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 5ae2d276f7f3..b826725989b1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	struct intel_wait *wait, *n, *first;
>  
>  	if (!b->irq_armed)
> -		return;
> +		goto wakeup_signaler;
>  
>  	/* We only disarm the irq when we are idle (all requests completed),
>  	 * so if the bottom-half remains asleep, it missed the request
> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	b->waiters = RB_ROOT;
>  
>  	spin_unlock_irq(&b->rb_lock);
> +
> +	/*
> +	 * The signaling thread may be asleep holding a reference to a request,
> +	 * that had its signaling cancelled prior to being preempted. We need
> +	 * to kick the signaler, just in case, to release any such reference.
> +	 */
> +wakeup_signaler:
> +	wake_up_process(b->signaler);
>  }
>  
>  static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
>  		}
>  
>  		if (unlikely(do_schedule)) {
> -			DEFINE_WAIT(exec);
> -
>  			if (kthread_should_park())
>  				kthread_parkme();
>  
> @@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>  				break;
>  			}
>  
> -			if (request)
> -				add_wait_queue(&request->execute, &exec);
> -
>  			schedule();
> -
> -			if (request)
> -				remove_wait_queue(&request->execute, &exec);
>  		}
>  		i915_gem_request_put(request);
>  	} while (1);
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Daniel Vetter]

On Fri, Dec 08, 2017 at 12:10:33PM +0000, Chris Wilson wrote:
> The intent here was that we would be listening to
> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> release the reference on the request. Cancelling the signaler is done
> directly via intel_engine_cancel_signaling (called from unsubmit), but
> that does not directly wake up the signaling thread, and neither does
> setting the request->global_seqno back to zero wake up listeners to the
> request->execute waitqueue. So the only time that listening to the
> request->execute waitqueue would wake up the signaling kthread would be
> on the request resubmission, during which time we would already receive
> wake ups from rejoining the global breadcrumbs wait rbtree.
> 
> Trying to wake up to release the request remains an issue. If the
> signaling was cancelled and no other request required signaling, then it
> is possible for us to shutdown with the reference on the request still
> held. To ensure that we do not try to shutdown, leaking that request, we
> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> parking the engine when idle.
> 
> 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: Michał Winiarski <michal.winiarski@intel.com>

Also

Reported-by: Peter Zijlstra <peterz@infradead.org>
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 5ae2d276f7f3..b826725989b1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	struct intel_wait *wait, *n, *first;
>  
>  	if (!b->irq_armed)
> -		return;
> +		goto wakeup_signaler;
>  
>  	/* We only disarm the irq when we are idle (all requests completed),
>  	 * so if the bottom-half remains asleep, it missed the request
> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	b->waiters = RB_ROOT;
>  
>  	spin_unlock_irq(&b->rb_lock);
> +
> +	/*
> +	 * The signaling thread may be asleep holding a reference to a request,
> +	 * that had its signaling cancelled prior to being preempted. We need
> +	 * to kick the signaler, just in case, to release any such reference.
> +	 */
> +wakeup_signaler:
> +	wake_up_process(b->signaler);
>  }
>  
>  static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
>  		}
>  
>  		if (unlikely(do_schedule)) {
> -			DEFINE_WAIT(exec);
> -
>  			if (kthread_should_park())
>  				kthread_parkme();
>  
> @@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>  				break;
>  			}
>  
> -			if (request)
> -				add_wait_queue(&request->execute, &exec);
> -
>  			schedule();
> -
> -			if (request)
> -				remove_wait_queue(&request->execute, &exec);
>  		}
>  		i915_gem_request_put(request);
>  	} while (1);
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Tvrtko Ursulin]

On 08/12/2017 12:10, Chris Wilson wrote:
> The intent here was that we would be listening to
> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> release the reference on the request. Cancelling the signaler is done
> directly via intel_engine_cancel_signaling (called from unsubmit), but
> that does not directly wake up the signaling thread, and neither does
> setting the request->global_seqno back to zero wake up listeners to the
> request->execute waitqueue. So the only time that listening to the
> request->execute waitqueue would wake up the signaling kthread would be
> on the request resubmission, during which time we would already receive
> wake ups from rejoining the global breadcrumbs wait rbtree.
> 
> Trying to wake up to release the request remains an issue. If the
> signaling was cancelled and no other request required signaling, then it
> is possible for us to shutdown with the reference on the request still
> held. To ensure that we do not try to shutdown, leaking that request, we
> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> parking the engine when idle.
> 
> 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: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 5ae2d276f7f3..b826725989b1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	struct intel_wait *wait, *n, *first;
>   
>   	if (!b->irq_armed)
> -		return;
> +		goto wakeup_signaler;
>   
>   	/* We only disarm the irq when we are idle (all requests completed),
>   	 * so if the bottom-half remains asleep, it missed the request
> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	b->waiters = RB_ROOT;
>   
>   	spin_unlock_irq(&b->rb_lock);
> +
> +	/*
> +	 * The signaling thread may be asleep holding a reference to a request,
> +	 * that had its signaling cancelled prior to being preempted. We need
> +	 * to kick the signaler, just in case, to release any such reference.
> +	 */
> +wakeup_signaler:
> +	wake_up_process(b->signaler);
>   }
>   
>   static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -683,8 +691,6 @@ static int intel_breadcrumbs_signaler(void *arg)
>   		}
>   
>   		if (unlikely(do_schedule)) {
> -			DEFINE_WAIT(exec);
> -
>   			if (kthread_should_park())
>   				kthread_parkme();
>   
> @@ -693,13 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>   				break;
>   			}
>   
> -			if (request)
> -				add_wait_queue(&request->execute, &exec);
> -
>   			schedule();
> -
> -			if (request)
> -				remove_wait_queue(&request->execute, &exec);
>   		}
>   		i915_gem_request_put(request);
>   	} while (1);
> 

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

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-11 14:19:47)
> 
> On 08/12/2017 12:10, Chris Wilson wrote:
> > The intent here was that we would be listening to
> > i915_gem_request_unsubmit in order to cancel the signaler quickly and
> > release the reference on the request. Cancelling the signaler is done
> > directly via intel_engine_cancel_signaling (called from unsubmit), but
> > that does not directly wake up the signaling thread, and neither does
> > setting the request->global_seqno back to zero wake up listeners to the
> > request->execute waitqueue. So the only time that listening to the
> > request->execute waitqueue would wake up the signaling kthread would be
> > on the request resubmission, during which time we would already receive
> > wake ups from rejoining the global breadcrumbs wait rbtree.
> > 
> > Trying to wake up to release the request remains an issue. If the
> > signaling was cancelled and no other request required signaling, then it
> > is possible for us to shutdown with the reference on the request still
> > held. To ensure that we do not try to shutdown, leaking that request, we
> > kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> > parking the engine when idle.
> > 
> > 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: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index 5ae2d276f7f3..b826725989b1 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >       struct intel_wait *wait, *n, *first;
> >   
> >       if (!b->irq_armed)
> > -             return;
> > +             goto wakeup_signaler;
> >   
> >       /* We only disarm the irq when we are idle (all requests completed),
> >        * so if the bottom-half remains asleep, it missed the request
> > @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >       b->waiters = RB_ROOT;
> >   
> >       spin_unlock_irq(&b->rb_lock);
> > +
> > +     /*
> > +      * The signaling thread may be asleep holding a reference to a request,
> > +      * that had its signaling cancelled prior to being preempted. We need
> > +      * to kick the signaler, just in case, to release any such reference.
> > +      */
> > +wakeup_signaler:
> > +     wake_up_process(b->signaler);

One thing I was thinking about from our conversation, is whether we can
rely on the kthread_stop() kick for the final ref release? Whether it's
even a good idea to let the reference live for that long?

Basically it will mean replacing the GEM_BUG_ON(request) there with
another unref. But it will remove the complication and assoicated
commentary here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

The intent here was that we would be listening to
i915_gem_request_unsubmit in order to cancel the signaler quickly and
release the reference on the request. Cancelling the signaler is done
directly via intel_engine_cancel_signaling (called from unsubmit), but
that does not directly wake up the signaling thread, and neither does
setting the request->global_seqno back to zero wake up listeners to the
request->execute waitqueue. So the only time that listening to the
request->execute waitqueue would wake up the signaling kthread would be
on the request resubmission, during which time we would already receive
wake ups from rejoining the global breadcrumbs wait rbtree.

Trying to wake up to release the request remains an issue. If the
signaling was cancelled and no other request required signaling, then it
is possible for us to shutdown with the reference on the request still
held. To ensure that we do not try to shutdown, leaking that request, we
must release that reference on kthread_should_stop().

v2: Defer releasing the local reference until the very end of the
thread, if we happen to never reuse the kthread. We should then still
free the request before we finalize the kmem caches.

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: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> #v1
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 24c6fefdd0b1..0c3b321cac3e 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -686,23 +686,15 @@ static int intel_breadcrumbs_signaler(void *arg)
 		}
 
 		if (unlikely(do_schedule)) {
-			DEFINE_WAIT(exec);
-
 			if (kthread_should_park())
 				kthread_parkme();
 
-			if (kthread_should_stop()) {
-				GEM_BUG_ON(request);
+			if (unlikely(kthread_should_stop())) {
+				i915_gem_request_put(request);
 				break;
 			}
 
-			if (request)
-				add_wait_queue(&request->execute, &exec);
-
 			schedule();
-
-			if (request)
-				remove_wait_queue(&request->execute, &exec);
 		}
 		i915_gem_request_put(request);
 	} while (1);
-- 
2.15.1

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

✓ Fi.CI.BAT: success for drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)
URL   : https://patchwork.freedesktop.org/series/35084/
State : success

== Summary ==

Series 35084v3 drm/i915: Stop listening to request resubmission from the signaler kthread
https://patchwork.freedesktop.org/api/1.0/series/35084/revisions/3/mbox/

fi-bdw-5557u failed to connect after reboot
fi-blb-e6850 failed to connect after reboot
fi-bsw-n3050 failed to connect after reboot
fi-bwr-2160 failed to connect after reboot
fi-cfl-s2 failed to connect after reboot
fi-cnl-y failed to connect after reboot
fi-gdg-551 failed to connect after reboot
fi-glk-1 failed to connect after reboot
fi-hsw-4770r failed to connect after reboot
fi-ivb-3770 failed to connect after reboot
fi-pnv-d510 failed to connect after reboot
fi-skl-6600u failed to connect after reboot
fi-snb-2520m failed to connect after reboot

bb55ba9311dea38dcf8fb1a6c447505dc635f54f drm-tip: 2017y-12m-11d-11h-51m-11s UTC integration manifest
0d8548dc7c72 drm/i915: Stop listening to request resubmission from the signaler kthread

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7467/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Tvrtko Ursulin]

On 11/12/2017 14:26, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-11 14:19:47)
>>
>> On 08/12/2017 12:10, Chris Wilson wrote:
>>> The intent here was that we would be listening to
>>> i915_gem_request_unsubmit in order to cancel the signaler quickly and
>>> release the reference on the request. Cancelling the signaler is done
>>> directly via intel_engine_cancel_signaling (called from unsubmit), but
>>> that does not directly wake up the signaling thread, and neither does
>>> setting the request->global_seqno back to zero wake up listeners to the
>>> request->execute waitqueue. So the only time that listening to the
>>> request->execute waitqueue would wake up the signaling kthread would be
>>> on the request resubmission, during which time we would already receive
>>> wake ups from rejoining the global breadcrumbs wait rbtree.
>>>
>>> Trying to wake up to release the request remains an issue. If the
>>> signaling was cancelled and no other request required signaling, then it
>>> is possible for us to shutdown with the reference on the request still
>>> held. To ensure that we do not try to shutdown, leaking that request, we
>>> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
>>> parking the engine when idle.
>>>
>>> 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: Michał Winiarski <michal.winiarski@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>>>    1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 5ae2d276f7f3..b826725989b1 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>>>        struct intel_wait *wait, *n, *first;
>>>    
>>>        if (!b->irq_armed)
>>> -             return;
>>> +             goto wakeup_signaler;
>>>    
>>>        /* We only disarm the irq when we are idle (all requests completed),
>>>         * so if the bottom-half remains asleep, it missed the request
>>> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>>>        b->waiters = RB_ROOT;
>>>    
>>>        spin_unlock_irq(&b->rb_lock);
>>> +
>>> +     /*
>>> +      * The signaling thread may be asleep holding a reference to a request,
>>> +      * that had its signaling cancelled prior to being preempted. We need
>>> +      * to kick the signaler, just in case, to release any such reference.
>>> +      */
>>> +wakeup_signaler:
>>> +     wake_up_process(b->signaler);
> 
> One thing I was thinking about from our conversation, is whether we can
> rely on the kthread_stop() kick for the final ref release? Whether it's
> even a good idea to let the reference live for that long?

Probably not, feels awkward. In that sense previous version feels better.

> Basically it will mean replacing the GEM_BUG_ON(request) there with
> another unref. But it will remove the complication and assoicated
> commentary here.

Hm, and is there a test which was able to hit the GEM_BUG_ON? It needed 
to go in the previous version as well, no?

Regards,

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

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-11 15:24:06)
> 
> On 11/12/2017 14:26, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-12-11 14:19:47)
> >>
> >> On 08/12/2017 12:10, Chris Wilson wrote:
> >>> The intent here was that we would be listening to
> >>> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> >>> release the reference on the request. Cancelling the signaler is done
> >>> directly via intel_engine_cancel_signaling (called from unsubmit), but
> >>> that does not directly wake up the signaling thread, and neither does
> >>> setting the request->global_seqno back to zero wake up listeners to the
> >>> request->execute waitqueue. So the only time that listening to the
> >>> request->execute waitqueue would wake up the signaling kthread would be
> >>> on the request resubmission, during which time we would already receive
> >>> wake ups from rejoining the global breadcrumbs wait rbtree.
> >>>
> >>> Trying to wake up to release the request remains an issue. If the
> >>> signaling was cancelled and no other request required signaling, then it
> >>> is possible for us to shutdown with the reference on the request still
> >>> held. To ensure that we do not try to shutdown, leaking that request, we
> >>> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> >>> parking the engine when idle.
> >>>
> >>> 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: Michał Winiarski <michal.winiarski@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
> >>>    1 file changed, 9 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>> index 5ae2d276f7f3..b826725989b1 100644
> >>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >>>        struct intel_wait *wait, *n, *first;
> >>>    
> >>>        if (!b->irq_armed)
> >>> -             return;
> >>> +             goto wakeup_signaler;
> >>>    
> >>>        /* We only disarm the irq when we are idle (all requests completed),
> >>>         * so if the bottom-half remains asleep, it missed the request
> >>> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >>>        b->waiters = RB_ROOT;
> >>>    
> >>>        spin_unlock_irq(&b->rb_lock);
> >>> +
> >>> +     /*
> >>> +      * The signaling thread may be asleep holding a reference to a request,
> >>> +      * that had its signaling cancelled prior to being preempted. We need
> >>> +      * to kick the signaler, just in case, to release any such reference.
> >>> +      */
> >>> +wakeup_signaler:
> >>> +     wake_up_process(b->signaler);
> > 
> > One thing I was thinking about from our conversation, is whether we can
> > rely on the kthread_stop() kick for the final ref release? Whether it's
> > even a good idea to let the reference live for that long?
> 
> Probably not, feels awkward. In that sense previous version feels better.

Ok.
 
> > Basically it will mean replacing the GEM_BUG_ON(request) there with
> > another unref. But it will remove the complication and assoicated
> > commentary here.
> 
> Hm, and is there a test which was able to hit the GEM_BUG_ON? It needed 
> to go in the previous version as well, no?

It does indeed need to be there today, and with the v1 patch. Since
there's no serialisation to allow the previous wakeup to complete and
schedule() again before we hit the kthread_should_stop.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)
URL   : https://patchwork.freedesktop.org/series/35084/
State : success

== Summary ==

Series 35084v3 drm/i915: Stop listening to request resubmission from the signaler kthread
https://patchwork.freedesktop.org/api/1.0/series/35084/revisions/3/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
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:442s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:448s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:510s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:509s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:494s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:474s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:268s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:358s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:261s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:392s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:523s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:597s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:544s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:572s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:498s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:453s
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:416s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:598s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:640s
fi-glk-dsi       total:146  pass:132  dwarn:0   dfail:0   fail:0   skip:13 

bb55ba9311dea38dcf8fb1a6c447505dc635f54f drm-tip: 2017y-12m-11d-11h-51m-11s UTC integration manifest
0d8548dc7c72 drm/i915: Stop listening to request resubmission from the signaler kthread

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7467/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Tvrtko Ursulin]

On 11/12/2017 15:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-11 15:24:06)
>>
>> On 11/12/2017 14:26, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-12-11 14:19:47)
>>>>
>>>> On 08/12/2017 12:10, Chris Wilson wrote:
>>>>> The intent here was that we would be listening to
>>>>> i915_gem_request_unsubmit in order to cancel the signaler quickly and
>>>>> release the reference on the request. Cancelling the signaler is done
>>>>> directly via intel_engine_cancel_signaling (called from unsubmit), but
>>>>> that does not directly wake up the signaling thread, and neither does
>>>>> setting the request->global_seqno back to zero wake up listeners to the
>>>>> request->execute waitqueue. So the only time that listening to the
>>>>> request->execute waitqueue would wake up the signaling kthread would be
>>>>> on the request resubmission, during which time we would already receive
>>>>> wake ups from rejoining the global breadcrumbs wait rbtree.
>>>>>
>>>>> Trying to wake up to release the request remains an issue. If the
>>>>> signaling was cancelled and no other request required signaling, then it
>>>>> is possible for us to shutdown with the reference on the request still
>>>>> held. To ensure that we do not try to shutdown, leaking that request, we
>>>>> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
>>>>> parking the engine when idle.
>>>>>
>>>>> 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: Michał Winiarski <michal.winiarski@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
>>>>>     1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>>>> index 5ae2d276f7f3..b826725989b1 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>>>> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>>>>>         struct intel_wait *wait, *n, *first;
>>>>>     
>>>>>         if (!b->irq_armed)
>>>>> -             return;
>>>>> +             goto wakeup_signaler;
>>>>>     
>>>>>         /* We only disarm the irq when we are idle (all requests completed),
>>>>>          * so if the bottom-half remains asleep, it missed the request
>>>>> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>>>>>         b->waiters = RB_ROOT;
>>>>>     
>>>>>         spin_unlock_irq(&b->rb_lock);
>>>>> +
>>>>> +     /*
>>>>> +      * The signaling thread may be asleep holding a reference to a request,
>>>>> +      * that had its signaling cancelled prior to being preempted. We need
>>>>> +      * to kick the signaler, just in case, to release any such reference.
>>>>> +      */
>>>>> +wakeup_signaler:
>>>>> +     wake_up_process(b->signaler);
>>>
>>> One thing I was thinking about from our conversation, is whether we can
>>> rely on the kthread_stop() kick for the final ref release? Whether it's
>>> even a good idea to let the reference live for that long?
>>
>> Probably not, feels awkward. In that sense previous version feels better.
> 
> Ok.
>   
>>> Basically it will mean replacing the GEM_BUG_ON(request) there with
>>> another unref. But it will remove the complication and assoicated
>>> commentary here.
>>
>> Hm, and is there a test which was able to hit the GEM_BUG_ON? It needed
>> to go in the previous version as well, no?
> 
> It does indeed need to be there today, and with the v1 patch. Since
> there's no serialisation to allow the previous wakeup to complete and
> schedule() again before we hit the kthread_should_stop.

Right, it shouldn't be a GEM_BUG_ON for an expected code path but 
replaced with i915_gem_request_put?

Regards,

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

Re: [PATCH v2] drm/i915: Stop listening to request resubmission from the signaler kthread

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-11 15:48:10)
> 
> On 11/12/2017 15:29, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-12-11 15:24:06)
> >>
> >> On 11/12/2017 14:26, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2017-12-11 14:19:47)
> >>>>
> >>>> On 08/12/2017 12:10, Chris Wilson wrote:
> >>>>> The intent here was that we would be listening to
> >>>>> i915_gem_request_unsubmit in order to cancel the signaler quickly and
> >>>>> release the reference on the request. Cancelling the signaler is done
> >>>>> directly via intel_engine_cancel_signaling (called from unsubmit), but
> >>>>> that does not directly wake up the signaling thread, and neither does
> >>>>> setting the request->global_seqno back to zero wake up listeners to the
> >>>>> request->execute waitqueue. So the only time that listening to the
> >>>>> request->execute waitqueue would wake up the signaling kthread would be
> >>>>> on the request resubmission, during which time we would already receive
> >>>>> wake ups from rejoining the global breadcrumbs wait rbtree.
> >>>>>
> >>>>> Trying to wake up to release the request remains an issue. If the
> >>>>> signaling was cancelled and no other request required signaling, then it
> >>>>> is possible for us to shutdown with the reference on the request still
> >>>>> held. To ensure that we do not try to shutdown, leaking that request, we
> >>>>> kick the signaling threads whenever we disarm the breadcrumbs, i.e. on
> >>>>> parking the engine when idle.
> >>>>>
> >>>>> 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: Michał Winiarski <michal.winiarski@intel.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 +++++++++---------
> >>>>>     1 file changed, 9 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>>>> index 5ae2d276f7f3..b826725989b1 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>>>> @@ -216,7 +216,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >>>>>         struct intel_wait *wait, *n, *first;
> >>>>>     
> >>>>>         if (!b->irq_armed)
> >>>>> -             return;
> >>>>> +             goto wakeup_signaler;
> >>>>>     
> >>>>>         /* We only disarm the irq when we are idle (all requests completed),
> >>>>>          * so if the bottom-half remains asleep, it missed the request
> >>>>> @@ -239,6 +239,14 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> >>>>>         b->waiters = RB_ROOT;
> >>>>>     
> >>>>>         spin_unlock_irq(&b->rb_lock);
> >>>>> +
> >>>>> +     /*
> >>>>> +      * The signaling thread may be asleep holding a reference to a request,
> >>>>> +      * that had its signaling cancelled prior to being preempted. We need
> >>>>> +      * to kick the signaler, just in case, to release any such reference.
> >>>>> +      */
> >>>>> +wakeup_signaler:
> >>>>> +     wake_up_process(b->signaler);
> >>>
> >>> One thing I was thinking about from our conversation, is whether we can
> >>> rely on the kthread_stop() kick for the final ref release? Whether it's
> >>> even a good idea to let the reference live for that long?
> >>
> >> Probably not, feels awkward. In that sense previous version feels better.
> > 
> > Ok.
> >   
> >>> Basically it will mean replacing the GEM_BUG_ON(request) there with
> >>> another unref. But it will remove the complication and assoicated
> >>> commentary here.
> >>
> >> Hm, and is there a test which was able to hit the GEM_BUG_ON? It needed
> >> to go in the previous version as well, no?
> > 
> > It does indeed need to be there today, and with the v1 patch. Since
> > there's no serialisation to allow the previous wakeup to complete and
> > schedule() again before we hit the kthread_should_stop.
> 
> Right, it shouldn't be a GEM_BUG_ON for an expected code path but 
> replaced with i915_gem_request_put?

Right, just pushed with that amendment. I'm thankful you didn't have a
different suggestion :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Stop listening to request resubmission from the signaler kthread (rev3)
URL   : https://patchwork.freedesktop.org/series/35084/
State : failure

== Summary ==

Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
                pass       -> SKIP       (shard-hsw)
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623 +1
Test gem_eio:
        Subgroup in-flight-suspend:
                skip       -> PASS       (shard-snb)
Test kms_flip:
        Subgroup vblank-vs-suspend-interruptible:
                incomplete -> PASS       (shard-hsw) fdo#100368
Test drv_suspend:
        Subgroup forcewake:
                pass       -> INCOMPLETE (shard-hsw)
Test gem_tiled_swapping:
        Subgroup non-threaded:
                incomplete -> PASS       (shard-hsw) fdo#104009
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-a-planes:
                pass       -> SKIP       (shard-snb) fdo#102365
Test gem_softpin:
        Subgroup noreloc-s4:
                fail       -> DMESG-FAIL (shard-snb) fdo#103375

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#104009 https://bugs.freedesktop.org/show_bug.cgi?id=104009
fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375

shard-hsw        total:2657 pass:1519 dwarn:1   dfail:1   fail:8   skip:1127 time:9196s
shard-snb        total:2692 pass:1307 dwarn:1   dfail:1   fail:12  skip:1371 time:8150s
Blacklisted hosts:
shard-apl        total:2692 pass:1688 dwarn:1   dfail:0   fail:22  skip:981 time:13820s
shard-kbl        total:2692 pass:1808 dwarn:1   dfail:0   fail:23  skip:860 time:11133s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7467/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx