[AMD Official Use Only - General]
Hi Lihuisong,
> -----Original Message-----
> From: lihuisong (C) <lihuis...@huawei.com>
> Sent: Friday, March 1, 2024 9:04 AM
> To: Tummala, Sivaprasad <sivaprasad.tumm...@amd.com>
> Cc: dev@dpdk.org; david.h...@intel.com; anatoly.bura...@intel.com;
> radu.nico...@intel.com; jer...@marvell.com; cristian.dumitre...@intel.com;
> konstantin.anan...@huawei.com; Yigit, Ferruh <ferruh.yi...@amd.com>;
> gak...@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.tumm...@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
