Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 16 Nov 2021 18:53:18 +0100 | Subject | Re: [PATCH v1] cpufreq: CPPC: Fix performance/frequency conversion |
| |
On Mon, Nov 15, 2021 at 11:19 AM Pierre Gondois <pierre.gondois@arm.com> wrote: > > Hello Rafael, > > On 11/5/21 15:40, Rafael J. Wysocki wrote: > > On Fri, Oct 22, 2021 at 9:51 AM Pierre Gondois <Pierre.Gondois@arm.com> wrote: > >> CPUfreq governors request CPU frequencies using information > >> on current CPU usage. The CPPC driver converts them to > >> performance requests. Frequency targets are computed as: > >> target_freq = (util / cpu_capacity) * max_freq > >> target_freq is then clamped between [policy->min, policy->max]. > >> > >> The CPPC driver converts performance values to frequencies > >> (and vice-versa) using cppc_cpufreq_perf_to_khz() and > >> cppc_cpufreq_khz_to_perf(). These functions both use two different > >> factors depending on the range of the input value. For > >> cppc_cpufreq_khz_to_perf(): > >> - (NOMINAL_PERF / NOMINAL_FREQ) or > >> - (LOWEST_PERF / LOWEST_FREQ) > >> and for cppc_cpufreq_perf_to_khz(): > >> - (NOMINAL_FREQ / NOMINAL_PERF) or > >> - ((NOMINAL_PERF - LOWEST_FREQ) / (NOMINAL_PERF - LOWEST_PERF)) > >> > >> This means the functions are not inverse for some values: > >> (perf_to_khz(khz_to_perf(x)) != x) > >> > >> This patch makes use of one single conversion factor, being > >> (MAX_PERF / MAX_FREQ). > >> > >> As LOWEST_FREQ is not used during conversion, the LOWEST_FREQ > >> advertised through policy->cpuinfo.min_freq might be different > >> from the LOWEST_FREQ value available in the CPPC object, > >> but the conversion will be correct. > > Well, this assumes that there is a linear perf <-> freq mapping which > > is a change in behavior. > The perf <-> freq relation is currently linear on 2 distinct segments. > > The patch is also intending handle the case of CPUs whose frequency and > performance values are not proportional. > > Example for the current code: > MAX_FREQ = 1.000.000 > MIN_FREQ = 300.000 > MAX_PERF = 100 ("nominal_perf" in the code) > MIN_PERF = 40 ("lowest_perf" in the code) > With these values, frequencies and performances are not proportional as > (MAX_FREQ / MAX_PERF) != (MIN_FREQ / MIN_PERF). > > A util of 40% gives: > target_freq = 40% * MAX_FREQ. > cppc_cpufreq_khz_to_perf() then requests: > target_perf = target_freq * (MIN_PERF / MIN_FREQ) > target_perf = 40% * 1.000.000 * (40 / 300.000) > target_perf = 53.3 > A performance request of 40 would have been sufficient. > > The error comes from the utilization of 2 different factors. > policy->max (frequency) is computed with this factor: > - (MAX_FREQ / MAX_PERF) > and frequency requests are mostly converted to performance > values with this factor: > - (MIN_PERF / MIN_FREQ) > > Using one single factor per conversion function, being > (MAX_PERF / MAX_FREQ) for cppc_cpufreq_khz_to_perf(): > target_perf = target_freq * (MAX_PERF / MAX_FREQ) > target_perf = 40% * 1.000.000 * (100 / 1.000.000) > target_perf = 40 > > While I agree that consistency is a good thing in general, won't this > > cause the values visible via sysfs to change? And if it does, won't > > it confuse anyone or break anything in user space? > Yes it changes the minimum frequency advertised on sysfs. It might > effectively be confusing. It should be possible to still advertise the > minimum frequency in the ACPI _CPC object while using one conversion > factor, but this will require more changes.
So why don't we make them?
| |