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

["Nirujogi, Pratap" <pnirujog@amd.com>]

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