These fix a few IRQ related issues I noticed when testing i.MX8MP. This
series, as of v3, is based on linux-media-stage, and does not include
i.MX8MP support, as that is not in upstream yet.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Changes in v3:
- Rebase on linux-media-stage
- Drop i.MX8MP support (which is a one line change) so that this can be
based on upstream. I have still kept Adam's tested-by for
imx8mp-beacon, as dropping the i.MX8MP support doesn't really affect the
changes.
- Use ARRAY_SIZE() in patch 3
- Link to v2: https://lore.kernel.org/r/[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 | 35 ++++++++++++++++------
.../media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++--
4 files changed, 66 insertions(+), 14 deletions(-)
---
base-commit: 8016943b5947cd485078e23899945c51e818aa63
change-id: 20231205-rkisp-irq-fix-e123a8a6732f
Best regards,
--
Tomi Valkeinen <[email protected]>
In all known platforms the ISP has dedicated IRQ lines, but for some
reason the driver uses IRQF_SHARED.
Supporting IRQF_SHARED properly requires handling interrupts even when
our device is disabled, and the driver does not handle this. To avoid
adding such code, and to be sure the driver won't accidentally be used
in a platform with shared interrupts, let's drop the IRQF_SHARED flag.
Reviewed-by: Laurent Pinchart <[email protected]>
Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index c41abd2833f1..4c4514e20673 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -542,7 +542,7 @@ static int rkisp1_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
- ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
+ ret = devm_request_irq(dev, irq, info->isrs[i].isr, 0,
dev_driver_string(dev), dev);
if (ret) {
dev_err(dev, "request irq failed: %d\n", ret);
--
2.34.1
Store the IRQ lines used by the driver for easy access. These are needed
in future patches which fix IRQ race issues.
Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++++++-
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 17 +++++++++++++----
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 1e7cea1bea5e..2d7f06281c39 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -61,6 +61,14 @@ struct dentry;
RKISP1_CIF_ISP_EXP_END | \
RKISP1_CIF_ISP_HIST_MEASURE_RDY)
+/* IRQ lines */
+enum rkisp1_irq_line {
+ RKISP1_IRQ_ISP = 0,
+ RKISP1_IRQ_MI,
+ RKISP1_IRQ_MIPI,
+ RKISP1_NUM_IRQS,
+};
+
/* enum for the resizer pads */
enum rkisp1_rsz_pad {
RKISP1_RSZ_PAD_SINK,
@@ -423,7 +431,6 @@ struct rkisp1_debug {
* struct rkisp1_device - ISP platform device
*
* @base_addr: base register address
- * @irq: the irq number
* @dev: a pointer to the struct device
* @clk_size: number of clocks
* @clks: array of clocks
@@ -441,6 +448,7 @@ struct rkisp1_debug {
* @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
* @debug: debug params to be exposed on debugfs
* @info: version-specific ISP information
+ * @irqs: IRQ line numbers
*/
struct rkisp1_device {
void __iomem *base_addr;
@@ -461,6 +469,7 @@ struct rkisp1_device {
struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */
struct rkisp1_debug debug;
const struct rkisp1_info *info;
+ int irqs[RKISP1_NUM_IRQS];
};
/*
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 22b2ae0e89c4..c3fa40976140 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -114,6 +114,7 @@
struct rkisp1_isr_data {
const char *name;
irqreturn_t (*isr)(int irq, void *ctx);
+ u32 line_mask;
};
/* ----------------------------------------------------------------------------
@@ -471,9 +472,9 @@ static const char * const px30_isp_clks[] = {
};
static const struct rkisp1_isr_data px30_isp_isrs[] = {
- { "isp", rkisp1_isp_isr },
- { "mi", rkisp1_capture_isr },
- { "mipi", rkisp1_csi_isr },
+ { "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
+ { "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
+ { "mipi", rkisp1_csi_isr, BIT(RKISP1_IRQ_MIPI) },
};
static const struct rkisp1_info px30_isp_info = {
@@ -492,7 +493,7 @@ static const char * const rk3399_isp_clks[] = {
};
static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
- { NULL, rkisp1_isr },
+ { NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
};
static const struct rkisp1_info rk3399_isp_info = {
@@ -543,6 +544,9 @@ static int rkisp1_probe(struct platform_device *pdev)
if (IS_ERR(rkisp1->base_addr))
return PTR_ERR(rkisp1->base_addr);
+ for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il)
+ rkisp1->irqs[il] = -1;
+
for (i = 0; i < info->isr_size; i++) {
irq = info->isrs[i].name
? platform_get_irq_byname(pdev, info->isrs[i].name)
@@ -550,6 +554,11 @@ static int rkisp1_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
+ for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
+ if (info->isrs[i].line_mask & BIT(il))
+ rkisp1->irqs[il] = irq;
+ }
+
ret = devm_request_irq(dev, irq, info->isrs[i].isr, 0,
dev_driver_string(dev), dev);
if (ret) {
--
2.34.1
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.
Reviewed-by: Laurent Pinchart <[email protected]>
Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
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 e6642a2167ef..b6e47e2f1b94 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 ca6703bfd27b..29b538a5e1bb 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -254,11 +254,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
The IRQ handler rkisp1_isr() calls sub-handlers, all of which returns an
irqreturn_t value, but rkisp1_isr() ignores those values and always
returns IRQ_HANDLED.
Fix this by collecting the return values, and returning IRQ_HANDLED or
IRQ_NONE as appropriate.
Reviewed-by: Laurent Pinchart <[email protected]>
Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 4c4514e20673..22b2ae0e89c4 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -442,17 +442,25 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
static irqreturn_t rkisp1_isr(int irq, void *ctx)
{
+ irqreturn_t ret = IRQ_NONE;
+
/*
* Call rkisp1_capture_isr() first to handle the frame that
* potentially completed using the current frame_sequence number before
* it is potentially incremented by rkisp1_isp_isr() in the vertical
* sync.
*/
- rkisp1_capture_isr(irq, ctx);
- rkisp1_isp_isr(irq, ctx);
- rkisp1_csi_isr(irq, ctx);
- return IRQ_HANDLED;
+ if (rkisp1_capture_isr(irq, ctx) == IRQ_HANDLED)
+ ret = IRQ_HANDLED;
+
+ if (rkisp1_isp_isr(irq, ctx) == IRQ_HANDLED)
+ ret = IRQ_HANDLED;
+
+ if (rkisp1_csi_isr(irq, ctx) == IRQ_HANDLED)
+ ret = IRQ_HANDLED;
+
+ return ret;
}
static const char * const px30_isp_clks[] = {
--
2.34.1
Hi Tomi,
Thank you for the patch.
On Thu, Dec 07, 2023 at 09:57:47AM +0200, Tomi Valkeinen wrote:
> Store the IRQ lines used by the driver for easy access. These are needed
> in future patches which fix IRQ race issues.
>
> Tested-by: Adam Ford <[email protected]> #imx8mp-beacon
> Signed-off-by: Tomi Valkeinen <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++++++-
> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 17 +++++++++++++----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 1e7cea1bea5e..2d7f06281c39 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -61,6 +61,14 @@ struct dentry;
> RKISP1_CIF_ISP_EXP_END | \
> RKISP1_CIF_ISP_HIST_MEASURE_RDY)
>
> +/* IRQ lines */
> +enum rkisp1_irq_line {
> + RKISP1_IRQ_ISP = 0,
> + RKISP1_IRQ_MI,
> + RKISP1_IRQ_MIPI,
> + RKISP1_NUM_IRQS,
> +};
> +
> /* enum for the resizer pads */
> enum rkisp1_rsz_pad {
> RKISP1_RSZ_PAD_SINK,
> @@ -423,7 +431,6 @@ struct rkisp1_debug {
> * struct rkisp1_device - ISP platform device
> *
> * @base_addr: base register address
> - * @irq: the irq number
> * @dev: a pointer to the struct device
> * @clk_size: number of clocks
> * @clks: array of clocks
> @@ -441,6 +448,7 @@ struct rkisp1_debug {
> * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
> * @debug: debug params to be exposed on debugfs
> * @info: version-specific ISP information
> + * @irqs: IRQ line numbers
> */
> struct rkisp1_device {
> void __iomem *base_addr;
> @@ -461,6 +469,7 @@ struct rkisp1_device {
> struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */
> struct rkisp1_debug debug;
> const struct rkisp1_info *info;
> + int irqs[RKISP1_NUM_IRQS];
> };
>
> /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 22b2ae0e89c4..c3fa40976140 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -114,6 +114,7 @@
> struct rkisp1_isr_data {
> const char *name;
> irqreturn_t (*isr)(int irq, void *ctx);
> + u32 line_mask;
> };
>
> /* ----------------------------------------------------------------------------
> @@ -471,9 +472,9 @@ static const char * const px30_isp_clks[] = {
> };
>
> static const struct rkisp1_isr_data px30_isp_isrs[] = {
> - { "isp", rkisp1_isp_isr },
> - { "mi", rkisp1_capture_isr },
> - { "mipi", rkisp1_csi_isr },
> + { "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
> + { "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
> + { "mipi", rkisp1_csi_isr, BIT(RKISP1_IRQ_MIPI) },
> };
>
> static const struct rkisp1_info px30_isp_info = {
> @@ -492,7 +493,7 @@ static const char * const rk3399_isp_clks[] = {
> };
>
> static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
> - { NULL, rkisp1_isr },
> + { NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
> };
>
> static const struct rkisp1_info rk3399_isp_info = {
> @@ -543,6 +544,9 @@ static int rkisp1_probe(struct platform_device *pdev)
> if (IS_ERR(rkisp1->base_addr))
> return PTR_ERR(rkisp1->base_addr);
>
> + for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il)
> + rkisp1->irqs[il] = -1;
> +
> for (i = 0; i < info->isr_size; i++) {
> irq = info->isrs[i].name
> ? platform_get_irq_byname(pdev, info->isrs[i].name)
> @@ -550,6 +554,11 @@ static int rkisp1_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> + for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
> + if (info->isrs[i].line_mask & BIT(il))
> + rkisp1->irqs[il] = irq;
> + }
> +
> ret = devm_request_irq(dev, irq, info->isrs[i].isr, 0,
> dev_driver_string(dev), dev);
> if (ret) {
>
--
Regards,
Laurent Pinchart