Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Fri, 2025-01-24 at 11:53 +0800, zhenglifeng (A) wrote:
> On 2025/1/24 1:05, Mario Limonciello wrote:
> 
> > On 1/23/2025 10:46, Srinivas Pandruvada wrote:
> > > 
> > > On 1/20/25 18:42, zhenglifeng (A) wrote:
> > > > On 2025/1/21 1:44, Mario Limonciello wrote:
> > > > 
> > > > > On 1/20/2025 08:49, Pierre Gondois wrote:
> > > > > > 
> > > > > > On 1/20/25 04:15, zhenglifeng (A) wrote:
> > > > > > > On 2025/1/17 22:30, Mario Limonciello wrote:
> > > > > > > 
> > > > > > > > On 1/16/2025 21:11, zhenglifeng (A) wrote:
> > > > > > > > > On 2025/1/16 19:39, Russell Haley wrote:
> > > > > > > > > 
> > > > > > > > > > Hello,
> > > > > > > > > > 
> > > > > > > > > > I noticed something here just as a user casually
> > > > > > > > > > browsing the mailing list.
> > > > > > > > > > 
> > > > > > > > > > On 1/13/25 6:21 AM, Lifeng Zheng wrote:
> > > > > > > > > > > Add sysfs interfaces for CPPC autonomous
> > > > > > > > > > > selection in the cppc_cpufreq
> > > > > > > > > > > driver.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Lifeng Zheng
> > > > > > > > > > > <zhenglifeng1@huawei.com>
> > > > > > > > > > > ---
> > > > > > > > > > >     .../ABI/testing/sysfs-devices-system-cpu     
> > > > > > > > > > > |  54 +++++++++
> > > > > > > > > > >     drivers/cpufreq/cppc_cpufreq.c               
> > > > > > > > > > > | 109 +++++++ ++++ +++++++
> > > > > > > > > > >     2 files changed, 163 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-
> > > > > > > > > > > devices-system-cpu b/
> > > > > > > > > > > Documentation/ABI/testing/sysfs-devices-system-
> > > > > > > > > > > cpu
> > > > > > > > > > > index 206079d3bd5b..3d87c3bb3fe2 100644
> > > > > > > > > > > --- a/Documentation/ABI/testing/sysfs-devices-
> > > > > > > > > > > system-cpu
> > > > > > > > > > > +++ b/Documentation/ABI/testing/sysfs-devices-
> > > > > > > > > > > system-cpu
> > > > > > > > > > > @@ -268,6 +268,60 @@ Description:    Discover
> > > > > > > > > > > CPUs in the same CPU frequency coordination
> > > > > > > > > > > domain
> > > > > > > > > > >             This file is only present if the
> > > > > > > > > > > acpi-cpufreq or the cppc-cpufreq
> > > > > > > > > > >             drivers are in use.
> > > > > > > > > > [...snip...]
> > > > > > > > > > 
> > > > > > > > > > > +What:       
> > > > > > > > > > > /sys/devices/system/cpu/cpuX/cpufreq/energy_perf
> > > > > > > > > > > +Date:        October 2024
> > > > > > > > > > > +Contact:    linux-pm@vger.kernel.org
> > > > > > > > > > > +Description:    Energy performance preference
> > > > > > > > > > > +
> > > > > > > > > > > +        Read/write an 8-bit integer from/to this
> > > > > > > > > > > file. This file
> > > > > > > > > > > +        represents a range of values from 0
> > > > > > > > > > > (performance preference) to
> > > > > > > > > > > +        0xFF (energy efficiency preference) that
> > > > > > > > > > > influences the rate of
> > > > > > > > > > > +        performance increase/decrease and the
> > > > > > > > > > > result of the hardware's
> > > > > > > > > > > +        energy efficiency and performance
> > > > > > > > > > > optimization policies.
> > > > > > > > > > > +
> > > > > > > > > > > +        Writing to this file only has meaning
> > > > > > > > > > > when Autonomous Selection is
> > > > > > > > > > > +        enabled.
> > > > > > > > > > > +
> > > > > > > > > > > +        This file only presents if the cppc-
> > > > > > > > > > > cpufreq driver is in use.
> > > > > > > > > > In intel_pstate driver, there is file with near-
> > > > > > > > > > identical semantics:
> > > > > > > > > > 
> > > > > > > > > > /sys/devices/system/cpu/cpuX/cpufreq/energy_perform
> > > > > > > > > > ance_preference
> > > > > > > > > > 
> > > > > > > > > > It also accepts a few string arguments and converts
> > > > > > > > > > them to integers.
> > > > > > > > > > 
> > > > > > > > > > Perhaps the same name should be used, and the
> > > > > > > > > > semantics made exactly
> > > > > > > > > > identical, and then it could be documented as
> > > > > > > > > > present for either
> > > > > > > > > > cppc_cpufreq OR intel_pstate?
> > > > > > > > > > 
> > > > > > > > > > I think would be more elegant if userspace tooling
> > > > > > > > > > could Just Work with
> > > > > > > > > > either driver.
> > > > > > > > > > 
> > > > > > > > > > One might object that the frequency selection
> > > > > > > > > > behavior that results from
> > > > > > > > > > any particular value of the register itself might
> > > > > > > > > > be different, but they
> > > > > > > > > > are *already* different between Intel's P and E-
> > > > > > > > > > cores in the same CPU
> > > > > > > > > > package. (Ugh.)
> > > > > > > > > Yes, I should use the same name. Thanks.
> > > > > > > > > 
> > > > > > > > > As for accepting string arguments and converting them
> > > > > > > > > to integers, I don't
> > > > > > > > > think it is necessary. It'll be a litte confused if
> > > > > > > > > someone writes a raw
> > > > > > > > > value and reads a string I think. I prefer to let
> > > > > > > > > users freely set this
> > > > > > > > > value.
> > > > > > > > > 
> > > > > > > > > In addition, there are many differences between the
> > > > > > > > > implementations of
> > > > > > > > > energy_performance_preference in intel_pstate and
> > > > > > > > > cppc_cpufreq (and
> > > > > > > > > amd-pstate...). It is really difficult to explain all
> > > > > > > > > this differences in
> > > > > > > > > this document. So I'll leave it to be documented as
> > > > > > > > > present for
> > > > > > > > > cppc_cpufreq only.
> > > > > > > > At least the interface to userspace I think we should
> > > > > > > > do the best we can to be the same between all the
> > > > > > > > drivers if possible.
> > > > > > > > 
> > > > > > > > For example; I've got a patch that I may bring up in a
> > > > > > > > future kernel cycle that adds raw integer writes to
> > > > > > > > amd-pstates energy_performance_profile to behave the
> > > > > > > > same way intel-pstate does.
> > > > > > > I agree that it's better to keep this interface
> > > > > > > consistent across different
> > > > > > > drivers. But in my opinion, the implementation of
> > > > > > > intel_pstate
> > > > > > > energy_performance_preference is not really nice. Someone
> > > > > > > may write a raw
> > > > > > > value but read a string, or read strings for some values
> > > > > > > and read raw
> > > > > > > values for some other values. It is inconsistent. It may
> > > > > > > be better to use
> > > > > > > some other implementation, such as seperating the
> > > > > > > operations of r/w strings
> > > > > > > and raw values into two files.
> > > > > > I agree it would be better to be sure of the type to expect
> > > > > > when reading the
> > > > > > energy_performance_preference file. The epp values in the
> > > > > > range 0-255 with 0
> > > > > > being the performance value for all interfaces.
> > > > > > 
> > > > > > In the current epp strings, it seems there is a big gap
> > > > > > between the PERFORMANCE
> > > > > > and the BALANCE_PERFORMANCE strings. Maybe it would be good
> > > > > > to complete it:
> > > > > > EPP_PERFORMANCE        0x00
> > > > > > EPP_BALANCE_PERFORMANCE    0x40      // state value changed
> > > > > > EPP_BALANCE        0x80      // new state
> > > > > > EPP_BALANCE_POWERSAVE    0xC0
> > > > > > EPP_POWERSAVE        0xFF
> > > > > > 
> > > > > > NIT: The mapping seems to be slightly different for
> > > > > > intel_pstate and amd-pstate
> > > > > > currently:
> > > > > > drivers/cpufreq/amd-pstate.c
> > > > > > #define AMD_CPPC_EPP_PERFORMANCE        0x00
> > > > > > #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
> > > > > > #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
> > > > > > #define AMD_CPPC_EPP_POWERSAVE            0xFF
> > > > > > 
> > > > > > arch/x86/include/asm/msr-index.h
> > > > > > #define HWP_EPP_PERFORMANCE        0x00
> > > > > > #define HWP_EPP_BALANCE_PERFORMANCE    0x80
> > > > > > #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------
> > > > > > Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
> > > > > > #define HWP_EPP_POWERSAVE        0xFF
> > > > > > 
> > > > > > > I think it's better to consult Rafael and Viresh about
> > > > > > > how this should
> > > > > > > evolve.
> > > > > > Yes indeed
> > > > > Maybe it's best to discuss what the goal of raw EPP number
> > > > > writes is to decide what to do with it.
> > > > > 
> > > > > IE in intel-pstate is it for userspace to be able to actually
> > > > > utilize something besides the strings all the time?  Or is it
> > > > > just for debugging to find better values for strings in the
> > > > > future?
> > > > > 
> > > > > If the former maybe we're better off splitting to
> > > > > 'energy_performance_preference' and
> > > > > 'energy_performance_preference_int'.
> > > > > 
> > > > > If the latter maybe we're better off putting the integer
> > > > > writes and reads into debugfs instead and making
> > > > > 'energy_performance_preference' return -EINVAL while a non-
> > > > > predefined value is in use.
> > > 
> > > In Intel case EPP values can be different based on processor. In
> > > some case they they end up sharing the same CPU model. So strings
> > > are not suitable for all cases. Also there is different
> > > preference of EPP between Chrome systems and non chrome distro.
> > > For example Chrome has some resource manager which can change and
> > > same on Intel distros with LPMD.
> > > 
> > 
> > Thanks for confirming it is intentional and changing it would break
> > existing userspace.
> > 
> > And FWIW even in Windows there are more than 4 situational values
> > used like we have in Linux today.
> > 
> > As the status quo is there I personally feel that we should do the
> > exact same for other implementation of
> > energy_performance_preference.
> 
> I still don't think this implementation is nice, for the following
> reasons:
> 
> 1. Users may write raw value but read string. It's odd.
> 
> 2. Sometimes a raw value is read and sometimes a character string is
> read.
> The userspace tool needs to adapt this.
> 
> 3. Reading and writing EPP strings is not really general in driver.
> It is
> more reasonable to use the userspace tool to implement it.
> 
> In order not to break existing userspace, I'll rename the file to
> 'energy_performance_preference_int' or
> 'energy_performance_preference_val'
> in cppc_cpufreq and only support reading and writing raw value. As
> for
> accepting string arguments, it's not necessary for cppc_cpufreq for
> now.
> It's easy to add this feature but hard to remove, so I'll leave it to
> the
> future if it is really needed.
> 
> As for amd-pstate and intel_pstate, you can decide how
> energy_performance_preference should evolve. But I strongly recommend
> splitting it.

No. User space can deal with this already. Atleast this has one
interface. If you split you need to keep them consistent. You can write
a raw value  to new attribute and then can read a string from the other
attribute, which means different.

This is not the only place where strings and raw values can be written
in sysfs. Also true for energy_perf_bias.


Thanks,
Srinivas



> 
> Regards,
> 
> Lifeng
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > Srinivas
> > > 
> > > 
> > > > I think it's the former.
> > > > 
> > > > I added the author of the patch that allows raw energy
> > > > performance
> > > > preference value in intel_pstate to ask about what the goal is
> > > > and if this
> > > > would be ok to do the modification mentioned above.
> > > > 
> > > > To see the patch from
> > > > https://lore.kernel.org/ all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.c
> > > > om/
> > > > 
> > > > Anyway, the purpose of this patch is to allow users write and
> > > > read raw EPP
> > > > number. So maybe I can just rename the file to
> > > > 'energy_performance_preference_int'?
> > > > 
> > 
>