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

[PATCH v2 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.

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 a spinlock to prevent starvation of irq_work.

 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 89 ++++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |  2 +-
 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 ++
 8 files changed, 74 insertions(+), 55 deletions(-)

-- 
2.53.0

[PATCH v2 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 1359fc9cb88ef..e11db81dca9c5 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 v2 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 v2 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 v2 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 v2 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 v2 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 v2 7/7] drm/i915/gt: Use a spinlock 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   | 89 ++++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |  2 +-
 2 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index c10ac0ab3bfa8..66ddd1ec68f3a 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -90,7 +90,7 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
 	lockdep_assert_held(&ce->signal_lock);
 
 	spin_lock(&b->signalers_lock);
-	list_add_rcu(&ce->signal_link, &b->signalers);
+	list_add(&ce->signal_link, &b->signalers);
 	spin_unlock(&b->signalers_lock);
 }
 
@@ -103,7 +103,7 @@ static bool remove_signaling_context(struct intel_breadcrumbs *b,
 		return false;
 
 	spin_lock(&b->signalers_lock);
-	list_del_rcu(&ce->signal_link);
+	list_del(&ce->signal_link);
 	spin_unlock(&b->signalers_lock);
 
 	return true;
@@ -174,7 +174,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,12 +208,12 @@ 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) {
-		struct i915_request *rq;
+	spin_lock(&b->signaler_active);
+	list_for_each_entry_safe(ce, next, &b->signalers, signal_link) {
+		struct i915_request *rq, *rq_next;
 
-		list_for_each_entry_rcu(rq, &ce->signals, signal_link) {
+		spin_lock(&ce->signal_lock);
+		list_for_each_entry_safe(rq, rq_next, &ce->signals, signal_link) {
 			bool release;
 
 			if (!__i915_request_is_complete(rq))
@@ -228,10 +228,8 @@ 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);
+			list_del(&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);
@@ -244,9 +242,9 @@ static void signal_irq_work(struct irq_work *work)
 			else
 				i915_request_put(rq);
 		}
+		spin_unlock(&ce->signal_lock);
 	}
-	atomic_dec(&b->signaler_active);
-	rcu_read_unlock();
+	spin_unlock(&b->signaler_active);
 
 	llist_for_each_safe(signal, sn, signal) {
 		struct i915_request *rq =
@@ -290,6 +288,7 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
 	init_llist_head(&b->signaled_requests);
 
 	spin_lock_init(&b->irq_lock);
+	spin_lock_init(&b->signaler_active);
 	init_irq_work(&b->irq_work, signal_irq_work);
 
 	b->irq_engine = irq_engine;
@@ -395,7 +394,7 @@ 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);
@@ -434,23 +433,49 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
 	return true;
 }
 
