[PATCH v2 1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter

[PATCH v2 1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter

[Chris Wilson]

Refactor the common task of updating the first_waiter, serialised with
the interrupt handler. When we update the first_waiter, we also need to
wakeup the new bottom-half in order to complete the actions that we may
have delegated to it (such as checking the irq-seqno coherency or waking
up other lower priority concurrent waiters).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 48 +++++++++++++-------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 235d4645a5cf..1cc50304f824 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -287,6 +287,21 @@ static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
 	wake_up_process(wait->tsk); /* implicit smp_wmb() */
 }
 
+static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
+					    struct rb_node *next)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+	GEM_BUG_ON(!b->irq_armed);
+	b->first_wait = to_wait(next);
+
+	/* 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
+	 * but also the task of waking up concurrent waiters.
+	 */
+	wake_up_process(to_wait(next)->tsk);
+}
+
 static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 				    struct intel_wait *wait)
 {
@@ -357,21 +372,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		GEM_BUG_ON(!next && !first);
 		if (next && next != &wait->node) {
 			GEM_BUG_ON(first);
-			b->first_wait = to_wait(next);
-			/* As there is a delay between reading the current
-			 * seqno, processing the completed tasks and selecting
-			 * the next waiter, we may have missed the interrupt
-			 * and so need for the next bottom-half to wakeup.
-			 *
-			 * Also as we enable the IRQ, we may miss the
-			 * interrupt for that seqno, so we have to wake up
-			 * the next bottom-half in order to do a coherent check
-			 * in case the seqno passed.
-			 */
-			__intel_breadcrumbs_enable_irq(b);
-			if (test_bit(ENGINE_IRQ_BREADCRUMB,
-				     &engine->irq_posted))
-				wake_up_process(to_wait(next)->tsk);
+			__intel_breadcrumbs_next(engine, next);
 		}
 
 		do {
@@ -473,21 +474,10 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 			}
 		}
 
-		if (next) {
-			/* In our haste, we may have completed the first waiter
-			 * before we enabled the interrupt. Do so now as we
-			 * have a second waiter for a future seqno. Afterwards,
-			 * we have to wake up that waiter in case we missed
-			 * the interrupt, or if we have to handle an
-			 * exception rather than a seqno completion.
-			 */
-			b->first_wait = to_wait(next);
-			if (b->first_wait->seqno != wait->seqno)
-				__intel_breadcrumbs_enable_irq(b);
-			wake_up_process(b->first_wait->tsk);
-		} else {
+		if (next)
+			__intel_breadcrumbs_next(engine, next);
+		else
 			b->first_wait = NULL;
-		}
 	} else {
 		GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
 	}
-- 
2.11.0

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

[PATCH v2 2/3] drm/i915: Split breadcrumbs spinlock into two

[Chris Wilson]

As we now take the breadcrumbs spinlock within the interrupt handler, we
wish to minimise its hold time. During the interrupt we do not care
about the state of the full rbtree, only that of the first element, so
we can guard that with a separate lock.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c      | 12 +++----
 drivers/gpu/drm/i915/i915_drv.h          |  4 +--
 drivers/gpu/drm/i915/i915_gpu_error.c    |  8 ++---
 drivers/gpu/drm/i915/i915_irq.c          |  4 +--
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 58 ++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  6 ++--
 6 files changed, 51 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 95046822e8e0..aa2d726b4349 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -700,14 +700,14 @@ static void i915_ring_seqno_info(struct seq_file *m,
 	seq_printf(m, "Current sequence (%s): %x\n",
 		   engine->name, intel_engine_get_seqno(engine));
 
-	spin_lock_irq(&b->lock);
+	spin_lock_irq(&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, "Waiting (%s): %s [%d] on %x\n",
 			   engine->name, w->tsk->comm, w->tsk->pid, w->seqno);
 	}
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->rb_lock);
 }
 
 static int i915_gem_seqno_info(struct seq_file *m, void *data)
@@ -1354,14 +1354,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->lock);
+		spin_lock_irq(&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->lock);
+		spin_unlock_irq(&b->rb_lock);
 
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
@@ -3359,14 +3359,14 @@ static int i915_engine_info(struct seq_file *m, void *unused)
 				   I915_READ(RING_PP_DIR_DCLV(engine)));
 		}
 
-		spin_lock_irq(&b->lock);
+		spin_lock_irq(&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->lock);
+		spin_unlock_irq(&b->rb_lock);
 
 		seq_puts(m, "\n");
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cb760156bbc5..0da14c304771 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4100,7 +4100,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 		 * the seqno before we believe it coherent since they see
 		 * irq_posted == false but we are still running).
 		 */
-		spin_lock_irqsave(&b->lock, flags);
+		spin_lock_irqsave(&b->irq_lock, flags);
 		if (b->first_wait && b->first_wait->tsk != current)
 			/* Note that if the bottom-half is changed as we
 			 * are sending the wake-up, the new bottom-half will
@@ -4109,7 +4109,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 			 * ourself.
 			 */
 			wake_up_process(b->first_wait->tsk);
