Re: [PATCH] media: amd: isp4: add system suspend/resume support

[Mario Limonciello]

On 3/3/2026 4:44 PM, Kinn Coelho Juliao wrote:
> The ISP4 capture platform driver currently has no dev_pm_ops. When the
> driver is loaded during a session, the ISP hardware is left in an active
> state on s2idle suspend, causing the system to hang and requiring a hard
> power-off.
>
> Add suspend and resume callbacks that properly tear down the ISP firmware
> and hardware state before sleep via isp4sd_pwroff_and_deinit(). On
> resume, the device is marked so that userspace re-opens the camera,
> which triggers isp4sd_pwron_and_init() to reinitialize the hardware.
>
> Tested on HP ZBook Ultra G1a (AMD Ryzen AI MAX+ PRO 395, Strix Halo)
> with CachyOS kernel 6.19.5 — multiple suspend/resume cycles with the
> camera active before suspend complete successfully.
>
> Signed-off-by: Kinn Coelho Juliao <kinncj@xxxxxxxxx>

I'm a bit surprised this is needed, I thought that we handled this from 
amdgpu side of things.  Will let Pratap and Bin comment.

> ---
>   drivers/media/platform/amd/isp4/isp4.c | 48 ++++++++++++++++++++++++++
>   drivers/media/platform/amd/isp4/isp4.h |  1 +
>   2 files changed, 49 insertions(+)
>
> diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/platform/amd/isp4/isp4.c
> index bf6b8e2..3e2c3bc 100644
> --- a/drivers/media/platform/amd/isp4/isp4.c
> +++ b/drivers/media/platform/amd/isp4/isp4.c
> @@ -4,6 +4,7 @@
>    */
>   
>   #include <linux/irq.h>
> +#include <linux/pm.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/vmalloc.h>
>   #include <media/v4l2-ioctl.h>
> @@ -221,11 +222,58 @@ static void isp4_capture_remove(struct platform_device *pdev)
>   	media_device_cleanup(&isp_dev->mdev);
>   }
>   
> +static int isp4_capture_suspend(struct device *dev)
> +{
> +	struct isp4_device *isp_dev = dev_get_drvdata(dev);
> +	struct isp4_subdev *isp_subdev;
> +	struct isp4_interface *ispif;
> +	int ret;
> +
> +	if (!isp_dev)
> +		return 0;
> +
> +	isp_subdev = &isp_dev->isp_subdev;
> +	ispif = &isp_subdev->ispif;
> +
> +	if (ispif->status == ISP4IF_STATUS_PWR_OFF)
> +		return 0;
> +
> +	dev_info(dev, "tearing down fw and hw state for suspend\n");

This is probably a bit too noisy for regular every day use.  I would 
just exclude this message.

> +
> +	ret = isp4sd_pwroff_and_deinit(&isp_subdev->sdev);
> +	if (ret)
> +		dev_err(dev, "suspend teardown failed: %d\n", ret);
> +
> +	isp_dev->was_powered_before_suspend = true;
> +
> +	return 0;
> +}
> +
> +static int isp4_capture_resume(struct device *dev)
> +{
> +	struct isp4_device *isp_dev = dev_get_drvdata(dev);
> +
> +	if (!isp_dev)
> +		return 0;
> +
> +	if (isp_dev->was_powered_before_suspend) {
> +		dev_info(dev, "ISP was active before suspend, camera must be reopened\n");

This is probably a bit too noisy for regular every day use.  I would 
just exclude this message.

> +		isp_dev->was_powered_before_suspend = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(isp4_capture_pm_ops,
> +				isp4_capture_suspend,
> +				isp4_capture_resume);
> +
>   static struct platform_driver isp4_capture_drv = {
>   	.probe = isp4_capture_probe,
>   	.remove = isp4_capture_remove,
>   	.driver = {
>   		.name = ISP4_DRV_NAME,
> +		.pm = pm_sleep_ptr(&isp4_capture_pm_ops),
>   	}
>   };
>   
> diff --git a/drivers/media/platform/amd/isp4/isp4.h b/drivers/media/platform/amd/isp4/isp4.h
> index 2db6683..f39be96 100644
> --- a/drivers/media/platform/amd/isp4/isp4.h
> +++ b/drivers/media/platform/amd/isp4/isp4.h
> @@ -13,6 +13,7 @@ struct isp4_device {
>   	struct v4l2_device v4l2_dev;
>   	struct isp4_subdev isp_subdev;
>   	struct media_device mdev;
> +	bool was_powered_before_suspend;
>   };
>   
>   void isp4_intr_enable(struct isp4_subdev *isp_subdev, u32 index, bool enable);