Re: [PATCH v2] PM: EM: Exclude boost frequencies from inefficiency ranking

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

On 11/3/26 11:21, Guo Chen wrote:
> em_compute_costs() ranks all the frequencies of the table, boost ones
> included. A non-boost frequency can be marked inefficient only because
> a faster boost frequency has a lower or equal cost, e.g.
> 
>    3.9 GHz  4.0 GHz (cost C)  4.1 GHz (boost, cost C)
> 
> With boost disabled 4.1 GHz cannot be selected, but 4.0 GHz is still
> skipped by governors rounding down to efficient frequencies (e.g.
> ondemand with CPUFREQ_RELATION_HE), which land on 3.9 GHz instead of
> the best selectable frequency. Several frequencies can be affected.
> 
> Exclude the boost frequencies from the ranking in em_compute_costs() and
> keep them all efficient, using the highest non-boost frequency of the
> CPUFreq table, returned by the new helper
> cpufreq_table_max_nonboost_freq(), as the boundary.
> 
> Fixes: c8ed99533dbc ("PM: EM: Mark inefficient states")
> Signed-off-by: Guo Chen <guo.chen@oss.qualcomm.com>
> ---
> Changes in v2:
> - Drop prev_boost and keep all boost states efficient by resetting
>    prev_cost for them, as suggested by Lukasz. Boost states are no
>    longer ranked against each other.
> - Retitle the patch to match the new behaviour (was "Rank boost and
>    non-boost frequencies separately") and reword the commit message.
> 
> Link to v1: https://lore.kernel.org/all/20261030-em-rank-boost-separately-v1-1-238a6b2bb888@oss.qualcomm.com/
> ---
>   include/linux/cpufreq.h     | 24 ++++++++++++++++++++++++
>   kernel/power/energy_model.c | 16 ++++++++++++++++
>   2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d3d0d9d02aa4..c93316141a11 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -1155,6 +1155,24 @@ cpufreq_table_set_inefficient(struct cpufreq_policy *policy,
>   	return -EINVAL;
>   }
>   
> +static inline unsigned int
> +cpufreq_table_max_nonboost_freq(struct cpufreq_policy *policy)
> +{
> +	struct cpufreq_frequency_table *pos;
> +	unsigned int max = 0;
> +
> +	if (!policy->freq_table)
> +		return 0;
> +
> +	/* The table may be unsorted, scan all of it for the highest one. */
> +	cpufreq_for_each_valid_entry(pos, policy->freq_table) {
> +		if (!(pos->flags & CPUFREQ_BOOST_FREQ) && pos->frequency > max)
> +			max = pos->frequency;
> +	}
> +
> +	return max;
> +}
> +
>   static inline int parse_perf_domain(int cpu, const char *list_name,
>   				    const char *cell_name,
>   				    struct of_phandle_args *args)
> @@ -1219,6 +1237,12 @@ cpufreq_table_set_inefficient(struct cpufreq_policy *policy,
>   	return -EINVAL;
>   }
>   
> +static inline unsigned int
> +cpufreq_table_max_nonboost_freq(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
>   static inline int of_perf_domain_get_sharing_cpumask(int pcpu, const char *list_name,
>   						     const char *cell_name, struct cpumask *cpumask,
>   						     struct of_phandle_args *pargs)
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index d19a65461ae8..5b1d095fd376 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -255,15 +255,24 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>   			    unsigned long flags)
>   {
>   	unsigned long prev_cost = ULONG_MAX;
> +	struct cpufreq_policy *policy;
> +	unsigned int max_nonboost = 0;
>   	int i, ret;
>   
>   	/* This is needed only for CPUs and EAS skip other devices */
>   	if (!_is_cpu_device(dev))
>   		return 0;
>   
> +	policy = cpufreq_cpu_get(dev->id);
> +	if (policy) {
> +		max_nonboost = cpufreq_table_max_nonboost_freq(policy);
> +		cpufreq_cpu_put(policy);
> +	}
> +
>   	/* Compute the cost of each performance state. */
>   	for (i = nr_states - 1; i >= 0; i--) {
>   		unsigned long power_res, cost;
> +		bool boost = max_nonboost && table[i].frequency > max_nonboost;
>   
>   		if ((flags & EM_PERF_DOMAIN_ARTIFICIAL) && cb->get_cost) {
>   			ret = cb->get_cost(dev, table[i].frequency, &cost);
> @@ -284,6 +293,13 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>   			table[i].flags = EM_PERF_STATE_INEFFICIENT;
>   			dev_dbg(dev, "EM: OPP:%lu is inefficient\n",
>   				table[i].frequency);
> +		} else if (boost) {
> +			/*
> +			 * Keep all boost states efficient. Resetting prev_cost
> +			 * also keeps the highest non-boost state efficient,
> +			 * then rank the rest on their own.
> +			 */
> +			prev_cost = ULONG_MAX;
>   		} else {
>   			prev_cost = table[i].cost;
>   		}
> 
> ---
> base-commit: f1ac44c32cba88ad65dcecd4a279661d14e93651
> base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> change-id: 20261029-em-rank-boost-separately-a7e7c7d78058
> 
> Best regards,
> --
> Guo Chen <guo.chen@oss.qualcomm.com>


LGTM,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>