drm/vc4: Correctly uninstall interrupts
Subsystems (3)
cover drm/vc4: Correctly uninstall interrupts
This set of patches fixes issues with vc4_irq_uninstall. The first patch fixes a NULL pointer dereference when the binner BO would disappear during an in flight overflow mem work callback.
The second patch ensures we return with all interrupts disabled. This was suspected to cause the NULL dereference but turned out to be unrelated.
Tested with a Raspberry Pi CM 3 that was previously stuck in a boot loop due to the issue. With the patch applied, the NULL dereference was no longer observed through numerous resets.
Stefan Schake (2):
drm/vc4: Account for interrupts in flight
drm/vc4: Ensure interrupts are disabled
drivers/gpu/drm/vc4/vc4_irq.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/2 drm/vc4: Account for interrupts in flight
Synchronously disable the IRQ to make the following cancel_work_sync invocation effective.
An interrupt in flight could enqueue further overflow mem work. As we free the binner BO immediately following vc4_irq_uninstall this caused a NULL pointer dereference in the work callback vc4_overflow_mem_work.
--- drivers/gpu/drm/vc4/vc4_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/gpu/drm/vc4/vc4_irq.c +6 -0
@@ -208,6 +208,9 @@ {struct vc4_dev *vc4 =to_vc4_dev (dev); +/* Undo the effects of a previous vc4_irq_uninstall. */ +enable_irq (dev->irq); +/* Enable both the render done and out of memory interrupts. */ V3D_WRITE (V3D_INTENA ,V3D_DRIVER_IRQS );
@@ -225,6 +228,9 @@/* Clear any pending interrupts we might have left. */ V3D_WRITE (V3D_INTCTL ,V3D_DRIVER_IRQS ); +/* Finish any interrupt handler still in flight. */ +disable_irq (dev->irq); +cancel_work_sync (&vc4->overflow_mem_work); }
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/vc4: Account for interrupts in flight
Quoted 10 lines · Stefan Schake
Synchronously disable the IRQ to make the following
cancel_work_syncinvocation effective.An interrupt in flight could enqueue further overflow mem work. As we free the binner BO immediately following
vc4_irq_uninstallthis caused a NULL pointer dereference in the work callbackvc4_overflow_mem_work.Signed-off-by Stefan Schake <[email protected]>Fixes d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
Reviewed and applied this one. Thanks!
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/2 drm/vc4: Ensure interrupts are disabled
In reply to Stefan SchakeThe overflow mem work callback vc4_overflow_mem_work reenables its associated interrupt upon completion. To ensure all interrupts are disabled when we return from vc4_irq_uninstall, we need to disable it again if cancel_work_sync indicated pending work.
--- drivers/gpu/drm/vc4/vc4_irq.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/gpu/drm/vc4/vc4_irq.c +8 -1
@@ -231,7 +231,14 @@/* Finish any interrupt handler still in flight. */ disable_irq (dev->irq); -cancel_work_sync (&vc4->overflow_mem_work); +if (cancel_work_sync (&vc4->overflow_mem_work)) { +/* +* Work was still pending. The overflow mem work's +* callback reenables the OUTOMEM interrupt upon +* completion, so ensure it is disabled here. +*/ +V3D_WRITE (V3D_INTDIS ,V3D_INT_OUTOMEM ); + } }/** Reinitializes interrupt registers when a GPU reset is performed. */
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
Quoted 4 lines · Stefan Schake
The overflow mem work callback
vc4_overflow_mem_workreenables its associated interrupt upon completion. To ensure all interrupts are disabled when we return fromvc4_irq_uninstall, we need to disable it again ifcancel_work_syncindicated pending work.
Is there a reason we need the interrupts disabled at the V3D level while we have the IRQ disabled at the irqchip level? Once we re-enable at the irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
Quoted 10 lines · Eric Anholt
Quoted 4 lines · Stefan Schake
The overflow mem work callback
vc4_overflow_mem_workreenables its associated interrupt upon completion. To ensure all interrupts are disabled when we return fromvc4_irq_uninstall, we need to disable it again ifcancel_work_syncindicated pending work.Is there a reason we need the interrupts disabled at the V3D level while we have the IRQ disabled at the irqchip level? Once we re-enable at the irqchip, we immediately
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS)anyway.
irqchip will mask it in the ARM interrupt controller, so we will certainly never see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and V3D_INTCTL make - does the state in INTENA affect if V3D will signal an interrupt in INTCTL? We're not currently clearing the latter in postinstall.
Quoted 1 line
From a practical perspective, we're not doing anything in between uninstall
and postinstall that would trigger the interrupt. So in that sense it's certainly
superfluous.
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
Quoted 8 lines · Stefan Schake
Quoted 7 lines · Eric Anholt
Quoted 4 lines · Stefan Schake
The overflow mem work callback
vc4_overflow_mem_workreenables its associated interrupt upon completion. To ensure all interrupts are disabled when we return fromvc4_irq_uninstall, we need to disable it again ifcancel_work_syncindicated pending work.
Quoted 8 lines
Quoted 3 lines
Is there a reason we need the interrupts disabled at the V3D level while we have the IRQ disabled at the irqchip level? Once we re-enable at the irqchip, we immediately
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS)anyway.irqchip will mask it in the ARM interrupt controller, so we will certainly never see an interrupt. I'm not sure on the exact guarantees
V3D_INTENAandV3D_INTCTLmake - does the state in INTENA affect if V3D will signal an interrupt in INTCTL? We're not currently clearing the latter in postinstall.
INTENA/INTDIS writes update the state of the single register that controls which bits of INTCTL get ORed together to raise the interrupt outside the V3D block.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
Quoted 14 lines · Eric Anholt
Quoted 12 lines · Stefan Schake
Quoted 10 lines · Eric Anholt
Quoted 4 lines · Stefan Schake
The overflow mem work callback
vc4_overflow_mem_workreenables its associated interrupt upon completion. To ensure all interrupts are disabled when we return fromvc4_irq_uninstall, we need to disable it again ifcancel_work_syncindicated pending work.Is there a reason we need the interrupts disabled at the V3D level while we have the IRQ disabled at the irqchip level? Once we re-enable at the irqchip, we immediately
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS)anyway.
Quoted 8 lines
Quoted 4 lines
irqchip will mask it in the ARM interrupt controller, so we will certainly never see an interrupt. I'm not sure on the exact guarantees
V3D_INTENAandV3D_INTCTLmake - does the state in INTENA affect if V3D will signal an interrupt in INTCTL? We're not currently clearing the latter in postinstall.INTENA/INTDIS writes update the state of the single register that controls which bits of INTCTL get ORed together to raise the interrupt outside the V3D block.
Then I certainly agree - this patch doesn't do anything and should be dropped. Good call!
Thanks,
Stefan
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel