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

[Sumit Gupta]

> > > 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@xxxxxxx/
>
> > 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@xxxxxxxxxx/
> > 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@xxxxxxxxxx>
> > > > > ---
> > > > >    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;
> > > > >    }