AMD General Might need a reliable synchronization between amdgpu driver and the OOB settings. Thanks.
-----Original Message----- From: Wang, Yang(Kevin) <[email protected]> Sent: Friday, August 14, 2026 4:07 PM To: Lazar, Lijo <[email protected]>; [email protected] Cc: Deucher, Alexander <[email protected]>; Zhang, Hawking <[email protected]>; Feng, Kenneth <[email protected]> Subject: RE: [PATCH] drm/amd/pm: Snapshot PPT limits for suspend and reset recovery AMD General Yes, you caught a RAS related risk, during the power PPT save stage, `GetPptLimit` may fail when a RAS error is detected for SMU 13.0.6/12 case. After syncing with Kenneth, I will hold this patch until we come up with a solution to synchronize OOB and in‑band power settings. Best Regards, Kevin > -----Original Message----- > From: Lazar, Lijo <[email protected]> > Sent: Friday, August 14, 2026 3:08 PM > To: Wang, Yang(Kevin) <[email protected]>; amd- > [email protected] > Cc: Deucher, Alexander <[email protected]>; Zhang, Hawking > <[email protected]>; Feng, Kenneth <[email protected]> > Subject: 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);
