Re: [PATCH v2 1/3] cppc_cpufreq: Return desired perf in ->get() if feedback counters are 0
From: Jie Zhan <zhanjie9@hisilicon.com>
Date: 2024-09-13 12:10:04
Also in:
linux-acpi, linux-pm
Subsystem:
cpu frequency drivers - cppc cpufreq, cpu frequency scaling framework, the rest · Maintainers:
"Rafael J. Wysocki", Viresh Kumar, Linus Torvalds
Hi Ionela, On 12/09/2024 17:43, Ionela Voinescu wrote: ...
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.quoted
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