[PATCH] drm/i915/breadcrumbs: Tighten irqoff to the inner irq_lock

Chris Wilson chris at chris-wilson.co.uk
Sat May 19 12:19:23 UTC 2018


Only the breadcrumbs.irq_lock is accessed from inside the interrupt
handler, but in many places we disabled the irqs around the outer
breadcrumbs.rb_lock for convenience (as we entered from various paths
under known irq state). However, we should aim to reduce our irqoff
latency as much as possible, so tighten up the locking.

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  4 +--
 drivers/gpu/drm/i915/i915_gpu_error.c    |  8 ++---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 43 ++++++++++++++----------
 drivers/gpu/drm/i915/intel_engine_cs.c   |  4 +--
 4 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 52515445ac40..09e7ba6e0533 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1365,14 +1365,14 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 					  &dev_priv->gpu_error.missed_irq_rings)),
 			   yesno(engine->hangcheck.stalled));
 
-		spin_lock_irq(&b->rb_lock);
+		spin_lock(&b->rb_lock);
 		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
 			struct intel_wait *w = rb_entry(rb, typeof(*w), node);
 
 			seq_printf(m, "\t%s [%d] waiting for %x\n",
 				   w->tsk->comm, w->tsk->pid, w->seqno);
 		}
-		spin_unlock_irq(&b->rb_lock);
+		spin_unlock(&b->rb_lock);
 
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 47721437a4c5..93fb2f63c70a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1140,7 +1140,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 	if (RB_EMPTY_ROOT(&b->waiters))
 		return;
 
-	if (!spin_trylock_irq(&b->rb_lock)) {
+	if (!spin_trylock(&b->rb_lock)) {
 		ee->waiters = ERR_PTR(-EDEADLK);
 		return;
 	}
@@ -1148,7 +1148,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 	count = 0;
 	for (rb = rb_first(&b->waiters); rb != NULL; rb = rb_next(rb))
 		count++;
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 
 	waiter = NULL;
 	if (count)
@@ -1158,7 +1158,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 	if (!waiter)
 		return;
 
-	if (!spin_trylock_irq(&b->rb_lock)) {
+	if (!spin_trylock(&b->rb_lock)) {
 		kfree(waiter);
 		ee->waiters = ERR_PTR(-EDEADLK);
 		return;
@@ -1176,7 +1176,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 		if (++ee->num_waiters == count)
 			break;
 	}
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 }
 
 static void error_record_engine_registers(struct i915_gpu_state *error,
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 18e643df523e..f5fa286ee5e2 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -160,6 +160,9 @@ static void intel_breadcrumbs_fake_irq(struct timer_list *t)
 
 static void irq_enable(struct intel_engine_cs *engine)
 {
+	/* Caller disables interrupts */
+	lockdep_assert_irqs_disabled();
+
 	/*
 	 * FIXME: Ideally we want this on the API boundary, but for the
 	 * sake of testing with mock breadcrumbs (no HW so unable to
@@ -174,7 +177,6 @@ static void irq_enable(struct intel_engine_cs *engine)
 	 */
 	set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
 
-	/* Caller disables interrupts */
 	if (engine->irq_enable) {
 		spin_lock(&engine->i915->irq_lock);
 		engine->irq_enable(engine);
@@ -185,6 +187,8 @@ static void irq_enable(struct intel_engine_cs *engine)
 static void irq_disable(struct intel_engine_cs *engine)
 {
 	/* Caller disables interrupts */
+	lockdep_assert_irqs_disabled();
+
 	if (engine->irq_disable) {
 		spin_lock(&engine->i915->irq_lock);
 		engine->irq_disable(engine);
@@ -197,6 +201,8 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
 	lockdep_assert_held(&b->irq_lock);
+	lockdep_assert_irqs_disabled();
+
 	GEM_BUG_ON(b->irq_wait);
 	GEM_BUG_ON(!b->irq_armed);
 
@@ -245,13 +251,13 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)
 		missed_breadcrumb(engine);
 
-	spin_lock_irq(&b->rb_lock);
+	spin_lock(&b->rb_lock);
 
-	spin_lock(&b->irq_lock);
+	spin_lock_irq(&b->irq_lock);
 	b->irq_wait = NULL;
 	if (b->irq_armed)
 		__intel_engine_disarm_breadcrumbs(engine);
-	spin_unlock(&b->irq_lock);
+	spin_unlock_irq(&b->irq_lock);
 
 	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
 		GEM_BUG_ON(!i915_seqno_passed(intel_engine_get_seqno(engine),
@@ -261,7 +267,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	}
 	b->waiters = RB_ROOT;
 
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 }
 
 static bool use_fake_irq(const struct intel_breadcrumbs *b)
@@ -369,12 +375,13 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
 					    struct rb_node *next)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	unsigned long flags;
 
-	spin_lock(&b->irq_lock);
+	spin_lock_irqsave(&b->irq_lock, flags);
 	GEM_BUG_ON(!b->irq_armed);
 	GEM_BUG_ON(!b->irq_wait);
 	b->irq_wait = to_wait(next);
-	spin_unlock(&b->irq_lock);
+	spin_unlock_irqrestore(&b->irq_lock, flags);
 
 	/* We always wake up the next waiter that takes over as the bottom-half
 	 * as we may delegate not only the irq-seqno barrier to the next waiter
@@ -452,7 +459,9 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	rb_insert_color(&wait->node, &b->waiters);
 
 	if (first) {
-		spin_lock(&b->irq_lock);
+		unsigned long flags;
+
+		spin_lock_irqsave(&b->irq_lock, flags);
 		b->irq_wait = wait;
 		/* After assigning ourselves as the new bottom-half, we must
 		 * perform a cursory check to prevent a missed interrupt.
@@ -463,7 +472,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		 * and so we miss the wake up.
 		 */
 		armed = __intel_breadcrumbs_enable_irq(b);
-		spin_unlock(&b->irq_lock);
+		spin_unlock_irqrestore(&b->irq_lock, flags);
 	}
 
 	if (completed) {
@@ -498,9 +507,9 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	bool armed;
 
-	spin_lock_irq(&b->rb_lock);
+	spin_lock(&b->rb_lock);
 	armed = __intel_engine_add_wait(engine, wait);
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 	if (armed)
 		return armed;
 
@@ -598,9 +607,9 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 		return;
 	}
 
-	spin_lock_irq(&b->rb_lock);
+	spin_lock(&b->rb_lock);
 	__intel_engine_remove_wait(engine, wait);
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 }
 
 static void signaler_set_rtpriority(void)
@@ -639,7 +648,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 		 */
 		seqno = intel_engine_get_seqno(engine);
 
-		spin_lock_irq(&b->rb_lock);
+		spin_lock(&b->rb_lock);
 		list_for_each_entry_safe(rq, n, &b->signals, signaling.link) {
 			u32 this = rq->signaling.wait.seqno;
 
@@ -663,7 +672,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 				}
 			}
 		}
