diff mbox series

[v2,12/17] PM: EM: Add argument to get_cost() for runtime modification

Message ID 20230512095743.3393563-13-lukasz.luba@arm.com
State New
Headers show
Series Introduce runtime modifiable Energy Model | expand

Commit Message

Lukasz Luba May 12, 2023, 9:57 a.m. UTC 
The Energy Model (EM) supports runtime modifications. Let also the
artificial EM use this new feature and allow to update the 'cost' values
at runtime. When the artificial EM is used there is a need to provide
two callbacks: get_cost() and update_power(), not only the last one.

Update also CPPC driver code, since the new argument is needed there
to compile properly and register EM.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 2 +-
 include/linux/energy_model.h   | 7 ++++++-
 kernel/power/energy_model.c    | 9 +++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

Comments

Dietmar Eggemann May 30, 2023, 9:53 a.m. UTC | #1 
On 12/05/2023 11:57, Lukasz Luba wrote:
> The Energy Model (EM) supports runtime modifications. Let also the
> artificial EM use this new feature and allow to update the 'cost' values
> at runtime. When the artificial EM is used there is a need to provide
> two callbacks: get_cost() and update_power(), not only the last one.
> 
> Update also CPPC driver code, since the new argument is needed there
> to compile properly and register EM.

Is there a real use case behind this? It can't be mobile which IMHO
drivers the rest of the 'Runtime modifiable EM' feature.

Do we know of any machine using the artificial EM. And do they care
about EM matching workload or static power?

[...]
Lukasz Luba July 3, 2023, 3:30 p.m. UTC | #2 
On 5/30/23 10:53, Dietmar Eggemann wrote:
> On 12/05/2023 11:57, Lukasz Luba wrote:
>> The Energy Model (EM) supports runtime modifications. Let also the
>> artificial EM use this new feature and allow to update the 'cost' values
>> at runtime. When the artificial EM is used there is a need to provide
>> two callbacks: get_cost() and update_power(), not only the last one.
>>
>> Update also CPPC driver code, since the new argument is needed there
>> to compile properly and register EM.
> 
> Is there a real use case behind this? It can't be mobile which IMHO
> drivers the rest of the 'Runtime modifiable EM' feature.

Correct, CPPC+EM is not for mobile phones, but notebooks. For now
the notebooks are not tested with that feature and were not in
requirement scope.

> 
> Do we know of any machine using the artificial EM. And do they care
> about EM matching workload or static power?
> 
> [...]

For now we don't know about such notebook which uses CPPC + EM
and if it's a candidate for this feature.

I thought, since it's just one patch w/ small change, it would be
consistent to not 'forget' about that notebooks angle. I know
that there are some folks who 'run'/'are willing to run' Arm laptop with
CPPC w/ artificial EM. They might pick that feature as well.
diff mbox series

Patch

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 022e3555407c..f5353c10552b 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -574,7 +574,7 @@  static int cppc_get_cpu_power(struct device *cpu_dev,
 }
 
 static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz,
-		unsigned long *cost)
+		unsigned long *cost, void *priv)
 {
 	unsigned long perf_step, perf_prev;
 	struct cppc_perf_caps *perf_caps;
diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index 8e3fa2b6bf28..b8506df9af2d 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -162,6 +162,8 @@  struct em_data_callback {
 	 * @freq	: Frequency at the performance state in kHz
 	 * @cost	: The cost value for the performance state
 	 *		(modified)
+	 * @priv	: Pointer to private data useful for tracking context
+	 *		during run-time modifications of EM.
 	 *
 	 * In case of CPUs, the cost is the one of a single CPU in the domain.
 	 * It is expected to fit in the [0, EM_MAX_POWER] range due to internal
@@ -170,7 +172,7 @@  struct em_data_callback {
 	 * Return 0 on success, or appropriate error value in case of failure.
 	 */
 	int (*get_cost)(struct device *dev, unsigned long freq,
-			unsigned long *cost);
+			unsigned long *cost, void *priv);
 
 	/**
 	 * update_power() - Provide new power at the given performance state of
@@ -199,6 +201,9 @@  struct em_data_callback {
 #define EM_DATA_CB(_active_power_cb)			\
 		EM_ADV_DATA_CB(_active_power_cb, NULL)
 #define EM_UPDATE_CB(_update_power_cb) { .update_power = &_update_power_cb }
+#define EM_ADV_UPDATE_CB(_update_power_cb, _cost_cb)	\
+	{ .update_power = &_update_power_cb,		\
+	  .get_cost = _cost_cb }
 
 struct em_perf_domain *em_cpu_get(int cpu);
 struct em_perf_domain *em_pd_get(struct device *dev);
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index b5675dda00e1..456d9f2b4370 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -165,7 +165,7 @@  static void em_perf_runtime_table_set(struct device *dev,
 
 static int em_compute_costs(struct device *dev, struct em_perf_state *table,
 			    struct em_data_callback *cb, int nr_states,
-			    unsigned long flags)
+			    unsigned long flags, void *priv)
 {
 	unsigned long prev_cost = ULONG_MAX;
 	u64 fmax;
@@ -177,7 +177,8 @@  static int em_compute_costs(struct device *dev, struct em_perf_state *table,
 		unsigned long power_res, cost;
 
 		if (flags & EM_PERF_DOMAIN_ARTIFICIAL && cb->get_cost) {
-			ret = cb->get_cost(dev, table[i].frequency, &cost);
+			ret = cb->get_cost(dev, table[i].frequency, &cost,
+					   priv);
 			if (ret || !cost || cost > EM_MAX_POWER) {
 				dev_err(dev, "EM: invalid cost %lu %d\n",
 					cost, ret);
@@ -277,7 +278,7 @@  int em_dev_update_perf_domain(struct device *dev, struct em_data_callback *cb,
 	}
 
 	ret = em_compute_costs(dev, runtime_table->state, cb,
-			       pd->nr_perf_states, pd->flags);
+			       pd->nr_perf_states, pd->flags, priv);
 	if (ret)
 		goto free_runtime_state_table;
 
@@ -347,7 +348,7 @@  static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
 		table[i].frequency = prev_freq = freq;
 	}
 
-	ret = em_compute_costs(dev, table, cb, nr_states, flags);
+	ret = em_compute_costs(dev, table, cb, nr_states, flags, NULL);
 	if (ret)
 		goto free_ps_table;