Re: [PATCH] drm/amd/pm: Snapshot PPT limits for suspend and reset recovery

On 14-Aug-26 10:58 AM, Yang Wang wrote:
> The DPM user profile records driver-programmed policy, not the effective
> PMFW limits. Replaying it after recovery can restore stale policy.
> 
> Maintain a PMFW PPT snapshot:
> - Query supported PPT controllers before suspend and mode1 reset.
> - Save successful queries with the current OS AC/DC source.
> - Clear the selected source mask before capture to prevent stale replay.
> - Refresh the OS power source before recovery.
> - Synchronize the current source with PMFW before restoring PPT limits.
> - Restore PPT limits only when the snapshot source still matches.
> - Do not replay PPT policy on normal AC/DC changes.
> 
> Capture reset state before DPM and PMFW teardown.
> Add debug logging for the snapshot, restore order, and PMFW AC/DC sync.
> 
> Signed-off-by: Yang Wang <[email protected]>
> ---
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c           |   2 +-
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 152 +++++++++++++-----
>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |   8 +-
>   3 files changed, 117 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index ce526db4d24a..c3688b3b12cc 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -508,7 +508,7 @@ void amdgpu_pm_acpi_event_handler(struct amdgpu_device *adev)
>   			amdgpu_dpm_notify_ac_dc(adev);
>   
>   		if (is_support_sw_smu(adev))
> -			smu_set_ac_dc(adev->powerplay.pp_handle, true);
> +			smu_set_ac_dc(adev->powerplay.pp_handle);
>   
>   		mutex_unlock(&adev->pm.mutex);
>   	}
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index bfe2c0bf426d..e5cb8d0420ab 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -489,44 +489,122 @@ static void smu_set_user_clk_dependencies(struct smu_context *smu, enum smu_clk_
>   		return;
>   }
>   
> -static void smu_restore_ppt_limits(struct smu_context *smu,
> -				   bool restore_defaults)
> +static void smu_save_ppt_limits(struct smu_context *smu)
>   {
> +	struct amdgpu_device *adev = smu->adev;
>   	enum smu_power_src_type power_source;
> -	struct smu_ppt_limit_range *range;
> -	uint32_t restore_mask;
> -	uint32_t limit;
> +	u32 limit;
>   	int i, ret;
>   
> -	power_source = smu->adev->pm.ac_power ?
> +	/* AC/DC is maintained by the OS and driver power-source events. */
> +	smu->ppt_limits.saved_ac_power = adev->pm.ac_power;
> +	power_source = adev->pm.ac_power ?
>   		SMU_POWER_SOURCE_AC : SMU_POWER_SOURCE_DC;
> -	restore_mask = smu->user_dpm_profile.ppt_limit_user_mask[power_source] &
> -		smu->ppt_limits.supported_mask;
> -	if (!restore_mask && !restore_defaults)
> -		return;
> -
> -	smu->user_dpm_profile.flags |= SMU_DPM_USER_PROFILE_RESTORE;
> +	smu->ppt_limits.saved_mask[power_source] = 0;
> +	dev_dbg(adev->dev, "PPT snapshot: source=%s\n",
> +		adev->pm.ac_power ? "AC" : "DC");
>   
>   	for (i = SMU_PPT_LIMIT_PPT0; i < SMU_LIMIT_TYPE_COUNT; i++) {
>   		if (!(smu->ppt_limits.supported_mask & BIT(i)))
>   			continue;
>   
> -		if (restore_mask & BIT(i)) {
> -			limit = smu->user_dpm_profile.ppt_limits[power_source][i];
> -		} else if (restore_defaults) {
> -			range = &smu->ppt_limits.range[power_source][i];
> -			limit = range->default_value;
> -		} else {
> +		ret = smu_get_asic_ppt_limit(smu, i, &limit);
> +		if (ret) {
> +			dev_err(adev->dev,
> +				"Failed to save PPT%d limit: %d\n", i, ret);

Not all SOCs support all source/limit types. If it's not supported like 
EOPNOTSUPP, it should skip throwing this error.
>   			continue;
>   		}
>   
> +		smu->ppt_limits.saved_values[power_source][i] = limit;
> +		smu->ppt_limits.saved_mask[power_source] |= BIT(i);

This mask can be avoided by keeping the saved value as default power 
limit or 0. Also, the limit should be saved during any set/get call 
also. If the FW message fails during suspend/reset, the last known limit 
can be used. The FW message will indeed fail on some SOCs during RAS 
recovery.

> +		dev_dbg(adev->dev, "PPT%d snapshot: source=%s limit=%u\n",
> +			i, adev->pm.ac_power ? "AC" : "DC", limit);
> +	}
> +
> +	dev_dbg(adev->dev, "PPT snapshot complete: source=%s mask=%#x\n",
> +		adev->pm.ac_power ? "AC" : "DC",
> +		smu->ppt_limits.saved_mask[power_source]);
> +}
> +
> +static int smu_restore_ac_dc(struct smu_context *smu, u32 *restore_mask)
> +{
> +	struct amdgpu_device *adev = smu->adev;
> +	enum smu_power_src_type current_source;
> +	enum smu_power_src_type saved_source;
> +
> +	*restore_mask = 0;
> +
> +	if (adev->in_suspend || amdgpu_reset_in_recovery(adev)) {
> +		/* The power source may change while the device is suspended. */
> +		adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> +		current_source = adev->pm.ac_power ?
> +			SMU_POWER_SOURCE_AC : SMU_POWER_SOURCE_DC;
> +		saved_source = smu->ppt_limits.saved_ac_power ?
> +			SMU_POWER_SOURCE_AC : SMU_POWER_SOURCE_DC;

This logic is unnecessary. What all needs to be restored can be figured 
out from the saved limit value itself.

> +		*restore_mask = smu->ppt_limits.saved_mask[saved_source] &
> +			smu->ppt_limits.supported_mask;
> +		dev_dbg(adev->dev,
> +			"PPT restore: saved source=%s mask=%#x, current source=%s\n",
> +			smu->ppt_limits.saved_ac_power ? "AC" : "DC",
> +			*restore_mask, adev->pm.ac_power ? "AC" : "DC");
> +		/*
> +		 * A snapshot is valid only for the source under which it was
> +		 * captured. Do not replay an AC snapshot after switching to DC,
> +		 * or vice versa. The current OS power source takes precedence.
> +		 */
> +		if (current_source != saved_source) {
> +			dev_dbg(adev->dev,
> +				"Discard PPT snapshot after source change: %s to %s\n",
> +				smu->ppt_limits.saved_ac_power ? "AC" : "DC",
> +				adev->pm.ac_power ? "AC" : "DC");
> +			*restore_mask = 0;
> +		}
> +	}
> +
> +	/*
> +	 * NOTE: Synchronize PMFW with the OS AC/DC source before restoring PPT
> +	 * limits so both sides use the same power policy. GPIO-controlled
> +	 * platforms perform the synchronization in PMFW.
> +	 */
> +	return smu_set_ac_dc(smu);

There is no need to associate ac/dc setting with PPT limit restore sequence.

> +}
> +
> +static int smu_restore_ppt_limits(struct smu_context *smu)
> +{
> +	struct amdgpu_device *adev = smu->adev;
> +	enum smu_power_src_type power_source;
> +	u32 restore_mask;
> +	u32 limit;
> +	int i, ret;
> +
> +	ret = smu_restore_ac_dc(smu, &restore_mask);

This should only be about fetching the limits that need to be restored. 
No need associate ac/dc restore with this.

> +	if (ret)
> +		return ret;
> +	if (!restore_mask)
> +		return 0;
> +
> +	power_source = adev->pm.ac_power ?
> +		SMU_POWER_SOURCE_AC : SMU_POWER_SOURCE_DC;
> +
> +	dev_dbg(adev->dev, "PPT restore: source=%s mask=%#x\n",
> +		adev->pm.ac_power ? "AC" : "DC", restore_mask);
> +
> +	for (i = SMU_PPT_LIMIT_PPT0; i < SMU_LIMIT_TYPE_COUNT; i++) {
> +		if (!(restore_mask & BIT(i)))
> +			continue;
> +

There is no need to keep a mask. The saved limit can be default limit or 
0 if not saved/changed. It just needs to check that value and decide 
whether to use that or not.

> +		limit = smu->ppt_limits.saved_values[power_source][i];
>   		ret = smu_set_ppt_limit(smu, i, limit);
>   		if (ret)
> -			dev_err(smu->adev->dev,
> +			dev_err(adev->dev,
>   				"Failed to restore PPT%d limit: %d\n", i, ret);
> +		else
> +			dev_dbg(adev->dev,
> +				"PPT%d restored: source=%s limit=%u\n", i,
> +				adev->pm.ac_power ? "AC" : "DC", limit);
>   	}
>   
> -	smu->user_dpm_profile.flags &= ~SMU_DPM_USER_PROFILE_RESTORE;
> +	return 0;
>   }
>   
>   /**
> @@ -954,15 +1032,9 @@ static int smu_late_init(struct amdgpu_ip_block *ip_block)
>   		return ret;
>   	}
>   
> -	/*
> -	 * Explicitly notify PMFW the power mode the system in. Since
> -	 * the PMFW may boot the ASIC with a different mode.
> -	 * For those supporting ACDC switch via gpio, PMFW will
> -	 * handle the switch automatically. Driver involvement
> -	 * is unnecessary.
> -	 */
> -	adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> -	smu_set_ac_dc(smu, false);
> +	ret = smu_restore_ppt_limits(smu);
> +	if (ret)
> +		return ret;
>   
>   	if ((amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(13, 0, 1)) ||
>   	    (amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(13, 0, 3)))
> @@ -997,8 +1069,6 @@ static int smu_late_init(struct amdgpu_ip_block *ip_block)
>   		return ret;
>   	}
>   
> -	if (adev->in_suspend || amdgpu_reset_in_recovery(adev))
> -		smu_restore_ppt_limits(smu, false);
>   	smu_restore_dpm_user_profile(smu);
>   
>   	return 0;
> @@ -2202,6 +2272,9 @@ static int smu_hw_fini(struct amdgpu_ip_block *ip_block)
>   	if (!smu->pm_enabled)
>   		return 0;
>   
> +	if (amdgpu_in_reset(adev))
> +		smu_save_ppt_limits(smu);
> +
>   	adev->pm.dpm_enabled = false;
>   
>   	ret = smu_smc_hw_cleanup(smu);
> @@ -2263,6 +2336,8 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>   	if (!smu->pm_enabled)
>   		return 0;
>   
> +	smu_save_ppt_limits(smu);
> +
>   	adev->pm.dpm_enabled = false;
>   
>   	ret = smu_smc_hw_cleanup(smu);
> @@ -2778,7 +2853,7 @@ static int smu_set_watermarks_for_clock_ranges(void *handle,
>   	return smu_set_watermarks_table(smu, clock_ranges);
>   }
>   
> -int smu_set_ac_dc(struct smu_context *smu, bool restore_ppt_policy)
> +int smu_set_ac_dc(struct smu_context *smu)
>   {
>   	int ret = 0;
>   
> @@ -2796,11 +2871,12 @@ int smu_set_ac_dc(struct smu_context *smu, bool restore_ppt_policy)
>   				smu->adev->pm.ac_power ? "AC" : "DC");
>   			return ret;
>   		}
> +	} else {
> +		dev_dbg(smu->adev->dev,
> +			"PMFW controls %s mode through GPIO\n",
> +			smu->adev->pm.ac_power ? "AC" : "DC");
>   	}
>   
> -	if (restore_ppt_policy)
> -		smu_restore_ppt_limits(smu, true);
> -
>   	return 0;
>   }
>   
> @@ -3057,12 +3133,6 @@ static int smu_set_ppt_limit(void *handle, uint32_t limit_type, uint32_t limit)
>   	ret = smu->ppt_funcs->set_ppt_limit(smu, limit_type, limit);
>   	if (ret)
>   		return ret;
> -	if (!(smu->user_dpm_profile.flags & SMU_DPM_USER_PROFILE_RESTORE)) {
> -		smu->user_dpm_profile.ppt_limits[power_source][limit_type] = limit;
> -		smu->user_dpm_profile.ppt_limit_user_mask[power_source] |=
> -			BIT(limit_type);
> -	}
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index d30951121e1f..9f85b302060e 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -238,6 +238,10 @@ struct smu_ppt_limit_range {
>   struct smu_ppt_limit_context {
>   	struct smu_ppt_limit_range
>   		range[SMU_POWER_SOURCE_COUNT][SMU_LIMIT_TYPE_COUNT];
> +	/* PMFW state saved before suspend or mode1 reset. */
> +	u32 saved_values[SMU_POWER_SOURCE_COUNT][SMU_LIMIT_TYPE_COUNT];
> +	u32 saved_mask[SMU_POWER_SOURCE_COUNT];
> +	bool saved_ac_power;

There is no need to save this. If changed to a different source, use the 
last saved value for that source or keep the default.

Thanks,
Lijo
>   	uint32_t supported_mask;
>   };
>   
> @@ -251,8 +255,6 @@ enum smu_memory_pool_size {
>   
>   struct smu_user_dpm_profile {
>   	uint32_t fan_mode;
> -	uint32_t ppt_limits[SMU_POWER_SOURCE_COUNT][SMU_LIMIT_TYPE_COUNT];
> -	uint32_t ppt_limit_user_mask[SMU_POWER_SOURCE_COUNT];
>   	uint32_t fan_speed_pwm;
>   	uint32_t fan_speed_rpm;
>   	uint32_t flags;
> @@ -1954,7 +1956,7 @@ int smu_set_soft_freq_range(struct smu_context *smu, enum pp_clock_type clk_type
>   
>   int smu_set_gfx_power_up_by_imu(struct smu_context *smu);
>   
> -int smu_set_ac_dc(struct smu_context *smu, bool restore_ppt_policy);
> +int smu_set_ac_dc(struct smu_context *smu);
>   
>   int smu_set_xgmi_plpd_mode(struct smu_context *smu,
>   			   enum pp_xgmi_plpd_mode mode);