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

[Jie Zhan]

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);
+       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;
+       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