Re: [PATCH v3 1/6] PM / EM: Fix inefficient states detection

[Matthias Kaehlcke <mka@chromium.org>]

On Fri, Jun 04, 2021 at 12:05:56PM +0100, Vincent Donnefort wrote:
> Currently, a debug message is printed if an inefficient state is detected
> in the Energy Model. Unfortunately, it won't detect if the first state is
> inefficient or if two successive states are. Fix this behavior.
> 
> Fixes: 27871f7a (PM: Introduce an Energy Model management framework)
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> Reviewed-by: Quentin Perret <qperret@google.com>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 0c620eb..c4871a8 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -107,8 +107,7 @@ static void em_debug_remove_pd(struct device *dev) {}
>  static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  				int nr_states, struct em_data_callback *cb)
>  {
> -	unsigned long opp_eff, prev_opp_eff = ULONG_MAX;
> -	unsigned long power, freq, prev_freq = 0;
> +	unsigned long power, freq, prev_freq = 0, prev_cost = ULONG_MAX;
>  	struct em_perf_state *table;
>  	int i, ret;
>  	u64 fmax;
> @@ -153,25 +152,19 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  
>  		table[i].power = power;
>  		table[i].frequency = prev_freq = freq;
> -
> -		/*
> -		 * The hertz/watts efficiency ratio should decrease as the
> -		 * frequency grows on sane platforms. But this isn't always
> -		 * true in practice so warn the user if a higher OPP is more
> -		 * power efficient than a lower one.
> -		 */
> -		opp_eff = freq / power;
> -		if (opp_eff >= prev_opp_eff)
> -			dev_dbg(dev, "EM: hertz/watts ratio non-monotonically decreasing: em_perf_state %d >= em_perf_state%d\n",
> -					i, i - 1);
> -		prev_opp_eff = opp_eff;
>  	}
>  
>  	/* Compute the cost of each performance state. */
>  	fmax = (u64) table[nr_states - 1].frequency;
> -	for (i = 0; i < nr_states; i++) {
> +	for (i = nr_states - 1; i >= 0; i--) {
>  		table[i].cost = div64_u64(fmax * table[i].power,
>  					  table[i].frequency);
> +		if (table[i].cost >= prev_cost) {
> +			dev_dbg(dev, "EM: OPP:%lu is inefficient\n",
> +				table[i].frequency);
> +		} else {
> +			prev_cost = table[i].cost;
> +		}

nit: curly braces aren't needed, especially if you change the 'dev_dbg'
statement to be a single line.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>