Re: [PATCH 16/19] cpupower: enable boost state support for amd-pstate module

[Huang Rui]

On Fri, Sep 10, 2021 at 06:42:42AM +0800, Shuah Khan wrote:
> 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

Will refine the commit log in V2.

> 
> > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > ---
> >   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?

You're right. As mentioned at last mail, I move all the amd_pstate* from
lib/cpufreq.c to utils/helpers/amd.c

> 
> > 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
> >   }
> >   #endif
> > diff --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
> 

The decode_pstates() is for legacy ACPI hardware Pstates (AMD only has 3),
but new amd_pstate function supports the finer grain frequency range. It's
the different hardware design. So we don't have the pstate number anymore.

> 		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;

OK, yes, it can be merged in one function here. Will update this in V2.

If the boost is not enabled, the maximum perf will be the normal perf
(similiar with P0 before).

Thanks,
Ray