Re: [PATCH v2 3/3] PM / EM: Skip inefficient OPPs

[Lukasz Luba <lukasz.luba@arm.com>]

On 5/21/21 5:54 PM, Vincent Donnefort wrote:
> Some SoCs, such as the sd855 have OPPs within the same performance domain,
> whose cost is higher than others with a higher frequency. Even though
> those OPPs are interesting from a cooling perspective, it makes no sense
> to use them when the device can run at full capacity. Those OPPs handicap
> the performance domain, when choosing the most energy-efficient CPU and
> are wasting energy. They are inefficient.
> 
> Hence, add support for such OPPs to the Energy Model. The table can now
> be read skipping inefficient performance states (and by extension,
> inefficient OPPs).
> 
> Currently, the efficient table is used in two paths. Schedutil, and
> find_energy_efficient_cpu(). We have to modify both paths in the same
> patch so they stay synchronized. The thermal framework still relies on
> the full table.
> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index 9be7bde..daaeccf 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -17,13 +17,25 @@
>    *		device). It can be a total power: static and dynamic.
>    * @cost:	The cost coefficient associated with this level, used during
>    *		energy calculation. Equal to: power * max_frequency / frequency
> + * @flags:	see "em_perf_state flags" description below.
>    */
>   struct em_perf_state {
>   	unsigned long frequency;
>   	unsigned long power;
>   	unsigned long cost;
> +	unsigned long flags;

Maybe for now, we can have 'bool' here?
We would avoid *Num_opps of 'and' operations below

[snip]

> +static inline
> +struct em_perf_state *em_pd_get_efficient_state(struct em_perf_domain *pd,
> +						unsigned long freq)
> +{
> +	struct em_perf_state *ps;
> +	int i;
> +
> +	for (i = 0; i < pd->nr_perf_states; i++) {
> +		ps = &pd->table[i];
> +		if (ps->flags & EM_PERF_STATE_INEFFICIENT)

Here, we can avoid this *N of '&', when having a simple bool

> +			continue;
> +		if (ps->frequency >= freq)
> +			break;
> +	}
> +
> +	return ps;
> +}
> +

[snip

> +static inline unsigned long em_pd_get_efficient_freq(struct em_perf_domain *pd,
> +						     unsigned long freq)
> +{
> +	struct em_perf_state *ps;
> +
> +	if (!pd || !(pd->flags & EM_PERF_DOMAIN_INEFFICIENCIES))

This one is OK, since we have two features for this 'flags' now.

The rest looks good.

Regards,
Lukasz