Re: [PATCH v9 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)

[Mario Limonciello]

On 1/4/23 23:49, Yuan, Perry wrote:
> [AMD Official Use Only - General]
>
> Hi Mario
>
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > Sent: Wednesday, January 4, 2023 8:31 AM
> > To: Yuan, Perry <Perry.Yuan@xxxxxxx>; rafael.j.wysocki@xxxxxxxxx; Huang,
> > Ray <Ray.Huang@xxxxxxx>; viresh.kumar@xxxxxxxxxx
> > Cc: Sharma, Deepak <Deepak.Sharma@xxxxxxx>; Fontenot, Nathan
> > <Nathan.Fontenot@xxxxxxx>; Deucher, Alexander
> > <Alexander.Deucher@xxxxxxx>; Huang, Shimmer
> > <Shimmer.Huang@xxxxxxx>; Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Meng,
> > Li (Jassmine) <Li.Meng@xxxxxxx>; Karny, Wyes <Wyes.Karny@xxxxxxx>;
> > linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v9 03/13] cpufreq: intel_pstate: use common macro
> > definition for Energy Preference Performance(EPP)
> >
> > On 12/25/2022 10:34, Perry Yuan wrote:
> > > make the energy preference performance strings and profiles using one
> > > common header for intel_pstate driver, then the amd_pstate epp driver
> > > can use the common header as well. This will simpify the intel_pstate
> > > and amd_pstate driver.
> > >
> > > Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> > > ---
> > >    drivers/cpufreq/Kconfig.x86    |  2 +-
> > >    drivers/cpufreq/intel_pstate.c | 13 +++----------
> > >    include/linux/cpufreq.h        | 10 ++++++++++
> > >    3 files changed, 14 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> > > index 00476e94db90..f64aef1e093d 100644
> > > --- a/drivers/cpufreq/Kconfig.x86
> > > +++ b/drivers/cpufreq/Kconfig.x86
> > > @@ -36,7 +36,7 @@ config X86_PCC_CPUFREQ
> > >
> > >    config X86_AMD_PSTATE
> > >    	bool "AMD Processor P-State driver"
> > > -	depends on X86 && ACPI
> > > +	depends on X86 && ACPI && X86_INTEL_PSTATE
> >
> > This doesn't seem right to me.  What if someone didn't compile in Intel
> > x86 support for their kernel?  They wouldn't be able to pick
> > X86_AMD_PSTATE.
>
> How about changed like this ? when amd pstate enabled, it will build intel-pstate.c as well.
>
>     depends on X86 && ACPI
> + select X86_INTEL_PSTATE

I still don't think that's the right way to do this.  Why not move the 
variables to the cppc library file?  Both intel and amd pstate drivers 
select it, so it can be a common place both wil use.


> > >    	select ACPI_PROCESSOR
> > >    	select ACPI_CPPC_LIB if X86_64
> > >    	select CPU_FREQ_GOV_SCHEDUTIL if SMP diff --git
> > > a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > index ad9be31753b6..93a60fdac0fc 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
> > >     *	4		power
> > >     */
> > >
> > > -enum energy_perf_value_index {
> > > -	EPP_INDEX_DEFAULT = 0,
> > > -	EPP_INDEX_PERFORMANCE,
> > > -	EPP_INDEX_BALANCE_PERFORMANCE,
> > > -	EPP_INDEX_BALANCE_POWERSAVE,
> > > -	EPP_INDEX_POWERSAVE,
> > > -};
> > > -
> > > -static const char * const energy_perf_strings[] = {
> > > +const char * const energy_perf_strings[] = {
> > >    	[EPP_INDEX_DEFAULT] = "default",
> > >    	[EPP_INDEX_PERFORMANCE] = "performance",
> > >    	[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> > @@ -656,7
> > > +648,8 @@ static const char * const energy_perf_strings[] = {
> > >    	[EPP_INDEX_POWERSAVE] = "power",
> > >    	NULL
> > >    };
> > > -static unsigned int epp_values[] = {
> > > +
> > > +unsigned int epp_values[] = {
> > >    	[EPP_INDEX_DEFAULT] = 0, /* Unused index */
> > >    	[EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
> > >    	[EPP_INDEX_BALANCE_PERFORMANCE] =
> > HWP_EPP_BALANCE_PERFORMANCE, diff
> > > --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index
> > > d5595d57f4e5..0693269fb775 100644
> > > --- a/include/linux/cpufreq.h
> > > +++ b/include/linux/cpufreq.h
> > > @@ -185,6 +185,16 @@ struct cpufreq_freqs {
> > >    	u8 flags;		/* flags of cpufreq_driver, see below. */
> > >    };
> > >
> > > +enum energy_perf_value_index {
> > > +	EPP_INDEX_DEFAULT = 0,
> > > +	EPP_INDEX_PERFORMANCE,
> > > +	EPP_INDEX_BALANCE_PERFORMANCE,
> > > +	EPP_INDEX_BALANCE_POWERSAVE,
> > > +	EPP_INDEX_POWERSAVE,
> > > +};
> > > +extern const char * const energy_perf_strings[]; extern unsigned int
> > > +epp_values[];
> > > +
> > >    /* Only for ACPI */
> > >    #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
> > >    #define CPUFREQ_SHARED_TYPE_HW	 (1) /* HW does needed
> > coordination */
> >
> > I think the right place for these variables and strings is in the cppc library
> > source file that is common across CPPC implementations.