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 3:45 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 12:21 PM, Wang, Yang(Kevin) wrote:
> > 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 poli=
cy.
> >>> - 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 =3D smu->adev->pm.ac_power ?
> >>> +           SMU_POWER_SOURCE_AC : SMU_POWER_SOURCE_DC;
> >>> +   restore_mask =3D 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 |=3D
> >> SMU_DPM_USER_PROFILE_RESTORE;
> >>> +
> >>> +   for (i =3D 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 =3D smu-
> >>> user_dpm_profile.ppt_limits[power_source][i];
> >>> +           } else if (restore_defaults) {
> >>> +                   range =3D &smu->ppt_limits.range[power_source][i]=
;
> >>> +                   limit =3D 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 repl=
ays
> only limits recorded in user_dpm_profile.
> >
>
> Yes, I'm specifically talking about late_init path. It gets called after =
a suspend
> or reset. It's about a case where user dpm limit is not present.
>
> > 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 address=
ed
> in a separate change.
> >
>
> This only requires restoring the current limit after a suspend or reset w=
hen
> called from smu late init if a user specified limit is not found.
> It doesn't need to revert to default limit.

Yes, the restore function takes no action if no custom limits are set by th=
e user.
And the logic has been verified on the target system, including suspend, re=
set and default cases.

Best Regards,
Kevin
>
> Thanks,
> Lijo
>
> > Best Regards,
> > Kevin
> >> Thanks,
> >> Lijo
> >>
> >>> +           } else {
> >>> +                   continue;
> >>> +           }
> >>> +
> >>> +           ret =3D 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 &=3D
> >> ~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 settin=
gs.
> >>> + * 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 |=3D
> >> SMU_DPM_USER_PROFILE_RESTORE;
> >>>
> >>> -   /* set the user dpm power limits */
> >>> -   for (int i =3D SMU_PPT_LIMIT_PPT0; i < SMU_LIMIT_TYPE_COUNT; i++)=
 {
> >>> -           if (!smu->user_dpm_profile.ppt_limits[i])
> >>> -                   continue;
> >>> -           ret =3D 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 =3D=3D
> >> 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 =3D 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) =3D=3D IP_VERSION(13, =
0,
> >>> 1))
> >> ||
> >>>          (amdgpu_ip_version(adev, MP1_HWIP, 0) =3D=3D 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 =3D 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 =3D 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 =3D 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 =3D { @@ -3020,8 +3056,11 @=
@
> >>> static int smu_set_ppt_limit(void *handle, uint32_t limit_type,
> >>> uint32_t
> >> limit)
> >>>      ret =3D 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] =3D limit;
> >>> +   if (!(smu->user_dpm_profile.flags &
> >> SMU_DPM_USER_PROFILE_RESTORE)) {
> >>> +           smu-
> >>> user_dpm_profile.ppt_limits[power_source][limit_type] =3D limit;
> >>> +           smu->user_dpm_profile.ppt_limit_user_mask[power_source]
> >> |=3D
> >>> +                   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);
> >