RE: [PATCH 3/4] drm/amd/pm: refactor user PPT policy save and restore

AMD General

> -----Original Message-----
> From: Lazar, Lijo <[email protected]>
> Sent: Friday, July 31, 2026 12:57 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 3/4] drm/amd/pm: refactor user PPT policy save and
> restore
>
>
>
> On 31-Jul-26 8:40 AM, Yang Wang wrote:
> > The existing user policy representation has three ambiguities:
> >
> > - A numeric value cannot distinguish explicit zero from an unset policy.
> > - One value per controller cannot preserve independent AC and DC
> requests.
> > - Suspend-only restore misses runtime resume, GPU reset, and table reload.
> >
> > Refactor policy storage and restore as follows:
> >
> > - Store values and validity masks by power source and PPT controller.
> > - Save writes against the active source.
> > - Restore the active source after default SMU setup.
> > - Reapply the target policy after live AC/DC transitions.
> > - Use the target source default when no explicit request exists.
> >
> > The late-init path now covers system resume, runtime resume, GPU
> > reset, and custom PPTable reload. Common code owns persistent policy;
> > PMFW continues to own effective current limits.
> >
> > 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     | 89 +++++++++++++-
> -----
> >   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  5 +-
> >   3 files changed, 68 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > index c3688b3b12cc..ce526db4d24a 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);
> > +                   smu_set_ac_dc(adev->powerplay.pp_handle, true);
> >
> >             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 99d42446cfc8..43d5dd7dce7e 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -488,12 +488,52 @@ 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)
> > +{
> > +   enum smu_power_src_type power_source;
> > +   struct smu_ppt_limit_range *range;
> > +   uint32_t restore_mask;
> > +   uint32_t limit;
> > +   int i, ret;
> > +
> > +   power_source = smu->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;
> > +
> > +   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;
>
> This should be restore the current limit before a suspend/reset and not the
> default limit. The current limit could be set through out of band also, user
> profile values won't reflect that.

Please check the call sites.

The quoted default path is an opt-in fallback for AC/DC policy restore, and it is not used for suspend or GPU reset.
Suspend and reset invoke smu_restore_ppt_limits(smu, false), which replays only limits recorded in user_dpm_profile.

The profile intentionally does not represent out-of-band state,
and the amdgpu driver has no ownership or synchronization contract for PMFW limits changed outside the driver.
Supporting that use case requires separate policy and should be addressed in a separate change.

Best Regards,
Kevin
> Thanks,
> Lijo
>
> > +           } else {
> > +                   continue;
> > +           }
> > +
> > +           ret = smu_set_ppt_limit(smu, i, limit);
> > +           if (ret)
> > +                   dev_err(smu->adev->dev,
> > +                           "Failed to restore PPT%d limit: %d\n", i, ret);
> > +   }
> > +
> > +   smu->user_dpm_profile.flags &=
> ~SMU_DPM_USER_PROFILE_RESTORE; }
> > +
> >   /**
> >    * smu_restore_dpm_user_profile - reinstate user dpm profile
> >    *
> >    * @smu:  smu_context pointer
> >    *
> > - * Restore saved user power limits, clock frequencies and fan settings.
> > + * Restore saved user clock frequencies and fan settings.
> >    */
> >   static void smu_restore_dpm_user_profile(struct smu_context *smu)
> >   {
> > @@ -509,17 +549,6 @@ static void smu_restore_dpm_user_profile(struct
> smu_context *smu)
> >     /* Enable restore flag */
> >     smu->user_dpm_profile.flags |=
> SMU_DPM_USER_PROFILE_RESTORE;
> >
> > -   /* set the user dpm power limits */
> > -   for (int i = SMU_PPT_LIMIT_PPT0; i < SMU_LIMIT_TYPE_COUNT; i++) {
> > -           if (!smu->user_dpm_profile.ppt_limits[i])
> > -                   continue;
> > -           ret = smu_set_ppt_limit(smu, i,
> > -                                       smu-
> >user_dpm_profile.ppt_limits[i]);
> > -           if (ret)
> > -                   dev_err(smu->adev->dev,
> > -                           "Failed to set %d PPT limit value\n", i);
> > -   }
> > -
> >     /* set the user dpm clock configurations */
> >     if (smu_dpm_ctx->dpm_level ==
> AMD_DPM_FORCED_LEVEL_MANUAL) {
> >             enum smu_clk_type clk_type;
> > @@ -932,7 +961,7 @@ static int smu_late_init(struct amdgpu_ip_block
> *ip_block)
> >      * is unnecessary.
> >      */
> >     adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> > -   smu_set_ac_dc(smu);
> > +   smu_set_ac_dc(smu, false);
> >
> >     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)))
> > @@ -967,6 +996,8 @@ static int smu_late_init(struct amdgpu_ip_block
> *ip_block)
> >             return ret;
> >     }
> >
> > +   if (adev->in_suspend)
> > +           smu_restore_ppt_limits(smu, false);
> >     smu_restore_dpm_user_profile(smu);
> >
> >     return 0;
> > @@ -2746,7 +2777,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)
> > +int smu_set_ac_dc(struct smu_context *smu, bool restore_ppt_policy)
> >   {
> >     int ret = 0;
> >
> > @@ -2754,17 +2785,22 @@ int smu_set_ac_dc(struct smu_context *smu)
> >             return -EOPNOTSUPP;
> >
> >     /* controlled by firmware */
> > -   if (smu->dc_controlled_by_gpio)
> > -           return 0;
> > +   if (!smu->dc_controlled_by_gpio) {
> > +           ret = smu_set_power_source(smu,
> > +                                      smu->adev->pm.ac_power ?
> > +                                      SMU_POWER_SOURCE_AC :
> > +                                      SMU_POWER_SOURCE_DC);
> > +           if (ret) {
> > +                   dev_err(smu->adev->dev, "Failed to switch to %s
> mode!\n",
> > +                           smu->adev->pm.ac_power ? "AC" : "DC");
> > +                   return ret;
> > +           }
> > +   }
> >
> > -   ret = smu_set_power_source(smu,
> > -                              smu->adev->pm.ac_power ?
> SMU_POWER_SOURCE_AC :
> > -                              SMU_POWER_SOURCE_DC);
> > -   if (ret)
> > -           dev_err(smu->adev->dev, "Failed to switch to %s mode!\n",
> > -                  smu->adev->pm.ac_power ? "AC" : "DC");
> > +   if (restore_ppt_policy)
> > +           smu_restore_ppt_limits(smu, true);
> >
> > -   return ret;
> > +   return 0;
> >   }
> >
> >   const struct amd_ip_funcs smu_ip_funcs = { @@ -3020,8 +3056,11 @@
> > 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[limit_type] = limit;
> > +   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 3d32ee723b8e..698197a285e3 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > @@ -251,7 +251,8 @@ enum smu_memory_pool_size {
> >
> >   struct smu_user_dpm_profile {
> >     uint32_t fan_mode;
> > -   uint32_t ppt_limits[SMU_LIMIT_TYPE_COUNT];
> > +   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;
> > @@ -1953,7 +1954,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);
> > +int smu_set_ac_dc(struct smu_context *smu, bool restore_ppt_policy);
> >
> >   int smu_set_xgmi_plpd_mode(struct smu_context *smu,
> >                        enum pp_xgmi_plpd_mode mode);