RE: [RFC PATCH 2/2] power: refactor uncore power management library

["Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>]

[AMD Official Use Only - General]

Hi Lihuisong,

> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Friday, March 1, 2024 9:04 AM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
> Cc: dev@dpdk.org; david.hunt@intel.com; anatoly.burakov@intel.com;
> radu.nicolau@intel.com; jerinj@marvell.com; cristian.dumitrescu@intel.com;
> konstantin.ananyev@huawei.com; Yigit, Ferruh <Ferruh.Yigit@amd.com>;
> gakhil@marvell.com
> Subject: Re: [RFC PATCH 2/2] power: refactor uncore power management library
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> 在 2024/2/20 23:33, Sivaprasad Tummala 写道:
> > This patch refactors the power management library, addressing uncore
> > power management. The primary changes involve the creation of
> > dedicated directories for each driver within 'drivers/power/uncore/*'.
> > The adjustment of meson.build files enables the selective activation
> > of individual drivers.
> +1 to discriminate core and uncore.
> >
> > This refactor significantly improves code organization, enhances
> > clarity and boosts maintainability. It lays the foundation for more
> > focused development on individual drivers and facilitates seamless
> > integration of future enhancements, particularly the AMD uncore driver.
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > ---
> >   drivers/power/meson.build                     |   1 +
> >   drivers/power/uncore/intel/meson.build        |   9 +
> >   .../power/uncore/intel}/power_intel_uncore.c  |  15 ++
> >   .../power/uncore/intel}/power_intel_uncore.h  |   0
> >   drivers/power/uncore/meson.build              |   8 +
> >   lib/power/meson.build                         |   1 -
> >   lib/power/rte_power_uncore.c                  | 163 +++++++-----------
> >   lib/power/rte_power_uncore.h                  | 150 ++++++++++++++--
> >   lib/power/version.map                         |   1 +
> >   9 files changed, 236 insertions(+), 112 deletions(-)
> >   create mode 100644 drivers/power/uncore/intel/meson.build
> >   rename {lib/power => drivers/power/uncore/intel}/power_intel_uncore.c
> (95%)
> >   rename {lib/power =>
> > drivers/power/uncore/intel}/power_intel_uncore.h (100%)
> How about remove 'power' in "power_intel_uncore.c"
ACK!

