[PATCH v2 1/3] cppc_cpufreq: Return desired perf in ->get() if feedback counters are 0

[Jie Zhan]

On 18/09/2024 18:15, Ionela Voinescu wrote:
> Hi,
> 
> On Wednesday 18 Sep 2024 at 10:05:13 (+0800), Jie Zhan wrote:
>>
>>
>> On 17/09/2024 18:36, Ionela Voinescu wrote:
>>
>> ...
>>
>>>>> @@ -747,19 +750,22 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>>>>         cpufreq_cpu_put(policy);
>>>>>
>>>>>         ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>>>>> -       if (ret)
>>>>> -               return 0;
>>>>> -
>>>>> -       udelay(2); /* 2usec delay between sampling */
>>>>> -
>>>>> -       ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>>>> -       if (ret)
>>>>> -               return 0;
>>>>> +       if (!ret) {
>>>>> +               udelay(2); /* 2usec delay between sampling */
>>>>> +               ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>>>> +       }
>>>>> +       if (!ret)
>>>>> +               delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>>>> +                                                      &fb_ctrs_t1);
>>>>
>>>> TBH, 'if (!ret)' style looks very strange to me.
>>>> We haven't done so anywhere in cppc_cpufreq, so let's keep consistency and make
>>>> it easier for people to read and maintain?
>>>
>>> I agree it's a bit of a difficult read, that's why I only sent my code
>>> as a suggestion. I did like the benefit of not having to have two
>>> different calls to cppc_perf_to_khz() and making the code below common
>>> for the error and non-error paths. But it's up to you. 
>>
>> Yeah understood. I did try minimizing duplicate code, but ended up with either
>> duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz().
>>
>> ...
>>>>
>>>>           delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>>>                                                  &fb_ctrs_t1);
>>>
>>> You need a check here for !delivered_perf (when at least one of the
>>> deltas is 0) in which case it would be good to take the same error path
>>> below. Something like:
>>>
>>>             if(delivered_perf)
>>> 	            return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>>> 	    else
>>> 		ret = -EFAULT;
>>>
>>> That's why I did the tricky if/else dance above as we need to take the
>>> error path below for multiple cases.
>>>
>>> Thanks,
>>> Ionela.
>>>
>>
>> Sure, thanks for reminding this.
>>
>> ...
>>
>> How does this look? I think this should have the least duplicate code except for
>> two cppc_perf_to_khz() calls, while keeping the logic easy to follow.
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bafa32dd375d..6070444ed098 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>  
>>         perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>>                                      &fb_ctrs);
>> +       if (!perf)
>> +               return;
>> +
>>         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>  
>>         perf <<= SCHED_CAPACITY_SHIFT;
>> @@ -726,11 +729,27 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>  
>>         /* Check to avoid divide-by zero and invalid delivered_perf */
>>         if (!delta_reference || !delta_delivered)
>> -               return cpu_data->perf_ctrls.desired_perf;
>> +               return 0;
>>  
>>         return (reference_perf * delta_delivered) / delta_reference;
>>  }
>>  
>> +static int cppc_get_perf_ctrs_sample(int cpu,
>> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t1)
>> +{
>> +       int ret;
>> +
>> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
>> +       if (ret)
>> +               return ret;
>> +
>> +       udelay(2); /* 2usec delay between sampling */
>> +
>> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
> 
> nit: white line before return.
> 
>> +       return ret;
>> +}
>> +
>>  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>  {
>>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> @@ -746,20 +765,30 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>  
>>         cpufreq_cpu_put(policy);
>>  
>> -       ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -       if (ret)
>> -               return 0;
>> -
>> -       udelay(2); /* 2usec delay between sampling */
>> -
>> -       ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>> -       if (ret)
>> -               return 0;
>> +       ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
>> +       if (ret) {
>> +               if (ret == -EFAULT)
>> +                       goto out_invalid_counters;
>> +               else
>> +                       return 0;
>> +       }
>>  
>>         delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>>                                                &fb_ctrs_t1);
>> +       if (!delivered_perf)
>> +               goto out_invalid_counters;
>>  
>>         return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>> +
>> +out_invalid_counters:
>> +       /*
>> +        * Feedback counters could be unchanged or 0 when a cpu enters a
>> +        * low-power idle state, e.g. clock-gated or power-gated.
>> +        * Get the lastest or cached desired perf for reflecting frequency.
>> +        */
>> +       if (cppc_get_desired_perf(cpu, &delivered_perf))
>> +               delivered_perf = cpu_data->perf_ctrls.desired_perf;
> 
> nit: same white line before return here :).
> 
> Looks good, thanks for the changes.
> 
> Reviewed-by: Ionela Voinescu <ionela.voinescu at arm.com>
> 
> Ionela.

Sure, thanks. I'll send a V3 based on this.

Jie

> 
>> +       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>>  }
>>  
>>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>>
>>
>> Thanks!
>> Jie
>