Re: [PATCH 16/19] cpupower: enable boost state support for amd-pstate module
From: Shuah Khan <skhan@linuxfoundation.org>
Date: 2021-09-09 22:42:47
Also in:
lkml
On 9/8/21 8:59 AM, Huang Rui wrote:
The AMD P-state boost API is different from ACPI hardware P-states, so implement the support for amd-pstate kernel module.
This commit log doesn't make sense. If these sysfs entries are used for amd-pstate kernel module, why are they defined here. Describe how these are used and the relationship between these defines and the amd-pstate kernel module
quoted hunk ↗ jump to hunk
Signed-off-by: Huang Rui <ray.huang@amd.com> --- tools/power/cpupower/lib/cpufreq.c | 20 ++++++++++++++++++++ tools/power/cpupower/lib/cpufreq.h | 3 +++ tools/power/cpupower/utils/helpers/misc.c | 7 +++++++ 3 files changed, 30 insertions(+)diff --git a/tools/power/cpupower/lib/cpufreq.c b/tools/power/cpupower/lib/cpufreq.c index 3f92ddadaad2..37da87bdcfb1 100644 --- a/tools/power/cpupower/lib/cpufreq.c +++ b/tools/power/cpupower/lib/cpufreq.c@@ -790,3 +790,23 @@ unsigned long cpufreq_get_transitions(unsigned int cpu) { return sysfs_cpufreq_get_one_value(cpu, STATS_NUM_TRANSITIONS); } + +int amd_pstate_boost_support(unsigned int cpu) +{ + unsigned int highest_perf, nominal_perf; + + highest_perf = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_HIGHEST_PERF); + nominal_perf = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_NOMINAL_PERF); + + return highest_perf > nominal_perf ? 1 : 0; +} + +int amd_pstate_boost_enabled(unsigned int cpu) +{ + unsigned int cpuinfo_max, amd_pstate_max; + + cpuinfo_max = sysfs_cpufreq_get_one_value(cpu, CPUINFO_MAX_FREQ); + amd_pstate_max = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_MAX_FREQ); + + return cpuinfo_max == amd_pstate_max ? 1 : 0; +}
Why are these amd specific routines added to common file. Why not add them to tools/power/cpupower/utils/helpers/amd.c?
quoted hunk ↗ jump to hunk
diff --git a/tools/power/cpupower/lib/cpufreq.h b/tools/power/cpupower/lib/cpufreq.h index 95f4fd9e2656..d54d02a7a4f4 100644 --- a/tools/power/cpupower/lib/cpufreq.h +++ b/tools/power/cpupower/lib/cpufreq.h@@ -203,6 +203,9 @@ int cpufreq_modify_policy_governor(unsigned int cpu, char *governor); int cpufreq_set_frequency(unsigned int cpu, unsigned long target_frequency); +int amd_pstate_boost_support(unsigned int cpu); +int amd_pstate_boost_enabled(unsigned int cpu); + #ifdef __cplusplus } #endifdiff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c index 07d80775fb68..aba979320760 100644 --- a/tools/power/cpupower/utils/helpers/misc.c +++ b/tools/power/cpupower/utils/helpers/misc.c@@ -10,6 +10,7 @@ #if defined(__i386__) || defined(__x86_64__) #include "cpupower_intern.h" +#include "cpufreq.h" #define MSR_AMD_HWCR 0xc0010015@@ -39,6 +40,12 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
This logic here calls amd_pci_get_num_boost_states() --- There is another routine called decode_pstates() in tools/power/cpupower/utils/helpers/amd.c if (ret)
return ret;
}
+ } if ((cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATE) &&
+ amd_pstate_boost_support(cpu)) {Coupled with the above routines, the naming amd_pstate_boost_support() is rather confusing. Also why is this amd_pstate_boost_support() added to
+ *support = 1; + + if (amd_pstate_boost_enabled(cpu)) + *active = 1; } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) *support = *active = 1; return 0;
thanks, -- Shuah