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

On 06-Aug-26 1:26 PM, Wang, Yang(Kevin) wrote:
> AMD General
> 
>> 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.
> 
> No, the original code already contains this restore logic, include suspend/resume + gpu recovery, and save/restore logic has been verified on Navi21 and Navi48 ASICs.
> 
> And the if no user edit power limit from user side, the driver will skip restore power limit, in this case, the driver will use default value after firmware reloading.
> 
> btw, your change will break gpu od + power limit logic in navi3x/navi4x.

This patch doesn't break that. Whatever user has set as the last limit 
will be the current limit in Navi3x/Navi4x case. That will be restored. 
If user hasn't edited limit, current limit will be equivalent to default 
limit and then it's not set again.

> 
> So What issue you have facing now ?
> 

Driver remains responsible for restoring the state triggered by inband 
actions like suspend/resume or reset as OOB doesn't monitor everything. 
In such cases, driver being the inband owner needs to restore the proper 
state after the event.

Thanks,
Lijo
> Best Regards,
> Kevin
> 
>> -----Original Message-----
>> From: Lazar, Lijo <[email protected]>
>> Sent: Thursday, August 6, 2026 3:48 PM
>> To: Wang, Yang(Kevin) <[email protected]>; amd-
>> [email protected]
>> Cc: Zhang, Hawking <[email protected]>; Deucher, Alexander
>> <[email protected]>; Kamal, Asad <[email protected]>
>> Subject: 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
>>>
>