+
+static void unlock_and_maybe_remove_signaling_context(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->signaler_active)) {
+			release = remove_signaling_context(b, ce);
+			spin_unlock(&b->signaler_active);
+		} else {
+			retry = true;
+		}
+	}
+	spin_unlock_irqrestore(&ce->signal_lock, flags);
+
+	if (retry) {
+		spin_lock_irqsave(&b->signaler_active, flags);
+		spin_lock(&ce->signal_lock);
+		release = remove_signaling_context(b, ce);
+		spin_unlock(&ce->signal_lock);
+		spin_unlock_irqrestore(&b->signaler_active, 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);
 		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_and_maybe_remove_signaling_context(ce, b, flags);
 
 	if (__i915_request_is_complete(rq))
 		irq_signal_request(rq, b);
@@ -462,7 +487,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 +500,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_and_maybe_remove_signaling_context(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->signaler_active, flags);
+	list_for_each_entry(ce, &b->signalers, signal_link) {
+		spin_lock(&b->signalers_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(&b->signalers_lock);
 	}
-	rcu_read_unlock();
+	spin_unlock_irqrestore(&b->signaler_active, 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..5e23074fc7b42 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -39,7 +39,7 @@ 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 signaler_active;
 
 	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
 	struct irq_work irq_work; /* for use from inside irq_lock */
-- 
2.53.0

Re: [PATCH v2 7/7] drm/i915/gt: Use a spinlock to prevent starvation of irq_work.

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Concurrent modification and lockless traversal of the `b->signalers` list.
- [High] Failure to restore interrupt state on early return in `i915_request_cancel_breadcrumb()`.
- [High] Use-after-free of `ce->signal_lock` in `signal_irq_work()` due to early `intel_context_put()`.
- [High] Double `list_add()` causing list corruption due to dropped lock in `unlock_and_maybe_remove_signaling_context()`.
- [High] Double `list_del()` and double `intel_context_put()` on already removed contexts in `intel_context_remove_breadcrumbs()`.
- [Medium] Incorrect locking used for iterating `ce->signals` in `print_signals()`.
--

commit e3123b26bfbdb1e44033e9e8c3f7ae5f8b76d837
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
drm/i915/gt: Use a spinlock to prevent starvation of irq_work.

This patch aims to prevent irq_work starvation on PREEMPT_RT by removing
RCU and introducing a global spinlock to force completion. It replaces
list_add_rcu/list_del_rcu with standard list operations and adds a
signaler_active spinlock.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
[ ... ]
> @@ -208,12 +208,12 @@ 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) {
> -		struct i915_request *rq;
> +	spin_lock(&b->signaler_active);
> +	list_for_each_entry_safe(ce, next, &b->signalers, signal_link) {

[Severity: High]
Is the b->signalers list properly protected against concurrent
modifications during this traversal?

This loop traverses b->signalers holding only b->signaler_active. However,
add_signaling_context() and remove_signaling_context() modify this same
list while holding b->signalers_lock, without acquiring b->signaler_active.

Any thread queuing a request could trigger an insertion while this
traversal is happening, which might cause data races or pointer corruption
since they are in different lock domains.

> +		spin_lock(&ce->signal_lock);
> +		list_for_each_entry_safe(rq, rq_next, &ce->signals, signal_link) {
[ ... ]
> -			spin_lock(&ce->signal_lock);
> -			list_del_rcu(&rq->signal_link);
> +			list_del(&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);
> @@ -244,9 +242,9 @@ static void signal_irq_work(struct irq_work *work)
>  			else
>  				i915_request_put(rq);
>  		}
> +		spin_unlock(&ce->signal_lock);

[Severity: High]
Could this lead to a use-after-free of ce->signal_lock?

If intel_context_put(ce) drops the final reference to the context, the
memory backing ce (including the lock itself) could be freed immediately.

Since RCU read-side protection was removed, the memory might be scheduled
for freeing before the loop concludes and calls
spin_unlock(&ce->signal_lock).

[ ... ]
> +static void unlock_and_maybe_remove_signaling_context(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->signaler_active)) {
> +			release = remove_signaling_context(b, ce);
> +			spin_unlock(&b->signaler_active);
> +		} else {
> +			retry = true;
> +		}
> +	}
> +	spin_unlock_irqrestore(&ce->signal_lock, flags);
> +
> +	if (retry) {

[Severity: High]
Could dropping the lock here result in double list additions?

If spin_trylock(&b->signaler_active) fails, this function drops
ce->signal_lock and retries to avoid an AB-BA deadlock. During this
lockless window, ce->signals is empty but ce->signal_link is still linked
in the global b->signalers list.

If a concurrent thread calls insert_breadcrumb(), it will observe an empty
ce->signals list and call add_signaling_context():

insert_breadcrumb() {
    if (list_empty(&ce->signals)) {
        intel_context_get(ce);
        add_signaling_context(b, ce);
}

This would blindly execute list_add(&ce->signal_link, &b->signalers) while
it is already linked, which might corrupt the pointers.

[ ... ]
>  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);
>  		return;
>  	}

[Severity: High]
Will this early return leave local interrupts permanently disabled?

The lock acquisition is upgraded to spin_lock_irqsave(), which disables
local interrupts and saves the state in the flags variable. However, this
early return path uses plain spin_unlock(&ce->signal_lock) instead of
spin_unlock_irqrestore(&ce->signal_lock, flags).

If a request is cancelled that has already been signaled, the CPU will
return from this function with IRQs disabled, which might cause a hard
lockup.

[ ... ]
>  void intel_context_remove_breadcrumbs(struct intel_context *ce,
>  				      struct intel_breadcrumbs *b)
>  {
[ ... ]
> -	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_and_maybe_remove_signaling_context(ce, b, flags);
>  }

[Severity: High]
Does this cause a double list_del() on contexts that are already removed?

If ce->signals is already empty when intel_context_remove_breadcrumbs()
is called (which happens for cleanly destroyed contexts), it will jump
to the unlock label and call unlock_and_maybe_remove_signaling_context().

That helper checks if ce->signals is empty, sees that it is, and calls
remove_signaling_context(), which blindly executes
list_del(&ce->signal_link). Since the link was already deleted when the
list originally became empty, this executes list_del() on LIST_POISON.
It could also cause a double intel_context_put() from the refcount
underflow.

[ ... ]
>  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->signaler_active, flags);
> +	list_for_each_entry(ce, &b->signalers, signal_link) {
> +		spin_lock(&b->signalers_lock);
> +		list_for_each_entry(rq, &ce->signals, signal_link)

[Severity: Medium]
Is b->signalers_lock the correct lock to protect the ce->signals list?

This acquires b->signalers_lock instead of ce->signal_lock to protect
the traversal of the ce->signals list. Since insert_breadcrumb()
modifies ce->signals under ce->signal_lock, traversing it here with the
wrong lock allows concurrent mutations during iteration.

When i915_engine_info is read via debugfs, this could lead to list
corruption or an oops.

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

Re: [PATCH v2 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] Re-enabling interrupts before `execlists_submit_ports()` violates the hardware ELSP submission constraint and causes premature `irq_work` flushing, breaking cross-engine request execution ordering.
--

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

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

This patch modifies execlists_dequeue() to use spin_lock_irq() and 
spin_unlock_irq() directly. It removes execlists_dequeue_irq() which 
previously wrapped the entire dequeue operation in local_irq_disable().

> 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]
Does re-enabling interrupts here before calling execlists_submit_ports() 
violate the hardware ELSP submission constraints?

The original execlists_dequeue_irq() kept interrupts disabled across the
entire dequeue process, including the final MMIO writes. By changing to
spin_unlock_irq() here, interrupts are re-enabled before the execution of:

execlists_submit_ports() {
    ...
    for (n = execlists_num_ports(execlists); n--; ) {
        struct i915_request *rq = execlists->pending[n];

        write_desc(execlists,
                   rq ? execlists_update_context(rq) : 0,
                   n);
    }
}

Hardware requires these consecutive ELSP MMIO writes to be atomic without
intervening accesses. If an interrupt fires during this loop, could it
corrupt the submission sequence and cause a GPU hang? 

Additionally, can enabling interrupts this early cause premature irq_work
flushing, breaking cross-engine request execution ordering?

>  
>  	/*
>  	 * We can skip poking the HW if we ended up with exactly the same set

[ ... ]

> -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) */
> -}
> -

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

