On Fri, 20 Oct 2023 at 18:05, Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
>
> Hello Vincent,
>
> On 10/18/23 19:26, Rafael J. Wysocki wrote:
> > On Wed, Oct 18, 2023 at 6:25 PM Vincent Guittot
> > <vincent.guittot@xxxxxxxxxx> wrote:
> >>
> >> Save the frequency associated to the performance that has been used when
> >> initializing the capacity of CPUs.
> >> Also, cppc cpufreq driver can register an artificial energy model. In such
> >> case, it needs the frequency for this compute capacity.
> >> We moved and renamed cppc_perf_to_khz and cppc_perf_to_khz to use them
> >> outside cppc_cpufreq in topology_init_cpu_capacity_cppc().
> >>
> >> Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> >
> > For the changes in drivers/acpi/cppc_acpi.c :
> >
> > Acked-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> >
> >> ---
> >> drivers/acpi/cppc_acpi.c | 93 ++++++++++++++++++++++
> >> drivers/base/arch_topology.c | 15 +++-
> >> drivers/cpufreq/cppc_cpufreq.c | 141 ++++++---------------------------
> >> include/acpi/cppc_acpi.h | 2 +
> >> 4 files changed, 133 insertions(+), 118 deletions(-)
> >>
>
> [snip]
>
> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >> index 9a073c2d2086..2372ce791bb4 100644
> >> --- a/drivers/base/arch_topology.c
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -350,6 +350,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> >> void topology_init_cpu_capacity_cppc(void)
> >> {
> >> struct cppc_perf_caps perf_caps;
> >> + u64 capacity, capacity_scale;
>
> I think capacity_scale should be initialized to 0 here,
> since it is used to find the max value of raw_capacity[cpu].
yes
>
> >> int cpu;
> >>
> >> if (likely(!acpi_cpc_valid()))
> >> @@ -365,6 +366,10 @@ void topology_init_cpu_capacity_cppc(void)
> >> (perf_caps.highest_perf >= perf_caps.nominal_perf) &&
> >> (perf_caps.highest_perf >= perf_caps.lowest_perf)) {
> >> raw_capacity[cpu] = perf_caps.highest_perf;
> >> + capacity_scale = max_t(u64, capacity_scale, raw_capacity[cpu]);
> >> +
> >> + per_cpu(capacity_ref_freq, cpu) = cppc_perf_to_khz(&perf_caps, raw_capacity[cpu]);
>
> I think capacity_ref_freq in in Hz, so the freq should be multiplied by 1000 .
yes, I forgot the *1000. I'm going to add * HZ_PER_KHZ
>
> With these two modifications, the patches worked well on a cppc-based platform.
>
> Sorry I forgot to detail what it was. It's a modified Juno with CPPC enabled. AMUs are not
> enabled, so the CPPC performance counters are not handled correctly and FIE cannot be enabled,
> but it is possible to change frequencies.
>
> The _CPC objects are setup as:
> little CPUs:
> - lowest_freq = 450 (MHz)
> - nominal_freq = 800 (MHz)
> - highest_perf = 383 * 1000
> - nominal_perf = 322 * 1000
> - lowest_perf = 181 * 1000
> - lowest_nonlinear_perf = 181 * 1000
>
> big CPUs:
> - lowest_freq = 600 (MHz)
> - nominal_freq = 1200 (MHz)
> - highest_perf = 1024 * 1000
> - nominal_perf = 833 * 1000
> - lowest_perf = 512 * 1000
> - lowest_nonlinear_perf = 512 * 1000
>
> As a remainder, available frequencies are:
> - little CPUs: 450, 800, 950 MHz
> - big CPUs: 600, 1000, 1200 Mhz
> So the platform is setup to have the last frequency as a boost frequency (for testing).
>
> ----
>
> Just to make a note of 2 potential side-issues for later (independent from these patches):
>
> - When testing with boosted/non-bossted frequencies, it didn't seem that cpu_overutilized()
> was taking the maximum frequency into consideration. This might mean that when lowering the
> maximum frequency of a policy, the maximum capacity of the CPUs of this policy is used
> to detect over-utilization.
> I would have thought that the over-utilization threshold would be lowered at the same time.
No it's not, It will be part of a next step patchset. This patchset
aims to consolidate and use the same reference so we can then easily
propagate changes if needed
>
> - Similarly for EAS, the energy computation doesn't take into account the maximum frequency
> of the policy. This should mean that EAS is taking into consideration frequencies that
> are not actually available.
>
>
> Regards,
> Pierre
