Re: [PATCH] drm/amd/amdgpu: Fix SMU warning during isp suspend-resume

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

Hi Mario,

On 12/9/2025 10:28 PM, Mario Limonciello wrote:
>
>
> On 12/9/2025 7:50 PM, Pratap Nirujogi wrote:
>> ISP mfd child devices are using genpd and the system suspend-resume
>> operations between genpd and amdgpu parent device which uses only
>> runtime suspend-resume are not in sync.
>>
>> Linux power manager during suspend-resume resuming the genpd devices
>> earlier than the amdgpu parent device. This is resulting in the below
>> warning as SMU is in suspended state when genpd attempts to resume ISP.
>>
>> WARNING: CPU: 13 PID: 5435 at 
>> drivers/gpu/drm/amd/amdgpu/../pm/swsmu/amdgpu_smu.c:398 
>> smu_dpm_set_power_gate+0x36f/0x380 [amdgpu]
>>
>> To fix this warning isp suspend-resume is handled as part of amdgpu
>> parent device suspend-resume instead of genpd sequence. Each ISP MFD
>> child device is marked as dev_pm_syscore_device to skip genpd
>> suspend-resume and use pm_runtime_force api's to suspend-resume
>> the devices when callbacks from amdgpu are received.
>>
>> Signed-off-by: Gjorgji Rosikopulos <grosikop@amd.com>
>> Signed-off-by: Bin Du <bin.du@amd.com>
>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>
> Who is the patch author?  If you guys worked together, there should be 
> Co-developed-by tags to represent it.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c | 24 ++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h |  2 +
>>   drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c | 59 +++++++++++++++++++++++++
>>   3 files changed, 85 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
>> index 37270c4dab8d..532f83d783d1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
>> @@ -318,12 +318,36 @@ void isp_kernel_buffer_free(void **buf_obj, u64 
>> *gpu_addr, void **cpu_addr)
>>   }
>>   EXPORT_SYMBOL(isp_kernel_buffer_free);
>>   +static int isp_resume(struct amdgpu_ip_block *ip_block)
>> +{
>> +    struct amdgpu_device *adev = ip_block->adev;
>> +    struct amdgpu_isp *isp = &adev->isp;
>> +
>> +    if (isp->funcs->hw_resume)
>> +        return isp->funcs->hw_resume(isp);
>> +
>> +    return -ENODEV;
>> +}
>> +
>> +static int isp_suspend(struct amdgpu_ip_block *ip_block)
>> +{
>> +    struct amdgpu_device *adev = ip_block->adev;
>> +    struct amdgpu_isp *isp = &adev->isp;
>> +
>> +    if (isp->funcs->hw_suspend)
>> +        return isp->funcs->hw_suspend(isp);
>> +
>> +    return -ENODEV;
>> +}
>> +
>>   static const struct amd_ip_funcs isp_ip_funcs = {
>>       .name = "isp_ip",
>>       .early_init = isp_early_init,
>>       .hw_init = isp_hw_init,
>>       .hw_fini = isp_hw_fini,
>>       .is_idle = isp_is_idle,
>> +    .suspend = isp_suspend,
>> +    .resume = isp_resume,
>>       .set_clockgating_state = isp_set_clockgating_state,
>>       .set_powergating_state = isp_set_powergating_state,
>>   };
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
>> index d6f4ffa4c97c..9a5d2b1dff9e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
>> @@ -38,6 +38,8 @@ struct amdgpu_isp;
>>   struct isp_funcs {
>>       int (*hw_init)(struct amdgpu_isp *isp);
>>       int (*hw_fini)(struct amdgpu_isp *isp);
>> +    int (*hw_suspend)(struct amdgpu_isp *isp);
>> +    int (*hw_resume)(struct amdgpu_isp *isp);
>>   };
>>     struct amdgpu_isp {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c 
>> b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>> index 4258d3e0b706..560c398e14fc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>> @@ -26,6 +26,7 @@
>>    */
>>     #include <linux/gpio/machine.h>
>> +#include <linux/pm_runtime.h>
>>   #include "amdgpu.h"
>>   #include "isp_v4_1_1.h"
>>   @@ -145,6 +146,9 @@ static int isp_genpd_add_device(struct device 
>> *dev, void *data)
>>           return -ENODEV;
>>       }
>>   +    /* The devcies will be managed by the pm ops from the parent */
>
> devices
>
>> +    dev_pm_syscore_device(dev, true);
>> +
>>   exit:
>>       /* Continue to add */
>>       return 0;
>> @@ -177,12 +181,65 @@ static int isp_genpd_remove_device(struct 
>> device *dev, void *data)
>>           drm_err(&adev->ddev, "Failed to remove dev from genpd 
>> %d\n", ret);
>>           return -ENODEV;
>>       }
>> +    dev_pm_syscore_device(dev, false);
>>     exit:
>>       /* Continue to remove */
>>       return 0;
>>   }
>>   +static int isp_suspend_device(struct device *dev, void *data)
>> +{
>> +    struct platform_device *pdev = container_of(dev, struct 
>> platform_device, dev);
>> +
>> +    if (!dev->type || !dev->type->name)
>> +        return 0;
>> +    if (strncmp(dev->type->name, "mfd_device", 10))
>> +        return 0;
>> +    if (!strncmp(pdev->mfd_cell->name, "amdisp-pinctrl", 14))
>> +        return 0;
>
> Could we store the mfd_cell pointer instead and just compare the 
> pointers?

I don't think I can do a pointer comparision to identify the correct 
mfd_cell, string comparision seems like required in this case.

Its because when isp mfd child devices are created using 
mfd_add_hotplug_devices(), it is not returning the pdev or mfd_cell handles
to store in the amdgpu_isp and later use in suspend/resume to compare 
with incoming pdev->mfd_cell to detect the correct the device.

The mfd-core is doing a kmemdup of mfd_cells data passed to 
mfd_add_hotplug_devices() to create the platform device.

https://github.com/torvalds/linux/blob/master/drivers/mfd/mfd-core.c#L163

I'm considering to add this function to check for valid isp mfd child 
devices that are allowed to do suspend-resume, this can minimize the 
checks, but still cannot eliminate the string comparsion, please let us 
know your thoughts.

static bool is_valid_mfd_device(struct platform_device *pdev)
{
     const struct mfd_cell *mc = mfd_get_cell(pdev);
     if (!mc)
         return false;
     if (!strncmp(mc->name, "amdisp-pinctrl", 14))
         return false;
     return true;
}

Thanks,

Pratap

>
>> +
>> +    return pm_runtime_force_suspend(dev);
>> +}
>> +
>> +static int isp_resume_device(struct device *dev, void *data)
>> +{
>> +    struct platform_device *pdev = container_of(dev, struct 
>> platform_device, dev);
>> +
>> +    if (!dev->type || !dev->type->name)
>> +        return 0;
>> +    if (strncmp(dev->type->name, "mfd_device", 10))
>> +        return 0;
>> +    if (!strncmp(pdev->mfd_cell->name, "amdisp-pinctrl", 14))
>> +        return 0;
>
> same comment as above
>
>> +
>> +    return pm_runtime_force_resume(dev);
>> +}
>> +
>> +static int isp_v4_1_1_hw_suspend(struct amdgpu_isp *isp)
>> +{
>> +    int r;
>> +
>> +    r = device_for_each_child(isp->parent, NULL,
>> +                  isp_suspend_device);
>> +    if (r)
>> +        dev_err(isp->parent, "failed to suspend hw devices (%d)\n", r);
>> +
>> +    return 0;
>
> Shouldn't you return r?
>
>> +}
>> +
>> +static int isp_v4_1_1_hw_resume(struct amdgpu_isp *isp)
>> +{
>> +    int r;
>> +
>> +    r = device_for_each_child(isp->parent, NULL,
>> +                  isp_resume_device);
>> +    if (r)
>> +        dev_err(isp->parent, "failed to resume hw device (%d)\n", r);
>> +
>> +    return 0;
>
> Shouldn't you return r?
>
>> +}
>> +
>>   static int isp_v4_1_1_hw_init(struct amdgpu_isp *isp)
>>   {
>>       const struct software_node *amd_camera_node, *isp4_node;
>> @@ -369,6 +426,8 @@ static int isp_v4_1_1_hw_fini(struct amdgpu_isp 
>> *isp)
>>   static const struct isp_funcs isp_v4_1_1_funcs = {
>>       .hw_init = isp_v4_1_1_hw_init,
>>       .hw_fini = isp_v4_1_1_hw_fini,
>> +    .hw_suspend = isp_v4_1_1_hw_suspend,
>> +    .hw_resume = isp_v4_1_1_hw_resume,
>>   };
>>     void isp_v4_1_1_set_isp_funcs(struct amdgpu_isp *isp)
>