[PATCH v3 1/7] drm/panthor: Make panthor_irq::state a non-atomic field
From: Boris Brezillon
Date: Tue Jun 23 2026 - 08:53:47 EST
The only place where panthor_irq::state is accessed without
panthor_irq::mask_lock held is in the prologue of _irq_suspend(),
which is not really a fast-path. So let's simplify things by assuming
panthor_irq::state must always be accessed with the mask_lock held,
and add a scoped_guard() in _irq_suspend().
While at it, rename the lock so it's clear it doesn't just protect
access to the panthor_irq::mask or the INT_MASK register.
Reviewed-by: Steven Price <steven.price@xxxxxxx>
Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
Reviewed-by: Chia-I Wu <olvaffe@xxxxxxxxx>
Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
---
drivers/gpu/drm/panthor/panthor_device.h | 53 ++++++++++++++++----------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 35679bfa1f3a..c4a03ac0812e 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -92,17 +92,21 @@ struct panthor_irq {
u32 mask;
/**
- * @mask_lock: protects modifications to _INT_MASK and @mask.
+ * @lock: protects modifications to _INT_MASK, @mask and @state.
*
* In paths where _INT_MASK is updated based on a state
* transition/check, it's crucial for the state update/check to be
* inside the locked section, otherwise it introduces a race window
* leading to potential _INT_MASK inconsistencies.
*/
- spinlock_t mask_lock;
+ spinlock_t lock;
- /** @state: one of &enum panthor_irq_state reflecting the current state. */
- atomic_t state;
+ /**
+ * @state: one of &enum panthor_irq_state reflecting the current state.
+ *
+ * Must be accessed with lock held.
+ */
+ enum panthor_irq_state state;
};
/**
@@ -510,18 +514,15 @@ const char *panthor_exception_name(struct panthor_device *ptdev,
static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data) \
{ \
struct panthor_irq *pirq = data; \
- enum panthor_irq_state old_state; \
\
if (!gpu_read(pirq->iomem, INT_STAT)) \
return IRQ_NONE; \
\
- guard(spinlock_irqsave)(&pirq->mask_lock); \
- old_state = atomic_cmpxchg(&pirq->state, \
- PANTHOR_IRQ_STATE_ACTIVE, \
- PANTHOR_IRQ_STATE_PROCESSING); \
- if (old_state != PANTHOR_IRQ_STATE_ACTIVE) \
+ guard(spinlock_irqsave)(&pirq->lock); \
+ if (pirq->state != PANTHOR_IRQ_STATE_ACTIVE) \
return IRQ_NONE; \
\
+ pirq->state = PANTHOR_IRQ_STATE_PROCESSING; \
gpu_write(pirq->iomem, INT_MASK, 0); \
return IRQ_WAKE_THREAD; \
} \
@@ -550,14 +551,11 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
ret = IRQ_HANDLED; \
} \
\
- scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
- enum panthor_irq_state old_state; \
- \
- old_state = atomic_cmpxchg(&pirq->state, \
- PANTHOR_IRQ_STATE_PROCESSING, \
- PANTHOR_IRQ_STATE_ACTIVE); \
- if (old_state == PANTHOR_IRQ_STATE_PROCESSING) \
+ scoped_guard(spinlock_irqsave, &pirq->lock) { \
+ if (pirq->state == PANTHOR_IRQ_STATE_PROCESSING) { \
+ pirq->state = PANTHOR_IRQ_STATE_ACTIVE; \
gpu_write(pirq->iomem, INT_MASK, pirq->mask); \
+ } \
} \
\
return ret; \
@@ -565,19 +563,20 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
\
static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
{ \
- scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
- atomic_set(&pirq->state, PANTHOR_IRQ_STATE_SUSPENDING); \
+ scoped_guard(spinlock_irqsave, &pirq->lock) { \
+ pirq->state = PANTHOR_IRQ_STATE_SUSPENDING; \
gpu_write(pirq->iomem, INT_MASK, 0); \
} \
synchronize_irq(pirq->irq); \
- atomic_set(&pirq->state, PANTHOR_IRQ_STATE_SUSPENDED); \
+ scoped_guard(spinlock_irqsave, &pirq->lock) \
+ pirq->state = PANTHOR_IRQ_STATE_SUSPENDED; \
} \
\
static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq) \
{ \
- guard(spinlock_irqsave)(&pirq->mask_lock); \
+ guard(spinlock_irqsave)(&pirq->lock); \
\
- atomic_set(&pirq->state, PANTHOR_IRQ_STATE_ACTIVE); \
+ pirq->state = PANTHOR_IRQ_STATE_ACTIVE; \
gpu_write(pirq->iomem, INT_CLEAR, pirq->mask); \
gpu_write(pirq->iomem, INT_MASK, pirq->mask); \
} \
@@ -590,7 +589,7 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
pirq->irq = irq; \
pirq->mask = mask; \
pirq->iomem = iomem; \
- spin_lock_init(&pirq->mask_lock); \
+ spin_lock_init(&pirq->lock); \
panthor_ ## __name ## _irq_resume(pirq); \
\
return devm_request_threaded_irq(ptdev->base.dev, irq, \
@@ -602,7 +601,7 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
\
static inline void panthor_ ## __name ## _irq_enable_events(struct panthor_irq *pirq, u32 mask) \
{ \
- guard(spinlock_irqsave)(&pirq->mask_lock); \
+ guard(spinlock_irqsave)(&pirq->lock); \
pirq->mask |= mask; \
\
/* The only situation where we need to write the new mask is if the IRQ is active. \
@@ -610,13 +609,13 @@ static inline void panthor_ ## __name ## _irq_enable_events(struct panthor_irq *
* on the PROCESSING -> ACTIVE transition. \
* If the IRQ is suspended/suspending, the mask is restored at resume time. \
*/ \
- if (atomic_read(&pirq->state) == PANTHOR_IRQ_STATE_ACTIVE) \
+ if (pirq->state == PANTHOR_IRQ_STATE_ACTIVE) \
gpu_write(pirq->iomem, INT_MASK, pirq->mask); \
} \
\
static inline void panthor_ ## __name ## _irq_disable_events(struct panthor_irq *pirq, u32 mask)\
{ \
- guard(spinlock_irqsave)(&pirq->mask_lock); \
+ guard(spinlock_irqsave)(&pirq->lock); \
pirq->mask &= ~mask; \
\
/* The only situation where we need to write the new mask is if the IRQ is active. \
@@ -624,7 +623,7 @@ static inline void panthor_ ## __name ## _irq_disable_events(struct panthor_irq
* on the PROCESSING -> ACTIVE transition. \
* If the IRQ is suspended/suspending, the mask is restored at resume time. \
*/ \
- if (atomic_read(&pirq->state) == PANTHOR_IRQ_STATE_ACTIVE) \
+ if (pirq->state == PANTHOR_IRQ_STATE_ACTIVE) \
gpu_write(pirq->iomem, INT_MASK, pirq->mask); \
}
--
2.54.0