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

On 06-Aug-26 1:32 PM, Lazar, Lijo wrote:
> 
> 
> 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.
> 

Hi Kevin,

Does this address your concern?

Thanks,
Lijo

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