Message-ID: <a0adb942-03a3-b0d0-1b4d-1e2111247fab@arm.com>
Date:   Mon, 21 Aug 2023 17:13:53 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     rui.zhang@...el.com, amit.kucheria@...durent.com,
        amit.kachhap@...il.com, daniel.lezcano@...aro.org,
        viresh.kumar@...aro.org, len.brown@...el.com, pavel@....cz,
        Pierre.Gondois@....com, ionela.voinescu@....com,
        mhiramat@...nel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, rafael@...nel.org
Subject: Re: [PATCH v3 12/12] Documentation: EM: Update with runtime
 modification design



On 8/16/23 14:08, Dietmar Eggemann wrote:
> On 21/07/2023 17:50, Lukasz Luba wrote:
>> Add a new section 'Design' which covers the information about Energy
>> Model. It contains the design decisions, describes models and how they
>> reflect the reality. Add description of the basic const. EM. Change the
> 
> Nit pick: Can we just use one tag for this EM? I would propose
> 'default'. So code and doc are aligned.

Yes, I'll change it.

> 
>> other section IDs. Add documentation bit for the new feature which
>> allows o modify the EM in runtime.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>> ---
>>   Documentation/power/energy-model.rst | 150 +++++++++++++++++++++++++--
>>   1 file changed, 140 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/power/energy-model.rst b/Documentation/power/energy-model.rst
>> index ef341be2882b..01d4d806a123 100644
>> --- a/Documentation/power/energy-model.rst
>> +++ b/Documentation/power/energy-model.rst
>> @@ -72,16 +72,70 @@ required to have the same micro-architecture. CPUs in different performance
>>   domains can have different micro-architectures.
>>   
>>   
>> -2. Core APIs
>> +2. Design
>> +-----------------
>> +
>> +2.1 Basic EM
>> +^^^^^^^^^^^^
>> +
>> +The basic EM is built around constant power information for each performance
>> +state, which is accessible in: 'dev->em_pd->default_table->state'. This model
>> +can be derived based on power measurements of the device e.g. CPU while
>> +running some benchmark. The benchmark might be integer heavy or floating point
>> +computation with a data set fitting into the CPU cache or registers. Bare in
>> +mind that this model might not cover all possible workloads running on CPUs.
>> +Thus, please run a few different benchmarks and verify with some real
>> +workloads your power model values. The power variation due to the workload
>> +instruction mix and data set is not modeled. The static power, which can
>> +change during runtime due to variation of SOC temperature, is not modeled in
>> +this basic EM.
> 
> s/in this basic EM/either

OK

> 
>> +
>> +2.2 Runtime modifiable EM
>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +To better reflect power variation due to static power (leakage) the EM
>> +supports runtime modifications of the power values. The mechanism relies on
>> +RCU to free the modifiable EM perf_state table memory. Its user, the task
>> +scheduler, also uses RCU to access this memory. The EM framework is
>> +responsible for allocating the new memory for the modifiable EM perf_state
>> +table. The old memory is freed automatically using RCU callback mechanism.
>> +This design decision is made based on task scheduler using that data and
>> +to prevent wrong usage of kernel modules if they would be responsible for the
>> +memory management.
>> +
>> +There are two structures with the performance state tables in the EM:
>> +a) dev->em_pd->default_table
>> +b) dev->em_pd->runtime_table
>> +They both point to the same memory location via:
>> +'em_perf_table::state' pointer, until the first modification of the values
>> +This should save memory on platforms which would never modify the EM. When
>> +the first modification is made the 'default_table' (a) contains the old
>> +EM which was created during the setup. The modified EM is available in the
>> +'runtime_table' (b).
>> +
>> +Only EAS uses the 'runtime_table' and benefits from the updates to the
>> +EM values. Other sub-systems (thermal, powercap) use the 'default_table' (a)
>> +since they don't need such optimization.
> 
> The interested user immediately ask WHY here. Maybe skip this last part
> of the sentence in case you don't want to provide the WHY here.

Agree, I'll remove that bit.

> 
>> +The drivers which want to modify the EM values are protected from concurrent
> 
> Does it have to be a driver?

No, it can be any kernel code. I'll change that description to:
s/drivers/kernel code

> 
>> +access using a mutex. Therefore, the drivers must use sleeping context when
>> +they want to modify the EM. The runtime modifiable EM might also be used for
>> +better reflecting real workload scenarios, e.g. when they pop-up on the screen
> 
> I would describe this a little bit more generic. The current description
> seems to be linked to Android foreground processes.

OK, I'll change to more generic scenario.

> 
>> +and will run for longer period, such as: games, video recoding or playing,
>> +video calls, etc. It is up to the platform engineers to experiment and choose
>> +the right approach for their device.
> 
> Maybe you can say here that with the runtime modifiable EM we switch
> from a 'single and during the entire runtime static EM' (system
> property) design to a 'single EM which can be changed during runtime
> according e.g. to the workload' (system and workload property) design.

Yes, that's better and more generic description. I'll take it.

> 
> [...]
> 
>> +3.4 Runtime modifications
>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Drivers willing to modify the EM at runtime should use the following API::
>> +
>>   
>> +  int em_dev_update_perf_domain(struct device *dev,
>> +			struct em_data_callback *cb, void *priv);
>>   
>> -2.4 Description details of this API
>> +Drivers must provide a callback .update_power() returning power value for each
>> +performance state. The callback function provided by the driver is free
>> +to fetch data from any relevant location (DT, firmware, ...) or sensor.
>> +The .update_power() callback is called by the EM for each performance state to
>> +provide new power value. In the Section 4.2 there is an example driver
>> +which shows simple implementation of this mechanism. The callback can be
>> +declared with EM_UPDATE_CB() macro. The caller of that callback also passes
>> +a private void pointer back to the driver which tries to update EM.
>> +It is useful and helps to maintain the consistent context for all performance
>> +state calls for a given EM.
>> +The artificial EM also supports runtime modifications. For this type of EM
>> +there is a need to provide one more callback: .get_cost(). The .get_cost()
>> +returns the cost value for each performance state, which better reflects the
>> +efficiency of the CPUs which use artificial EM. Those two callbacks:
>> +.update_power() and get .get_cost() can be declared with one macro
>> +EM_ADV_UPDATE_CB() and then passed to the em_dev_update_perf_domain().
> 
> Can't find EM_ADV_UPDATE_CB().
> 
> [...]
> 

Good catch, that macro was for the artificial EM but we dropped that
feature from it.

Powered by blists - more mailing lists