[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, January 9, 2025 7:24 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Stabellini, Stefano <stefano.stabell...@amd.com>; Huang, Ray
> <ray.hu...@amd.com>; Ragiadakou, Xenia <xenia.ragiada...@amd.com>;
> Andryuk, Jason <jason.andr...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
> <sstabell...@kernel.org>; Roger Pau Monné <roger....@citrix.com>; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v1 08/11] x86/cpufreq: add "cpufreq=amd-pstate,active"
> para
>
> On 03.12.2024 09:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zh...@amd.com>
> >
> > The amd-pstate driver may support multiple working modes, passive and
> > active.
> >
> > Introduce a new variable to keep track of which mode is currently enabled.
> > This variable will also help to choose which cpufreq driver to be
> > registered.
> >
> > Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> > ---
> > docs/misc/xen-command-line.pandoc | 9 ++++++++-
> > xen/arch/x86/acpi/cpufreq/amd-pstate.c | 12 +++++++++++-
> > 2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.pandoc
> > b/docs/misc/xen-command-line.pandoc
> > index 30f855fa18..a9a3e0ef79 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -499,7 +499,8 @@ If set, force use of the performance counters for
> > oprofile, rather than detectin available support.
> >
> > ### cpufreq
> > -> `= none | {{ <boolean> | xen } {
> > -> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfr
> > -> eq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] |
> > -> amd-pstate[:[verbose]]`
> > +> `= none | {{ <boolean> | xen } {
> > +> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfr
> > +> eq>]]] }
> > +[,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] |
> > +amd-pstate[:[active][,verbose]]`
> >
> > > Default: `xen`
> >
> > @@ -522,6 +523,12 @@ choice of `dom0-kernel` is deprecated and not
> supported by all Dom0 kernels.
> > on supported AMD hardware to provide a finer grained frequency control
> mechanism.
> > The default is disabled. If `amd-pstate` is selected, but hardware
> > support
> > is not available, Xen will fallback to cpufreq=xen.
> > +* `active` is a boolean to enable amd-pstate driver in active(autonomous)
> > mode.
> > + In this mode, users could provide a hint with energy performance
> > +preference
> > + register to the hardware if they want to bias toward
> > +performance(0x0) or
> > + energy efficiency(0xff), then CPPC power algorithm will calculate
> > +the runtime
> > + workload and adjust the realtime cores frequency according to the
> > +power supply
> > + and themal, core voltage and some other hardware conditions.
>
> Nit: thermal
>
> > --- a/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > @@ -27,6 +27,8 @@
> > #define amd_pstate_warn(fmt, args...) \
> > printk(XENLOG_WARNING "AMD_PSTATE: CPU%u warning: " fmt, cpu, ##
> > args)
> >
> > +static bool __ro_after_init opt_cpufreq_active = false;
>
> Pointless initializer.
>
> > @@ -398,5 +407,6 @@ int __init amd_pstate_register_driver(void)
> > if ( !cpu_has_cppc )
> > return -ENODEV;
> >
> > - return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
> > + if ( !opt_cpufreq_active )
> > + return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
> > }
>
> I'm afraid the description is of no help in determining why this is a correct
> change to
> make (here). How would the user provided hint (see cmdline option
> description) be
> communicated to hardware when the driver isn't even registered?
>
Maybe I shall combine this commit into the next one about implementing epp
driver for active mode,
and the changes will be like:
- return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
+ if ( opt_cpufreq_active )
+ return cpufreq_register_driver(&&amd_cppc_epp_driver);
+ else
+ return cpufreq_register_driver(&amd_cppc_cpufreq_driver);
Now, the description makes more sense.
> Finally I don't think the change above would build, as it leaves a return
> from the
> function without return value.
>
> Jan
Many thanks
Penny
