Hello Rafael & Viresh
On 2024/12/9 21:15, Pierre Gondois wrote:
> Hello Lifeng,
>
> On 12/9/24 09:40, zhenglifeng (A) wrote:
>> Hello Pierre,
>>
>> On 2024/12/6 22:23, Pierre Gondois wrote:
>>> Hello Lifeng,
>>>
>>> On 11/14/24 09:48, Lifeng Zheng wrote:
>>>> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
>>>> driver.
>>>>
>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@xxxxxxxxxx>
>>>> ---
>>>> .../ABI/testing/sysfs-devices-system-cpu | 54 +++++++
>>>> drivers/cpufreq/cppc_cpufreq.c | 141 ++++++++++++++++++
>>>> 2 files changed, 195 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
>>>> index 206079d3bd5b..ba7b8ea613e5 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.
>>>> +What: /sys/devices/system/cpu/cpuX/cpufreq/auto_select
>>>> +Date: October 2024
>>>> +Contact: linux-pm@xxxxxxxxxxxxxxx
>>>> +Description: Autonomous selection enable
>>>> +
>>>> + Read/write interface to control autonomous selection enable
>>>> + Read returns autonomous selection status:
>>>> + 0: autonomous selection is disabled
>>>> + 1: autonomous selection is enabled
>>>> +
>>>> + Write '1' to enable autonomous selection.
>>>> + Write '0' to disable autonomous selection.
>>>> +
>>>> + This file only presents if the cppc-cpufreq driver is in use.
>>>> +
>>>> +What: /sys/devices/system/cpu/cpuX/cpufreq/auto_act_window
>>>> +Date: October 2024
>>>> +Contact: linux-pm@xxxxxxxxxxxxxxx
>>>> +Description: Autonomous activity window
>>>> +
>>>> + This file indicates a moving utilization sensitivity window to
>>>> + the platform's autonomous selection policy.
>>>> +
>>>> + Read/write an integer represents autonomous activity window (in
>>>> + microseconds) from/to this file. The max value to write is
>>>> + 1270000000 but the max significand is 127. This means that if 128
>>>> + is written to this file, 127 will be stored. If the value is
>>>> + greater than 130, only the first two digits will be saved as
>>>> + significand.
>>>> +
>>>> + Writing a zero value to this file enable the platform to
>>>> + determine an appropriate Activity Window depending on the workload.
>>>> +
>>>> + Writing to this file only has meaning when Autonomous Selection is
>>>> + enabled.
>>>> +
>>>> + This file only presents if the cppc-cpufreq driver is in use.
>>>> +
>>>> +What: /sys/devices/system/cpu/cpuX/cpufreq/energy_perf
>>>> +Date: October 2024
>>>> +Contact: linux-pm@xxxxxxxxxxxxxxx
>>>> +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.
>>>> +
>>>> What: /sys/devices/system/cpu/cpu*/cache/index3/cache_disable_{0,1}
>>>> Date: August 2008
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index 2b8708475ac7..b435e1751d0d 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -792,10 +792,151 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>>>> return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
>>>> }
>>>> +
>>>> +static ssize_t show_auto_select(struct cpufreq_policy *policy, char *buf)
>>>> +{
>>>> + u64 val;
>>>> + int ret;
>>>> +
>>>> + ret = cppc_get_auto_sel(policy->cpu, &val);
>>>> +
>>>> + /* show "<unsupported>" when this register is not supported by cpc */
>>>> + if (ret == -EOPNOTSUPP)
>>>> + return sysfs_emit(buf, "%s\n", "<unsupported>");
>>>> +
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return sysfs_emit(buf, "%lld\n", val);
>>>> +}
>>>> +
>>>> +static ssize_t store_auto_select(struct cpufreq_policy *policy,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + unsigned long val;
>>>> + int ret;
>>>> +
>>>> + ret = kstrtoul(buf, 0, &val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (val > 1)
>>>> + return -EINVAL;
>>>> +
>>>> + ret = cppc_set_auto_sel(policy->cpu, val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return count;
>>>> +}
>>>> +
>>>> +#define AUTO_ACT_WINDOW_SIG_BIT_SIZE (7)
>>>> +#define AUTO_ACT_WINDOW_EXP_BIT_SIZE (3)
>>>> +#define AUTO_ACT_WINDOW_MAX_SIG ((1 << AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1)
>>>> +#define AUTO_ACT_WINDOW_MAX_EXP ((1 << AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1)
>>>> +/* AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
>>>> +#define AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
>>>
>>> Maybe this would be better to place these macros in include/acpi/cppc_acpi.h
>>> (with a CPPC_XXX prefix)
>>
>> Will move them, Thanks.
>>
>>>
>>>> +
>>>> +static ssize_t show_auto_act_window(struct cpufreq_policy *policy, char *buf)
>>>> +{
>>>> + int sig, exp;
>>>> + u64 val;
>>>> + int ret;
>>>> +
>>>> + ret = cppc_get_auto_act_window(policy->cpu, &val);
>>>> +
>>>> + /* show "<unsupported>" when this register is not supported by cpc */
>>>> + if (ret == -EOPNOTSUPP)
>>>> + return sysfs_emit(buf, "%s\n", "<unsupported>");
>>>> +
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + sig = val & AUTO_ACT_WINDOW_MAX_SIG;
>>>> + exp = (val >> AUTO_ACT_WINDOW_SIG_BIT_SIZE) & AUTO_ACT_WINDOW_MAX_EXP;
>>>> +
>>>> + return sysfs_emit(buf, "%lld\n", sig * int_pow(10, exp));
>>>> +}
>>>> +
>>>> +static ssize_t store_auto_act_window(struct cpufreq_policy *policy,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + unsigned long usec;
>>>> + int digits = 0;
>>>> + int ret;
>>>> +
>>>> + ret = kstrtoul(buf, 0, &usec);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (usec > AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, AUTO_ACT_WINDOW_MAX_EXP))
>>>> + return -EINVAL;
>>>> +
>>>> + while (usec > AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
>>>> + usec /= 10;
>>>> + digits += 1;
>>>> + }
>>>> +
>>>> + if (usec > AUTO_ACT_WINDOW_MAX_SIG)
>>>> + usec = AUTO_ACT_WINDOW_MAX_SIG;
>>>> +
>>>> + ret = cppc_set_auto_act_window(policy->cpu,
>>>> + (digits << AUTO_ACT_WINDOW_SIG_BIT_SIZE) + usec);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return count;
>>>> +}
>
> Also I tested the logic and it was working correctly for me.
>
>>>> +
>>>> +static ssize_t show_energy_perf(struct cpufreq_policy *policy, char *buf)
>>>> +{
>>>> + u64 val;
>>>> + int ret;
>>>> +
>>>> + ret = cppc_get_epp_perf(policy->cpu, &val);
>>>> +
>>>> + /* show "<unsupported>" when this register is not supported by cpc */
>>>> + if (ret == -EOPNOTSUPP)
>>>> + return sysfs_emit(buf, "%s\n", "<unsupported>");
>>>> +
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return sysfs_emit(buf, "%lld\n", val);
>>>> +}
>>>> +
>>>> +#define ENERGY_PERF_MAX (0xFF)
>>>
>>> Same comment to move to include/acpi/cppc_acpi.h
>>>
>>>> +
>>>> +static ssize_t store_energy_perf(struct cpufreq_policy *policy,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + unsigned long val;
>>>> + int ret;
>>>> +
>>>> + ret = kstrtoul(buf, 0, &val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (val > ENERGY_PERF_MAX)
>>>> + return -EINVAL;
>>>> +
>>>> + ret = cppc_set_epp(policy->cpu, val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return count;
>>>> +}
>>>> +
>>>> cpufreq_freq_attr_ro(freqdomain_cpus);
>>>> +cpufreq_freq_attr_rw(auto_select);
>>>> +cpufreq_freq_attr_rw(auto_act_window);
>>>> +cpufreq_freq_attr_rw(energy_perf);
>>>
>>> It might be better from a user PoV to hide the following entries:
>>> - auto_act_window
>>> - energy_perf
>>> if auto_select is not available or disabled.
>>
>> Users might like to modify the value of auto_act_window and energy_perf
>> before turning on auto_select. So I think it is freer for users to read and
>> write them no matter what auto_select is. What do you think?
>
> Autonomous selection is not the most common case for the CPPC cpufreq drivers,
> so these new files might bring questions to people currently using it.
>
> On the other side, making these files visible only when 'auto_select' is enabled
> will require additional logic in the code (while the current implementation is
> quite clear).
>
> I think Rafael or Viresh should take the decision. So it might be better to
> directly ping them,
>
> Regards,
> Pierre
Since Pierre and me have discussed about whether or not to show
auto_act_window and energy_perf when auto_select is disabled. It seems
like whether to show these two files has their own points. We'd like to
ask your advice and look forward to your reply!
Regards,
Lifeng
>
>>
>>>
>>> ------
>>>
>>> Also just for reference, in ACPI 6.5, s8.4.6.1.2.3 Desired Performance Register
>>> """
>>> When Autonomous Selection is enabled, it is not necessary for OSPM to assess processor workload performance
>>> demand and convey a corresponding performance delivery request to the platform via the Desired Register. If the
>>> Desired Performance Register exists, OSPM may provide an explicit performance requirement hint to the platform by
>>> writing a non-zero value.
>>> """
>>>
>>> So it seems it still makes sense to have cpufreq requesting a certain performance
>>> level even though autonomous selection is enabled.
>>
>> We did struggle with this. This solves our doubts. Thanks!
>>
>>>
>>
>