> >   create mode 100644 drivers/power/uncore/meson.build
> >
> > diff --git a/drivers/power/meson.build b/drivers/power/meson.build
> > index 7d9034c7ac..0803e99027 100644
> > --- a/drivers/power/meson.build
> > +++ b/drivers/power/meson.build
> > @@ -3,6 +3,7 @@
> >
> >   drivers = [
> >           'core',
> > +        'uncore',
> >   ]
> >
> >   std_deps = ['power']
> > diff --git a/drivers/power/uncore/intel/meson.build
> > b/drivers/power/uncore/intel/meson.build
> > new file mode 100644
> > index 0000000000..187ab15aec
> > --- /dev/null
> > +++ b/drivers/power/uncore/intel/meson.build
> > @@ -0,0 +1,9 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2017 Intel
> > +Corporation # Copyright(c) 2024 AMD Limited
> > +
> > +sources = files('power_intel_uncore.c')
> > +
> > +headers = files('power_intel_uncore.h')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_intel_uncore.c
> > b/drivers/power/uncore/intel/power_intel_uncore.c
> > similarity index 95%
> > rename from lib/power/power_intel_uncore.c rename to
> > drivers/power/uncore/intel/power_intel_uncore.c
> > index 3ce8fccec2..3af4cc3bc7 100644
> > --- a/lib/power/power_intel_uncore.c
> > +++ b/drivers/power/uncore/intel/power_intel_uncore.c
> > @@ -476,3 +476,18 @@ power_intel_uncore_get_num_dies(unsigned int pkg)
> >
> >       return count;
> >   }
> > +
> > +static struct rte_power_uncore_ops intel_uncore_ops = {
> > +     .init = power_intel_uncore_init,
> > +     .exit = power_intel_uncore_exit,
> > +     .get_avail_freqs = power_intel_uncore_freqs,
> > +     .get_num_pkgs = power_intel_uncore_get_num_pkgs,
> > +     .get_num_dies = power_intel_uncore_get_num_dies,
> > +     .get_num_freqs = power_intel_uncore_get_num_freqs,
> > +     .get_freq = power_get_intel_uncore_freq,
> > +     .set_freq = power_set_intel_uncore_freq,
> > +     .freq_max = power_intel_uncore_freq_max,
> > +     .freq_min = power_intel_uncore_freq_min, };
> > +
> > +RTE_POWER_REGISTER_UNCORE_OPS(intel_uncore_ops);
> <...>
> > +
> > +/** Structure defining uncore power operations structure */ struct
> > +rte_power_uncore_ops {
> > +     uint8_t status;                         /**< ops register status. */
> > +     enum rte_uncore_power_mgmt_env env;          /**< power mgmt env. */
> > +     rte_power_uncore_init_t init;    /**< Initialize power management. */
> > +     rte_power_uncore_exit_t exit;    /**< Exit power management. */
> > +     rte_power_uncore_get_num_pkgs_t get_num_pkgs;
> > +     rte_power_uncore_get_num_dies_t get_num_dies;
> > +     rte_power_uncore_get_num_freqs_t get_num_freqs; /**< Number of
> available frequencies. */
> > +     rte_power_uncore_freqs_t get_avail_freqs; /**< Get the available
> frequencies. */
> > +     rte_power_get_uncore_freq_t get_freq; /**< Get frequency index. */
> > +     rte_power_set_uncore_freq_t set_freq; /**< Set frequency index. */
> > +     rte_power_uncore_freq_change_t freq_max;  /**< Scale up frequency to
> highest. */
> > +     rte_power_uncore_freq_change_t freq_min;  /**< Scale up
> > +frequency to lowest. */ } __rte_cache_aligned;
> For all core drivers (cpufreq),  they all basically follow the ACPI specification.
> So libray can extract a common ops for all core DVFS driver.
> AFAIS, there is only one uncore driver in kernel, namely intel uncore driver.
> But there is not an unify specification to control uncore frequency
> scaling(UFS) in kernel.
> That is to say, every chip manufacturers can implement their uncore driver as
> themselves request.
> As a result, there is different system interface for userspace between
> manufacturer.
> So I am not sure if this new extracted rte_power_uncore_ops sturcture is very
> common for all uncore drivers in future.
Agreed! The uncore implementation (vendor specific) are expected to be abstracted
At driver level. One possible approach I think is to provide different performance levels
(instead of num_freqs) by the uncore library and each driver implementation can
interpret/implement the perf level independently (uncore/crosssocket/pcie/umc frequencies).
Application can query the no. of performance levels (highest to lowest) and can select a
Performance level as needed for power savings.
> > +
> > +/**
> > + * Register power uncore frequency operations.
> > + * @param ops
> > + *   Pointer to an ops structure to register.
> > + * @return
> > + *   - >=0: Success; return the index of the ops struct in the table.
> > + *   - -EINVAL - error while registering ops struct.
> > + */
> > +__rte_internal
> > +int rte_power_register_uncore_ops(const struct rte_power_uncore_ops
> > +*ops);
> > +
> > +/**
> > + * Macro to statically register the ops of an uncore driver.
> > + */
> > +#define RTE_POWER_REGISTER_UNCORE_OPS(ops)           \
> > +     (RTE_INIT(power_hdlr_init_uncore_##ops)         \
> > +     {                                               \
> > +             rte_power_register_uncore_ops(&ops);    \
> > +     })
> > +
> <...>

Thanks & Regards,
Sivaprasad