Re: [PATCH] drm/amd/pm: fix smu messages after system resume for smu 11.0.x

On 17-Aug-26 11:57 AM, Yang Wang wrote:
> SMU 11.0.x can retain an unfinished command across system
> suspend, causing SMU messages such as RunDcBtc to fail during resume.
> 
> Prepare MP1 for unload during system suspend and record successful completion,
> reload SMC firmware after PSP restart before resume sends further SMU messages,
> then clear the reload state.
> 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5620
> 
> Signed-off-by: Yang Wang <[email protected]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c   |  9 ++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h   |  2 ++
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 38 ++++++++++++++++++++---
>   3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 04f6ebf31cca..5c00683c1edd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -3512,11 +3512,16 @@ static int psp_load_non_psp_fw(struct psp_context *psp)
>   	struct amdgpu_firmware_info *ucode;
>   	struct amdgpu_device *adev = psp->adev;
>   
> -	if (psp->autoload_supported &&
> -	    !psp->pmfw_centralized_cstate_management) {
> +	if (psp->smu_fw_needs_reload ||
> +	    (psp->autoload_supported &&
> +	     !psp->pmfw_centralized_cstate_management)) {

Is this really needed? For PMFW managed cstate ones, load_smu_fw is 
called before TMR setup already. SMU versions 11.0.x listed in this 
patch have pmfw_centralized_cstate_management set to true.

Thanks,
Lijo

> +		if (psp->smu_fw_needs_reload)
> +			dev_dbg(adev->dev,
> +				"Reloading SMU firmware after MP1 unload\n");
>   		ret = psp_load_smu_fw(psp);
>   		if (ret)
>   			return ret;
> +		psp->smu_fw_needs_reload = false;
>   	}
>   
>   	/* Load P2S table first if it's available */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> index d80c85793e3b..20338caa4921 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> @@ -443,6 +443,8 @@ struct psp_context {
>   	bool				boot_time_tmr;
>   	/* flag to mark whether df cstate management centralized to PMFW */
>   	bool				pmfw_centralized_cstate_management;
> +	/* SMC firmware reload required during resume */
> +	bool				smu_fw_needs_reload;
>   
>   	/* xgmi ta firmware and buffer */
>   	const struct firmware		*ta_fw;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 559afdc5815d..8d499d9a86df 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2182,12 +2182,36 @@ static int smu_smc_hw_cleanup(struct smu_context *smu)
>   static int smu_reset_mp1_state(struct smu_context *smu)
>   {
>   	struct amdgpu_device *adev = smu->adev;
> -	int ret = 0;
> +	u32 mp1_version = amdgpu_ip_version(adev, MP1_HWIP, 0);
> +	bool needs_mp1_unload;
> +	int ret;
>   
> -	if ((!adev->in_runpm) && (!adev->in_suspend) &&
> -		(!amdgpu_in_reset(adev)) && !smu->is_apu &&
> -			amdgpu_ip_version(adev, MP1_HWIP, 0) >= IP_VERSION(13, 0, 0))
> -		ret = smu_set_mp1_state(smu, PP_MP1_STATE_UNLOAD);
> +	if (adev->in_runpm || amdgpu_in_reset(adev) || smu->is_apu)
> +		return 0;
> +
> +	switch (mp1_version) {
> +	case IP_VERSION(11, 0, 7):
> +	case IP_VERSION(11, 0, 11):
> +	case IP_VERSION(11, 0, 12):
> +	case IP_VERSION(11, 0, 13):
> +		/*
> +		 * These SMU versions require MP1 unload before system suspend.
> +		 * The shutdown path already requests it through mp1_state.
> +		 */
> +		needs_mp1_unload = adev->mp1_state != PP_MP1_STATE_UNLOAD;
> +		break;
> +	default:
> +		needs_mp1_unload = !adev->in_suspend &&
> +			mp1_version >= IP_VERSION(13, 0, 0);
> +		break;
> +	}
> +
> +	if (!needs_mp1_unload)
> +		return 0;
> +
> +	ret = smu_set_mp1_state(smu, PP_MP1_STATE_UNLOAD);
> +	if (!ret && adev->in_suspend)
> +		adev->psp.smu_fw_needs_reload = true;
>   
>   	return ret;
>   }
> @@ -2279,6 +2303,10 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>   	if (ret)
>   		return ret;
>   
> +	ret = smu_reset_mp1_state(smu);
> +	if (ret)
> +		return ret;
> +
>   	smu->watermarks_bitmap &= ~(WATERMARKS_LOADED);
>   
>   	smu_set_gfx_cgpg(smu, false);