These fix a few IRQ related issues I noticed when testing i.MX8MP. These
are based on Paul's recently sent "[PATCH v4 00/11] media: rkisp1: Add
support for i.MX8MP" series, but could also be rebased on top of
mainline if needed.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Changes in v2:
- New patch: "media: rkisp1: Drop IRQF_SHARED"
- Update "media: rkisp1: Fix IRQ handler return values" according to
Laurent's comment.
- Drop "media: rkisp1: Fix IRQ handling due to shared interrupts"
- Update description for "media: rkisp1: Fix IRQ disable race issue"
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Tomi Valkeinen (4):
media: rkisp1: Drop IRQF_SHARED
media: rkisp1: Fix IRQ handler return values
media: rkisp1: Store IRQ lines
media: rkisp1: Fix IRQ disable race issue
.../media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++-
.../media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++-
.../media/platform/rockchip/rkisp1/rkisp1-dev.c | 37 ++++++++++++++++------
.../media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 ++++++++++--
4 files changed, 67 insertions(+), 15 deletions(-)
---
base-commit: dd19f89b915c203d49e3b23ca02446d4fb05d955
change-id: 20231205-rkisp-irq-fix-e123a8a6732f
Best regards,
--
Tomi Valkeinen <[email protected]>
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 <[email protected]>
---
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 */
--
2.34.1
On Wed, Dec 6, 2023 at 4:12 AM Tomi Valkeinen
<[email protected]> wrote:
>
> These fix a few IRQ related issues I noticed when testing i.MX8MP. These
> are based on Paul's recently sent "[PATCH v4 00/11] media: rkisp1: Add
> support for i.MX8MP" series, but could also be rebased on top of
> mainline if needed.
>
I applied the whole series on top of your DMA patch and the series
from Paul porting the rkisp1 to the imx8mp and ran the camera for 15
minutes streaming to my monitor. I didn't see any glitches or video
distortion at 640x480.
For the series...
Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> Changes in v2:
> - New patch: "media: rkisp1: Drop IRQF_SHARED"
> - Update "media: rkisp1: Fix IRQ handler return values" according to
> Laurent's comment.
> - Drop "media: rkisp1: Fix IRQ handling due to shared interrupts"
> - Update description for "media: rkisp1: Fix IRQ disable race issue"
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Tomi Valkeinen (4):
> media: rkisp1: Drop IRQF_SHARED
> media: rkisp1: Fix IRQ handler return values
> media: rkisp1: Store IRQ lines
> media: rkisp1: Fix IRQ disable race issue
>
> .../media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++-
> .../media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++-
> .../media/platform/rockchip/rkisp1/rkisp1-dev.c | 37 ++++++++++++++++------
> .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 ++++++++++--
> 4 files changed, 67 insertions(+), 15 deletions(-)
> ---
> base-commit: dd19f89b915c203d49e3b23ca02446d4fb05d955
> change-id: 20231205-rkisp-irq-fix-e123a8a6732f
>
> Best regards,
> --
> Tomi Valkeinen <[email protected]>
>
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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