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

On 31-Jul-26 1:35 PM, Wang, Yang(Kevin) wrote:
> 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 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.
> 
> Yes, the restore function takes no action if no custom limits are set by the user.
> And the logic has been verified on the target system, including suspend, reset and default cases.
> 

That's right. Currently it doesn't do any action.

What I was asking is -

restore_default => if user explicitly wants to restore default values
restore_user => restore user specified limits
auto => if user specified limit is present, restore_user, else restore 
current limit of the power source mode.

During late init with suspend or reset, restore the limit.

if (suspend or reset)
  restore_limit(auto)

refresh_current_limit()

Thanks,
Lijo


> Best Regards,
> Kevin
>>
>> 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);
>>>
>