-		spin_unlock_irqrestore(&b->lock, flags);
+		spin_unlock_irqrestore(&b->irq_lock, flags);
 
 		if (__i915_gem_request_completed(req, seqno))
 			return true;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 061af8040498..8effc59f5cb5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1111,7 +1111,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 	if (RB_EMPTY_ROOT(&b->waiters))
 		return;
 
-	if (!spin_trylock_irq(&b->lock)) {
+	if (!spin_trylock_irq(&b->rb_lock)) {
 		ee->waiters = ERR_PTR(-EDEADLK);
 		return;
 	}
@@ -1119,7 +1119,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->lock);
+	spin_unlock_irq(&b->rb_lock);
 
 	waiter = NULL;
 	if (count)
@@ -1129,7 +1129,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 	if (!waiter)
 		return;
 
-	if (!spin_trylock_irq(&b->lock)) {
+	if (!spin_trylock_irq(&b->rb_lock)) {
 		kfree(waiter);
 		ee->waiters = ERR_PTR(-EDEADLK);
 		return;
@@ -1147,7 +1147,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
 		if (++ee->num_waiters == count)
 			break;
 	}
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->rb_lock);
 }
 
 static void error_record_engine_registers(struct i915_gpu_state *error,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5fa2c4c56b09..3f39e36fa566 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1041,7 +1041,7 @@ static void notify_ring(struct intel_engine_cs *engine)
 
 	rcu_read_lock();
 
-	spin_lock(&engine->breadcrumbs.lock);
+	spin_lock(&engine->breadcrumbs.irq_lock);
 	wait = engine->breadcrumbs.first_wait;
 	if (wait) {
 		/* We use a callback from the dma-fence to submit
@@ -1063,7 +1063,7 @@ static void notify_ring(struct intel_engine_cs *engine)
 	} else {
 		__intel_engine_disarm_breadcrumbs(engine);
 	}
-	spin_unlock(&engine->breadcrumbs.lock);
+	spin_unlock(&engine->breadcrumbs.irq_lock);
 
 	if (rq)
 		dma_fence_signal(&rq->fence);
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 1cc50304f824..34200f14bade 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -31,6 +31,8 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
 	struct intel_wait *wait;
 	unsigned int result = 0;
 
+	lockdep_assert_held(&b->irq_lock);
+
 	wait = b->first_wait;
 	if (wait) {
 		result = ENGINE_WAKEUP_WAITER;
@@ -47,9 +49,9 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
 	unsigned long flags;
 	unsigned int result;
 
-	spin_lock_irqsave(&b->lock, flags);
+	spin_lock_irqsave(&b->irq_lock, flags);
 	result = __intel_breadcrumbs_wakeup(b);
-	spin_unlock_irqrestore(&b->lock, flags);
+	spin_unlock_irqrestore(&b->irq_lock, flags);
 
 	return result;
 }
@@ -117,10 +119,10 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
 	 * coherent seqno check.
 	 */
 
-	spin_lock_irqsave(&b->lock, flags);
+	spin_lock_irqsave(&b->irq_lock, flags);
 	if (!__intel_breadcrumbs_wakeup(b))
 		__intel_engine_disarm_breadcrumbs(engine);
-	spin_unlock_irqrestore(&b->lock, flags);
+	spin_unlock_irqrestore(&b->irq_lock, flags);
 	if (!b->irq_armed)
 		return;
 
@@ -164,7 +166,7 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
-	lockdep_assert_held(&b->lock);
+	lockdep_assert_held(&b->irq_lock);
 
 	if (b->irq_enabled) {
 		irq_disable(engine);
@@ -182,7 +184,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	if (!b->irq_armed)
 		return;
 
-	spin_lock_irqsave(&b->lock, flags);
+	spin_lock_irqsave(&b->irq_lock, flags);
 
 	/* We only disarm the irq when we are idle (all requests completed),
 	 * so if there remains a sleeping waiter, it missed the request
@@ -193,7 +195,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 
 	__intel_engine_disarm_breadcrumbs(engine);
 
-	spin_unlock_irqrestore(&b->lock, flags);
+	spin_unlock_irqrestore(&b->irq_lock, flags);
 }
 
 static bool use_fake_irq(const struct intel_breadcrumbs *b)
@@ -228,7 +230,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 		container_of(b, struct intel_engine_cs, breadcrumbs);
 	struct drm_i915_private *i915 = engine->i915;
 
-	lockdep_assert_held(&b->lock);
+	lockdep_assert_held(&b->irq_lock);
 	if (b->irq_armed)
 		return;
 
@@ -276,7 +278,7 @@ static inline struct intel_wait *to_wait(struct rb_node *node)
 static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
 					      struct intel_wait *wait)
 {
-	lockdep_assert_held(&b->lock);
+	lockdep_assert_held(&b->rb_lock);
 
 	/* This request is completed, so remove it from the tree, mark it as
 	 * complete, and *then* wake up the associated task.
@@ -292,8 +294,10 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
+	spin_lock(&b->irq_lock);
 	GEM_BUG_ON(!b->irq_armed);
 	b->first_wait = to_wait(next);
+	spin_unlock(&b->irq_lock);
 
 	/* 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
@@ -383,6 +387,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	}
 
 	if (first) {
+		spin_lock(&b->irq_lock);
 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
 		b->first_wait = wait;
 		/* After assigning ourselves as the new bottom-half, we must
@@ -394,6 +399,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		 * and so we miss the wake up.
 		 */
 		__intel_breadcrumbs_enable_irq(b);
+		spin_unlock(&b->irq_lock);
 	}
 	GEM_BUG_ON(!b->first_wait);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
@@ -407,9 +413,9 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	bool first;
 
-	spin_lock_irq(&b->lock);
+	spin_lock_irq(&b->rb_lock);
 	first = __intel_engine_add_wait(engine, wait);
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->rb_lock);
 
 	return first;
 }
@@ -433,7 +439,7 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
-	lockdep_assert_held(&b->lock);
+	lockdep_assert_held(&b->rb_lock);
 
 	if (RB_EMPTY_NODE(&wait->node))
 		goto out;
@@ -503,9 +509,9 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 	if (RB_EMPTY_NODE(&wait->node))
 		return;
 
-	spin_lock_irq(&b->lock);
+	spin_lock_irq(&b->rb_lock);
 	__intel_engine_remove_wait(engine, wait);
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->rb_lock);
 }
 
 static bool signal_valid(const struct drm_i915_gem_request *request)
@@ -575,7 +581,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 			dma_fence_signal(&request->fence);
 			local_bh_enable(); /* kick start the tasklets */
 
-			spin_lock_irq(&b->lock);
+			spin_lock_irq(&b->rb_lock);
 
 			/* Wake up all other completed waiters and select the
 			 * next bottom-half for the next user interrupt.
@@ -598,7 +604,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 			rb_erase(&request->signaling.node, &b->signals);
 			RB_CLEAR_NODE(&request->signaling.node);
 
-			spin_unlock_irq(&b->lock);
+			spin_unlock_irq(&b->rb_lock);
 
 			i915_gem_request_put(request);
 		} else {
@@ -655,7 +661,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 	request->signaling.wait.seqno = seqno;
 	i915_gem_request_get(request);
 
-	spin_lock(&b->lock);
+	spin_lock(&b->rb_lock);
 
 	/* First add ourselves into the list of waiters, but register our
 	 * bottom-half as the signaller thread. As per usual, only the oldest
@@ -689,7 +695,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 	if (first)
 		rcu_assign_pointer(b->first_signal, request);
 
-	spin_unlock(&b->lock);
+	spin_unlock(&b->rb_lock);
 
 	if (wakeup)
 		wake_up_process(b->signaler);
@@ -704,7 +710,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
 	lockdep_assert_held(&request->lock);
 	GEM_BUG_ON(!request->signaling.wait.seqno);
 
-	spin_lock(&b->lock);
+	spin_lock(&b->rb_lock);
 
 	if (!RB_EMPTY_NODE(&request->signaling.node)) {
 		if (request == rcu_access_pointer(b->first_signal)) {
@@ -720,7 +726,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
 
 	__intel_engine_remove_wait(engine, &request->signaling.wait);
 
-	spin_unlock(&b->lock);
+	spin_unlock(&b->rb_lock);
 
 	request->signaling.wait.seqno = 0;
 }
@@ -730,7 +736,9 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	struct task_struct *tsk;
 
-	spin_lock_init(&b->lock);
+	spin_lock_init(&b->rb_lock);
+	spin_lock_init(&b->irq_lock);
+
 	setup_timer(&b->fake_irq,
 		    intel_breadcrumbs_fake_irq,
 		    (unsigned long)engine);
@@ -768,7 +776,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
 	cancel_fake_irq(engine);
-	spin_lock_irq(&b->lock);
+	spin_lock_irq(&b->irq_lock);
 
 	if (b->irq_enabled)
 		irq_enable(engine);
@@ -787,7 +795,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
 	if (b->irq_armed)
 		enable_fake_irq(b);
 
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->irq_lock);
 }
 
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
@@ -811,7 +819,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	bool busy = false;
 
-	spin_lock_irq(&b->lock);
+	spin_lock_irq(&b->rb_lock);
 
 	if (b->first_wait) {
 		wake_up_process(b->first_wait->tsk);
@@ -823,7 +831,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
 		busy |= intel_engine_flag(engine);
 	}
 
-	spin_unlock_irq(&b->lock);
+	spin_unlock_irq(&b->rb_lock);
 
 	return busy;
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 55a6a3f8274c..621ac9998d16 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,10 +235,12 @@ struct intel_engine_cs {
 	 * the overhead of waking that client is much preferred.
 	 */
 	struct intel_breadcrumbs {
-		spinlock_t lock; /* protects the lists of requests; irqsafe */
+		spinlock_t irq_lock; /* protects first_wait & irq_*; irqsafe */
+		struct intel_wait *first_wait; /* oldest waiter by retirement */
+
+		spinlock_t rb_lock; /* protects the rbtrees; irqsafe */
 		struct rb_root waiters; /* sorted by retirement, priority */
 		struct rb_root signals; /* sorted by retirement */
-		struct intel_wait *first_wait; /* oldest waiter by retirement */
 		struct task_struct *signaler; /* used for fence signalling */
 		struct drm_i915_gem_request __rcu *first_signal;
 		struct timer_list fake_irq; /* used after a missed interrupt */
-- 
2.11.0

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

[PATCH v2 3/3] drm/i915: Only wake the waiter from the interrupt if passed

[Chris Wilson]

As we now check if the seqno is complete in order to signal the fence,
we can also decide not to wake up the first_waiter until it is ready
(since it is waiting on the same seqno). The only caveat is that if we
need the engine->irq_seqno_barrier to enforce some coherency between an
interrupt and the seqno read, we have to always wake the waiter into to
perform that heavyweight barrier.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3f39e36fa566..c902aff61a9d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1059,7 +1059,8 @@ static void notify_ring(struct intel_engine_cs *engine)
 				      wait->seqno))
 			rq = wait->request;
 
