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