Re: [PATCH 4/6] cpupower: Add EPP value change support

[Huang Rui]

On Fri, Jun 16, 2023 at 05:03:46PM +0800, Karny, Wyes wrote:
> Hi Ray,
> 
> On 16 Jun 15:22, Huang Rui wrote:
> > On Mon, Jun 12, 2023 at 07:36:13PM +0800, Karny, Wyes wrote:
> > > amd_pstate and intel_pstate active mode drivers support energy
> > > performance preference feature. Through this user can convey it's
> > > energy/performance preference to platform. Add this value change
> > > capability to cpupower.
> > > 
> > > To change the EPP value use below command:
> > > cpupower set --epp performance
> > > 
> > > Signed-off-by: Wyes Karny <wyes.karny@xxxxxxx>
> > > ---
> > >  tools/power/cpupower/utils/cpupower-set.c    | 23 +++++++++++++++++++-
> > >  tools/power/cpupower/utils/helpers/helpers.h |  5 +++++
> > >  tools/power/cpupower/utils/helpers/misc.c    | 19 ++++++++++++++++
> > >  3 files changed, 46 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/power/cpupower/utils/cpupower-set.c b/tools/power/cpupower/utils/cpupower-set.c
> > > index 180d5ba877e6..a789b123dbd4 100644
> > > --- a/tools/power/cpupower/utils/cpupower-set.c
> > > +++ b/tools/power/cpupower/utils/cpupower-set.c
> > > @@ -18,6 +18,7 @@
> > >  
> > >  static struct option set_opts[] = {
> > >  	{"perf-bias", required_argument, NULL, 'b'},
> > > +	{"epp", required_argument, NULL, 'e'},
> > 
> > How about re-using the "perf-bias", I think it should be the simliar
> > function with Intel's processor?
> 
> AFAIU 'perf-bias' is not same as 'epp'. Perf-bias sysfs file expects
> 0-15 integer value, whereas epp sysfs file expects one of the string
> form energy_performance_available_preferences file.

I am thinking whether we can have a unify energy performance preferences
input in cpupower for both intel and amd processors. But it seems not easy
to align with them because different hardware interfaces here.

Anyway, it's not a big problem. Path looks good for me.

Thanks,
Ray

> 
> Thanks,
> Wyes
> > 
> > Thanks,
> > Ray
> > 
> > >  	{ },
> > >  };
> > >  
> > > @@ -37,11 +38,13 @@ int cmd_set(int argc, char **argv)
> > >  	union {
> > >  		struct {
> > >  			int perf_bias:1;
> > > +			int epp:1;
> > >  		};
> > >  		int params;
> > >  	} params;
> > >  	int perf_bias = 0;
> > >  	int ret = 0;
> > > +	char epp[30];
> > >  
> > >  	ret = uname(&uts);
> > >  	if (!ret && (!strcmp(uts.machine, "ppc64le") ||
> > > @@ -55,7 +58,7 @@ int cmd_set(int argc, char **argv)
> > >  
> > >  	params.params = 0;
> > >  	/* parameter parsing */
> > > -	while ((ret = getopt_long(argc, argv, "b:",
> > > +	while ((ret = getopt_long(argc, argv, "b:e:",
> > >  						set_opts, NULL)) != -1) {
> > >  		switch (ret) {
> > >  		case 'b':
> > > @@ -69,6 +72,15 @@ int cmd_set(int argc, char **argv)
> > >  			}
> > >  			params.perf_bias = 1;
> > >  			break;
> > > +		case 'e':
> > > +			if (params.epp)
> > > +				print_wrong_arg_exit();
> > > +			if (sscanf(optarg, "%29s", epp) != 1) {
> > > +				print_wrong_arg_exit();
> > > +				return -EINVAL;
> > > +			}
> > > +			params.epp = 1;
> > > +			break;
> > >  		default:
> > >  			print_wrong_arg_exit();
> > >  		}
> > > @@ -102,6 +114,15 @@ int cmd_set(int argc, char **argv)
> > >  				break;
> > >  			}
> > >  		}
> > > +
> > > +		if (params.epp) {
> > > +			ret = cpupower_set_epp(cpu, epp);
> > > +			if (ret) {
> > > +				fprintf(stderr,
> > > +					"Error setting epp value on CPU %d\n", cpu);
> > > +				break;
> > > +			}
> > > +		}
> > >  	}
> > >  	return ret;
> > >  }
> > > diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
> > > index 96e4bede078b..5d998de2d291 100644
> > > --- a/tools/power/cpupower/utils/helpers/helpers.h
> > > +++ b/tools/power/cpupower/utils/helpers/helpers.h
> > > @@ -116,6 +116,8 @@ extern int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val);
> > >  extern int cpupower_intel_get_perf_bias(unsigned int cpu);
> > >  extern unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu);
> > >  
> > > +extern int cpupower_set_epp(unsigned int cpu, char *epp);
> > > +
> > >  /* Read/Write msr ****************************/
> > >  
> > >  /* PCI stuff ****************************/
> > > @@ -173,6 +175,9 @@ static inline int cpupower_intel_get_perf_bias(unsigned int cpu)
> > >  static inline unsigned long long msr_intel_get_turbo_ratio(unsigned int cpu)
> > >  { return 0; };
> > >  
> > > +static inline int cpupower_set_epp(unsigned int cpu, char *epp)
> > > +{ return -1; };
> > > +
> > >  /* Read/Write msr ****************************/
> > >  
> > >  static inline int cpufreq_has_boost_support(unsigned int cpu, int *support,
> > > diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
> > > index 21f653cd472c..63c3f26ef874 100644
> > > --- a/tools/power/cpupower/utils/helpers/misc.c
> > > +++ b/tools/power/cpupower/utils/helpers/misc.c
> > > @@ -87,6 +87,25 @@ int cpupower_intel_set_perf_bias(unsigned int cpu, unsigned int val)
> > >  	return 0;
> > >  }
> > >  
> > > +int cpupower_set_epp(unsigned int cpu, char *epp)
> > > +{
> > > +	char path[SYSFS_PATH_MAX];
> > > +	char linebuf[30] = {};
> > > +
> > > +	snprintf(path, sizeof(path),
> > > +		PATH_TO_CPU "cpu%u/cpufreq/energy_performance_preference", cpu);
> > > +
> > > +	if (!is_valid_path(path))
> > > +		return -1;
> > > +
> > > +	snprintf(linebuf, sizeof(linebuf), "%s", epp);
> > > +
> > > +	if (cpupower_write_sysfs(path, linebuf, 30) <= 0)
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  bool cpupower_amd_pstate_enabled(void)
> > >  {
> > >  	char *driver = cpufreq_get_driver(0);
> > > -- 
> > > 2.34.1
> > >