-		wake_up_process(wait->tsk);
+		if (rq || engine->irq_seqno_barrier)
+			wake_up_process(wait->tsk);
 	} else {
 		__intel_engine_disarm_breadcrumbs(engine);
 	}
-- 
2.11.0

_______________________________________________
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 [v2,1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter
URL   : https://patchwork.freedesktop.org/series/20616/
State : success

== Summary ==

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

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

76a0b805abb234682f24695a9e3742125e401ec3 drm-tip: 2017y-03m-03d-10h-38m-03s UTC integration manifest
e6d7476 drm/i915: Only wake the waiter from the interrupt if passed
43886cc drm/i915: Split breadcrumbs spinlock into two
f90ac0c drm/i915: Refactor wakeup of the next breadcrumb waiter

== Logs ==

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

Re: [PATCH v2 1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter

[Chris Wilson]

On Fri, Mar 03, 2017 at 12:17:08PM +0000, Chris Wilson wrote:
> Refactor the common task of updating the first_waiter, serialised with
> the interrupt handler. When we update the first_waiter, we also need to
> wakeup the new bottom-half in order to complete the actions that we may
> have delegated to it (such as checking the irq-seqno coherency or waking
> up other lower priority concurrent waiters).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 48 +++++++++++++-------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 235d4645a5cf..1cc50304f824 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -287,6 +287,21 @@ static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
>  	wake_up_process(wait->tsk); /* implicit smp_wmb() */
>  }
>  
> +static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
> +					    struct rb_node *next)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	GEM_BUG_ON(!b->irq_armed);
> +	b->first_wait = to_wait(next);
> +
> +	/* 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
> +	 * but also the task of waking up concurrent waiters.
> +	 */
> +	wake_up_process(to_wait(next)->tsk);

Alternatively:

-       wake_up_process(to_wait(next)->tsk);
+       if ((engine->irq_seqno_barrier &&
+            test_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) ||
+           i915_seqno_passed(intel_engine_get_seqno(engine),
+                             to_wait(next)->seqno))
+               wake_up_process(to_wait(next)->tsk);

A plain wake_up_process() is certainly much less fragile.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Refactor wakeup of the next breadcrumb waiter

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Refactor the common task of updating the first_waiter, serialised with
> the interrupt handler. When we update the first_waiter, we also need to
> wakeup the new bottom-half in order to complete the actions that we may
> have delegated to it (such as checking the irq-seqno coherency or waking
> up other lower priority concurrent waiters).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 48 +++++++++++++-------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 235d4645a5cf..1cc50304f824 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -287,6 +287,21 @@ static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
>  	wake_up_process(wait->tsk); /* implicit smp_wmb() */
>  }
>  
> +static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
> +					    struct rb_node *next)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	GEM_BUG_ON(!b->irq_armed);
> +	b->first_wait = to_wait(next);
> +
> +	/* 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
> +	 * but also the task of waking up concurrent waiters.
> +	 */
> +	wake_up_process(to_wait(next)->tsk);
> +}
> +
>  static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  				    struct intel_wait *wait)
>  {
> @@ -357,21 +372,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		GEM_BUG_ON(!next && !first);
>  		if (next && next != &wait->node) {
>  			GEM_BUG_ON(first);
> -			b->first_wait = to_wait(next);
> -			/* As there is a delay between reading the current
> -			 * seqno, processing the completed tasks and selecting
> -			 * the next waiter, we may have missed the interrupt
> -			 * and so need for the next bottom-half to wakeup.
> -			 *
> -			 * Also as we enable the IRQ, we may miss the
> -			 * interrupt for that seqno, so we have to wake up
> -			 * the next bottom-half in order to do a coherent check
> -			 * in case the seqno passed.
> -			 */
> -			__intel_breadcrumbs_enable_irq(b);
> -			if (test_bit(ENGINE_IRQ_BREADCRUMB,
> -				     &engine->irq_posted))
> -				wake_up_process(to_wait(next)->tsk);
> +			__intel_breadcrumbs_next(engine, next);
>  		}
>  
>  		do {
> @@ -473,21 +474,10 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  			}
>  		}
>  
> -		if (next) {
> -			/* In our haste, we may have completed the first waiter
> -			 * before we enabled the interrupt. Do so now as we
> -			 * have a second waiter for a future seqno. Afterwards,
> -			 * we have to wake up that waiter in case we missed
> -			 * the interrupt, or if we have to handle an
> -			 * exception rather than a seqno completion.
> -			 */
> -			b->first_wait = to_wait(next);
> -			if (b->first_wait->seqno != wait->seqno)
> -				__intel_breadcrumbs_enable_irq(b);
> -			wake_up_process(b->first_wait->tsk);
> -		} else {
> +		if (next)
> +			__intel_breadcrumbs_next(engine, next);
> +		else
>  			b->first_wait = NULL;
> -		}
>  	} else {
>  		GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
>  	}
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Split breadcrumbs spinlock into two

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> As we now take the breadcrumbs spinlock within the interrupt handler, we
> wish to minimise its hold time. During the interrupt we do not care
> about the state of the full rbtree, only that of the first element, so
> we can guard that with a separate lock.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      | 12 +++----
>  drivers/gpu/drm/i915/i915_drv.h          |  4 +--
>  drivers/gpu/drm/i915/i915_gpu_error.c    |  8 ++---
>  drivers/gpu/drm/i915/i915_irq.c          |  4 +--
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 58 ++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  6 ++--
>  6 files changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 95046822e8e0..aa2d726b4349 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -700,14 +700,14 @@ static void i915_ring_seqno_info(struct seq_file *m,
>  	seq_printf(m, "Current sequence (%s): %x\n",
>  		   engine->name, intel_engine_get_seqno(engine));
>  
> -	spin_lock_irq(&b->lock);
> +	spin_lock_irq(&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, "Waiting (%s): %s [%d] on %x\n",
>  			   engine->name, w->tsk->comm, w->tsk->pid, w->seqno);
>  	}
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  }
>  
>  static int i915_gem_seqno_info(struct seq_file *m, void *data)
> @@ -1354,14 +1354,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->lock);
> +		spin_lock_irq(&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->lock);
> +		spin_unlock_irq(&b->rb_lock);
>  
>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>  			   (long long)engine->hangcheck.acthd,
> @@ -3359,14 +3359,14 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>  				   I915_READ(RING_PP_DIR_DCLV(engine)));
>  		}
>  
> -		spin_lock_irq(&b->lock);
> +		spin_lock_irq(&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->lock);
> +		spin_unlock_irq(&b->rb_lock);
>  
>  		seq_puts(m, "\n");
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb760156bbc5..0da14c304771 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4100,7 +4100,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>  		 * the seqno before we believe it coherent since they see
>  		 * irq_posted == false but we are still running).
>  		 */
> -		spin_lock_irqsave(&b->lock, flags);
> +		spin_lock_irqsave(&b->irq_lock, flags);
>  		if (b->first_wait && b->first_wait->tsk != current)
>  			/* Note that if the bottom-half is changed as we
>  			 * are sending the wake-up, the new bottom-half will
> @@ -4109,7 +4109,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>  			 * ourself.
>  			 */
>  			wake_up_process(b->first_wait->tsk);
> -		spin_unlock_irqrestore(&b->lock, flags);
> +		spin_unlock_irqrestore(&b->irq_lock, flags);
>  
>  		if (__i915_gem_request_completed(req, seqno))
>  			return true;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 061af8040498..8effc59f5cb5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1111,7 +1111,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
>  	if (RB_EMPTY_ROOT(&b->waiters))
>  		return;
>  
> -	if (!spin_trylock_irq(&b->lock)) {
> +	if (!spin_trylock_irq(&b->rb_lock)) {
>  		ee->waiters = ERR_PTR(-EDEADLK);
>  		return;
>  	}
> @@ -1119,7 +1119,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->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  
>  	waiter = NULL;
>  	if (count)
> @@ -1129,7 +1129,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
>  	if (!waiter)
>  		return;
>  
> -	if (!spin_trylock_irq(&b->lock)) {
> +	if (!spin_trylock_irq(&b->rb_lock)) {
>  		kfree(waiter);
>  		ee->waiters = ERR_PTR(-EDEADLK);
>  		return;
> @@ -1147,7 +1147,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
>  		if (++ee->num_waiters == count)
>  			break;
>  	}
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  }
>  
>  static void error_record_engine_registers(struct i915_gpu_state *error,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5fa2c4c56b09..3f39e36fa566 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1041,7 +1041,7 @@ static void notify_ring(struct intel_engine_cs *engine)
>  
>  	rcu_read_lock();
>  
> -	spin_lock(&engine->breadcrumbs.lock);
> +	spin_lock(&engine->breadcrumbs.irq_lock);
>  	wait = engine->breadcrumbs.first_wait;
>  	if (wait) {
>  		/* We use a callback from the dma-fence to submit
> @@ -1063,7 +1063,7 @@ static void notify_ring(struct intel_engine_cs *engine)
>  	} else {
>  		__intel_engine_disarm_breadcrumbs(engine);
>  	}
> -	spin_unlock(&engine->breadcrumbs.lock);
> +	spin_unlock(&engine->breadcrumbs.irq_lock);
>  
>  	if (rq)
>  		dma_fence_signal(&rq->fence);
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 1cc50304f824..34200f14bade 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -31,6 +31,8 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>  	struct intel_wait *wait;
>  	unsigned int result = 0;
>  
> +	lockdep_assert_held(&b->irq_lock);
> +
>  	wait = b->first_wait;
>  	if (wait) {
>  		result = ENGINE_WAKEUP_WAITER;
> @@ -47,9 +49,9 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
>  	unsigned long flags;
>  	unsigned int result;
>  
> -	spin_lock_irqsave(&b->lock, flags);
> +	spin_lock_irqsave(&b->irq_lock, flags);
>  	result = __intel_breadcrumbs_wakeup(b);
> -	spin_unlock_irqrestore(&b->lock, flags);
> +	spin_unlock_irqrestore(&b->irq_lock, flags);
>  
>  	return result;
>  }
> @@ -117,10 +119,10 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
>  	 * coherent seqno check.
>  	 */
>  
> -	spin_lock_irqsave(&b->lock, flags);
> +	spin_lock_irqsave(&b->irq_lock, flags);
>  	if (!__intel_breadcrumbs_wakeup(b))
>  		__intel_engine_disarm_breadcrumbs(engine);
> -	spin_unlock_irqrestore(&b->lock, flags);
> +	spin_unlock_irqrestore(&b->irq_lock, flags);
>  	if (!b->irq_armed)
>  		return;
>  
> @@ -164,7 +166,7 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  
> -	lockdep_assert_held(&b->lock);
> +	lockdep_assert_held(&b->irq_lock);
>  
>  	if (b->irq_enabled) {
>  		irq_disable(engine);
> @@ -182,7 +184,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	if (!b->irq_armed)
>  		return;
>  
> -	spin_lock_irqsave(&b->lock, flags);
> +	spin_lock_irqsave(&b->irq_lock, flags);
>  
>  	/* We only disarm the irq when we are idle (all requests completed),
>  	 * so if there remains a sleeping waiter, it missed the request
> @@ -193,7 +195,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  
>  	__intel_engine_disarm_breadcrumbs(engine);
>  
> -	spin_unlock_irqrestore(&b->lock, flags);
> +	spin_unlock_irqrestore(&b->irq_lock, flags);
>  }
>  
>  static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -228,7 +230,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  		container_of(b, struct intel_engine_cs, breadcrumbs);
>  	struct drm_i915_private *i915 = engine->i915;
>  
> -	lockdep_assert_held(&b->lock);
> +	lockdep_assert_held(&b->irq_lock);
>  	if (b->irq_armed)
>  		return;
>  
> @@ -276,7 +278,7 @@ static inline struct intel_wait *to_wait(struct rb_node *node)
>  static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
>  					      struct intel_wait *wait)
>  {
> -	lockdep_assert_held(&b->lock);
> +	lockdep_assert_held(&b->rb_lock);
>  
>  	/* This request is completed, so remove it from the tree, mark it as
>  	 * complete, and *then* wake up the associated task.
> @@ -292,8 +294,10 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  
> +	spin_lock(&b->irq_lock);
>  	GEM_BUG_ON(!b->irq_armed);
>  	b->first_wait = to_wait(next);
> +	spin_unlock(&b->irq_lock);
>  
>  	/* 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
> @@ -383,6 +387,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	}
>  
>  	if (first) {
> +		spin_lock(&b->irq_lock);
>  		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>  		b->first_wait = wait;
>  		/* After assigning ourselves as the new bottom-half, we must
> @@ -394,6 +399,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		 * and so we miss the wake up.
>  		 */
>  		__intel_breadcrumbs_enable_irq(b);
> +		spin_unlock(&b->irq_lock);
>  	}
>  	GEM_BUG_ON(!b->first_wait);
>  	GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
> @@ -407,9 +413,9 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  	bool first;
>  
> -	spin_lock_irq(&b->lock);
> +	spin_lock_irq(&b->rb_lock);
>  	first = __intel_engine_add_wait(engine, wait);
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  
>  	return first;
>  }
> @@ -433,7 +439,7 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  
> -	lockdep_assert_held(&b->lock);
> +	lockdep_assert_held(&b->rb_lock);
>  
>  	if (RB_EMPTY_NODE(&wait->node))
>  		goto out;
> @@ -503,9 +509,9 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	if (RB_EMPTY_NODE(&wait->node))
>  		return;
>  
> -	spin_lock_irq(&b->lock);
> +	spin_lock_irq(&b->rb_lock);
>  	__intel_engine_remove_wait(engine, wait);
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  }
>  
>  static bool signal_valid(const struct drm_i915_gem_request *request)
> @@ -575,7 +581,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			dma_fence_signal(&request->fence);
>  			local_bh_enable(); /* kick start the tasklets */
>  
> -			spin_lock_irq(&b->lock);
> +			spin_lock_irq(&b->rb_lock);
>  
>  			/* Wake up all other completed waiters and select the
>  			 * next bottom-half for the next user interrupt.
> @@ -598,7 +604,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			rb_erase(&request->signaling.node, &b->signals);
>  			RB_CLEAR_NODE(&request->signaling.node);
>  
> -			spin_unlock_irq(&b->lock);
> +			spin_unlock_irq(&b->rb_lock);
>  
>  			i915_gem_request_put(request);
>  		} else {
> @@ -655,7 +661,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
>  	request->signaling.wait.seqno = seqno;
>  	i915_gem_request_get(request);
>  
> -	spin_lock(&b->lock);
> +	spin_lock(&b->rb_lock);
>  
>  	/* First add ourselves into the list of waiters, but register our
>  	 * bottom-half as the signaller thread. As per usual, only the oldest
> @@ -689,7 +695,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
>  	if (first)
>  		rcu_assign_pointer(b->first_signal, request);
>  
> -	spin_unlock(&b->lock);
> +	spin_unlock(&b->rb_lock);
>  
>  	if (wakeup)
>  		wake_up_process(b->signaler);
> @@ -704,7 +710,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
>  	lockdep_assert_held(&request->lock);
>  	GEM_BUG_ON(!request->signaling.wait.seqno);
>  
> -	spin_lock(&b->lock);
> +	spin_lock(&b->rb_lock);
>  
>  	if (!RB_EMPTY_NODE(&request->signaling.node)) {
>  		if (request == rcu_access_pointer(b->first_signal)) {
> @@ -720,7 +726,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
>  
>  	__intel_engine_remove_wait(engine, &request->signaling.wait);
>  
> -	spin_unlock(&b->lock);
> +	spin_unlock(&b->rb_lock);
>  
>  	request->signaling.wait.seqno = 0;
>  }
> @@ -730,7 +736,9 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  	struct task_struct *tsk;
>  
> -	spin_lock_init(&b->lock);
> +	spin_lock_init(&b->rb_lock);
> +	spin_lock_init(&b->irq_lock);
> +
>  	setup_timer(&b->fake_irq,
>  		    intel_breadcrumbs_fake_irq,
>  		    (unsigned long)engine);
> @@ -768,7 +776,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  
>  	cancel_fake_irq(engine);
> -	spin_lock_irq(&b->lock);
> +	spin_lock_irq(&b->irq_lock);

In here I was thinking that you want both locks help, but
then can't find a reason why. Perhaps just to ensure that
the wait tree stays still.
>  
>  	if (b->irq_enabled)
>  		irq_enable(engine);
> @@ -787,7 +795,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>  	if (b->irq_armed)
>  		enable_fake_irq(b);
>  
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->irq_lock);
>  }
>  
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
> @@ -811,7 +819,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  	bool busy = false;
>  
> -	spin_lock_irq(&b->lock);
> +	spin_lock_irq(&b->rb_lock);

Wrong lock taken and relased in this function?

-Mika

>  
>  	if (b->first_wait) {
>  		wake_up_process(b->first_wait->tsk);
> @@ -823,7 +831,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
>  		busy |= intel_engine_flag(engine);
>  	}
>  
> -	spin_unlock_irq(&b->lock);
> +	spin_unlock_irq(&b->rb_lock);
>  
>  	return busy;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 55a6a3f8274c..621ac9998d16 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -235,10 +235,12 @@ struct intel_engine_cs {
>  	 * the overhead of waking that client is much preferred.
>  	 */
>  	struct intel_breadcrumbs {
> -		spinlock_t lock; /* protects the lists of requests; irqsafe */
> +		spinlock_t irq_lock; /* protects first_wait & irq_*; irqsafe */
> +		struct intel_wait *first_wait; /* oldest waiter by retirement */
> +
> +		spinlock_t rb_lock; /* protects the rbtrees; irqsafe */
>  		struct rb_root waiters; /* sorted by retirement, priority */
>  		struct rb_root signals; /* sorted by retirement */
> -		struct intel_wait *first_wait; /* oldest waiter by retirement */
>  		struct task_struct *signaler; /* used for fence signalling */
>  		struct drm_i915_gem_request __rcu *first_signal;
>  		struct timer_list fake_irq; /* used after a missed interrupt */
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Split breadcrumbs spinlock into two

[Chris Wilson]

On Fri, Mar 03, 2017 at 07:26:34PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > @@ -768,7 +776,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> >  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >  
> >  	cancel_fake_irq(engine);
> > -	spin_lock_irq(&b->lock);
> > +	spin_lock_irq(&b->irq_lock);
> 
> In here I was thinking that you want both locks help, but
> then can't find a reason why. Perhaps just to ensure that
> the wait tree stays still.

Right, here we are just rearming the irq, so I didn't think taking the
rb_lock helped with clarity. (Just the opposite, this showed nicely the
divison in labour between the locks :)

> >  	if (b->irq_enabled)
> >  		irq_enable(engine);
> > @@ -787,7 +795,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> >  	if (b->irq_armed)
> >  		enable_fake_irq(b);
> >  
> > -	spin_unlock_irq(&b->lock);
> > +	spin_unlock_irq(&b->irq_lock);
> >  }
> >  
> >  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
> > @@ -811,7 +819,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
> >  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >  	bool busy = false;
> >  
> > -	spin_lock_irq(&b->lock);
> > +	spin_lock_irq(&b->rb_lock);
> 
> Wrong lock taken and relased in this function?

Here we can simply take the outer rb_lock, as that guarantees the rbtree
won't change and therefore also the first_wait/irq_wait cannot be lost.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Split breadcrumbs spinlock into two

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Mar 03, 2017 at 07:26:34PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > @@ -768,7 +776,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>> >  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>> >  
>> >  	cancel_fake_irq(engine);
>> > -	spin_lock_irq(&b->lock);
>> > +	spin_lock_irq(&b->irq_lock);
>> 
>> In here I was thinking that you want both locks help, but
>> then can't find a reason why. Perhaps just to ensure that
>> the wait tree stays still.
>
> Right, here we are just rearming the irq, so I didn't think taking the
> rb_lock helped with clarity. (Just the opposite, this showed nicely the
> divison in labour between the locks :)
>
>> >  	if (b->irq_enabled)
>> >  		irq_enable(engine);
>> > @@ -787,7 +795,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>> >  	if (b->irq_armed)
>> >  		enable_fake_irq(b);
>> >  
>> > -	spin_unlock_irq(&b->lock);
>> > +	spin_unlock_irq(&b->irq_lock);
>> >  }
>> >  
>> >  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>> > @@ -811,7 +819,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
>> >  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>> >  	bool busy = false;
>> >  
>> > -	spin_lock_irq(&b->lock);
>> > +	spin_lock_irq(&b->rb_lock);
>> 
>> Wrong lock taken and relased in this function?
>
> Here we can simply take the outer rb_lock, as that guarantees the rbtree
> won't change and therefore also the first_wait/irq_wait cannot be
> lost.

