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

[Jie Zhan]

Hi Ionela,

On 12/09/2024 17:43, Ionela Voinescu wrote:

...

> 
> A possible (slimmer) alternative implementation for you to consider
> (this merges patches 1 & 2):
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..c16be9651a6f 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)
> +               perf = cpu_data->perf_ctrls.desired_perf;
> +

I think it's better to just return here.
If feedback counters are successfully read but unchanged, the following
calculation and update in cppc_scale_freq_workfn() is meaningless because it
won't change anything.

>         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
> 
>         perf <<= SCHED_CAPACITY_SHIFT;
> @@ -726,7 +729,7 @@ 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;

This makes sense to me.
Here is probably why Patch 2 looks bulky.

> 
>         return (reference_perf * delta_delivered) / delta_reference;
>  }
> @@ -736,7 +739,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>         struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>         struct cppc_cpudata *cpu_data;
> -       u64 delivered_perf;
> +       u64 delivered_perf = 0;
>         int ret;
> 
>         if (!policy)
> @@ -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?

> +       if ((ret == -EFAULT) || !delivered_perf) {
> +               if (cppc_get_desired_perf(cpu, &delivered_perf))
> +                       delivered_perf = cpu_data->perf_ctrls.desired_perf;

will take this.

> +       }
> 
> -       delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> -                                              &fb_ctrs_t1);
> +       if (delivered_perf)
> +               return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> 
> -       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +       return 0;
>  }
> 
> disclaimer: not fully checked so likely not "production ready" code :)
> 
> Hope it helps,
> Ionela.
> 
>>  
>>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>> -- 
>> 2.33.0
>>
> 

How about this? merged patch 1 & 2 as well.

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..411303f2e8cb 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,7 +729,7 @@ 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;
   }
@@ -748,18 +751,32 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)

          ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
          if (ret)
-               return 0;
+               goto out_err;

          udelay(2); /* 2usec delay between sampling */

          ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
          if (ret)
-               return 0;
+               goto out_err;

          delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
                                                 &fb_ctrs_t1);

          return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
+
+out_err:
+       /*
+        * Feedback counters could be 0 when cores are powered down.
+        * Take desired perf for reflecting frequency in this case.
+        */
+       if (ret == -EFAULT) {
+               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);
+       }
+
+       return 0;
   }

   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
---

Thanks indeed!
Jie