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.
@@ -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;
@@ -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);
@@ -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;