Re: [PATCH] drm/amd/pm: Track current PPT limit for restore

On 06-Aug-26 1:03 PM, Wang, Yang(Kevin) wrote:
> AMD General
> 
> I do not agree with adding a read-to-cache path for the current PPT limit.
> 
> The current limit is PMFW runtime state. A read of that state must remain an observation; it must not implicitly create or update driver-owned persistent policy.
> This separation is intentional: the driver keeps immutable capabilities from the PPTable and restores only limits that it successfully programmed itself.
> 
> Caching a value returned by GetPptLimit would turn a potentially transient or out-of-band PMFW state into a value that the driver replays after suspend or reset.
> 
> There is no ownership, notification, or synchronization mechanism that makes such a cache authoritative, so it can only become stale.
> and the existing user_dpm_profile policy *CACHE ALREADY* handles the supported driver-owned update path.
> 

The existing path also doesn't have any synchronization with 
out-of-band, so the same argument holds. i.e., it overwrites whatever 
set by external source.
> If no bug fix or feature improve, please drop this patch.

Driver triggers resets which could relaod the whole FW. In such cases 
driver remains responsible for restoring the last known power limit. The 
current method doesn't do that.

I will add a bug fix tag as the previous patch doesn't restore the 
current limit properly.

Thanks,
Lijo

> 
> Best Regards,
> Kevin
> 
>> -----Original Message-----
>> From: Lazar, Lijo <[email protected]>
>> Sent: Thursday, August 6, 2026 2:14 PM
>> To: [email protected]
>> Cc: Zhang, Hawking <[email protected]>; Deucher, Alexander
>> <[email protected]>; Kamal, Asad <[email protected]>;
>> Wang, Yang(Kevin) <[email protected]>
>> Subject: [PATCH] drm/amd/pm: Track current PPT limit for restore
>>
>> Cache the applied PPT limit in the ppt limit range and refresh it on every get
>> and set. Initialize with the default value on fresh load; a value equal to
>> default is treated as unset.
>>
>> On suspend or reset recovery, restore the cached current limit when it differs
>> from the default, falling back to the default otherwise. This replaces the user-
>> mask based restore, covering limits changed outside the user profile path as
>> well.
>>
>> Signed-off-by: Lijo Lazar <[email protected]>
>> ---
>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 39 +++++++++++++----
>> --
>>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  1 +
>>   2 files changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> index f45cd4e31415..7edd919a7832 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> @@ -495,16 +495,11 @@ static void smu_restore_ppt_limits(struct
>> smu_context *smu,  {
>>        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;
>>
>> @@ -512,14 +507,14 @@ static void smu_restore_ppt_limits(struct
>> smu_context *smu,
>>                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];
>> +             range = &smu->ppt_limits.range[power_source][i];
>> +             if (range->current_value &&
>> +                 range->current_value != range->default_value)
>> +                     limit = range->current_value;
>> +             else if (restore_defaults)
>>                        limit = range->default_value;
>> -             } else {
>> +             else
>>                        continue;
>> -             }
>>
>>                ret = smu_set_ppt_limit(smu, i, limit);
>>                if (ret)
>> @@ -875,6 +870,21 @@ static int smu_early_init(struct amdgpu_ip_block
>> *ip_block)
>>        return smu_init_microcode(smu);
>>   }
>>
>> +static void smu_init_ppt_limits_current(struct smu_context *smu) {
>> +     struct smu_ppt_limit_range *range;
>> +     int i, j;
>> +
>> +     for (i = SMU_POWER_SOURCE_AC; i < SMU_POWER_SOURCE_COUNT;
>> i++) {
>> +             for (j = SMU_PPT_LIMIT_PPT0; j < SMU_LIMIT_TYPE_COUNT;
>> j++) {
>> +                     if (!(smu->ppt_limits.supported_mask & BIT(j)))
>> +                             continue;
>> +                     range = &smu->ppt_limits.range[i][j];
>> +                     range->current_value = range->default_value;
>> +             }
>> +     }
>> +}
>> +
>>   static int smu_set_default_dpm_table(struct smu_context *smu)  {
>>        struct amdgpu_device *adev = smu->adev; @@ -1911,6 +1921,9 @@
>> static int smu_smc_hw_setup(struct smu_context *smu)
>>        if (ret)
>>                dev_err(adev->dev, "Error during wbrf init call\n");
>>
>> +     if (!adev->in_suspend && !amdgpu_reset_in_recovery(adev))
>> +             smu_init_ppt_limits_current(smu);
>> +
>>        return ret;
>>   }
>>
>> @@ -2999,6 +3012,9 @@ int smu_get_ppt_limit(void *handle,
>>        switch (limit_level) {
>>        case SMU_PPT_LIMIT_CURRENT:
>>                ret = smu_get_asic_ppt_limit(smu, limit_type, limit);
>> +             if (!ret)
>> +                     smu-
>>> ppt_limits.range[power_source][limit_type].current_value =
>> +                             *limit;
>>                break;
>>        case SMU_PPT_LIMIT_DEFAULT:
>>                *limit = smu-
>>> ppt_limits.range[power_source][limit_type].default_value;
>> @@ -3061,6 +3077,7 @@ 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;
>> +     range->current_value = 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]
>> |= 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 92658eb3886d..5222b48eba25 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>> @@ -229,6 +229,7 @@ enum smu_ppt_limit_level {
>>
>>   struct smu_ppt_limit_range {
>>        uint32_t default_value;
>> +     uint32_t current_value;
>>        uint32_t min;
>>        uint32_t max;
>>        uint32_t od_min;
>> --
>> 2.49.0
>