Re: [PATCH] media: rkisp1: request/free irqs in PM runtime ops

Laurent Pinchart
2026-01-06T16:04:12Z

On Tue, Jan 06, 2026 at 06:01:38PM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 05/01/2026 18:19, Daniel Scally wrote:
> > The rkisp1 driver tracks whether the IRQ handlers should take any
> > action through the use of an "irqs_enabled" flag which is set true
> > at the end of .runtime_resume() and false at the start of
> > .runtime_suspend(). In .runtime_suspend(), after setting the flag,
> > there's then a short window during which the hardware has not yet
> > been disabled by the clock APIs but interrupts remain enabled. If an
> > interrupt is triggered during that window the IRQ handlers will return
> > IRQ_NONE and fail to clear the ISP's IRQ reset registers.
> >
> > Instead, delay calling request_irq() from .probe() to the end of the
> > .runtime_resume() callback, and call free_irq() at the start of the
> > .runtime_suspend() callback. This will prevent the interrupt handlers
> > being called at all for the device once .runtime_suspend() has been
> > called for it.
>
> Shouldn't we usually always properly disable the IP before suspend? I've
> seen IPs that definitely did not like at all cutting the clocks
> arbitrarily when it's active.

Yes we should. The driver should have real system suspend/resume
handlers that stop and restart streaming.

> And I don't think clk disable should be considered "disabling the
> hardware". The clocks may not even be disabled at that time, if they're
> shared clocks or always on clocks. So if the driver assumes that after
> clk disable call it won't get any interrupts... I don't think that right.
>
> If we can't sensibly disable the IP, I think we can at least mask the
> IP's interrupts in its interrupt enable register (which we probably
> should do even if we can disable the IP), and wait for any possible irq
> handler to stop running. After that we won't get any interrupts,
> regardless of the clocks.
>
> Tomi
>
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > We noticed this problem when testing hibernation with the streams
> > running. In a typical use-case the stream might be stopped before the
> > runtime PM suspend callback is called, and so the problem is avoided,
> > but when hibernated with active streams there are sufficient interrupts
> > coming in to reliably land one in the window between the irqs_enabled
> > flag being set to false and the hardware being disabled through
> > clk_bulk_disable_unprepare().
> >
> > I'm under the impression that requesting the IRQs when the device is
> > in use in the manner of this patch and releasing the when it is not
> > in use is preferred over requesting them in the .probe() function -
> > possibly an impression I picked up from Linux Device Drivers. It
> > doesn't seem to be a particularly common model though, so I thought
> > I'd flag the method here.
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-capture.c | 3 -
> > .../media/platform/rockchip/rkisp1/rkisp1-common.h | 2 -
> > .../media/platform/rockchip/rkisp1/rkisp1-csi.c | 3 -
> > .../media/platform/rockchip/rkisp1/rkisp1-dev.c | 82 +++++++++++++++-------
> > .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 3 -
> > 5 files changed, 57 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
> > unsigned int i;
> > u32 status;
> >
> > - if (!rkisp1->irqs_enabled)
> > - return IRQ_NONE;
> > -
> > status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
> > if (!status)
> > return IRQ_NONE;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -507,7 +507,6 @@ struct rkisp1_debug {
> > * @debug: debug params to be exposed on debugfs
> > * @info: version-specific ISP information
> > * @irqs: IRQ line numbers
> > - * @irqs_enabled: the hardware is enabled and can cause interrupts
> > */
> > struct rkisp1_device {
> > void __iomem *base_addr;
> > @@ -532,7 +531,6 @@ struct rkisp1_device {
> > struct rkisp1_debug debug;
> > const struct rkisp1_info *info;
> > int irqs[RKISP1_NUM_IRQS];
> > - bool irqs_enabled;
> > };
> >
> > /*
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > @@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
> > struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > u32 val, status;
> >
> > - if (!rkisp1->irqs_enabled)
> > - return IRQ_NONE;
> > -
> > status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
> > if (!status)
> > return IRQ_NONE;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
> > * Power
> > */
> >
> > -static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> > +static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
> > {
> > - struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > + for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
> > + if (rkisp1->irqs[i] == -1)
> > + continue;
> >
> > - rkisp1->irqs_enabled = false;
> > - /* Make sure the IRQ handler will see the above */
> > - mb();
> > + if (irq_has_action(rkisp1->irqs[i]))
> > + free_irq(rkisp1->irqs[i], rkisp1->dev);
> > + }
> > +}
> >
> > - /*
> > - * Wait until any running IRQ handler has returned. The IRQ handler
> > - * may get called even after this (as it's a shared interrupt line)
> > - * but the 'irqs_enabled' flag will make the handler return immediately.
> > - */
> > - for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
> > - if (rkisp1->irqs[il] == -1)
> > +static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
> > +{
> > + const struct rkisp1_info *info = rkisp1->info;
> > + int ret;
> > +
> > + for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
> > + unsigned int isrn;
> > +
> > + if (rkisp1->irqs[irqn] == -1)
> > continue;
> >
> > - /* Skip if the irq line is the same as previous */
> > - if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
> > - synchronize_irq(rkisp1->irqs[il]);
> > + if (irq_has_action(rkisp1->irqs[irqn]))
> > + continue;
> > +
> > + for (isrn = 0; isrn < info->isr_size; isrn++)
> > + if ((info->isrs[isrn].line_mask & BIT(irqn)))
> > + break;
> > +
> > + if (isrn == info->isr_size) {
> > + dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
> > + IRQF_SHARED, dev_driver_string(rkisp1->dev),
> > + rkisp1->dev);
> > + if (ret) {
> > + dev_err(rkisp1->dev, "Failed to request IRQ\n");
> > + goto err_free_irqs;
> > + }
> > }
> >
> > + return 0;
> > +
> > +err_free_irqs:
> > + rkisp1_free_irqs(rkisp1);
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> > +{
> > + struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > +
> > + rkisp1_free_irqs(rkisp1);
> > +
> > clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> > return pinctrl_pm_select_sleep_state(dev);
> > }
> > @@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > - rkisp1->irqs_enabled = true;
> > - /* Make sure the IRQ handler will see the above */
> > - mb();
> > + ret = rkisp1_request_irqs(rkisp1);
> > + if (ret)
> > + goto err_clk_disable;
> >
> > return 0;
> > +
> > +err_clk_disable:
> > + clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> > +
> > + return ret;
> > }
> >
> > static const struct dev_pm_ops rkisp1_pm_ops = {
> > @@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
> > if (info->isrs[i].line_mask & BIT(il))
> > rkisp1->irqs[il] = irq;
> > }
> > -
> > - ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
> > - dev_driver_string(dev), dev);
> > - if (ret) {
> > - dev_err(dev, "request irq failed: %d\n", ret);
> > - return ret;
> > - }
> > }
> >
> > ret = rkisp1_init_clocks(rkisp1);
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> > struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > u32 status, isp_err;
> >
> > - if (!rkisp1->irqs_enabled)
> > - return IRQ_NONE;
> > -
> > status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
> > if (!status)
> > return IRQ_NONE;
> >
> > ---
> > base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
> > change-id: 20260105-rkisp1-irqs-8af5a1e0b887

--
Regards,

Laurent Pinchart