[PATCH] drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

[PATCH] drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

[vishnu.saini]

From: Naman S Thaker <namathak@qti.qualcomm.com>

On a PREEMPT_RT kernel, dpu_core_irq() runs as a true hardirq handler,
but it dispatches per-encoder callbacks that take sleepable locks
(spinlock_t becomes an rt_mutex on RT, and some DRM-core locks reached
through vblank/CRC/writeback handling are sleepable as well). Sleeping
inside a hardirq handler is not allowed and eventually crashes the
display, which is what happens after running GLMark2 for a while.

Split dpu_core_irq() into a minimal hardirq handler that only
acknowledges the hardware and records which interrupts fired, plus a
new dpu_core_irq_thread() that performs the actual callback dispatch
from a real, preemptible IRQ thread. This split only takes effect
under CONFIG_PREEMPT_RT; non-RT kernels keep dispatching callbacks
directly from dpu_core_irq() as before.

irq_lock is changed from spinlock_t to raw_spinlock_t unconditionally,
since the hardirq handler needs a lock that never sleeps under RT, and
raw_spinlock_t behaves the same as spinlock_t on non-RT kernels.

dpu_core_irq() itself now takes irq_lock with plain raw_spin_lock()
instead of raw_spin_lock_irqsave(), dropping the irqsave/irqrestore
pair it previously needed as a bottom-half-safe spinlock user. This is
safe because dpu_core_irq() only ever runs as a primary IRQ handler
(hardirq context on non-RT, forced-thread primary handler on RT), both
of which are always entered with local IRQs already disabled by genirq
before the handler is called, so there is nothing left for irqsave to
save here. dpu_core_irq_read(), by contrast, is called from process
context and still needs raw_spin_lock_irqsave().

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Cc: stable@vger.kernel.org
Signed-off-by: Naman S Thaker <namathak@qti.qualcomm.com>
Signed-off-by: Vishnu Saini <vishnu.saini@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h      |  10 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 128 ++++++++++++++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  11 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |   3 +
 drivers/gpu/drm/msm/msm_kms.c                     |  32 ++++++
 drivers/gpu/drm/msm/msm_kms.h                     |   9 ++
 6 files changed, 170 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
index e7183cf05776..383e89813db3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
@@ -14,6 +14,16 @@ void dpu_core_irq_uninstall(struct msm_kms *kms);
 
 irqreturn_t dpu_core_irq(struct msm_kms *kms);
 
+#ifdef CONFIG_PREEMPT_RT
+/**
+ * dpu_core_irq_thread - core IRQ threaded handler, dispatches the per-IRQ
+ *                       callbacks recorded by dpu_core_irq()
+ * @kms:		MSM KMS handle
+ * @return:		interrupt handling status
+ */
+irqreturn_t dpu_core_irq_thread(struct msm_kms *kms);
+#endif
+
 u32 dpu_core_irq_read(
 		struct dpu_kms *dpu_kms,
 		unsigned int irq_idx);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 5b7cd5241f45..95b016e8d7df 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -322,6 +322,52 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, unsigned int
 	irq_entry->cb(irq_entry->arg);
 }
 
+/*
+ * dpu_core_irq_dispatch() runs the fired bits for @reg_idx through their
+ * registered callbacks directly. Only used on non-PREEMPT_RT kernels, where
+ * dpu_core_irq() itself is allowed to take the sleepable locks reached via
+ * those callbacks.
+ */
+static void dpu_core_irq_dispatch(struct dpu_kms *dpu_kms, int reg_idx, u32 irq_status)
+{
+	unsigned int irq_idx;
+	int bit;
+
+	/*
+	 * Search through matching intr status.
+	 */
+	while ((bit = ffs(irq_status)) != 0) {
+		irq_idx = DPU_IRQ_IDX(reg_idx, bit - 1);
+
+		dpu_core_irq_callback_handler(dpu_kms, irq_idx);
+
+		/*
+		 * When callback finish, clear the irq_status
+		 * with the matching mask. Once irq_status
+		 * is all cleared, the search can be stopped.
+		 */
+		irq_status &= ~BIT(bit - 1);
+	}
+}
+
+#ifdef CONFIG_PREEMPT_RT
+/*
+ * dpu_core_irq_defer_to_thread() hands the fired bits for @reg_idx off to
+ * dpu_core_irq_thread(), which dispatches them from a genuine preemptible
+ * IRQ thread instead of the hardirq context dpu_core_irq() runs in on
+ * PREEMPT_RT.
+ */
+static void dpu_core_irq_defer_to_thread(struct dpu_hw_intr *intr, int reg_idx, u32 irq_status)
+{
+	intr->irq_pending_mask[reg_idx] |= irq_status;
+}
+#else
+static inline void dpu_core_irq_defer_to_thread(struct dpu_hw_intr *intr, int reg_idx,
+						u32 irq_status)
+{
+}
+#endif
+
 /**
  * dpu_core_irq - core IRQ handler
  * @kms:		MSM KMS handle
@@ -332,16 +378,14 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
 	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
 	int reg_idx;
-	unsigned int irq_idx;
 	u32 irq_status;
 	u32 enable_mask;
-	int bit;
-	unsigned long irq_flags;
+	bool wake_thread = false;
 
 	if (!intr)
 		return IRQ_NONE;
 
-	spin_lock_irqsave(&intr->irq_lock, irq_flags);
+	raw_spin_lock(&intr->irq_lock);
 	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
 		if (!test_bit(reg_idx, &intr->irq_mask))
 			continue;
@@ -363,6 +407,52 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
 		if (!irq_status)
 			continue;
 
+		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+			dpu_core_irq_defer_to_thread(intr, reg_idx, irq_status);
+			wake_thread = true;
+		} else {
+			dpu_core_irq_dispatch(dpu_kms, reg_idx, irq_status);
+		}
+	}
+
+	/* ensure register writes go through */
+	wmb();
+
+	raw_spin_unlock(&intr->irq_lock);
+
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		return wake_thread ? IRQ_WAKE_THREAD : IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_PREEMPT_RT
+/*
+ * dpu_core_irq_thread() runs in a genuine preemptible IRQ thread (woken via
+ * IRQ_WAKE_THREAD from dpu_core_irq() above), so it's safe for it -- and the
+ * per-encoder callbacks it dispatches to -- to take spinlock_t/rt_mutex
+ * locks such as enc_spinlock, dpu_crtc::spin_lock, and the various DRM-core
+ * locks reached via vblank/CRC/writeback handling.
+ */
+irqreturn_t dpu_core_irq_thread(struct msm_kms *kms)
+{
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
+	int reg_idx;
+	unsigned int irq_idx;
+	u32 irq_status;
+	unsigned long irq_flags;
+	int bit;
+
+	if (!intr)
+		return IRQ_NONE;
+
+	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
+		raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
+		irq_status = intr->irq_pending_mask[reg_idx];
+		intr->irq_pending_mask[reg_idx] = 0;
+		raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
+
 		/*
 		 * Search through matching intr status.
 		 */
@@ -380,13 +470,9 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
 		}
 	}
 
-	/* ensure register writes go through */
-	wmb();
-
-	spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
-
 	return IRQ_HANDLED;
 }
+#endif
 
 static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr,
 					 unsigned int irq_idx)
@@ -410,7 +496,7 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr,
 	 * under irq_lock and it's the caller's responsibility to ensure that's
 	 * held.
 	 */
-	assert_spin_locked(&intr->irq_lock);
+	assert_raw_spin_locked(&intr->irq_lock);
 
 	reg_idx = DPU_IRQ_REG(irq_idx);
 	reg = &intr->intr_set[reg_idx];
@@ -466,7 +552,7 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr,
 	 * under irq_lock and it's the caller's responsibility to ensure that's
 	 * held.
 	 */
-	assert_spin_locked(&intr->irq_lock);
+	assert_raw_spin_locked(&intr->irq_lock);
 
 	reg_idx = DPU_IRQ_REG(irq_idx);
 	reg = &intr->intr_set[reg_idx];
@@ -554,7 +640,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
 		return 0;
 	}
 
-	spin_lock_irqsave(&intr->irq_lock, irq_flags);
+	raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
 
 	reg_idx = DPU_IRQ_REG(irq_idx);
 	intr_status = DPU_REG_READ(&intr->hw,
@@ -567,7 +653,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
 	/* ensure register writes go through */
 	wmb();
 
-	spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
+	raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
 
 	return intr_status;
 }
