Hi Tomi, Thank you for the patch. On Wed, Dec 06, 2023 at 12:12:31PM +0200, Tomi Valkeinen wrote: > In rkisp1_isp_stop() and rkisp1_csi_disable() the driver masks the > interrupts and then apparently assumes that the interrupt handler won't > be running, and proceeds in the stop procedure. This is not the case, as > the interrupt handler can already be running, which would lead to the > ISP being disabled while the interrupt handler handling a captured > frame. > > This brings up two issues: 1) the ISP could be powered off while the > interrupt handler is still running and accessing registers, leading to > board lockup, and 2) the interrupt handler code and the code that > disables the streaming might do things that conflict. > > It is not clear to me if 2) causes a real issue, but 1) can be seen with > a suitable delay (or printk in my case) in the interrupt handler, > leading to board lockup. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > --- > drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++++++++- > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++++++++--- > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c > index 47f4353a1784..0bab3303f2e4 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c > @@ -125,8 +125,20 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi) > struct rkisp1_device *rkisp1 = csi->rkisp1; > u32 val; > > - /* Mask and clear interrupts. */ > + /* Mask MIPI interrupts. */ > rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMSC, 0); > + > + /* Flush posted writes */ > + rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC); > + > + /* > + * Wait until the IRQ handler has ended. The IRQ handler may get called > + * even after this, but it will return immediately as the MIPI > + * interrupts have been masked. > + */ > + synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MIPI]); > + > + /* Clear MIPI interrupt status */ > rkisp1_write(rkisp1, RKISP1_CIF_MIPI_ICR, ~0); > > val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_CTRL); > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > index dafbfd230542..33b5a714d117 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > @@ -364,11 +364,25 @@ static void rkisp1_isp_stop(struct rkisp1_isp *isp) > * ISP(mi) stop in mi frame end -> Stop ISP(mipi) -> > * Stop ISP(isp) ->wait for ISP isp off > */ > - /* stop and clear MI and ISP interrupts */ > - rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0); > - rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0); > > + /* Mask MI and ISP interrupts */ > + rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0); > rkisp1_write(rkisp1, RKISP1_CIF_MI_IMSC, 0); > + > + /* Flush posted writes */ > + rkisp1_read(rkisp1, RKISP1_CIF_MI_IMSC); > + > + /* > + * Wait until the IRQ handler has ended. The IRQ handler may get called > + * even after this, but it will return immediately as the MI and ISP > + * interrupts have been masked. > + */ > + synchronize_irq(rkisp1->irqs[RKISP1_IRQ_ISP]); > + if (rkisp1->irqs[RKISP1_IRQ_ISP] != rkisp1->irqs[RKISP1_IRQ_MI]) > + synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MI]); > + > + /* Clear MI and ISP interrupt status */ > + rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0); > rkisp1_write(rkisp1, RKISP1_CIF_MI_ICR, ~0); > > /* stop ISP */ -- Regards, Laurent Pinchart
