[PATCH v2] media: mali-c55: Keep IRQ requested during suspend

[PATCH v2] media: mali-c55: Keep IRQ requested during suspend

[Linus Walleij]

The interrupt is currently freed on every runtime suspend and requested
again on runtime resume. Apart from tying interrupt ownership to the
power state rather than to the driver lifetime, this leaves remove to
guess whether an action is installed for the IRQ.

Request the interrupt once during probe and free it during remove.

Disable and synchronize the IRQ before powering the ISP off, and enable
it only after a successful power-on.

Hold a runtime PM reference during probe until the IRQ is installed,
and stop runtime PM and drain the IRQ before unregistering the media
entities during remove.

Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc: stable@vger.kernel.org
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
This is essentially my reply with the "proper fix" for:
https://lore.kernel.org/linux-media/20260903025526.2657105-1-dayou5941@163.com/

Keep the Mali-C55 interrupt requested for the driver's lifetime instead
of changing interrupt ownership along with the runtime power state.
Synchronize and disable the IRQ before power-off, and enable it after
power-on. Hold a PM reference until probe installs the IRQ, and stop
runtime PM and drain the IRQ before removing the media entities.

Tested on the Renesas RZ/V2H board with the other in-flight patches
(DTS changes and the TPG enablement) doing full suspend/resume cycles
with wakeup from RTC and NMI and capturing images before and after
the test.

This patch was authored with the assistance of an LLM.
---
Changes in v2:
- Drop the wakeup-source binding patch and all ISP wakeup handling.
- Drop the patch retaining ISP power for wakeup; keep the IRQ lifetime fix.
- Link to v1: https://patch.msgid.link/20260929-mali-c55-irq-supend-resume-v1-0-e3af34afff12@kernel.org
---
 .../media/platform/arm/mali-c55/mali-c55-core.c    | 44 ++++++++++++----------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
index f28e9f4354ac..3c2816b6a96b 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
@@ -675,8 +675,7 @@ static int __maybe_unused mali_c55_runtime_suspend(struct device *dev)
 {
 	struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
 
-	if (irq_has_action(mali_c55->irqnum))
-		free_irq(mali_c55->irqnum, dev);
+	disable_irq(mali_c55->irqnum);
 	__mali_c55_power_off(mali_c55);
 
 	return 0;
@@ -745,20 +744,9 @@ static int __maybe_unused mali_c55_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	/*
-	 * The driver needs to transfer large amounts of register settings to
-	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
-	 * threaded IRQ to avoid disabling interrupts the entire time that's
-	 * happening.
-	 */
-	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
-				   IRQF_ONESHOT, dev_driver_string(dev), dev);
-	if (ret) {
-		__mali_c55_power_off(mali_c55);
-		dev_err(dev, "failed to request irq\n");
-	}
+	enable_irq(mali_c55->irqnum);
 
-	return ret;
+	return 0;
 }
 
 static const struct dev_pm_ops mali_c55_pm_ops = {
@@ -825,27 +813,42 @@ static int mali_c55_probe(struct platform_device *pdev)
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_get_noresume(dev);
 	pm_runtime_enable(&pdev->dev);
 
 	ret = mali_c55_media_frameworks_init(mali_c55);
 	if (ret)
 		goto err_pm_runtime_disable;
 
-	pm_runtime_idle(&pdev->dev);
-
 	mali_c55->irqnum = platform_get_irq(pdev, 0);
 	if (mali_c55->irqnum < 0) {
 		ret = mali_c55->irqnum;
 		goto err_deinit_media_frameworks;
 	}
 
+	/*
+	 * The driver needs to transfer large amounts of register settings to
+	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
+	 * threaded IRQ to avoid disabling interrupts the entire time that's
+	 * happening.
+	 */
+	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
+				   IRQF_ONESHOT, dev_driver_string(dev), dev);
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		goto err_deinit_media_frameworks;
+	}
+
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
 err_deinit_media_frameworks:
 	mali_c55_media_frameworks_deinit(mali_c55);
 err_pm_runtime_disable:
