media: mali-c55: Keep IRQ requested during suspend

PATCH In review Linus Walleij 2 participants 3 messages Oct 27 – Nov 10

Subsystems (3)

Stable branch Media (V4L/DVB) Arm Mali-C55 ISP driver

lore

Share & tools

lore

Linus Walleij

author

2026-10-27 14:08 UTC #0 lore raw

PATCH v2 1/1 media: mali-c55: Keep IRQ requested during suspend

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 [email protected]

Signed-off-by Linus Walleij <[email protected]>

---
This is essentially my reply with the "proper fix" for:
https://lore.kernel.org/linux-media/[email protected]/

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/[email protected]

---
 .../media/platform/arm/mali-c55/mali-c55-core.c    | 44 ++++++++++++----------
 1 file changed, 24 insertions(+), 20 deletions(-)

drivers/media/platform/arm/mali-c55/mali-c55-core.c +24 ~~-20~~

index f28e9f4354ac..3c2816b6a96b 100644
@@ -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,

Signature · 1 line

Linus Walleij <[email protected]>

Linus Walleij

author

2026-11-05 08:07 UTC #1 lore raw

Quoted 10 lines · 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.

Quoted 7 lines

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 [email protected]
Signed-off-by Linus Walleij <[email protected]>

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

Jacopo Mondi

maintainer

2026-11-10 12:51 UTC #2 lore raw

In reply to Linus Walleij

Hi Linus

Quoted 80 lines · 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 [email protected]
Signed-off-by Linus Walleij <[email protected]>
---
This is essentially my reply with the "proper fix" for:
https://lore.kernel.org/linux-media/[email protected]/
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/[email protected]
---
 .../media/platform/arm/mali-c55/mali-c55-core.c    | 44 ++++++++++++----------
 1 file changed, 24 insertions(+), 20 deletions(-)
drivers/media/platform/arm/mali-c55/mali-c55-core.c +3 ~~-15~~
index f28e9f4354ac..3c2816b6a96b 100644
@@ -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 = {

drivers/media/platform/arm/mali-c55/mali-c55-core.c static int mali_c55_probe(struct platform_device *pdev) · line 816

Quoted 4 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +1 -0

@@ -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 ?

Quoted 9 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +0 -2

 	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);

drivers/media/platform/arm/mali-c55/mali-c55-core.c static int mali_c55_probe(struct platform_device *pdev) · line 833

Quoted 10 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +5 -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 ?

Quoted 23 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +12 -1

+	 */
+	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);

drivers/media/platform/arm/mali-c55/mali-c55-core.c static void mali_c55_remove(struct platform_device *pdev) · line 865

Quoted 4 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +1 -0

@@ -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")

drivers/media/platform/arm/mali-c55/mali-c55-core.c static void mali_c55_remove(struct platform_device *pdev) · line 868

Quoted 4 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +2 -1

+	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

Quoted 16 lines

drivers/media/platform/arm/mali-c55/mali-c55-core.c +0 -2
 		__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 <[email protected]>