[PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct

[Lazar, Lijo]

On 10/5/2021 10:34 AM, Powell, Darren wrote:
> [AMD Official Use Only]
> 
> 
> I'm just looking to clarify this code. The macro eventually expands to 
> look like this
> 
>     if ((smu)->ppt_funcs)
>     {
>        if ((smu)->ppt_funcs->get_power_limit)
>            (smu)->ppt_funcs->get_power_limit(smu,
>                                              &smu->current_power_limit, 
> &smu->default_power_limit,
> &smu->max_power_limit);
>        else
>           return 0;
>     }
>     else
>        return -EINVAL;
> 
> But you have to dig to realize that it's a macro, and that it makes no 
> modification if the function is not defined. It's not clear without then 
> searching and following the function pointers which platforms are using 
> the saved value. I thought of inserting the following comment or should 
> I just drop this altogether?

Yes. The limit values are by default initialized to 0. If the function 
is not supported, it's not considered an error to fail late_init (it 
only reports 0 values in hwmon).

late_init also gets called on other occasions like resume or after a 
GPU-only reset. If you want to change the invalid limit value in hwmon 
from 0 to something else (to differentiate unsupported vs API returning 
0 value), then better to do in sw_init.

Thanks,
Lijo

>   /* seed the cached smu power limit values iff get_power_limit is 
> defined, otherwise they remain 0 */
> 
> Thanks
> Darren
> 
> ------------------------------------------------------------------------
> *From:* Lazar, Lijo <Lijo.Lazar at amd.com>
> *Sent:* Monday, October 4, 2021 6:43 AM
> *To:* Powell, Darren <Darren.Powell at amd.com>; 
> amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
> *Subject:* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached 
> power limits in smu struct
> 
> 
> On 10/3/2021 10:16 AM, Darren Powell wrote:
>> Code appears to initialize values but macro will exit without error
>> or initializing value if function is not implmented
>> 
>> Signed-off-by: Darren Powell <darren.powell at amd.com>
>> ---
>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> index faa78a048b1f..210f047e136d 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> @@ -712,6 +712,10 @@ static int smu_late_init(void *handle)
>>                return ret;
>>        }
>>   
>> +     smu->current_power_limit = 0;
>> +     smu->default_power_limit = 0;
>> +     smu->max_power_limit = 0;
>> +
> 
> If this is only about first-time init - smu_context is part of adev, it
> will be zero initialized when adev is allocated.
> 
> 
> Thanks,
> Lijo
> 
>>        ret = smu_get_asic_power_limits(smu,
>>                                        &smu->current_power_limit,
>>                                        &smu->default_power_limit,
>>