[CI 1/4] drm/i915: Check signaled state after enabling signaling

[CI 1/4] drm/i915: Check signaled state after enabling signaling

[Chris Wilson]

Setting up the irq to signal the request completion takes a finite
amount of time, during which it is possible that the request already
completed. Check afterwards, just in case, so that we can respond
immediately.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 0d1e0d8873ef..46d869e26b4d 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -62,7 +62,7 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
 		return false;
 
 	intel_engine_enable_signaling(to_request(fence), true);
-	return true;
+	return !i915_fence_signaled(fence);
 }
 
 static signed long i915_fence_wait(struct dma_fence *fence,
-- 
2.11.0

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

[CI 2/4] drm/i915: Report back whether the irq was armed when adding the waiter

[Chris Wilson]

The important condition that we need to check after enabling the
interrupt for signaling is whether the request completed in the process
(and so we missed that interrupt). A large cost in enabling the
signaling (rather than waiters) is in waking up the auxiliary signaling
thread, but we only need to do so to catch that missed interrupt. If we
know we didn't miss any interrupts (because we didn't arm the interrupt)
then we can skip waking the auxiliary thread.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 183afcb036aa..c90a72f87d82 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -234,7 +234,7 @@ static void enable_fake_irq(struct intel_breadcrumbs *b)
 		mod_timer(&b->hangcheck, wait_timeout());
 }
 
-static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
+static bool __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 {
 	struct intel_engine_cs *engine =
 		container_of(b, struct intel_engine_cs, breadcrumbs);
@@ -242,7 +242,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 
 	lockdep_assert_held(&b->irq_lock);
 	if (b->irq_armed)
-		return;
+		return false;
 
 	/* The breadcrumb irq will be disarmed on the interrupt after the
 	 * waiters are signaled. This gives us a single interrupt window in
@@ -260,7 +260,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 		 * implementation to call intel_engine_wakeup()
 		 * itself when it wants to simulate a user interrupt,
 		 */
-		return;
+		return true;
 	}
 
 	/* Since we are waiting on a request, the GPU should be busy
@@ -278,6 +278,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 	}
 
 	enable_fake_irq(b);
+	return true;
 }
 
 static inline struct intel_wait *to_wait(struct rb_node *node)
@@ -329,7 +330,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	struct rb_node **p, *parent, *completed;
-	bool first;
+	bool first, armed;
 	u32 seqno;
 
 	/* Insert the request into the retirement ordered list
@@ -344,6 +345,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	 * removing stale elements in the tree, we may be able to reduce the
 	 * ping-pong between the old bottom-half and ourselves as first-waiter.
 	 */
+	armed = false;
 	first = true;
 	parent = NULL;
 	completed = NULL;
@@ -399,7 +401,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		 * in the unlocked read of b->irq_seqno_bh in the irq handler)
 		 * and so we miss the wake up.
 		 */
-		__intel_breadcrumbs_enable_irq(b);
+		armed = __intel_breadcrumbs_enable_irq(b);
 		spin_unlock(&b->irq_lock);
 	}
 
@@ -426,20 +428,24 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	GEM_BUG_ON(!b->irq_armed);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
 
-	return first;
+	return armed;
 }
 
 bool intel_engine_add_wait(struct intel_engine_cs *engine,
 			   struct intel_wait *wait)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	bool first;
+	bool armed;
 
 	spin_lock_irq(&b->rb_lock);
-	first = __intel_engine_add_wait(engine, wait);
+	armed = __intel_engine_add_wait(engine, wait);
 	spin_unlock_irq(&b->rb_lock);
+	if (armed)
+		return armed;
 
-	return first;
+	/* Make the caller recheck if its request has already started. */
+	return i915_seqno_passed(intel_engine_get_seqno(engine),
+				 wait->seqno - 1);
 }
 
 static inline bool chain_wakeup(struct rb_node *rb, int priority)
-- 
2.11.0

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

[CI 3/4] drm/i915: Skip adding the request to the signal tree is complete

[Chris Wilson]

Enabling the interrupt for the signaler takes a finite amount of time (a
few microseconds) during which it is possible for the request to
complete. Check afterwards and skip adding the request to the signal
rbtree if it complete.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 49 ++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index c90a72f87d82..4e00e5cb9fa1 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -678,8 +678,6 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
 {
 	struct intel_engine_cs *engine = request->engine;
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	struct rb_node *parent, **p;
-	bool first;
 	u32 seqno;
 
 	/* Note that we may be called from an interrupt handler on another
@@ -714,27 +712,36 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
 	 */
 	wakeup &= __intel_engine_add_wait(engine, &request->signaling.wait);
 
-	/* Now insert ourselves into the retirement ordered list of signals
-	 * on this engine. We track the oldest seqno as that will be the
-	 * first signal to complete.
-	 */
-	parent = NULL;
-	first = true;
-	p = &b->signals.rb_node;
-	while (*p) {
-		parent = *p;
-		if (i915_seqno_passed(seqno,
-				      to_signaler(parent)->signaling.wait.seqno)) {
-			p = &parent->rb_right;
-			first = false;
-		} else {
-			p = &parent->rb_left;
+	if (!__i915_gem_request_completed(request, seqno)) {
+		struct rb_node *parent, **p;
+		bool first;
+
+		/* Now insert ourselves into the retirement ordered list of
+		 * signals on this engine. We track the oldest seqno as that
+		 * will be the first signal to complete.
+		 */
+		parent = NULL;
+		first = true;
+		p = &b->signals.rb_node;
+		while (*p) {
+			parent = *p;
+			if (i915_seqno_passed(seqno,
+					      to_signaler(parent)->signaling.wait.seqno)) {
+				p = &parent->rb_right;
+				first = false;
+			} else {
+				p = &parent->rb_left;
+			}
 		}
+		rb_link_node(&request->signaling.node, parent, p);
+		rb_insert_color(&request->signaling.node, &b->signals);
+		if (first)
+			rcu_assign_pointer(b->first_signal, request);
+	} else {
+		__intel_engine_remove_wait(engine, &request->signaling.wait);
+		i915_gem_request_put(request);
+		wakeup = false;
 	}
-	rb_link_node(&request->signaling.node, parent, p);
-	rb_insert_color(&request->signaling.node, &b->signals);
-	if (first)
-		rcu_assign_pointer(b->first_signal, request);
 
 	spin_unlock(&b->rb_lock);
 
-- 
2.11.0

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

[CI 4/4] drm/i915: Remove the spin-request during execbuf await_request

[Chris Wilson]

Originally we would enable and disable the breadcrumb interrupt
immediately on demand. This was slow enough to have a large impact
(>30%) on tasks that hopped between engines. However, by using a shadow
to keep the irq alive for an extra interrupt (see commit 67b807a89230
("drm/i915: Delay disabling the user interrupt for breadcrumbs")) and
by recently reducing the cost in adding ourselves to the signal tree, we
no longer need to spin-request during await_request to avoid delays in
throughput tests. Without the earlier patches to stop the wakeup when
signaling if the irq was already active, we saw no improvement in
execbuf overhead (and corresponding contention in other clients) despite
the removal of the spinner in a simple test like glxgears. This means
there will be scenarios where now we spend longer enabling the interrupt
than we would have spent spinning, but these are not likely to have as
noticeable an impact as the high frequency test cases (where there
should not be any regression).

Ulterior motive: generalising the engine->sync_to to handle different
types of semaphores and non-semaphores.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 46d869e26b4d..8c59c79cbd8b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -683,7 +683,6 @@ static int
 i915_gem_request_await_request(struct drm_i915_gem_request *to,
 			       struct drm_i915_gem_request *from)
 {
-	u32 seqno;
 	int ret;
 
 	GEM_BUG_ON(to == from);
@@ -707,18 +706,14 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
 		return ret < 0 ? ret : 0;
 	}
 
-	seqno = i915_gem_request_global_seqno(from);
-	if (!seqno)
-		goto await_dma_fence;
+	if (to->engine->semaphore.sync_to) {
+		u32 seqno;
 
-	if (!to->engine->semaphore.sync_to) {
-		if (!__i915_gem_request_started(from, seqno))
-			goto await_dma_fence;
+		GEM_BUG_ON(!from->engine->semaphore.signal);
 
-		if (!__i915_spin_request(from, seqno, TASK_INTERRUPTIBLE, 2))
+		seqno = i915_gem_request_global_seqno(from);
+		if (!seqno)
 			goto await_dma_fence;
-	} else {
-		GEM_BUG_ON(!from->engine->semaphore.signal);
 
 		if (seqno <= to->timeline->global_sync[from->engine->id])
 			return 0;
@@ -729,10 +724,9 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
 			return ret;
 
 		to->timeline->global_sync[from->engine->id] = seqno;
+		return 0;
 	}
 
-	return 0;
-
 await_dma_fence:
 	ret = i915_sw_fence_await_dma_fence(&to->submit,
 					    &from->fence, 0,
-- 
2.11.0

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

✗ Fi.CI.BAT: warning for series starting with [CI,1/4] drm/i915: Check signaled state after enabling signaling

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/4] drm/i915: Check signaled state after enabling signaling
URL   : https://patchwork.freedesktop.org/series/25470/
State : warning

== Summary ==

Series 25470v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/25470/revisions/1/mbox/


24db004734623b21acb25f58084e5b8eadc4f908 drm-tip: 2017y-06m-08d-09h-22m-33s UTC integration manifest
290cab1 drm/i915: Remove the spin-request during execbuf await_request
56f1199 drm/i915: Skip adding the request to the signal tree is complete
ddc0ffa drm/i915: Report back whether the irq was armed when adding the waiter
f2dd46b drm/i915: Check signaled state after enabling signaling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4908/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [CI,1/4] drm/i915: Check signaled state after enabling signaling

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/4] drm/i915: Check signaled state after enabling signaling
URL   : https://patchwork.freedesktop.org/series/25470/
State : success

== Summary ==

Series 25470v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/25470/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125 +1
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +1

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:447s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:431s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:510s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:481s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:582s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:439s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:413s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:415s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:496s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:567s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:573s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:459s
fi-skl-6700hq    total:278  pass:228  dwarn:1   dfail:0   fail:27  skip:22  time:401s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:469s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:440s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:400s

24db004734623b21acb25f58084e5b8eadc4f908 drm-tip: 2017y-06m-08d-09h-22m-33s UTC integration manifest
290cab1 drm/i915: Remove the spin-request during execbuf await_request
56f1199 drm/i915: Skip adding the request to the signal tree is complete
ddc0ffa drm/i915: Report back whether the irq was armed when adding the waiter
f2dd46b drm/i915: Check signaled state after enabling signaling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4908/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx