[PATCH v4 0/7] drm/i915: All fixes to make i915 work well with PREEMPT_RT.

[PATCH v4 0/7] drm/i915: All fixes to make i915 work well with PREEMPT_RT.

[Maarten Lankhorst]

All fixes to make the core i915 module (without display) working correctly on
PREEMPT_RT.

Some fixes are still needed, as the selftests still fail. But otherwise the
test results were looking good on PREEMPT_RT.

It looks like the patch that added a spinlock caused a regression on !PREEMPT_RT,
and may even have caused similar test failures on PREEMPT_RT, so to fix it
I removed RCU entirely, and converted to a locked implementation instead.

This time at least, i915 live selftests pass on !PREEMPRT_RT for at least 1 machine
with same config as CI.

Changes since v3:
- Fix some issues in last patch found by sashiko.
Changes since v2:
- Remove the extra signaler_active lock, it broke because
  it didn't protect anything, instead use signalers_lock.

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Maarten Lankhorst (3):
  drm/i915/gt: Fix selftests on PREEMPT_RT
  drm/i915/gt: Set stop_timeout() correctly on PREEMPT-RT
  drm/i915: Use sleeping selftests for igt_atomic on PREEMPT_RT

Sebastian Andrzej Siewior (4):
  drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() +
    spin_lock()
  drm/i915: Drop the irqs_disabled() check
  drm/i915/guc: Consider also RCU depth in busy loop.
  drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 172 +++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   1 -
 drivers/gpu/drm/i915/gt/intel_context.c       |   1 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   2 +-
 .../drm/i915/gt/intel_execlists_submission.c  |  17 +-
 drivers/gpu/drm/i915/gt/selftest_engine_pm.c  |   8 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   2 +-
 drivers/gpu/drm/i915/i915_request.c           |   2 -
 drivers/gpu/drm/i915/selftests/igt_atomic.c   |   7 +
 9 files changed, 130 insertions(+), 82 deletions(-)

-- 
2.53.0

[PATCH v4 1/7] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

[Maarten Lankhorst]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

execlists_dequeue() is invoked from a function which uses
local_irq_disable() to disable interrupts so the spin_lock() behaves
like spin_lock_irq().
This breaks PREEMPT_RT because local_irq_disable() + spin_lock() is not
the same as spin_lock_irq().

execlists_dequeue_irq() and execlists_dequeue() has each one caller
only. If intel_engine_cs::active::lock is acquired and released with the
_irq suffix then it behaves almost as if execlists_dequeue() would be
invoked with disabled interrupts. The difference is the last part of the
function which is then invoked with enabled interrupts.
I can't tell if this makes a difference. From looking at it, it might
work to move the last unlock at the end of the function as I didn't find
anything that would acquire the lock again.

Reported-by: Clark Williams <williams@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 .../drm/i915/gt/intel_execlists_submission.c    | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index e693b0c9d2a3e..841f719326756 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -1300,7 +1300,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	 * and context switches) submission.
 	 */
 
-	spin_lock(&sched_engine->lock);
+	spin_lock_irq(&sched_engine->lock);
 
 	/*
 	 * If the queue is higher priority than the last
@@ -1400,7 +1400,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				 * Even if ELSP[1] is occupied and not worthy
 				 * of timeslices, our queue might be.
 				 */
-				spin_unlock(&sched_engine->lock);
+				spin_unlock_irq(&sched_engine->lock);
 				return;
 			}
 		}
@@ -1426,7 +1426,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 
 		if (last && !can_merge_rq(last, rq)) {
 			spin_unlock(&ve->base.sched_engine->lock);
-			spin_unlock(&engine->sched_engine->lock);
+			spin_unlock_irq(&engine->sched_engine->lock);
 			return; /* leave this for another sibling */
 		}
 
@@ -1588,7 +1588,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	 */
 	sched_engine->queue_priority_hint = queue_prio(sched_engine);
 	i915_sched_engine_reset_on_empty(sched_engine);
-	spin_unlock(&sched_engine->lock);
+	spin_unlock_irq(&sched_engine->lock);
 
 	/*
 	 * We can skip poking the HW if we ended up with exactly the same set
@@ -1614,13 +1614,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	}
 }
 
-static void execlists_dequeue_irq(struct intel_engine_cs *engine)
-{
-	local_irq_disable(); /* Suspend interrupts across request submission */
-	execlists_dequeue(engine);
-	local_irq_enable(); /* flush irq_work (e.g. breadcrumb enabling) */
-}
-
 static void clear_ports(struct i915_request **ports, int count)
 {
 	memset_p((void **)ports, NULL, count);
@@ -2475,7 +2468,7 @@ static void execlists_submission_tasklet(struct tasklet_struct *t)
 	}
 
 	if (!engine->execlists.pending[0]) {
-		execlists_dequeue_irq(engine);
+		execlists_dequeue(engine);
 		start_timeslice(engine);
 	}
 
-- 
2.53.0

[PATCH v4 2/7] drm/i915: Drop the irqs_disabled() check

[Maarten Lankhorst]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The !irqs_disabled() check triggers on PREEMPT_RT even with
i915_sched_engine::lock acquired. The reason is the lock is transformed
into a sleeping lock on PREEMPT_RT and does not disable interrupts.

There is no need to check for disabled interrupts. The lockdep
annotation below already check if the lock has been acquired by the
caller and will yell if the interrupts are not disabled.

Remove the !irqs_disabled() check.

Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/i915_request.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index d2c7b1090df08..f66f8efc70629 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -610,7 +610,6 @@ bool __i915_request_submit(struct i915_request *request)
 
 	RQ_TRACE(request, "\n");
 
-	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->sched_engine->lock);
 
 	/*
@@ -719,7 +718,6 @@ void __i915_request_unsubmit(struct i915_request *request)
 	 */
 	RQ_TRACE(request, "\n");
 
-	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->sched_engine->lock);
 
 	/*
-- 
2.53.0

[PATCH v4 3/7] drm/i915/guc: Consider also RCU depth in busy loop.

[Maarten Lankhorst]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

intel_guc_send_busy_loop() looks at in_atomic() and irqs_disabled() to
decide if it should busy-spin while waiting or if it may sleep.
Both checks will report false on PREEMPT_RT if sleeping spinlocks are
acquired leading to RCU splats while the function sleeps.

Check also if RCU has been disabled.

Reported-by: "John B. Wyatt IV" <jwyatt@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 053780f562c1a..b25fa8f4dc4bd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -362,7 +362,7 @@ static inline int intel_guc_send_busy_loop(struct intel_guc *guc,
 {
 	int err;
 	unsigned int sleep_period_ms = 1;
-	bool not_atomic = !in_atomic() && !irqs_disabled();
+	bool not_atomic = !in_atomic() && !irqs_disabled() && !rcu_preempt_depth();
 
 	/*
 	 * FIXME: Have caller pass in if we are in an atomic context to avoid
-- 
2.53.0

[PATCH v4 4/7] drm/i915/gt: Fix selftests on PREEMPT_RT

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/gt/selftest_engine_pm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
index 10e556a7eac45..c1eff9edd8a5e 100644
--- a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
@@ -277,11 +277,11 @@ static int live_engine_busy_stats(void *arg)
 		st_engine_heartbeat_disable(engine);
 
 		ENGINE_TRACE(engine, "measuring idle time\n");
-		preempt_disable();
+		migrate_disable();
 		de = intel_engine_get_busy_time(engine, &t[0]);
 		udelay(100);
 		de = ktime_sub(intel_engine_get_busy_time(engine, &t[1]), de);
-		preempt_enable();
+		migrate_enable();
 		dt = ktime_sub(t[1], t[0]);
 		if (de < 0 || de > 10) {
 			pr_err("%s: reported %lldns [%d%%] busyness while sleeping [for %lldns]\n",
@@ -316,11 +316,11 @@ static int live_engine_busy_stats(void *arg)
 		}
 
 		ENGINE_TRACE(engine, "measuring busy time\n");
-		preempt_disable();
+		migrate_disable();
 		de = intel_engine_get_busy_time(engine, &t[0]);
 		mdelay(100);
 		de = ktime_sub(intel_engine_get_busy_time(engine, &t[1]), de);
-		preempt_enable();
+		migrate_enable();
 		dt = ktime_sub(t[1], t[0]);
 		if (100 * de < 95 * dt || 95 * de > 100 * dt) {
 			pr_err("%s: reported %lldns [%d%%] busyness while spinning [for %lldns]\n",
-- 
2.53.0

[PATCH v4 5/7] drm/i915/gt: Set stop_timeout() correctly on PREEMPT-RT

[Maarten Lankhorst]

Also check if RCU is disabled for PREEMPT-RT, which is the case when
local_bh_disable() is called.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c0fd349a4600c..9dd9665128caa 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1607,7 +1607,7 @@ u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine)
 
 static unsigned long stop_timeout(const struct intel_engine_cs *engine)
 {
-	if (in_atomic() || irqs_disabled()) /* inside atomic preempt-reset? */
+	if (in_atomic() || irqs_disabled() || rcu_preempt_depth()) /* inside atomic preempt-reset? */
 		return 0;
 
 	/*
-- 
2.53.0

[PATCH v4 6/7] drm/i915: Use sleeping selftests for igt_atomic on PREEMPT_RT

[Maarten Lankhorst]

This makes the i915 selftests slightly happier, especially
related to GPU reset.

I believe this may be a better approach than trying to convert
uncore->lock to raw_spinlock

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/selftests/igt_atomic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/selftests/igt_atomic.c b/drivers/gpu/drm/i915/selftests/igt_atomic.c
index fb506b6990956..8ae39cf570b76 100644
--- a/drivers/gpu/drm/i915/selftests/igt_atomic.c
+++ b/drivers/gpu/drm/i915/selftests/igt_atomic.c
@@ -39,7 +39,14 @@ static void __hardirq_end(void)
 	local_irq_enable();
 }
 
+static void __maybe_unused __nop(void)
+{}
+
 const struct igt_atomic_section igt_atomic_phases[] = {
+#if IS_ENABLED(CONFIG_PREEMPT_RT)
+	{ "sleeping", __nop, __nop },
+	{ },
+#endif
 	{ "preempt", __preempt_begin, __preempt_end },
 	{ "softirq", __softirq_begin, __softirq_end },
 	{ "hardirq", __hardirq_begin, __hardirq_end },
-- 
2.53.0

[PATCH v4 7/7] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

[Maarten Lankhorst]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

IRQ-Work (FIFO-1) will be preempted by the threaded-interrupt (FIFO-50)
and the interrupt will poll on signaler_active while the irq-work can't
make progress.

Solve this by adding a global spinlock to prevent starvation and force
completion.

The existing RCU handling gets in the way on PREEMPT_RT, and would likely
require conversion to raw spinlock to take them inside a
rcu_read_lock(), so remove RCU as well.

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 172 +++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   1 -
 drivers/gpu/drm/i915/gt/intel_context.c       |   1 +
 3 files changed, 112 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index c10ac0ab3bfa8..0ae7759dc74a3 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -88,24 +88,25 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
 				  struct intel_context *ce)
 {
 	lockdep_assert_held(&ce->signal_lock);
+	lockdep_assert_held(&b->signalers_lock);
 
-	spin_lock(&b->signalers_lock);
-	list_add_rcu(&ce->signal_link, &b->signalers);
-	spin_unlock(&b->signalers_lock);
+	if (list_empty(&ce->signals))
+		return;
+
+	intel_context_get(ce);
+	list_add(&ce->signal_link, &b->signalers);
 }
 
 static bool remove_signaling_context(struct intel_breadcrumbs *b,
 				     struct intel_context *ce)
 {
 	lockdep_assert_held(&ce->signal_lock);
+	lockdep_assert_held(&b->signalers_lock);
 
-	if (!list_empty(&ce->signals))
+	if (!list_empty(&ce->signals) || list_empty(&ce->signal_link))
 		return false;
 
-	spin_lock(&b->signalers_lock);
-	list_del_rcu(&ce->signal_link);
-	spin_unlock(&b->signalers_lock);
-
+	list_del_init(&ce->signal_link);
 	return true;
 }
 
@@ -174,7 +175,7 @@ static void signal_irq_work(struct irq_work *work)
 	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
 	const ktime_t timestamp = ktime_get();
 	struct llist_node *signal, *sn;
-	struct intel_context *ce;
+	struct intel_context *ce, *next;
 
 	signal = NULL;
 	if (unlikely(!llist_empty(&b->signaled_requests)))
@@ -208,13 +209,13 @@ static void signal_irq_work(struct irq_work *work)
 	if (!signal && READ_ONCE(b->irq_armed) && list_empty(&b->signalers))
 		intel_breadcrumbs_disarm_irq(b);
 
-	rcu_read_lock();
-	atomic_inc(&b->signaler_active);
-	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+	spin_lock(&b->signalers_lock);
+	list_for_each_entry_safe(ce, next, &b->signalers, signal_link) {
 		struct i915_request *rq;
+		bool release;
 
-		list_for_each_entry_rcu(rq, &ce->signals, signal_link) {
-			bool release;
+		spin_lock(&ce->signal_lock);
+		while ((rq = list_first_entry_or_null(&ce->signals, typeof(*rq), signal_link))) {
 
 			if (!__i915_request_is_complete(rq))
 				break;
@@ -228,15 +229,9 @@ static void signal_irq_work(struct irq_work *work)
 			 * spinlock as the callback chain may end up adding
 			 * more signalers to the same context or engine.
 			 */
-			spin_lock(&ce->signal_lock);
-			list_del_rcu(&rq->signal_link);
-			release = remove_signaling_context(b, ce);
-			spin_unlock(&ce->signal_lock);
-			if (release) {
-				if (intel_timeline_is_last(ce->timeline, rq))
-					add_retire(b, ce->timeline);
-				intel_context_put(ce);
-			}
+			list_del(&rq->signal_link);
+			if (list_empty(&ce->signals) && intel_timeline_is_last(ce->timeline, rq))
+				add_retire(b, ce->timeline);
 
 			if (__dma_fence_signal(&rq->fence))
 				/* We own signal_node now, xfer to local list */
@@ -244,9 +239,13 @@ static void signal_irq_work(struct irq_work *work)
 			else
 				i915_request_put(rq);
 		}
+
+		release = remove_signaling_context(b, ce);
+		spin_unlock(&ce->signal_lock);
+		if (release)
+			intel_context_put(ce);
 	}
-	atomic_dec(&b->signaler_active);
-	rcu_read_unlock();
+	spin_unlock(&b->signalers_lock);
 
 	llist_for_each_safe(signal, sn, signal) {
 		struct i915_request *rq =
@@ -347,14 +346,15 @@ static void irq_signal_request(struct i915_request *rq,
 		irq_work_queue(&b->irq_work);
 }
 
-static void insert_breadcrumb(struct i915_request *rq)
+static bool insert_breadcrumb(struct i915_request *rq,
+			      struct intel_breadcrumbs *b)
 {
-	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
 	struct intel_context *ce = rq->context;
 	struct list_head *pos;
+	bool ret;
 
 	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
-		return;
+		return false;
 
 	/*
 	 * If the request is already completed, we can transfer it
@@ -363,14 +363,15 @@ static void insert_breadcrumb(struct i915_request *rq)
 	 */
 	if (__i915_request_is_complete(rq)) {
 		irq_signal_request(rq, b);
-		return;
+		return false;
 	}
 
 	if (list_empty(&ce->signals)) {
-		intel_context_get(ce);
-		add_signaling_context(b, ce);
+		ret = true;
 		pos = &ce->signals;
 	} else {
+		ret = false;
+
 		/*
 		 * We keep the seqno in retirement order, so we can break
 		 * inside intel_engine_signal_breadcrumbs as soon as we've
@@ -395,23 +396,19 @@ static void insert_breadcrumb(struct i915_request *rq)
 	}
 
 	i915_request_get(rq);
-	list_add_rcu(&rq->signal_link, pos);
+	list_add(&rq->signal_link, pos);
 	GEM_BUG_ON(!check_signal_order(ce, rq));
 	GEM_BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags));
 	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 
-	/*
-	 * Defer enabling the interrupt to after HW submission and recheck
-	 * the request as it may have completed and raised the interrupt as
-	 * we were attaching it into the lists.
-	 */
-	if (!READ_ONCE(b->irq_armed) || __i915_request_is_complete(rq))
-		irq_work_queue(&b->irq_work);
+	return ret;
 }
 
 bool i915_request_enable_breadcrumb(struct i915_request *rq)
 {
 	struct intel_context *ce = rq->context;
+	struct intel_breadcrumbs *b;
+	bool add_context = false;
 
 	/* Serialises with i915_request_retire() using rq->lock */
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
@@ -427,30 +424,87 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
 		return true;
 
 	spin_lock(&ce->signal_lock);
+	b = READ_ONCE(rq->engine)->breadcrumbs;
+
 	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
-		insert_breadcrumb(rq);
+		add_context = insert_breadcrumb(rq, b);
+
+	if (add_context && spin_trylock(&b->signalers_lock)) {
+		add_signaling_context(b, ce);
+		spin_unlock(&b->signalers_lock);
+		add_context = false;
+	}
 	spin_unlock(&ce->signal_lock);
 
+	if (add_context) {
+		/*
+		 * Fast trylock didn't work, use slow locking.
+		 *
+		 * Dropping the lock to solve the inversion is safe, since
+		 * no race is possible against remove_signaling_context()
+		 * without being added as signaling context.
+		 */
+		spin_lock(&b->signalers_lock);
+		spin_lock(&ce->signal_lock);
+		add_signaling_context(b, ce);
+		spin_unlock(&ce->signal_lock);
+		spin_unlock(&b->signalers_lock);
+	}
+
+	/*
+	 * Defer enabling the interrupt to after HW submission and recheck
+	 * the request as it may have completed and raised the interrupt as
+	 * we were attaching it into the lists.
+	 */
+	if (!READ_ONCE(b->irq_armed) || __i915_request_is_complete(rq))
+		irq_work_queue(&b->irq_work);
+
 	return true;
 }
 
+
+static void unlock_context_remove_signaling(struct intel_context *ce,
+					    struct intel_breadcrumbs *b,
+					    unsigned long flags)
+{
+	bool release = false, retry = false;
+
+	if (list_empty(&ce->signals)) {
+		if (spin_trylock(&b->signalers_lock)) {
+			release = remove_signaling_context(b, ce);
+			spin_unlock(&b->signalers_lock);
+		} else {
+			retry = true;
+		}
+	}
+	spin_unlock_irqrestore(&ce->signal_lock, flags);
+
+	if (retry) {
+		spin_lock_irqsave(&b->signalers_lock, flags);
+		spin_lock(&ce->signal_lock);
+		release = remove_signaling_context(b, ce);
+		spin_unlock(&ce->signal_lock);
+		spin_unlock_irqrestore(&b->signalers_lock, flags);
+	}
+
+	if (release)
+		intel_context_put(ce);
+}
+
 void i915_request_cancel_breadcrumb(struct i915_request *rq)
 {
 	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
 	struct intel_context *ce = rq->context;
-	bool release;
+	unsigned long flags;
 
-	spin_lock(&ce->signal_lock);
+	spin_lock_irqsave(&ce->signal_lock, flags);
 	if (!test_and_clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
-		spin_unlock(&ce->signal_lock);
+		spin_unlock_irqrestore(&ce->signal_lock, flags);
 		return;
 	}
 
-	list_del_rcu(&rq->signal_link);
-	release = remove_signaling_context(b, ce);
-	spin_unlock(&ce->signal_lock);
-	if (release)
-		intel_context_put(ce);
+	list_del(&rq->signal_link);
+	unlock_context_remove_signaling(ce, b, flags);
 
 	if (__i915_request_is_complete(rq))
 		irq_signal_request(rq, b);
@@ -462,7 +516,6 @@ void intel_context_remove_breadcrumbs(struct intel_context *ce,
 				      struct intel_breadcrumbs *b)
 {
 	struct i915_request *rq, *rn;
-	bool release = false;
 	unsigned long flags;
 
 	spin_lock_irqsave(&ce->signal_lock, flags);
@@ -476,39 +529,36 @@ void intel_context_remove_breadcrumbs(struct intel_context *ce,
 					&rq->fence.flags))
 			continue;
 
-		list_del_rcu(&rq->signal_link);
+		list_del(&rq->signal_link);
 		irq_signal_request(rq, b);
 		i915_request_put(rq);
 	}
-	release = remove_signaling_context(b, ce);
 
 unlock:
-	spin_unlock_irqrestore(&ce->signal_lock, flags);
-	if (release)
-		intel_context_put(ce);
-
-	while (atomic_read(&b->signaler_active))
-		cpu_relax();
+	unlock_context_remove_signaling(ce, b, flags);
 }
 
 static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
 {
 	struct intel_context *ce;
 	struct i915_request *rq;
+	unsigned long flags;
 
 	drm_printf(p, "Signals:\n");
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
-		list_for_each_entry_rcu(rq, &ce->signals, signal_link)
+	spin_lock_irqsave(&b->signalers_lock, flags);
+	list_for_each_entry(ce, &b->signalers, signal_link) {
+		spin_lock(&ce->signal_lock);
+		list_for_each_entry(rq, &ce->signals, signal_link)
 			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
 				   rq->fence.context, rq->fence.seqno,
 				   __i915_request_is_complete(rq) ? "!" :
 				   __i915_request_has_started(rq) ? "*" :
 				   "",
 				   jiffies_to_msecs(jiffies - rq->emitted_jiffies));
+		spin_unlock(&ce->signal_lock);
 	}
-	rcu_read_unlock();
+	spin_unlock_irqrestore(&b->signalers_lock, flags);
 }
 
 void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
index bdf09fd67b6e7..648e5600587b4 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -39,7 +39,6 @@ struct intel_breadcrumbs {
 	spinlock_t signalers_lock; /* protects the list of signalers */
 	struct list_head signalers;
 	struct llist_head signaled_requests;
-	atomic_t signaler_active;
 
 	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
 	struct irq_work irq_work; /* for use from inside irq_lock */
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index b1b8695ba7c97..b938ff5be7b80 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -409,6 +409,7 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 	/* NB ce->signal_link/lock is used under RCU */
 	spin_lock_init(&ce->signal_lock);
 	INIT_LIST_HEAD(&ce->signals);
+	INIT_LIST_HEAD(&ce->signal_link);
 
 	mutex_init(&ce->pin_mutex);
 
-- 
2.53.0

Re: [PATCH v4 4/7] drm/i915/gt: Fix selftests on PREEMPT_RT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete implementation of PREEMPT_RT fixes for i915 selftests, leaving preemption-disabled busy-waits and delays that cause deadlocks and latency spikes.
--

commit c008dff1b2cefd9ee50ca924f83ff7c237a6fb3a
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915/gt: Fix selftests on PREEMPT_RT

This commit modifies the i915 engine PM selftest to use migrate_disable()
instead of preempt_disable() around delay functions. This helps prevent 
latency issues on PREEMPT_RT kernels.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
> index 10e556a7eac45..c1eff9edd8a5e 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
[ ... ]
> @@ -316,11 +316,11 @@ static int live_engine_busy_stats(void *arg)
>  		}
>  
>  		ENGINE_TRACE(engine, "measuring busy time\n");
> -		preempt_disable();
> +		migrate_disable();
>  		de = intel_engine_get_busy_time(engine, &t[0]);
>  		mdelay(100);
>  		de = ktime_sub(intel_engine_get_busy_time(engine, &t[1]), de);
> -		preempt_enable();
> +		migrate_enable();

[Severity: High]
This isn't a bug introduced by this patch, but since this series addresses
PREEMPT_RT issues in i915 selftests, are there similar preemption-disabled
delays and waits remaining in other selftests that should be addressed?

For example, looking at live_rps_clock_interval() in
drivers/gpu/drm/i915/gt/selftest_rps.c, there is a block that disables
preemption for at least 1 millisecond:

    preempt_disable();
    cycles_[i] = -intel_uncore_read_fw(gt->uncore, GEN6_RP_CUR_UP_EI);
    dt_[i] = ktime_get();
    
    udelay(1000);
    
    cycles_[i] += intel_uncore_read_fw(gt->uncore, GEN6_RP_CUR_UP_EI);
    dt_[i] = ktime_sub(ktime_get(), dt_[i]);
    preempt_enable();

Could this cause a severe latency spike on PREEMPT_RT?

Additionally, in drivers/gpu/drm/i915/selftests/i915_request.c,
measure_completion() disables preemption while busy-waiting for a callback:

    preempt_disable();
    semaphore_set(sema, i);
    while (!READ_ONCE(cb.seen))
        cpu_relax();
        
    elapsed[i - 1] = ENGINE_READ_FW(ce->engine, RING_TIMESTAMP);
    preempt_enable();

If the threaded handler that executes this callback is scheduled on the same
CPU on a PREEMPT_RT kernel, would this create a hard deadlock since the
busy-wait loop will never yield to the handler?

>  		dt = ktime_sub(t[1], t[0]);
>  		if (100 * de < 95 * dt || 95 * de > 100 * dt) {
>  			pr_err("%s: reported %lldns [%d%%] busyness while spinning [for %lldns]\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706114823.752313-1-dev@lankhorst.se?part=4

Re: [PATCH v4 3/7] drm/i915/guc: Consider also RCU depth in busy loop.

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Looping with IRQs disabled causes soft-deadlocks and CT buffer breakage because the G2H draining tasklet cannot run.
--

commit fa66d9f372062707343fef0e57e8f112f58ecabc
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/guc: Consider also RCU depth in busy loop.

This commit updates the not_atomic condition in intel_guc_send_busy_loop
to additionally check rcu_preempt_depth. This prevents RCU splats on
PREEMPT_RT systems where sleeping spinlocks are acquired, which would
cause in_atomic and irqs_disabled to report false.

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 053780f562c1a..b25fa8f4dc4bd 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -362,7 +362,7 @@ static inline int intel_guc_send_busy_loop(struct intel_guc *guc,
>  {
>  	int err;
>  	unsigned int sleep_period_ms = 1;
> -	bool not_atomic = !in_atomic() && !irqs_disabled();
> +	bool not_atomic = !in_atomic() && !irqs_disabled() && !rcu_preempt_depth();

[Severity: High]
This isn't a bug introduced by this patch, but does spinning with IRQs
disabled here cause a soft-deadlock that breaks the GuC CT buffer?

If the G2H buffer fills up and intel_guc_send_nb returns -EBUSY, the code
enters a busy loop. If the caller acquired spinlocks that disabled IRQs,
not_atomic will evaluate to false, causing the loop to use cpu_relax
instead of sleeping.

Inside this process, has_room_nb is called, which attempts to drain the
credits by queuing a tasklet:

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:has_room_nb() {
    ...
    tasklet_hi_schedule(&ct->receive_tasklet);
    ...
}

This schedules the tasklet on the local CPU's tasklet list and sets
TASKLET_STATE_SCHED. However, because the local CPU is spinning in
cpu_relax with IRQs disabled, softirqs will never run.

Since the tasklet is pinned to the local deadlocked CPU, other CPUs
cannot drain the buffer even if they receive the GuC hard interrupt.
The G2H buffer never drains, and after 1.5 seconds, ct_deadlocked
returns true, permanently breaking the CT buffer.

>  
>  	/*
>  	 * FIXME: Have caller pass in if we are in an atomic context to avoid
>  	 * using in_atomic() as it doesn't work for all configs (e.g. PREEMPT).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706114823.752313-1-dev@lankhorst.se?part=3

Re: [PATCH v4 1/7] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware MMIO submission sequence in `execlists_submit_ports()` is executed with interrupts and preemption enabled, violating the requirement for atomic hardware submission.
--

commit a1c2160ec1a5e4791ddd349b07f9fe856a1bb2e5
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

This commit replaces local_irq_disable() and spin_lock() with spin_lock_irq()
to improve PREEMPT_RT compatibility, and removes the execlists_dequeue_irq()
wrapper.

> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 1359fc9cb88ef..e11db81dca9c5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c

[ ... ]

> @@ -1588,7 +1588,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  	 */
>  	sched_engine->queue_priority_hint = queue_prio(sched_engine);
>  	i915_sched_engine_reset_on_empty(sched_engine);
> -	spin_unlock(&sched_engine->lock);
> +	spin_unlock_irq(&sched_engine->lock);

[Severity: High]
Is it safe to unconditionally re-enable interrupts and preemption here before
the hardware submission?

Looking at execlists_dequeue(), shortly after this unlock the code calls
execlists_submit_ports(). The Execlist Submit Port (ELSP) mechanism typically
requires consecutive MMIO writes to submit hardware contexts.

Previously, the entire function was wrapped in local_irq_disable() via
execlists_dequeue_irq(). With spin_unlock_irq() completely enabling
interrupts (and preemption on PREEMPT_RT) prior to the submission, could a
hard interrupt or task preemption delay the sequence of MMIO writes?

This seems like it might lead to partial submissions, hardware timeouts, or
GPU hangs if the hardware submission sequence is no longer atomic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706114823.752313-1-dev@lankhorst.se?part=1

Re: [PATCH v4 7/7] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Dropping `ce->signal_lock` to avoid lock inversion in `i915_request_enable_breadcrumb` allows concurrent threads to double-add a context to the `b->signalers` list, corrupting the list and leaking `intel_context` references.
--

commit 51101492996df475bcd91b7f36bd0af9680441f1
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

This patch adds a global spinlock to prevent starvation of irq_work by the
threaded interrupt and removes the existing RCU handling.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index c10ac0ab3bfa8..0ae7759dc74a3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -88,24 +88,25 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
>  				  struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->signal_lock);
> +	lockdep_assert_held(&b->signalers_lock);
>  
> -	spin_lock(&b->signalers_lock);
> -	list_add_rcu(&ce->signal_link, &b->signalers);
> -	spin_unlock(&b->signalers_lock);
> +	if (list_empty(&ce->signals))
> +		return;
> +
> +	intel_context_get(ce);
> +	list_add(&ce->signal_link, &b->signalers);
>  }

[Severity: Critical]
Could this unconditionally corrupt the signalers list? 

If add_signaling_context() is called concurrently for the same context, it
will call list_add() without verifying whether ce->signal_link is already
linked, leading to list corruption and a leak of intel_context objects.

[ ... ]

> @@ -427,30 +424,87 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>  		return true;
>  
>  	spin_lock(&ce->signal_lock);
> +	b = READ_ONCE(rq->engine)->breadcrumbs;
> +
>  	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> -		insert_breadcrumb(rq);
> +		add_context = insert_breadcrumb(rq, b);
> +
> +	if (add_context && spin_trylock(&b->signalers_lock)) {
> +		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +		add_context = false;
> +	}
>  	spin_unlock(&ce->signal_lock);
>  
> +	if (add_context) {
> +		/*
> +		 * Fast trylock didn't work, use slow locking.
> +		 *
> +		 * Dropping the lock to solve the inversion is safe, since
> +		 * no race is possible against remove_signaling_context()
> +		 * without being added as signaling context.
> +		 */
> +		spin_lock(&b->signalers_lock);
> +		spin_lock(&ce->signal_lock);
> +		add_signaling_context(b, ce);
> +		spin_unlock(&ce->signal_lock);
> +		spin_unlock(&b->signalers_lock);
> +	}

[Severity: Critical]
Does dropping ce->signal_lock here create a race condition that leads to the
double-add in add_signaling_context()?

If multiple threads attempt to enable breadcrumbs for the same context and
fail the fast trylock, they will drop ce->signal_lock and enter the slow path.
This can allow a sequence where:

Thread 1 calls i915_request_enable_breadcrumb() for rq1, gets
add_context = true, fails the trylock, drops ce->signal_lock, and waits
on the slow path.

Thread 2 cancels rq1, making ce->signals empty again.

Thread 3 calls i915_request_enable_breadcrumb() for rq3. Because ce->signals
is empty, it also gets add_context = true. It fails the trylock, drops the
lock, and waits on the slow path.

Thread 1 and Thread 3 sequentially acquire the locks in the slow path and both
call add_signaling_context(). Since Thread 3 added rq3, ce->signals is not
empty. Both threads will then execute intel_context_get() and list_add()
unconditionally, permanently corrupting the doubly-linked list.

Should add_signaling_context() verify that list_empty(&ce->signal_link) is
true before calling list_add() to prevent this regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706114823.752313-1-dev@lankhorst.se?part=7

Re: [PATCH v4 0/7] drm/i915: All fixes to make i915 work well with PREEMPT_RT.

[Sebastian Andrzej Siewior]

On 2026-07-06 13:48:16 [+0200], Maarten Lankhorst wrote:
> All fixes to make the core i915 module (without display) working correctly on
> PREEMPT_RT.
> 
> Some fixes are still needed, as the selftests still fail. But otherwise the
> test results were looking good on PREEMPT_RT.
> 
> It looks like the patch that added a spinlock caused a regression on !PREEMPT_RT,
> and may even have caused similar test failures on PREEMPT_RT, so to fix it
> I removed RCU entirely, and converted to a locked implementation instead.
> 
> This time at least, i915 live selftests pass on !PREEMPRT_RT for at least 1 machine
> with same config as CI.

I saw the version counter overflowed and is at 4 here. I appreciate all
the work. May I ask how are things?
 
Sebastian

Re: [PATCH v4 0/7] drm/i915: All fixes to make i915 work well with PREEMPT_RT.

[Maarten Lankhorst]

Hey,

On 7/6/26 14:20, Sebastian Andrzej Siewior wrote:
> On 2026-07-06 13:48:16 [+0200], Maarten Lankhorst wrote:
>> All fixes to make the core i915 module (without display) working correctly on
>> PREEMPT_RT.
>>
>> Some fixes are still needed, as the selftests still fail. But otherwise the
>> test results were looking good on PREEMPT_RT.
>>
>> It looks like the patch that added a spinlock caused a regression on !PREEMPT_RT,
>> and may even have caused similar test failures on PREEMPT_RT, so to fix it
>> I removed RCU entirely, and converted to a locked implementation instead.
>>
>> This time at least, i915 live selftests pass on !PREEMPRT_RT for at least 1 machine
>> with same config as CI.
> 
> I saw the version counter overflowed and is at 4 here. I appreciate all
> the work. May I ask how are things?

I'm fixing the bugs in core i915 with this specific PREEMPT_RT series,
and left out the display changes. This showed that the failures in i915's
selftests were at least partially related to the PREEMPT_RT changes in the
last patch.

Hopefully v4 passes CI and if you want, you can review those changes so they
can be merged. Looks like sashiko is great at finding concurrency bugs,
changing from RCU back to a normal lock is non-trivial. It unfortunately found
1 more in add_signaling_context for a missing !list_empty(&ce->signal_link),
but should be safe to review regardless.

Kind regards,
~Maarten