✗ i915.CI.BAT: failure for drm/i915: All fixes to make i915 work well with PREEMPT_RT. (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7030 bytes --]

== Series Details ==

Series: drm/i915: All fixes to make i915 work well with PREEMPT_RT. (rev2)
URL   : https://patchwork.freedesktop.org/series/169677/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_18756 -> Patchwork_169677v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_169677v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_169677v2, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/index.html

Participating hosts (41 -> 39)
------------------------------

  Additional (1): bat-jsl-5 
  Missing    (3): bat-dg2-13 fi-glk-j4005 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_169677v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live:
    - bat-adls-6:         [PASS][1] -> [INCOMPLETE][2] +1 other test incomplete
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/bat-adls-6/igt@i915_selftest@live.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-adls-6/igt@i915_selftest@live.html
    - fi-cfl-8109u:       [PASS][3] -> [INCOMPLETE][4] +1 other test incomplete
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/fi-cfl-8109u/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/fi-cfl-8109u/igt@i915_selftest@live.html

  * igt@i915_selftest@live@execlists:
    - bat-kbl-2:          [PASS][5] -> [ABORT][6] +1 other test abort
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/bat-kbl-2/igt@i915_selftest@live@execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-kbl-2/igt@i915_selftest@live@execlists.html

  * igt@kms_pipe_crc_basic@read-crc@pipe-a-dp-1:
    - bat-rpls-4:         [PASS][7] -> [INCOMPLETE][8] +1 other test incomplete
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/bat-rpls-4/igt@kms_pipe_crc_basic@read-crc@pipe-a-dp-1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-rpls-4/igt@kms_pipe_crc_basic@read-crc@pipe-a-dp-1.html

  * igt@runner@aborted:
    - bat-dg2-14:         NOTRUN -> [FAIL][9]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-dg2-14/igt@runner@aborted.html

  
#### Warnings ####

  * igt@i915_selftest@live:
    - fi-kbl-7567u:       [DMESG-WARN][10] ([i915#13735]) -> [INCOMPLETE][11] +1 other test incomplete
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/fi-kbl-7567u/igt@i915_selftest@live.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/fi-kbl-7567u/igt@i915_selftest@live.html

  
Known issues
------------

  Here are the changes found in Patchwork_169677v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@dmabuf@all-tests:
    - bat-jsl-5:          NOTRUN -> [SKIP][12] ([i915#15931])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@dmabuf@all-tests.html

  * igt@gem_huc_copy@huc-copy:
    - bat-jsl-5:          NOTRUN -> [SKIP][13] ([i915#2190])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-jsl-5:          NOTRUN -> [SKIP][14] ([i915#4613]) +3 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@intel_hwmon@hwmon-read:
    - bat-jsl-5:          NOTRUN -> [SKIP][15] ([i915#7707]) +1 other test skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@intel_hwmon@hwmon-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-jsl-5:          NOTRUN -> [SKIP][16] ([i915#4103]) +1 other test skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-jsl-5:          NOTRUN -> [SKIP][17] ([i915#16361])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-jsl-5:          NOTRUN -> [SKIP][18]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-jsl-5:          NOTRUN -> [SKIP][19] ([i915#15205])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-page-flip:
    - bat-jsl-5:          NOTRUN -> [SKIP][20] ([i915#1072]) +3 other tests skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_psr@psr-primary-page-flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-jsl-5:          NOTRUN -> [SKIP][21] ([i915#3555])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/bat-jsl-5/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@client:
    - fi-kbl-7567u:       [DMESG-WARN][22] ([i915#13735]) -> [PASS][23] +4 other tests pass
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18756/fi-kbl-7567u/igt@i915_selftest@live@client.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/fi-kbl-7567u/igt@i915_selftest@live@client.html

  
  [i915#1072]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1072
  [i915#13735]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13735
  [i915#15205]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15205
  [i915#15931]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15931
  [i915#16361]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/16361
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#7707]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7707


Build changes
-------------

  * Linux: CI_DRM_18756 -> Patchwork_169677v2

  CI-20190529: 20190529
  CI_DRM_18756: 539ef0bb9cab59e164d7b737287384864a457d05 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8990: 8990
  Patchwork_169677v2: 539ef0bb9cab59e164d7b737287384864a457d05 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169677v2/index.html

[-- Attachment #2: Type: text/html, Size: 8135 bytes --]