Re: [RFC][PATCH v0.1 6/6] cpufreq: intel_pstate: Add basic EAS support on hybrid platforms

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 08/11/2024 17:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify intel_pstate to register stub EM perf domains for CPUs on
> hybrid platforms via em_dev_register_perf_domain() and to use
> em_dev_expand_perf_domain() introduced previously for adding new
> CPUs to existing EM perf domains when those CPUs become online for
> the first time after driver initialization.
> 
> This change is targeting platforms (for example, Lunar Lake) where
> "small" CPUs (E-cores) are always more energy-efficient than the "big"
> or "performance" CPUs (P-cores) when run at the same HWP performance
> level, so it is sufficient to tell the EAS that E-cores are always
> preferred (so long as there is enough spare capacity on one of them
> to run the given task).

By treating all big CPUs (ignoring the different itmt prio values
between them) we would have a system in which PD's are not in sync with
the asym_cap_list* or the CPU capacities of individual CPUs and sched
groups within the sched domain. Not sure if we want to go this way?

* used by misfit handling - 22d5607400c6 ("sched/fair: Check if a task
has a fitting CPU when updating misfit")

> Accordingly, the perf domains are registered per CPU type (that is,
> all P-cores belong to one perf domain and all E-cores belong to another
> perf domain) and they are registered only if asymmetric CPU capacity is
> enabled.  Each perf domain has a one-element states table and that
> element only contains the relative cost value (the other fields in
> it are not initialized, so they are all equal to zero), and the cost
> value for the E-core perf domain is lower.

[...]

> +static int hybrid_pcore_cost(struct device *dev, unsigned long freq,
> +			     unsigned long *cost)
> +{
> +	/*
> +	 * The number used here needs to be higher than the analogous
> +	 * one in hybrid_ecore_cost() below.  The units and the actual
> +	 * values don't matter.
> +	 */
> +	*cost = 2;
> +	return 0;

So you're not tying this to HFI energy scores?

> +}
> +
> +static int hybrid_ecore_cost(struct device *dev, unsigned long freq,
> +			     unsigned long *cost)
> +{
> +	*cost = 1;
> +	return 0;
> +}
> +
> +static struct hybrid_em_perf_domain perf_domains[HYBRID_NR_TYPES] = {
> +	[HYBRID_PCORE] = { .cb.get_cost = hybrid_pcore_cost, },
> +	[HYBRID_ECORE] = { .cb.get_cost = hybrid_ecore_cost, }
> +};
> +
> +static bool hybrid_register_perf_domain(struct hybrid_em_perf_domain *pd)
> +{
> +	/*
> +	 * Registering EM perf domains without asymmetric CPU capacity
> +	 * support enabled is wasteful, so don't do that.
> +	 */
> +	if (!hybrid_max_perf_cpu)
> +		return false;
> +
> +	pd->dev = get_cpu_device(cpumask_first(&pd->cpumask));
> +	if (!pd->dev)
> +		return false;
> +
> +	if (em_dev_register_perf_domain(pd->dev, 1, &pd->cb, &pd->cpumask, false)) {
> +		pd->dev = NULL;
> +		return false;
> +	}
> +
> +	return true;
> +}

What are the issues in case you would use the existing ways (non-stub)
to setup the EM?

static int intel_pstate_get_cpu_cost()

static void intel_pstate_register_em(struct cpufreq_policy *policy)

  struct em_data_callback em_cb = EM_ADV_DATA_CB(NULL,
                                              intel_pstate_get_cpu_cost)

  em_dev_register_perf_domain(get_cpu_device(policy->cpu), 1,
                              &em_cb, policy->related_cpus, 1);
                                      ^^^^^^^^^^^^^^^^^^^^*

static void intel_pstate_set_register_em_fct(void)

  default_driver->register_em = intel_pstate_register_em

static int __init intel_pstate_init(void)

  ...
  intel_pstate_set_register_em_fct()
  ...

I guess one issue is the per-CPU policy as an argument to
em_dev_register_perf_domain() (*) ?

> +static void hybrid_register_all_perf_domains(void)
> +{
> +	enum hybrid_cpu_type type;
> +
> +	for (type = HYBRID_PCORE; type < HYBRID_NR_TYPES; type++)
> +		hybrid_register_perf_domain(&perf_domains[type]);
> +}
> +
> +static void hybrid_add_to_perf_domain(int cpu, enum hybrid_cpu_type type)
> +{
> +	struct hybrid_em_perf_domain *pd = &perf_domains[type];
> +
> +	guard(mutex)(&hybrid_capacity_lock);
> +
> +	if (cpumask_test_cpu(cpu, &pd->cpumask))
> +		return;
> +
> +	cpumask_set_cpu(cpu, &pd->cpumask);
> +	if (pd->dev)
> +		em_dev_expand_perf_domain(pd->dev, cpu);
> +	else if (hybrid_register_perf_domain(pd))
> +		em_rebuild_perf_domains();

I assume that the 'if' and the 'else if' condition here are only taken
when the CPU is brought online after boot?

[...]