Hi,
On 12/10/2024 18:07, Devarsh Thakkar wrote:
It is possible that dispc_{k2g/k3}_set_irqenable can be called for
disabling some interrupt events which were previously enabled. However
instead of clearing any pending events for the interrupt events that are
required to be disabled, it was instead clearing the new interrupt events
which were not even enabled.
That's on purpose. When we enable a new interrupt, we want to first
clear the irqstatus for that interrupt to make sure there's no old
status left lying around. If I'm not mistaken, enabling an interrupt
with an irqstatus bit set will immediately trigger the interrupt.
For e.g. While disabling the vsync events, dispc_k3_set_irqenable tries to
clear DSS_IRQ_DEVICE_OCP_ERR which was not enabled per the old_mask at all
as shown below :
"dispc_k3_set_irqenable : irqenabled - mask = 91, old = f0, clr = 1" where
clr = (mask ^ old_mask) & old_mask
That's a bit odd... Why was the DSS_IRQ_DEVICE_OCP_ERR not already
enabled? It is enabled in the tidss_irq_install().
Or maybe it had been enabled by the driver, but as the HW doesn't
support that bit, it reads always as 0. I have an unsent patch to drop
DSS_IRQ_DEVICE_OCP_ERR.
This corrects the bit mask to make sure that it always clears any pending
interrupt events that are requested to be disabled before disabling them
actually.
I think the point here makes sense: if we disable interrupts with
dispc_set_irqenable(), we don't want to see interrupt handling for the
disabled interrupts after the call.
However, if you clear the irqstatus for an interrupt that will be
disabled, but clear it _before_ disabling the interrupt, the interrupt
might trigger right after clearing the irqstatus but before disabling it.
Tomi
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Reported-by: Jonathan Cormier <jcormier@xxxxxxxxxxxxxxxx>
Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 1ad711f8d2a8..b04419b24863 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -700,8 +700,8 @@ 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 */
- dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & mask);
+ /* clear the irqstatus for irqs that are being disabled now */
+ dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & old_mask);
dispc_k2g_vp_set_irqenable(dispc, 0, mask);
dispc_k2g_vid_set_irqenable(dispc, 0, mask);
@@ -843,8 +843,8 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
old_mask = dispc_k3_read_irqenable(dispc);
- /* clear the irqstatus for newly enabled irqs */
- dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & mask);
+ /* clear the irqstatus for irqs that are being disabled now */
+ dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & old_mask);
for (i = 0; i < dispc->feat->num_vps; ++i) {
dispc_k3_vp_set_irqenable(dispc, i, mask);
