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@gmail.com>
---
 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");
+
+	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");
+		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);
-- 
2.53.0

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@gmail.com>

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

On 3/3/2026 5:49 PM, Mario Limonciello wrote:
>
>
> 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@gmail.com>
>
> 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.

yes, this issue was addressed with this fix in AMDGPU available since 
v6.19-rc5 onwards.

https://github.com/torvalds/linux/commit/0288a345f19b2162546352161509bb24614729e1

ISP suspend/resume is managed as part of AMDGPU pm_runtime to ensure 
that the ISP is suspended before the AMDGPU device. This sequence is 
required because, in the AMD hardware architecture, the ISP is a child 
device of GFX, and all child devices are expected to suspend before the 
GFX device. If this sequence is not followed, warnings may occur when 
the ISP remains active and attempts to access AMDGPU resources while it 
is suspended.

Thanks,

Pratap


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

Thanks Pratap, Mario.

I reviewed commit 0288a345f19b ("drm/amdgpu/isp: fix SMU warning
during ISP suspend-resume"). I understand it marks ISP MFD children as
syscore devices and manages their suspend/resume from the amdgpu parent
via pm_runtime_force_suspend/resume.

I'm curious about one thing though; on my current system (7.0-rc1),
s2idle resume still hangs with the in-tree V4L2 module loaded. My
out-of-tree module, which adds dev_pm_ops that call
isp4sd_pwroff_and_deinit() before suspend, resolves the hang.

pm_runtime_force_suspend() manages the power domain but doesn't tear
down firmware ring buffers or V4L2 streaming state. If the camera was
active at any point during the session when suspend occurs, the driver
would have stale state on resume. That seems to be what's causing the
hang on my hardware (HP ZBook Ultra G1a, Strix Halo).

Is the firmware/streaming state teardown handled elsewhere that I'm
missing, or does the V4L2 driver still need its own suspend path?

Thanks,
Kinn