quoted

quoted

n 08/01/26 22:16, Pierre Gondois wrote:

quoted

External email: Use caution opening links or attachments


Hello Sumit, Lifeng,

On 12/23/25 13:13, Sumit Gupta wrote:

quoted

When autonomous selection (auto_sel) is enabled, the hardware controls
performance within min_perf/max_perf register bounds making the
scaling_min/max_freq effectively read-only.

If auto_sel is set, the governor associated to the policy will have no
actual control.

E.g.:
If the schedutil governor is used, attempts to set the
frequency based on CPU utilization will be periodically
sent, but they will have no effect.

The same thing will happen for the ondemand, performance,
powersave, userspace, etc. governors. They can only work if
frequency requests are taken into account.

------------

This looks like the intel_pstate governor handling where it is possible
not to have .target() or .target_index() callback and the hardware is in
charge (IIUC).
For this case, only 2 governor seem available: performance and powersave.

As you mentioned in [2], 'it still makes sense to have cpufreq requesting a
certain performance level even though autonomous selection is enabled'. So I
think it's OK to have a governor when auto_selection is enabled.

[2] https://lore.kernel.org/all/9f46991d-98c3-41f5-8133-6612b397e33a@arm.com/ (local)

quoted

Thanks for pointing me to the first version, I forgot how your
first implementation was.

quoted

In v1 [1], I added a separate cppc_cpufreq_epp_driver instance without
target*() hooks, using setpolicy() instead (similar to AMD pstate).
However, this approach doesn't allow per-CPU control: if we boot with the
EPP driver, we can't dynamically disable auto_sel for individual CPUs and
return to OS governor control (no target hook available). AMD and Intel
pstate drivers seem to set HW autonomous mode for all CPUs globally,
not per-CPU. So, changed it in v2.
[1] https://lore.kernel.org/lkml/20250211103737.447704-6-sumitg@nvidia.com/ (local)

Ok right.
This is something I don't really understand in the current intel/amd cpufreq
drivers. FWIU:
- the cpufreq drivers abstractions allow to access different hardware
- the governor abstraction allows to switch between different algorithms
to select the 'correct' frequency.

So IMO switching to autonomous selection should be done by switching
to another governor and the 'auto_sel' file should not be accessible to users.

------------

Being able to enable/disable the autonomous selection on a per-policy
base seems a valid use-case. It also seems to fit the per-policy governor
capabilities.

I'm OK with adding an auto-selection governor. It's better to keep this
governor only in cppc_cpufreq for now I think.

quoted

However toggling the auto_sel on different CPUs inside the same policy
seems inappropriate (this is is not what is done in  this patchset IIUC).

I think Sumit means per-policy when he said per-CPU.

Yes, it's per-policy.

Thank you,
Sumit Gupta

quoted

quoted

quoted

------------

In our case, I think it is desired to unload the scaling governor
currently in
use if auto_sel is selected. Letting the rest of the system think it has
control
over the freq. selection seems incorrect.
I am not sure what to replace it with:
-
There are no specific performance/powersave modes for CPPC.
There is a range of values between 0-255
-
A firmware auto-selection governor could be created just for this case.
Being able to switch between OS-driven and firmware driven freq. selection
is not specific to CPPC (for the future).
However I am not really able to say the implications of doing that.

------------

I think it would be better to split your patchset in 2:
1. adding APIs for the CPPC spec.
2. using the APIs, especially for auto_sel

1. is likely to be straightforward as the APIs will still be used
by the driver at some point.
2. is likely to bring more discussion.

We discussed adding a hw_auto_sel governor as a second step, though the
approach may need refinement during implementation.

I didn't find in the thread adding a new governor was discussed in the
threads, in case you have a direct link.

quoted

Deferred it (to second step) because adding a new governor requires
broader discussion.

This issue already exists in current code - store_auto_select() enables
auto_sel without any governor awareness. These patches improve the
situation by:
- Updating scaling_min/max_freq when toggling auto_sel mode
- Syncing policy limits with actual HW min/max_perf bounds
- Making scaling_min/max_freq read-only in auto_sel mode

Would it be acceptable to merge this as a first step, with the governor
handling as a follow-up?
If not and you prefer splitting, which grouping works better:
   A) Patches 1-8 then 9-11.
   B) "ACPI: CPPC *" patches then "cpufreq: CPPC *" patches.

If it's possible I would like to understand what the end result should
look like. If ultimately enabling auto_sel implies switching governor
I understand, but I didn't find the thread that discussed about that
unfortunately.

quoted

quoted

quoted

Enforce this by setting policy limits to min/max_perf bounds in
cppc_verify_policy(). Users must use min_perf/max_perf sysfs interfaces
to change performance limits in autonomous mode.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
   drivers/cpufreq/cppc_cpufreq.c | 32 +++++++++++++++++++++++++++++++-
   1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index b1f570d6de34..b3da263c18b0 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c

@@ -305,7 +305,37 @@ static unsigned int cppc_cpufreq_fast_switch(struct cpufreq_policy *policy,

   static int cppc_verify_policy(struct cpufreq_policy_data *policy)
   {
-     cpufreq_verify_within_cpu_limits(policy);
+     unsigned int min_freq = policy->cpuinfo.min_freq;
+     unsigned int max_freq = policy->cpuinfo.max_freq;
+     struct cpufreq_policy *cpu_policy;
+     struct cppc_cpudata *cpu_data;
+     struct cppc_perf_caps *caps;
+
+     cpu_policy = cpufreq_cpu_get(policy->cpu);
+     if (!cpu_policy)
+             return -ENODEV;
+
+     cpu_data = cpu_policy->driver_data;
+     caps = &cpu_data->perf_caps;
+
+     if (cpu_data->perf_ctrls.auto_sel) {
+             u32 min_perf, max_perf;
+
+             /*
+              * Set policy limits to HW min/max_perf bounds. In autonomous
+              * mode, scaling_min/max_freq is effectively read-only.
+              */
+             min_perf = cpu_data->perf_ctrls.min_perf ?:
+                        caps->lowest_nonlinear_perf;
+             max_perf = cpu_data->perf_ctrls.max_perf ?: caps->nominal_perf;
+
+             policy->min = cppc_perf_to_khz(caps, min_perf);
+             policy->max = cppc_perf_to_khz(caps, max_perf);

policy->min/max values are overwritten, but the governor which is
supposed to use them to select the most fitting frequency will be
ignored by the firmware I think.

Yes.

quoted

quoted

+     } else {
+             cpufreq_verify_within_limits(policy, min_freq, max_freq);
+     }
+
+     cpufreq_cpu_put(cpu_policy);
       return 0;
   }