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-18 02:07:21
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
On 17/09/2024 18:36, Ionela Voinescu wrote: ...
quoted
quoted
@@ -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(). ...
quoted
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