Applied and indeed the first_wait is always modified by under rb_lock.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Only wake the waiter from the interrupt if passed

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> As we now check if the seqno is complete in order to signal the fence,
> we can also decide not to wake up the first_waiter until it is ready
> (since it is waiting on the same seqno). The only caveat is that if we
> need the engine->irq_seqno_barrier to enforce some coherency between an
> interrupt and the seqno read, we have to always wake the waiter into to
> perform that heavyweight barrier.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3f39e36fa566..c902aff61a9d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1059,7 +1059,8 @@ static void notify_ring(struct intel_engine_cs *engine)
>  				      wait->seqno))
>  			rq = wait->request;
>
> -		wake_up_process(wait->tsk);
> +		if (rq || engine->irq_seqno_barrier)
> +			wake_up_process(wait->tsk);

This also needs to be respinned on top of getting the request ref.

Were you thinking < gen5 or daydreaming that the next shiny one doesn't
need a barrier? :)

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  	} else {
>  		__intel_engine_disarm_breadcrumbs(engine);
>  	}
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Only wake the waiter from the interrupt if passed

[Chris Wilson]

On Fri, Mar 03, 2017 at 07:57:24PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > As we now check if the seqno is complete in order to signal the fence,
> > we can also decide not to wake up the first_waiter until it is ready
> > (since it is waiting on the same seqno). The only caveat is that if we
> > need the engine->irq_seqno_barrier to enforce some coherency between an
> > interrupt and the seqno read, we have to always wake the waiter into to
> > perform that heavyweight barrier.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 3f39e36fa566..c902aff61a9d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1059,7 +1059,8 @@ static void notify_ring(struct intel_engine_cs *engine)
> >  				      wait->seqno))
> >  			rq = wait->request;
> >
> > -		wake_up_process(wait->tsk);
> > +		if (rq || engine->irq_seqno_barrier)
> > +			wake_up_process(wait->tsk);
> 
> This also needs to be respinned on top of getting the request ref.
> 
> Were you thinking < gen5 or daydreaming that the next shiny one doesn't
> need a barrier? :)

execlists+ doesn't use a barrier, and we depend upon that for the guc
scheduler. And there's nothing wrong with giving Pineview that little
bit of an extra boost!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx