| Message ID | 1510275907-993-3-git-send-email-stschake@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Stefan Schake <stschake@gmail.com> writes: > The 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. 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.
On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote: > Stefan Schake <stschake@gmail.com> writes: > >> The 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. > > 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. 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.
Stefan Schake <stschake@gmail.com> writes: > On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote: >> Stefan Schake <stschake@gmail.com> writes: >> >>> The 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. >> >> 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. 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.
On Tue, Nov 14, 2017 at 8:44 PM, Eric Anholt <eric@anholt.net> wrote: > Stefan Schake <stschake@gmail.com> writes: > >> On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote: >>> Stefan Schake <stschake@gmail.com> writes: >>> >>>> The 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. >>> >>> 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. > > 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
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c index 61b2e53..7d780149d 100644 --- a/drivers/gpu/drm/vc4/vc4_irq.c +++ b/drivers/gpu/drm/vc4/vc4_irq.c @@ -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. */
The 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. Signed-off-by: Stefan Schake <stschake@gmail.com> --- drivers/gpu/drm/vc4/vc4_irq.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)