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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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