@@ -616,7 +702,7 @@ struct dpu_hw_intr *dpu_hw_intr_init(struct drm_device *dev,
 			intr->irq_mask |= BIT(DPU_IRQ_REG(intf->intr_tear_rd_ptr));
 	}
 
-	spin_lock_init(&intr->irq_lock);
+	raw_spin_lock_init(&intr->irq_lock);
 
 	return intr;
 }
@@ -656,11 +742,11 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms,
 	VERB("[%pS] IRQ=[%d, %d]\n", __builtin_return_address(0),
 	     DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 
-	spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
+	raw_spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
 
 	irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, irq_idx);
 	if (unlikely(WARN_ON(irq_entry->cb))) {
-		spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
+		raw_spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
 
 		return -EBUSY;
 	}
@@ -675,7 +761,7 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms,
 	if (ret)
 		DPU_ERROR("Failed/ to enable IRQ=[%d, %d]\n",
 			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
-	spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
+	raw_spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
 
 	trace_dpu_irq_register_success(DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 
@@ -707,7 +793,7 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms,
 	VERB("[%pS] IRQ=[%d, %d]\n", __builtin_return_address(0),
 	     DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 
-	spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
+	raw_spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
 	trace_dpu_core_irq_unregister_callback(DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 
 	ret = dpu_hw_intr_disable_irq_locked(dpu_kms->hw_intr, irq_idx);
@@ -719,7 +805,7 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms,
 	irq_entry->cb = NULL;
 	irq_entry->arg = NULL;
 
-	spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
+	raw_spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
 
 	trace_dpu_irq_unregister_success(DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 
@@ -736,11 +822,11 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v)
 	void *cb;
 
 	for (i = 1; i <= DPU_NUM_IRQS; i++) {
-		spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
+		raw_spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
 		irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, i);
 		irq_count = atomic_read(&irq_entry->count);
 		cb = irq_entry->cb;
-		spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
+		raw_spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
 
 		if (irq_count || cb)
 			seq_printf(s, "IRQ=[%d, %d] count:%d cb:%ps\n",
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index 142358a105c5..2bd16f9341c2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -54,14 +54,21 @@ struct dpu_hw_intr_entry {
  * @ops:              function pointer mapping for IRQ handling
  * @cache_irq_mask:   array of IRQ enable masks reg storage created during init
  * @save_irq_status:  array of IRQ status reg storage created during init
- * @irq_lock:         spinlock for accessing IRQ resources
+ * @irq_lock:         raw spinlock for accessing IRQ resources.
+ * @irq_pending_mask: per-register bitmask of enabled+fired IRQs that the
+ *                    hardirq primary handler has acked in hardware but not
+ *                    yet handed off to the IRQ thread for callback dispatch
+ *                    (CONFIG_PREEMPT_RT only)
  * @irq_cb_tbl:       array of IRQ callbacks
  */
 struct dpu_hw_intr {
 	struct dpu_hw_blk_reg_map hw;
 	u32 cache_irq_mask[MDP_INTR_MAX];
 	u32 *save_irq_status;
-	spinlock_t irq_lock;
+	raw_spinlock_t irq_lock;
+#ifdef CONFIG_PREEMPT_RT
+	u32 irq_pending_mask[MDP_INTR_MAX];
+#endif
 	unsigned long irq_mask;
 	const struct dpu_intr_reg *intr_set;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index da3556eb6ecc..f85218494665 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1072,6 +1072,9 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq_postinstall = dpu_irq_postinstall,
 	.irq_uninstall   = dpu_core_irq_uninstall,
 	.irq             = dpu_core_irq,
+#ifdef CONFIG_PREEMPT_RT
+	.irq_thread      = dpu_core_irq_thread,
+#endif
 	.enable_commit   = dpu_kms_enable_commit,
 	.disable_commit  = dpu_kms_disable_commit,
 	.check_mode_changed = dpu_kms_check_mode_changed,
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index e5d0ea629448..f7d045f283ad 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -42,6 +42,20 @@ static irqreturn_t msm_irq(int irq, void *arg)
 	return kms->funcs->irq(kms);
 }
 
+#ifdef CONFIG_PREEMPT_RT
+static irqreturn_t msm_irq_thread(int irq, void *arg)
+{
+	struct drm_device *dev = arg;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+
+	if (WARN_ON_ONCE(!kms || !kms->funcs->irq_thread))
+		return IRQ_NONE;
+
+	return kms->funcs->irq_thread(kms);
+}
+#endif
+
 static void msm_irq_preinstall(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -76,7 +90,25 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 
 	msm_irq_preinstall(dev);
 
+#ifdef CONFIG_PREEMPT_RT
+	/*
+	 * Some KMS backends (e.g. dpu1) split their handler into a minimal
+	 * hardirq primary handler that only acks hardware and a threaded
+	 * handler that does the actual (sleep-capable) callback dispatch.
+	 * IRQF_ONESHOT keeps the primary handler running as a true hardirq
+	 * even under PREEMPT_RT's forced-threading (see
+	 * irq_setup_forced_threading() in kernel/irq/manage.c), the same
+	 * property IRQF_NO_THREAD gives the backends that don't split their
+	 * handler and must run their whole ->irq() in hardirq context.
+	 */
+	if (kms->funcs->irq_thread)
+		ret = request_threaded_irq(irq, msm_irq, msm_irq_thread,
+					   IRQF_ONESHOT, dev->driver->name, dev);
+	else
+		ret = request_irq(irq, msm_irq, IRQF_NO_THREAD, dev->driver->name, dev);
+#else
 	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
+#endif
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index f25b31e502d2..f0396cfbfc0c 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -30,6 +30,15 @@ struct msm_kms_funcs {
 	int (*irq_postinstall)(struct msm_kms *kms);
 	void (*irq_uninstall)(struct msm_kms *kms);
 	irqreturn_t (*irq)(struct msm_kms *kms);
+#ifdef CONFIG_PREEMPT_RT
+	/*
+	 * Optional threaded companion to ->irq(), used only on PREEMPT_RT.
+	 * When set, ->irq() must behave as a true hardirq handler (only
+	 * raw_spinlock_t, no sleeping) and hand off any deferred work to
+	 * ->irq_thread(), which runs in a real, preemptible IRQ thread.
+	 */
+	irqreturn_t (*irq_thread)(struct msm_kms *kms);
+#endif
 	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 

---
base-commit: 730c6d8072d8d9a6dbfd06c7c5ab4479167a792a
change-id: 20260909-drm-mis-next-split-irq-db46ae06c24a

Best regards,
-- 
Vishnu Saini <vishnu.saini@oss.qualcomm.com>

Re: [PATCH] drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Setting `IRQF_NO_THREAD` for backends without `irq_thread` forces them into hardirq context on PREEMPT_RT, causing kernel panics.
- [High] Race condition in threaded IRQ handler leading to NULL pointer dereference or Use-After-Free on PREEMPT_RT.
- [High] Acquiring sleepable `spinlock_t` inside `raw_spinlock_t` on non-RT kernels violates wait-context rules, triggering lockdep splats.
- [High] `dpu_core_irq_read()` reads hardware registers directly without checking `irq_pending_mask`, causing fault recovery loops to falsely timeout on PREEMPT_RT.
- [Medium] Primary IRQ handler incorrectly returns `IRQ_NONE` after clearing masked interrupts, risking permanent IRQ disablement.
--

commit de5a98b2b15837b087edf7ff28d14a0650953129
Author: Naman S Thaker <namathak@qti.qualcomm.com>
Subject: drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

This commit splits dpu_core_irq() into a minimal hardirq handler and a new
dpu_core_irq_thread() to dispatch per-encoder callbacks on PREEMPT_RT
kernels. It also changes irq_lock to raw_spinlock_t and drops IRQF_NO_THREAD
where threaded handlers are not implemented.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index 5b7cd5241f450..95b016e8d7df1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -322,6 +322,52 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, unsigned int
>  	irq_entry->cb(irq_entry->arg);
>  }
>  
> +/*
> + * dpu_core_irq_dispatch() runs the fired bits for @reg_idx through their
> + * registered callbacks directly. Only used on non-PREEMPT_RT kernels, where
> + * dpu_core_irq() itself is allowed to take the sleepable locks reached via
> + * those callbacks.
> + */
> +static void dpu_core_irq_dispatch(struct dpu_kms *dpu_kms, int reg_idx, u32 irq_status)
> +{
> +	unsigned int irq_idx;
> +	int bit;
> +
> +	/*
> +	 * Search through matching intr status.
> +	 */
> +	while ((bit = ffs(irq_status)) != 0) {
> +		irq_idx = DPU_IRQ_IDX(reg_idx, bit - 1);
> +
> +		dpu_core_irq_callback_handler(dpu_kms, irq_idx);

[Severity: High]
On non-RT kernels, this dispatch happens while holding the raw_spin_lock 
acquired in dpu_core_irq(). Does this violate wait-context rules?

The dispatched callbacks (e.g., DRM vblank event handlers) acquire standard
spinlock_t locks. Acquiring a spinlock_t (wait context LD_WAIT_CONFIG) while
holding a raw_spinlock_t (wait context LD_WAIT_SPIN) constitutes an invalid
wait context and triggers a lockdep splat on non-RT kernels.

[ ... ]

> @@ -332,16 +378,14 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
>  	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>  	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
>  	int reg_idx;
> -	unsigned int irq_idx;
>  	u32 irq_status;
>  	u32 enable_mask;
> -	int bit;
> -	unsigned long irq_flags;
> +	bool wake_thread = false;
>  
>  	if (!intr)
>  		return IRQ_NONE;
>  
> -	spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +	raw_spin_lock(&intr->irq_lock);
>  	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
>  		if (!test_bit(reg_idx, &intr->irq_mask))
>  			continue;
> @@ -363,6 +407,52 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
>  		if (!irq_status)
>  			continue;

[Severity: Medium]
Will this lead to the generic IRQ subsystem permanently disabling the display
IRQ line?

If an interrupt fires but is filtered out by enable_mask, the hardware
interrupt is actively cleared a few lines above this, but irq_status
becomes 0, meaning the loop continues and wake_thread remains false.

>  
> +		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +			dpu_core_irq_defer_to_thread(intr, reg_idx, irq_status);
> +			wake_thread = true;
> +		} else {
> +			dpu_core_irq_dispatch(dpu_kms, reg_idx, irq_status);
> +		}
> +	}
> +
> +	/* ensure register writes go through */
> +	wmb();
> +
> +	raw_spin_unlock(&intr->irq_lock);
> +
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		return wake_thread ? IRQ_WAKE_THREAD : IRQ_NONE;

[Severity: Medium]
(Continued from above)
On PREEMPT_RT, returning IRQ_NONE after actively clearing a masked interrupt
falsely tells the generic IRQ subsystem that the interrupt was unhandled.
This could cause the spurious interrupt detector to disable the IRQ line.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_PREEMPT_RT
> +/*
> + * dpu_core_irq_thread() runs in a genuine preemptible IRQ thread (woken via
> + * IRQ_WAKE_THREAD from dpu_core_irq() above), so it's safe for it -- and the
> + * per-encoder callbacks it dispatches to -- to take spinlock_t/rt_mutex
> + * locks such as enc_spinlock, dpu_crtc::spin_lock, and the various DRM-core
> + * locks reached via vblank/CRC/writeback handling.
> + */
> +irqreturn_t dpu_core_irq_thread(struct msm_kms *kms)
> +{
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> +	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
> +	int reg_idx;
> +	unsigned int irq_idx;
> +	u32 irq_status;
> +	unsigned long irq_flags;
> +	int bit;
> +
> +	if (!intr)
> +		return IRQ_NONE;
> +
> +	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
> +		raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +		irq_status = intr->irq_pending_mask[reg_idx];
> +		intr->irq_pending_mask[reg_idx] = 0;
> +		raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> +
>  		/*
>  		 * Search through matching intr status.
>  		 */

[Severity: High]
Does this leave the callback dispatch completely unprotected from concurrent
modifications?

Since raw_spin_unlock_irqrestore() is called before iterating over the bits,
the callback pointers can be concurrently set to NULL by
dpu_core_irq_unregister_callback() (which acquires irq_lock). 

This allows the callback pointer to be zeroed between the check and execution
in the thread, which could lead to a deterministic NULL pointer dereference.
Furthermore, if the caller frees the callback context immediately after
unregistering, it causes a Use-After-Free since no synchronization prevents
the thread from executing the concurrently running callback.

[ ... ]

> @@ -554,7 +640,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
>  		return 0;
>  	}
>  
> -	spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +	raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
>  
>  	reg_idx = DPU_IRQ_REG(irq_idx);
>  	intr_status = DPU_REG_READ(&intr->hw,
> @@ -567,7 +653,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
>  	/* ensure register writes go through */
>  	wmb();
>  
> -	spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> +	raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
>  
>  	return intr_status;
>  }

[Severity: High]
Could this result in false timeout errors on PREEMPT_RT?

dpu_core_irq_read() is used by driver wait paths to verify if a hardware event
fired but the software callback was delayed. Because it reads the hardware
register directly (which reads as 0 because the hardirq already cleared it)
and ignores the new irq_pending_mask, it could falsely conclude that the
interrupt never fired.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index e5d0ea6294484..f7d045f283ad9 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -76,7 +90,25 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>  
>  	msm_irq_preinstall(dev);
>  
> +#ifdef CONFIG_PREEMPT_RT
> +	/*
> +	 * Some KMS backends (e.g. dpu1) split their handler into a minimal
> +	 * hardirq primary handler that only acks hardware and a threaded
> +	 * handler that does the actual (sleep-capable) callback dispatch.
> +	 * IRQF_ONESHOT keeps the primary handler running as a true hardirq
> +	 * even under PREEMPT_RT's forced-threading (see
> +	 * irq_setup_forced_threading() in kernel/irq/manage.c), the same
> +	 * property IRQF_NO_THREAD gives the backends that don't split their
> +	 * handler and must run their whole ->irq() in hardirq context.
> +	 */
> +	if (kms->funcs->irq_thread)
> +		ret = request_threaded_irq(irq, msm_irq, msm_irq_thread,
> +					   IRQF_ONESHOT, dev->driver->name, dev);
> +	else
> +		ret = request_irq(irq, msm_irq, IRQF_NO_THREAD, dev->driver->name, dev);

[Severity: Critical]
Will setting IRQF_NO_THREAD force legacy backends into hardirq context on
PREEMPT_RT, causing kernel panics?

The patch sets IRQF_NO_THREAD for legacy KMS backends (e.g., MDP5, MDP4)
because they don't implement the new irq_thread callback. However, this
explicitly disables forced-threading on PREEMPT_RT. Because these legacy
handlers acquire standard spinlock_t locks (which are sleepable rt_mutexes
on RT), running them strictly in hardirq context is invalid and will result
in a "scheduling while atomic" crash.

> +#else
>  	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
> +#endif
>  	if (ret)
>  		return ret;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-drm-mis-next-split-irq-v1-1-89bc9c512c53@oss.qualcomm.com?part=1

Re: [PATCH] drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2026-09-09 14:22:16 [+0530], vishnu.saini@oss.qualcomm.com wrote:
> From: Naman S Thaker <namathak@qti.qualcomm.com>
> 
> On a PREEMPT_RT kernel, dpu_core_irq() runs as a true hardirq handler,

*why* is this the case. The code you replaces adds some ifdefs around
request_irq() with 0 as flags. This does not make it run has hardirq.

> but it dispatches per-encoder callbacks that take sleepable locks
> (spinlock_t becomes an rt_mutex on RT, and some DRM-core locks reached
> through vblank/CRC/writeback handling are sleepable as well). Sleeping
> inside a hardirq handler is not allowed and eventually crashes the
> display, which is what happens after running GLMark2 for a while.

That is correct. That is the irq handler are threaded by default and
only non-threaded if explicitly requested.

I suggest to stick with non-threaded by default.
There is only one request_threaded_irq() as far as I can tell and this
msm_dp_display_request_irq():

|         rc = devm_request_threaded_irq(&pdev->dev, dp->irq,
|                                        msm_dp_display_irq_handler,
|                                        msm_dp_display_irq_thread,
|                                        IRQ_TYPE_LEVEL_HIGH,
|                                        "dp_display_isr", dp);

and its primary handler will be threaded on PREEMPT_RT, too. So you end
up with two threads here.

Sebastian