* [PATCH v2 1/4] media: rkisp1: Drop IRQF_SHARED
2023-12-06 10:12 [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Tomi Valkeinen
@ 2023-12-06 10:12 ` Tomi Valkeinen
2023-12-06 20:51 ` Laurent Pinchart
2023-12-06 10:12 ` [PATCH v2 2/4] media: rkisp1: Fix IRQ handler return values Tomi Valkeinen
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2023-12-06 10:12 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Paul Elder
Cc: Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Tomi Valkeinen
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.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
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 2b9886fd0800..d4950294b7b9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -573,7 +573,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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] media: rkisp1: Drop IRQF_SHARED
2023-12-06 10:12 ` [PATCH v2 1/4] media: rkisp1: Drop IRQF_SHARED Tomi Valkeinen
@ 2023-12-06 20:51 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-12-06 20:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner,
Paul Elder, Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel
Hi Tomi,
Thank you for the patch.
On Wed, Dec 06, 2023 at 12:12:28PM +0200, Tomi Valkeinen wrote:
> 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.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 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 2b9886fd0800..d4950294b7b9 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -573,7 +573,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);
>
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] media: rkisp1: Fix IRQ handler return values
2023-12-06 10:12 [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Tomi Valkeinen
2023-12-06 10:12 ` [PATCH v2 1/4] media: rkisp1: Drop IRQF_SHARED Tomi Valkeinen
@ 2023-12-06 10:12 ` Tomi Valkeinen
2023-12-06 10:12 ` [PATCH v2 3/4] media: rkisp1: Store IRQ lines Tomi Valkeinen
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Tomi Valkeinen @ 2023-12-06 10:12 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Paul Elder
Cc: Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Tomi Valkeinen
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.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
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 d4950294b7b9..030eb8c79546 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -444,17 +444,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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 3/4] media: rkisp1: Store IRQ lines
2023-12-06 10:12 [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Tomi Valkeinen
2023-12-06 10:12 ` [PATCH v2 1/4] media: rkisp1: Drop IRQF_SHARED Tomi Valkeinen
2023-12-06 10:12 ` [PATCH v2 2/4] media: rkisp1: Fix IRQ handler return values Tomi Valkeinen
@ 2023-12-06 10:12 ` Tomi Valkeinen
2023-12-06 22:05 ` Laurent Pinchart
2023-12-06 10:12 ` [PATCH v2 4/4] media: rkisp1: Fix IRQ disable race issue Tomi Valkeinen
2023-12-06 11:43 ` [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Adam Ford
4 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2023-12-06 10:12 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Paul Elder
Cc: Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Tomi Valkeinen
Store the IRQ lines used by the driver for easy access. These are needed
in future patches which fix IRQ race issues.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
.../media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++++++-
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 19 ++++++++++++++-----
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 960ab89c659b..ec28907d978e 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -62,6 +62,14 @@ struct regmap;
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,
@@ -437,7 +445,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
@@ -457,6 +464,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;
@@ -479,6 +487,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 030eb8c79546..492ff5e6770d 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -115,6 +115,7 @@
struct rkisp1_isr_data {
const char *name;
irqreturn_t (*isr)(int irq, void *ctx);
+ u32 line_mask;
};
/* ----------------------------------------------------------------------------
@@ -473,9 +474,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 = {
@@ -496,7 +497,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 = {
@@ -517,7 +518,7 @@ static const char * const imx8mp_isp_clks[] = {
};
static const struct rkisp1_isr_data imx8mp_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 imx8mp_isp_info = {
@@ -574,6 +575,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 < RKISP1_NUM_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)
@@ -581,6 +585,11 @@ static int rkisp1_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
+ for (unsigned int il = 0; il < RKISP1_NUM_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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/4] media: rkisp1: Store IRQ lines
2023-12-06 10:12 ` [PATCH v2 3/4] media: rkisp1: Store IRQ lines Tomi Valkeinen
@ 2023-12-06 22:05 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-12-06 22:05 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner,
Paul Elder, Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel
Hi Tomi,
Thank you for the patch.
On Wed, Dec 06, 2023 at 12:12:30PM +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.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> .../media/platform/rockchip/rkisp1/rkisp1-common.h | 11 ++++++++++-
> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 19 ++++++++++++++-----
> 2 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 960ab89c659b..ec28907d978e 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -62,6 +62,14 @@ struct regmap;
> 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,
> @@ -437,7 +445,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
> @@ -457,6 +464,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;
> @@ -479,6 +487,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 030eb8c79546..492ff5e6770d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -115,6 +115,7 @@
> struct rkisp1_isr_data {
> const char *name;
> irqreturn_t (*isr)(int irq, void *ctx);
> + u32 line_mask;
> };
>
> /* ----------------------------------------------------------------------------
> @@ -473,9 +474,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 = {
> @@ -496,7 +497,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 = {
> @@ -517,7 +518,7 @@ static const char * const imx8mp_isp_clks[] = {
> };
>
> static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
> - { NULL, rkisp1_isr },
> + { NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
The i.MX8MP has no CSI-2 RX in the ISP, you can drop RKISP1_IRQ_MIPI.
I think we can merge this series before the i.MX8MP support, could you
base v3 on top of the master branch of the linux-media stage tree ?
> };
>
> static const struct rkisp1_info imx8mp_isp_info = {
> @@ -574,6 +575,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 < RKISP1_NUM_IRQS; ++il)
I would use ARRAY_SIZE(rkisp1->irqs) instead of RKISP1_NUM_IRQS here.
> + 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)
> @@ -581,6 +585,11 @@ static int rkisp1_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> + for (unsigned int il = 0; il < RKISP1_NUM_IRQS; ++il) {
Same here.
> + 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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] media: rkisp1: Fix IRQ disable race issue
2023-12-06 10:12 [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Tomi Valkeinen
` (2 preceding siblings ...)
2023-12-06 10:12 ` [PATCH v2 3/4] media: rkisp1: Store IRQ lines Tomi Valkeinen
@ 2023-12-06 10:12 ` Tomi Valkeinen
2023-12-06 22:13 ` Laurent Pinchart
2023-12-06 11:43 ` [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Adam Ford
4 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2023-12-06 10:12 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Paul Elder
Cc: Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Tomi Valkeinen
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@ideasonboard.com>
---
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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 4/4] media: rkisp1: Fix IRQ disable race issue
2023-12-06 10:12 ` [PATCH v2 4/4] media: rkisp1: Fix IRQ disable race issue Tomi Valkeinen
@ 2023-12-06 22:13 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2023-12-06 22:13 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner,
Paul Elder, Alexander Stein, kieran.bingham, umang.jain, aford173,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel
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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues
2023-12-06 10:12 [PATCH v2 0/4] media: rkisp1: Fix IRQ related issues Tomi Valkeinen
` (3 preceding siblings ...)
2023-12-06 10:12 ` [PATCH v2 4/4] media: rkisp1: Fix IRQ disable race issue Tomi Valkeinen
@ 2023-12-06 11:43 ` Adam Ford
4 siblings, 0 replies; 9+ messages in thread
From: Adam Ford @ 2023-12-06 11:43 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Paul Elder, Alexander Stein, kieran.bingham,
umang.jain, linux-media, linux-rockchip, linux-arm-kernel,
linux-kernel
On Wed, Dec 6, 2023 at 4:12 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> 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 <aford173@gmail.com> #imx8mp-beacon
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> 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/20231205-rkisp-irq-fix-v1-0-f4045c74ba45@ideasonboard.com
>
> ---
> 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 <tomi.valkeinen@ideasonboard.com>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 9+ messages in thread