-	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_set_suspended(&pdev->dev);
 	kfree(mali_c55->context.registers);
 err_power_off:
 	__mali_c55_power_off(mali_c55);
@@ -859,12 +862,13 @@ static void mali_c55_remove(struct platform_device *pdev)
 {
 	struct mali_c55 *mali_c55 = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+	free_irq(mali_c55->irqnum, &pdev->dev);
 	mali_c55_media_frameworks_deinit(mali_c55);
-	if (!pm_runtime_suspended(&pdev->dev)) {
+	if (!pm_runtime_status_suspended(&pdev->dev)) {
 		__mali_c55_power_off(mali_c55);
 		pm_runtime_set_suspended(&pdev->dev);
 	}
-	pm_runtime_disable(&pdev->dev);
 	kfree(mali_c55->context.registers);
 	of_reserved_mem_device_release(&pdev->dev);
 }

---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260918-mali-c55-irq-supend-resume-30b507a20011

Best regards,
--  
Linus Walleij <linusw@kernel.org>

Re: [PATCH v2] media: mali-c55: Keep IRQ requested during suspend

[Linus Walleij]

On Tue, Oct 27, 2026 at 3:08 PM Linus Walleij <linusw@kernel.org> wrote:

> The interrupt is currently freed on every runtime suspend and requested
> again on runtime resume. Apart from tying interrupt ownership to the
> power state rather than to the driver lifetime, this leaves remove to
> guess whether an action is installed for the IRQ.
>
> Request the interrupt once during probe and free it during remove.
>
> Disable and synchronize the IRQ before powering the ISP off, and enable
> it only after a successful power-on.
>
> Hold a runtime PM reference during probe until the IRQ is installed,
> and stop runtime PM and drain the IRQ before unregistering the media
> entities during remove.
>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Walleij <linusw@kernel.org>

The PW bot replies:

Thanks for your patches! Unfortunately the Media CI robot detected some
issues:

# Test virtme64:test-virtme
Error running test. Final Summary not found

I take it that the PW bot has issues with itself (don't we all).
So not my problem?

Jacopo, do you think we can apply this?

Yours,
Linus Walleij

Re: [PATCH v2] media: mali-c55: Keep IRQ requested during suspend

[Jacopo Mondi]

Hi Linus

On Tue, Oct 27, 2026 at 03:08:05PM +0100, Linus Walleij wrote:
> The interrupt is currently freed on every runtime suspend and requested
> again on runtime resume. Apart from tying interrupt ownership to the
> power state rather than to the driver lifetime, this leaves remove to
> guess whether an action is installed for the IRQ.
>
> Request the interrupt once during probe and free it during remove.
>
> Disable and synchronize the IRQ before powering the ISP off, and enable
> it only after a successful power-on.
>
> Hold a runtime PM reference during probe until the IRQ is installed,
> and stop runtime PM and drain the IRQ before unregistering the media
> entities during remove.
>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> This is essentially my reply with the "proper fix" for:
> https://lore.kernel.org/linux-media/20260903025526.2657105-1-dayou5941@163.com/
>
> Keep the Mali-C55 interrupt requested for the driver's lifetime instead
> of changing interrupt ownership along with the runtime power state.
> Synchronize and disable the IRQ before power-off, and enable it after
> power-on. Hold a PM reference until probe installs the IRQ, and stop
> runtime PM and drain the IRQ before removing the media entities.
>
> Tested on the Renesas RZ/V2H board with the other in-flight patches
> (DTS changes and the TPG enablement) doing full suspend/resume cycles
> with wakeup from RTC and NMI and capturing images before and after
> the test.
>
> This patch was authored with the assistance of an LLM.
> ---
> Changes in v2:
> - Drop the wakeup-source binding patch and all ISP wakeup handling.
> - Drop the patch retaining ISP power for wakeup; keep the IRQ lifetime fix.
> - Link to v1: https://patch.msgid.link/20260929-mali-c55-irq-supend-resume-v1-0-e3af34afff12@kernel.org
> ---
>  .../media/platform/arm/mali-c55/mali-c55-core.c    | 44 ++++++++++++----------
>  1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> index f28e9f4354ac..3c2816b6a96b 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -675,8 +675,7 @@ static int __maybe_unused mali_c55_runtime_suspend(struct device *dev)
>  {
>  	struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
>
> -	if (irq_has_action(mali_c55->irqnum))
> -		free_irq(mali_c55->irqnum, dev);
> +	disable_irq(mali_c55->irqnum);
>  	__mali_c55_power_off(mali_c55);
>
>  	return 0;
> @@ -745,20 +744,9 @@ static int __maybe_unused mali_c55_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>
> -	/*
> -	 * The driver needs to transfer large amounts of register settings to
> -	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
> -	 * threaded IRQ to avoid disabling interrupts the entire time that's
> -	 * happening.
> -	 */
> -	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
> -				   IRQF_ONESHOT, dev_driver_string(dev), dev);
> -	if (ret) {
> -		__mali_c55_power_off(mali_c55);
> -		dev_err(dev, "failed to request irq\n");
> -	}
> +	enable_irq(mali_c55->irqnum);
>
> -	return ret;
> +	return 0;
>  }
>
>  static const struct dev_pm_ops mali_c55_pm_ops = {
> @@ -825,27 +813,42 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_get_noresume(dev);

Why is it relevant to hold a PM reference during the installation of
the IRQ handler ?

>  	pm_runtime_enable(&pdev->dev);
>
>  	ret = mali_c55_media_frameworks_init(mali_c55);
>  	if (ret)
>  		goto err_pm_runtime_disable;
>
> -	pm_runtime_idle(&pdev->dev);
> -
>  	mali_c55->irqnum = platform_get_irq(pdev, 0);
>  	if (mali_c55->irqnum < 0) {
>  		ret = mali_c55->irqnum;
>  		goto err_deinit_media_frameworks;
>  	}
>
> +	/*
> +	 * The driver needs to transfer large amounts of register settings to
> +	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
> +	 * threaded IRQ to avoid disabling interrupts the entire time that's
> +	 * happening.

Does this comment still apply in your opinion ? I mean, thread_irq are
pretty regular stuff, do we need to justify its usage ?

> +	 */
> +	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
> +				   IRQF_ONESHOT, dev_driver_string(dev), dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		goto err_deinit_media_frameworks;
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);
> +
>  	return 0;
>
>  err_deinit_media_frameworks:
>  	mali_c55_media_frameworks_deinit(mali_c55);
>  err_pm_runtime_disable:
> -	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(&pdev->dev);
>  	kfree(mali_c55->context.registers);
>  err_power_off:
>  	__mali_c55_power_off(mali_c55);
> @@ -859,12 +862,13 @@ static void mali_c55_remove(struct platform_device *pdev)
>  {
>  	struct mali_c55 *mali_c55 = platform_get_drvdata(pdev);
>
> +	pm_runtime_disable(&pdev->dev);

It amazes me how, after so many years, I still can't find a clear, firm
indication of what is the intended call order for pm_runtime_disable()
and the other pm_runtime calls is during a driver's remove.

Media drivers seem to do different things with some disabling pm
before setting the state to suspended, other doing the other way
around.

In the end, I think this doesn't make any difference, but in this
specific occurance:

1) Is this re-ordering required to support the new interrupt handling?
or
2) Is it a change that is worth doing anyway ? (and I should say "should
go in a different patch on its own")

> +	free_irq(mali_c55->irqnum, &pdev->dev);
>  	mali_c55_media_frameworks_deinit(mali_c55);
> -	if (!pm_runtime_suspended(&pdev->dev)) {
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {

I guess this is required because you moved pm_runtime_disable() up

Thanks
  j

>  		__mali_c55_power_off(mali_c55);
>  		pm_runtime_set_suspended(&pdev->dev);
>  	}
> -	pm_runtime_disable(&pdev->dev);
>  	kfree(mali_c55->context.registers);
>  	of_reserved_mem_device_release(&pdev->dev);
>  }
>
> ---
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
> change-id: 20260918-mali-c55-irq-supend-resume-30b507a20011
>
> Best regards,
> --
> Linus Walleij <linusw@kernel.org>
>