-		spin_unlock_irq(&b->rb_lock);
+		spin_unlock(&b->rb_lock);
 
 		if (!list_empty(&list)) {
 			local_bh_disable();
@@ -774,7 +783,7 @@ bool intel_engine_enable_signaling(struct i915_request *request, bool wakeup)
 	 * If we are the oldest waiter, enable the irq (after which we
 	 * must double check that the seqno did not complete).
 	 */
-	spin_lock(&b->rb_lock);
+	spin_lock_nested(&b->rb_lock, SINGLE_DEPTH_NESTING);
 	insert_signal(b, request, seqno);
 	wakeup &= __intel_engine_add_wait(engine, wait);
 	spin_unlock(&b->rb_lock);
@@ -798,7 +807,7 @@ void intel_engine_cancel_signaling(struct i915_request *request)
 	if (!READ_ONCE(request->signaling.wait.seqno))
 		return;
 
-	spin_lock(&b->rb_lock);
+	spin_lock_nested(&b->rb_lock, SINGLE_DEPTH_NESTING);
 	__intel_engine_remove_wait(engine, &request->signaling.wait);
 	if (fetch_and_zero(&request->signaling.wait.seqno))
 		__list_del_entry(&request->signaling.link);
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 26f9f8aab949..226007eef546 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1468,14 +1468,14 @@ void intel_engine_dump(struct intel_engine_cs *engine,
 
 	spin_unlock_irq(&engine->timeline.lock);
 
-	spin_lock_irq(&b->rb_lock);
+	spin_lock(&b->rb_lock);
 	for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
 		struct intel_wait *w = rb_entry(rb, typeof(*w), node);
 
 		drm_printf(m, "\t%s [%d] waiting for %x\n",
 			   w->tsk->comm, w->tsk->pid, w->seqno);
 	}
-	spin_unlock_irq(&b->rb_lock);
+	spin_unlock(&b->rb_lock);
 
 	drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s) (execlists? %s)\n",
 		   engine->irq_posted,
-- 
2.17.0



More information about the Intel-gfx-trybot mailing list