A collection of interrupt related fixes and cleanups. A few patches are from Devarsh and have been posted to dri-devel, which I've included here with a permission from Devarsh, so that we have all interrupt patches together. I have modified both of those patches compared to the posted versions.
Tomi
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- Devarsh Thakkar (2): drm/tidss: Clear the interrupt status for interrupts being disabled drm/tidss: Fix race condition while handling interrupt registers
Tomi Valkeinen (5): drm/tidss: Fix issue in irq handling causing irq-flood issue drm/tidss: Remove unused OCP error flag drm/tidss: Remove extra K2G check drm/tidss: Add printing of underflows drm/tidss: Rename 'wait_lock' to 'irq_lock'
drivers/gpu/drm/tidss/tidss_dispc.c | 28 ++++++++++++++++------------ drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tidss/tidss_drv.h | 5 +++-- drivers/gpu/drm/tidss/tidss_irq.c | 34 +++++++++++++++++++++++----------- drivers/gpu/drm/tidss/tidss_irq.h | 4 +--- drivers/gpu/drm/tidss/tidss_plane.c | 8 ++++++++ drivers/gpu/drm/tidss/tidss_plane.h | 2 ++ 7 files changed, 54 insertions(+), 29 deletions(-) --- base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652 change-id: 20240918-tidss-irq-fix-f687b149a42c
Best regards,
It has been observed that sometimes DSS will trigger an interrupt and the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and VID level interrupt-statuses are zero.
As the top level irqstatus is supposed to tell whether we have VP/VID interrupts, the thinking of the driver authors was that this particular case could never happen. Thus the driver only clears the DISPC_IRQSTATUS bits which has corresponding interrupts in VP/VID status. So when this issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an interrupt flood.
It is unclear why the issue happens. It could be a race issue in the driver, but no such race has been found. It could also be an issue with the HW. However a similar case can be easily triggered by manually writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears the bit, we get an interrupt flood.
To fix the issue, always clear DISPC_IRQSTATUS. The concern with this solution is that if the top level irqstatus is the one that triggers the interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts unhandled if VP/VID interrupt statuses have bits set. However, testing shows that if any of the irqstatuses is set (i.e. even if DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an interrupt.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Co-developed-by: Bin Liu b-liu@ti.com Signed-off-by: Bin Liu b-liu@ti.com Co-developed-by: Devarsh Thakkar devarsht@ti.com Signed-off-by: Devarsh Thakkar devarsht@ti.com Co-developed-by: Jonathan Cormier jcormier@criticallink.com Signed-off-by: Jonathan Cormier jcormier@criticallink.com Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org --- drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 1ad711f8d2a8..f81111067578 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -780,24 +780,20 @@ static void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask) { unsigned int i; - u32 top_clear = 0;
for (i = 0; i < dispc->feat->num_vps; ++i) { - if (clearmask & DSS_IRQ_VP_MASK(i)) { + if (clearmask & DSS_IRQ_VP_MASK(i)) dispc_k3_vp_write_irqstatus(dispc, i, clearmask); - top_clear |= BIT(i); - } } for (i = 0; i < dispc->feat->num_planes; ++i) { - if (clearmask & DSS_IRQ_PLANE_MASK(i)) { + if (clearmask & DSS_IRQ_PLANE_MASK(i)) dispc_k3_vid_write_irqstatus(dispc, i, clearmask); - top_clear |= BIT(4 + i); - } } if (dispc->feat->subrev == DISPC_K2G) return;
- dispc_write(dispc, DISPC_IRQSTATUS, top_clear); + /* always clear the top level irqstatus */ + dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS));
/* Flush posted writes */ dispc_read(dispc, DISPC_IRQSTATUS);
On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen tomi.valkeinen@ideasonboard.com wrote:
It has been observed that sometimes DSS will trigger an interrupt and the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and VID level interrupt-statuses are zero.
As the top level irqstatus is supposed to tell whether we have VP/VID interrupts, the thinking of the driver authors was that this particular case could never happen. Thus the driver only clears the DISPC_IRQSTATUS bits which has corresponding interrupts in VP/VID status. So when this issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an interrupt flood.
It is unclear why the issue happens. It could be a race issue in the driver, but no such race has been found. It could also be an issue with the HW. However a similar case can be easily triggered by manually writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears the bit, we get an interrupt flood.
To fix the issue, always clear DISPC_IRQSTATUS. The concern with this solution is that if the top level irqstatus is the one that triggers the interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts unhandled if VP/VID interrupt statuses have bits set. However, testing shows that if any of the irqstatuses is set (i.e. even if DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an interrupt.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Co-developed-by: Bin Liu b-liu@ti.com Signed-off-by: Bin Liu b-liu@ti.com Co-developed-by: Devarsh Thakkar devarsht@ti.com Signed-off-by: Devarsh Thakkar devarsht@ti.com Co-developed-by: Jonathan Cormier jcormier@criticallink.com Signed-off-by: Jonathan Cormier jcormier@criticallink.com Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org
drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
I assume a reviewed by doesn't make sense since I co-developed this patch but adding my tested by, hopefully, that makes sense.
Tested an equivalent patch for several weeks. Tested-by: Jonathan Cormier jcormier@criticallink.com
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 1ad711f8d2a8..f81111067578 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -780,24 +780,20 @@ static void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask) { unsigned int i;
u32 top_clear = 0; for (i = 0; i < dispc->feat->num_vps; ++i) {if (clearmask & DSS_IRQ_VP_MASK(i)) {
if (clearmask & DSS_IRQ_VP_MASK(i)) dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(i);} } for (i = 0; i < dispc->feat->num_planes; ++i) {if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
if (clearmask & DSS_IRQ_PLANE_MASK(i)) dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(4 + i);} } if (dispc->feat->subrev == DISPC_K2G) return;dispc_write(dispc, DISPC_IRQSTATUS, top_clear);
/* always clear the top level irqstatus */dispc_write(dispc, DISPC_IRQSTATUS, dispc_read(dispc, DISPC_IRQSTATUS)); /* Flush posted writes */ dispc_read(dispc, DISPC_IRQSTATUS);-- 2.43.0
Hi Tomi, Devarsh,
On 10/21/24 19:37, Tomi Valkeinen wrote:
It has been observed that sometimes DSS will trigger an interrupt and the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and VID level interrupt-statuses are zero.
Does this mean that there was a legitimate interrupt that potentially went unrecognized? Or that there was a, for the lack of a better word, fake interrupt trigger that doesn't need handling but just clearing?
As the top level irqstatus is supposed to tell whether we have VP/VID interrupts, the thinking of the driver authors was that this particular case could never happen. Thus the driver only clears the DISPC_IRQSTATUS bits which has corresponding interrupts in VP/VID status. So when this issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an interrupt flood.
It is unclear why the issue happens. It could be a race issue in the driver, but no such race has been found. It could also be an issue with the HW. However a similar case can be easily triggered by manually writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears the bit, we get an interrupt flood.
To fix the issue, always clear DISPC_IRQSTATUS. The concern with this solution is that if the top level irqstatus is the one that triggers the interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts unhandled if VP/VID interrupt statuses have bits set. However, testing shows that if any of the irqstatuses is set (i.e. even if DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an interrupt.
Does this mean if VID/VP irqstatus has been set right around the time the equivalent DISPC_IRQSTATUS bit is being cleared, the equivalent DISPC_IRQSTATUS bit is going to get set again, and make the driver handle the event as we expect it to?
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Co-developed-by: Bin Liu b-liu@ti.com Signed-off-by: Bin Liu b-liu@ti.com Co-developed-by: Devarsh Thakkar devarsht@ti.com Signed-off-by: Devarsh Thakkar devarsht@ti.com Co-developed-by: Jonathan Cormier jcormier@criticallink.com Signed-off-by: Jonathan Cormier jcormier@criticallink.com Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org
drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 1ad711f8d2a8..f81111067578 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -780,24 +780,20 @@ static void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask) { unsigned int i;
- u32 top_clear = 0;
for (i = 0; i < dispc->feat->num_vps; ++i) {
if (clearmask & DSS_IRQ_VP_MASK(i)) {
if (clearmask & DSS_IRQ_VP_MASK(i)) dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(i);}if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
if (clearmask & DSS_IRQ_PLANE_MASK(i)) dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(4 + i);}
nit: Maybe these for-loop braces could be dropped as well.
Otherwise, LGTM,
Reviewed-by: Aradhya Bhatia aradhya.bhatia@linux.dev
Regards Aradhya
[..]
Hi,
On 24/11/2024 19:18, Aradhya Bhatia wrote:
Hi Tomi, Devarsh,
On 10/21/24 19:37, Tomi Valkeinen wrote:
It has been observed that sometimes DSS will trigger an interrupt and the top level interrupt (DISPC_IRQSTATUS) is not zero, but the VP and VID level interrupt-statuses are zero.
Does this mean that there was a legitimate interrupt that potentially went unrecognized? Or that there was a, for the lack of a better word, fake interrupt trigger that doesn't need handling but just clearing?
I don't have an answer to that. I haven't been able to trigger this issue, and I guess it's difficult to say for certain in any case.
My guess is that it's some kind of race issue either in the HW or the combination of HW+SW.
As the top level irqstatus is supposed to tell whether we have VP/VID interrupts, the thinking of the driver authors was that this particular case could never happen. Thus the driver only clears the DISPC_IRQSTATUS bits which has corresponding interrupts in VP/VID status. So when this issue happens, the driver will not clear DISPC_IRQSTATUS, and we get an interrupt flood.
It is unclear why the issue happens. It could be a race issue in the driver, but no such race has been found. It could also be an issue with the HW. However a similar case can be easily triggered by manually writing to DISPC_IRQSTATUS_RAW. This will forcibly set a bit in the DISPC_IRQSTATUS and trigger an interrupt, and as the driver never clears the bit, we get an interrupt flood.
To fix the issue, always clear DISPC_IRQSTATUS. The concern with this solution is that if the top level irqstatus is the one that triggers the interrupt, always clearing DISPC_IRQSTATUS might leave some interrupts unhandled if VP/VID interrupt statuses have bits set. However, testing shows that if any of the irqstatuses is set (i.e. even if DISPC_IRQSTATUS == 0, but a VID irqstatus has a bit set), we will get an interrupt.
Does this mean if VID/VP irqstatus has been set right around the time the equivalent DISPC_IRQSTATUS bit is being cleared, the equivalent DISPC_IRQSTATUS bit is going to get set again, and make the driver handle the event as we expect it to?
(If I recall right) no, DISPC_IRQSTATUS won't be set. But it doesn't matter, the interrupt will be triggered anyway, and the driver will handle the interrupt.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Co-developed-by: Bin Liu b-liu@ti.com Signed-off-by: Bin Liu b-liu@ti.com Co-developed-by: Devarsh Thakkar devarsht@ti.com Signed-off-by: Devarsh Thakkar devarsht@ti.com Co-developed-by: Jonathan Cormier jcormier@criticallink.com Signed-off-by: Jonathan Cormier jcormier@criticallink.com Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org
drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 1ad711f8d2a8..f81111067578 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -780,24 +780,20 @@ static void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask) { unsigned int i;
- u32 top_clear = 0;
for (i = 0; i < dispc->feat->num_vps; ++i) {
if (clearmask & DSS_IRQ_VP_MASK(i)) {
if (clearmask & DSS_IRQ_VP_MASK(i)) dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(i);}if (clearmask & DSS_IRQ_PLANE_MASK(i)) {
if (clearmask & DSS_IRQ_PLANE_MASK(i)) dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
top_clear |= BIT(4 + i);}nit: Maybe these for-loop braces could be dropped as well.
I like to have braces if there are multiple lines under it.
Otherwise, LGTM,
Reviewed-by: Aradhya Bhatia aradhya.bhatia@linux.dev
Thanks!
Tomi
From: Devarsh Thakkar devarsht@ti.com
The driver does not touch the irqstatus register when it is disabling interrupts. This might cause an interrupt to trigger for an interrupt that was just disabled.
To fix the issue, clear the irqstatus registers right after disabling the interrupts.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Reported-by: Jonathan Cormier jcormier@criticallink.com Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/13... Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: mostly rewrote the patch] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 99a1138f3e69..515f82e8a0a5 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -700,7 +700,7 @@ void dispc_k2g_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask) { dispc_irq_t old_mask = dispc_k2g_read_irqenable(dispc);
- /* clear the irqstatus for newly enabled irqs */ + /* clear the irqstatus for irqs that will be enabled */ dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & mask);
dispc_k2g_vp_set_irqenable(dispc, 0, mask); @@ -708,6 +708,9 @@ void dispc_k2g_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
dispc_write(dispc, DISPC_IRQENABLE_SET, (1 << 0) | (1 << 7));
+ /* clear the irqstatus for irqs that were disabled */ + dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & old_mask); + /* flush posted write */ dispc_k2g_read_irqenable(dispc); } @@ -837,7 +840,7 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
old_mask = dispc_k3_read_irqenable(dispc);
- /* clear the irqstatus for newly enabled irqs */ + /* clear the irqstatus for irqs that will be enabled */ dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & mask);
for (i = 0; i < dispc->feat->num_vps; ++i) { @@ -862,6 +865,9 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc, if (main_disable) dispc_write(dispc, DISPC_IRQENABLE_CLR, main_disable);
+ /* clear the irqstatus for irqs that were disabled */ + dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & old_mask); + /* Flush posted writes */ dispc_read(dispc, DISPC_IRQENABLE_SET); }
On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen tomi.valkeinen@ideasonboard.com wrote:
From: Devarsh Thakkar devarsht@ti.com
The driver does not touch the irqstatus register when it is disabling interrupts. This might cause an interrupt to trigger for an interrupt that was just disabled.
To fix the issue, clear the irqstatus registers right after disabling the interrupts.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Reported-by: Jonathan Cormier jcormier@criticallink.com Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/13... Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: mostly rewrote the patch] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
Thanks for the updates. They look pretty similar to the changes I proposed and thus look good to me. Reviewed-by: Jonathan Cormier jcormier@criticallink.com Tested an equivalent patch for several weeks. Tested-by: Jonathan Cormier jcormier@criticallink.com
drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 99a1138f3e69..515f82e8a0a5 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
On 10/21/24 19:37, Tomi Valkeinen wrote:
From: Devarsh Thakkar devarsht@ti.com
The driver does not touch the irqstatus register when it is disabling interrupts. This might cause an interrupt to trigger for an interrupt that was just disabled.
To fix the issue, clear the irqstatus registers right after disabling the interrupts.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Reported-by: Jonathan Cormier jcormier@criticallink.com Closes: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/13... Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: mostly rewrote the patch] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
Reviewed-by: Aradhya Bhatia aradhya.bhatia@linux.dev
Regards Aradhya
[...]
From: Devarsh Thakkar devarsht@ti.com
The driver has a spinlock for protecting the irq_masks field and irq enable registers. However, the driver misses protecting the irq status registers which can lead to races.
Take the spinlock when accessing irqstatus too.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: updated the desc] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/gpu/drm/tidss/tidss_dispc.c | 4 ++++ drivers/gpu/drm/tidss/tidss_irq.c | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 515f82e8a0a5..07f5c26cfa26 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -2767,8 +2767,12 @@ static void dispc_init_errata(struct dispc_device *dispc) */ static void dispc_softreset_k2g(struct dispc_device *dispc) { + unsigned long flags; + + spin_lock_irqsave(&dispc->tidss->wait_lock, flags); dispc_set_irqenable(dispc, 0); dispc_read_and_clear_irqstatus(dispc); + spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags);
for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx) VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0); diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c index 3cc4024ec7ff..8af4682ba56b 100644 --- a/drivers/gpu/drm/tidss/tidss_irq.c +++ b/drivers/gpu/drm/tidss/tidss_irq.c @@ -60,7 +60,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg) unsigned int id; dispc_irq_t irqstatus;
+ spin_lock(&tidss->wait_lock); irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc); + spin_unlock(&tidss->wait_lock);
for (id = 0; id < tidss->num_crtcs; id++) { struct drm_crtc *crtc = tidss->crtcs[id];
On Mon, Oct 21, 2024 at 10:08 AM Tomi Valkeinen tomi.valkeinen@ideasonboard.com wrote:
From: Devarsh Thakkar devarsht@ti.com
The driver has a spinlock for protecting the irq_masks field and irq enable registers. However, the driver misses protecting the irq status registers which can lead to races.
Take the spinlock when accessing irqstatus too.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: updated the desc] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
drivers/gpu/drm/tidss/tidss_dispc.c | 4 ++++ drivers/gpu/drm/tidss/tidss_irq.c | 2 ++ 2 files changed, 6 insertions(+)
Reviewed-by: Jonathan Cormier jcormier@criticallink.com Tested an equivalent patch for several weeks. Tested-by: Jonathan Cormier jcormier@criticallink.com
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index 515f82e8a0a5..07f5c26cfa26 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -2767,8 +2767,12 @@ static void dispc_init_errata(struct dispc_device *dispc) */ static void dispc_softreset_k2g(struct dispc_device *dispc) {
unsigned long flags;spin_lock_irqsave(&dispc->tidss->wait_lock, flags); dispc_set_irqenable(dispc, 0); dispc_read_and_clear_irqstatus(dispc);spin_unlock_irqrestore(&dispc->tidss->wait_lock, flags); for (unsigned int vp_idx = 0; vp_idx < dispc->feat->num_vps; ++vp_idx) VP_REG_FLD_MOD(dispc, vp_idx, DISPC_VP_CONTROL, 0, 0, 0);diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c index 3cc4024ec7ff..8af4682ba56b 100644 --- a/drivers/gpu/drm/tidss/tidss_irq.c +++ b/drivers/gpu/drm/tidss/tidss_irq.c @@ -60,7 +60,9 @@ static irqreturn_t tidss_irq_handler(int irq, void *arg) unsigned int id; dispc_irq_t irqstatus;
spin_lock(&tidss->wait_lock); irqstatus = dispc_read_and_clear_irqstatus(tidss->dispc);spin_unlock(&tidss->wait_lock); for (id = 0; id < tidss->num_crtcs; id++) { struct drm_crtc *crtc = tidss->crtcs[id];-- 2.43.0
On 10/21/24 19:37, Tomi Valkeinen wrote:
From: Devarsh Thakkar devarsht@ti.com
The driver has a spinlock for protecting the irq_masks field and irq enable registers. However, the driver misses protecting the irq status registers which can lead to races.
Take the spinlock when accessing irqstatus too.
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem") Cc: stable@vger.kernel.org Signed-off-by: Devarsh Thakkar devarsht@ti.com [Tomi: updated the desc] Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
Reviewed-by: Aradhya Bhatia aradhya.bhatia@linux.dev
Regards Aradhya
[...]
linux-stable-mirror@lists.linaro.org
-
Aradhya Bhatia -
Jon Cormier -
Tomi Valkeinen