Re: [PATCH v5 10/11] cpufreq: CPPC: make scaling_min/max_freq read-only when auto_sel enabled

[Sumit Gupta <sumitg@nvidia.com>]

>>> n 08/01/26 22:16, Pierre Gondois wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> Hello Sumit, Lifeng,
>>>>
>>>> On 12/23/25 13:13, Sumit Gupta wrote:
>>>>> 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/
>
>> Thanks for pointing me to the first version, I forgot how your
>> first implementation was.
>>
>>
>>> 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/
>>>
>> 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.
>
>> 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


>>>> ------------
>>>>
>>>> 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.
>>
>>> 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.
>>
>>
>>>>> 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.
>>>
>>>>> +     } else {
>>>>> +             cpufreq_verify_within_limits(policy, min_freq, max_freq);
>>>>> +     }
>>>>> +
>>>>> +     cpufreq_cpu_put(cpu_policy);
>>>>>        return 0;
>>>>>    }
>>>>>