diff mbox series

[v4,6/7] cpufreq/cppc: set the frequency used for computing the capacity

Message ID 20231027080400.56703-7-vincent.guittot@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series consolidate and cleanup CPU capacity | expand

Commit Message

Vincent Guittot Oct. 27, 2023, 8:03 a.m. UTC
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.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/arch_topology.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Pierre Gondois Nov. 2, 2023, 9:06 a.m. UTC | #1
Hello Vincent,

On 10/27/23 10:03, Vincent Guittot 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.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>   drivers/base/arch_topology.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 9a073c2d2086..d4bef370feb3 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -349,6 +349,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
>   
>   void topology_init_cpu_capacity_cppc(void)
>   {
> +	u64 capacity, capacity_scale = 0;
>   	struct cppc_perf_caps perf_caps;
>   	int cpu;
>   
> @@ -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]);
> +

To continue Beata's point, I think there is an issue with the following:
cppc_perf_to_khz() and cppc_khz_to_perf() were previously used with a struct containing
frequencies in KHz, cf. [1].
In the original _CPC object, frequencies are in MHz. It means that the perf_caps struct
here contains frequencies in MHz, and per_cpu(capacity_ref_freq, cpu) is in MHz aswell.

[1] https://github.com/torvalds/linux/blob/master/drivers/cpufreq/cppc_cpufreq.c#L682


>   			pr_debug("cpu_capacity: CPU%d cpu_capacity=%u (raw).\n",
>   				 cpu, raw_capacity[cpu]);
>   			continue;
> @@ -375,7 +380,15 @@ void topology_init_cpu_capacity_cppc(void)
>   		goto exit;
>   	}
>   
> -	topology_normalize_cpu_scale();
> +	for_each_possible_cpu(cpu) {
> +		capacity = raw_capacity[cpu];
> +		capacity = div64_u64(capacity << SCHED_CAPACITY_SHIFT,
> +				     capacity_scale);
> +		topology_set_cpu_scale(cpu, capacity);
> +		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> +			cpu, topology_get_cpu_scale(cpu));
> +	}
> +
>   	schedule_work(&update_topology_flags_work);
>   	pr_debug("cpu_capacity: cpu_capacity initialization done\n");
>
Vincent Guittot Nov. 2, 2023, 10:42 a.m. UTC | #2
Hi Pierre,

On Thu, 2 Nov 2023 at 10:07, Pierre Gondois <pierre.gondois@arm.com> wrote:
>
> Hello Vincent,
>
> On 10/27/23 10:03, Vincent Guittot 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.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >   drivers/base/arch_topology.c | 15 ++++++++++++++-
> >   1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > index 9a073c2d2086..d4bef370feb3 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -349,6 +349,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> >
> >   void topology_init_cpu_capacity_cppc(void)
> >   {
> > +     u64 capacity, capacity_scale = 0;
> >       struct cppc_perf_caps perf_caps;
> >       int cpu;
> >
> > @@ -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]);
> > +
>
> To continue Beata's point, I think there is an issue with the following:
> cppc_perf_to_khz() and cppc_khz_to_perf() were previously used with a struct containing
> frequencies in KHz, cf. [1].
> In the original _CPC object, frequencies are in MHz. It means that the perf_caps struct
> here contains frequencies in MHz, and per_cpu(capacity_ref_freq, cpu) is in MHz aswell.

Yeah, I haven't noticed this intermediate step in cppc_cpufreq. I'm
going to fix this

>
> [1] https://github.com/torvalds/linux/blob/master/drivers/cpufreq/cppc_cpufreq.c#L682
>
>
> >                       pr_debug("cpu_capacity: CPU%d cpu_capacity=%u (raw).\n",
> >                                cpu, raw_capacity[cpu]);
> >                       continue;
> > @@ -375,7 +380,15 @@ void topology_init_cpu_capacity_cppc(void)
> >               goto exit;
> >       }
> >
> > -     topology_normalize_cpu_scale();
> > +     for_each_possible_cpu(cpu) {
> > +             capacity = raw_capacity[cpu];
> > +             capacity = div64_u64(capacity << SCHED_CAPACITY_SHIFT,
> > +                                  capacity_scale);
> > +             topology_set_cpu_scale(cpu, capacity);
> > +             pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> > +                     cpu, topology_get_cpu_scale(cpu));
> > +     }
> > +
> >       schedule_work(&update_topology_flags_work);
> >       pr_debug("cpu_capacity: cpu_capacity initialization done\n");
> >
diff mbox series

Patch

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 9a073c2d2086..d4bef370feb3 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -349,6 +349,7 @@  bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 
 void topology_init_cpu_capacity_cppc(void)
 {
+	u64 capacity, capacity_scale = 0;
 	struct cppc_perf_caps perf_caps;
 	int cpu;
 
@@ -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]);
+
 			pr_debug("cpu_capacity: CPU%d cpu_capacity=%u (raw).\n",
 				 cpu, raw_capacity[cpu]);
 			continue;
@@ -375,7 +380,15 @@  void topology_init_cpu_capacity_cppc(void)
 		goto exit;
 	}
 
-	topology_normalize_cpu_scale();
+	for_each_possible_cpu(cpu) {
+		capacity = raw_capacity[cpu];
+		capacity = div64_u64(capacity << SCHED_CAPACITY_SHIFT,
+				     capacity_scale);
+		topology_set_cpu_scale(cpu, capacity);
+		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
+			cpu, topology_get_cpu_scale(cpu));
+	}
+
 	schedule_work(&update_topology_flags_work);
 	pr_debug("cpu_capacity: cpu_capacity initialization done\n");