[CI,1/4] drm/i915: Reduce breadcrumb lock coverage for intel_engine_enable_signaling()

Commit Message

Chris Wilson July 26, 2016, 11:01 a.m. UTC

Since intel_engine_enable_signaling() is now only called via
fence_enable_sw_signaling(), we can rely on it to provide serialisation
and run-once for us and so make ourselves slightly simpler.

Signed-off-by: Chris Wilson <[e-mail]>
Link: http://patchwork.freedesktop.org/patch/msgid/[e-mail]
Cc: Tvrtko Ursulin <[e-mail]>
Reviewed-by: Joonas Lahtinen <[e-mail]>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Chris Wilson July 26, 2016, 12:48 p.m. UTC | #1

On Tue, Jul 26, 2016 at 11:44:37AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [CI,1/4] drm/i915: Reduce breadcrumb lock coverage for intel_engine_enable_signaling()
> URL   : https://patchwork.freedesktop.org/series/10268/
> State : failure
> 
> == Summary ==
> 
> Series 10268v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/10268/revisions/1/mbox
> 
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 pass       -> INCOMPLETE (fi-hsw-i7-4770k)
>                 dmesg-warn -> PASS       (fi-skl-i5-6260u)
> Test gem_sync:
>         Subgroup basic-store-each:
>                 pass       -> SKIP       (ro-bdw-i7-5600u)
>                 pass       -> SKIP       (ro-bdw-i5-5250u)

Confirmed with a trybot nop patch these are not due to these patches
(which confirms earlier successes).
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index d893ccdd62ac..90867446f1a5 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -480,19 +480,15 @@  void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 	struct rb_node *parent, **p;
 	bool first, wakeup;
 
-	if (unlikely(READ_ONCE(request->signaling.wait.tsk)))
-		return;
-
-	spin_lock(&b->lock);
-	if (unlikely(request->signaling.wait.tsk)) {
-		wakeup = false;
-		goto unlock;
-	}
+	/* locked by fence_enable_sw_signaling() */
+	assert_spin_locked(&request->lock);
 
 	request->signaling.wait.tsk = b->signaler;
 	request->signaling.wait.seqno = request->fence.seqno;
 	i915_gem_request_get(request);
 
+	spin_lock(&b->lock);
+
 	/* First add ourselves into the list of waiters, but register our
 	 * bottom-half as the signaller thread. As per usual, only the oldest
 	 * waiter (not just signaller) is tasked as the bottom-half waking
@@ -525,7 +521,6 @@  void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 	if (first)
 		smp_store_mb(b->first_signal, request);
 
-unlock:
 	spin_unlock(&b->lock);
 
 	if (wakeup)