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

[Powell, Darren]

[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?
 /* seed the cached smu power limit values iff get_power_limit is defined, 
otherwise they remain 0 */

Thanks
Darren

________________________________
From: Lazar, Lijo <lijo.la...@amd.com>
Sent: Monday, October 4, 2021 6:43 AM
To: Powell, Darren <darren.pow...@amd.com>; amd-gfx@lists.freedesktop.org 
<amd-gfx@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.pow...@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,
>