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 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.
> 

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 addressed in a separate change.
> 

This only requires restoring the current limit after a suspend or reset 
when called from smu late init if a user specified limit is not found. 
It doesn't need to revert to default limit.

Thanks,
Lijo

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