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

[Mario Limonciello <mario.limonciello@amd.com>]

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?

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