[AMD Official Use Only - AMD Internal Distribution Only]
Hi Lihuisong,
> -----Original Message-----
> From: lihuisong (C) <lihuis...@huawei.com>
> Sent: Tuesday, August 27, 2024 6:33 PM
> 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: [PATCH v2 2/4] 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 Sivaprasad,
>
> Suggest to split this patch into two patches for easiler to review:
> patch-1: abstract a file for uncore dvfs core level, namely, the
> rte_power_uncore_ops.c you did.
> patch-2: move and rename, lib/power/power_intel_uncore.c =>
> drivers/power/intel_uncore/intel_uncore.c
>
> patch[1/4] is also too big and not good to review.
>
> In addition, I have some question and am not sure if we can adjust uncore init
> process.
>
> /Huisong
>
>
> 在 2024/8/26 21:06, 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.
> >
> > 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>
> > ---
> > .../power/intel_uncore/intel_uncore.c | 18 +-
> > .../power/intel_uncore/intel_uncore.h | 8 +-
> > drivers/power/intel_uncore/meson.build | 6 +
> > drivers/power/meson.build | 3 +-
> > lib/power/meson.build | 2 +-
> > lib/power/rte_power_uncore.c | 205 ++++++---------
> > lib/power/rte_power_uncore.h | 87 ++++---
> > lib/power/rte_power_uncore_ops.h | 239 ++++++++++++++++++
> > lib/power/version.map | 1 +
> > 9 files changed, 405 insertions(+), 164 deletions(-)
> > rename lib/power/power_intel_uncore.c =>
> drivers/power/intel_uncore/intel_uncore.c (95%)
> > rename lib/power/power_intel_uncore.h =>
> drivers/power/intel_uncore/intel_uncore.h (97%)
> > create mode 100644 drivers/power/intel_uncore/meson.build
> > create mode 100644 lib/power/rte_power_uncore_ops.h
> >
> > diff --git a/lib/power/power_intel_uncore.c
> > b/drivers/power/intel_uncore/intel_uncore.c
> > similarity index 95%
> > rename from lib/power/power_intel_uncore.c rename to
> > drivers/power/intel_uncore/intel_uncore.c
> > index 4eb9c5900a..804ad5d755 100644
> > --- a/lib/power/power_intel_uncore.c
> > +++ b/drivers/power/intel_uncore/intel_uncore.c
> > @@ -8,7 +8,7 @@
> >
> > #include <rte_memcpy.h>
> >
> > -#include "power_intel_uncore.h"
> > +#include "intel_uncore.h"
> > #include "power_common.h"
> >
> > #define MAX_NUMA_DIE 8
> > @@ -475,3 +475,19 @@ power_intel_uncore_get_num_dies(unsigned int pkg)
> >
> > return count;
> > }
> <...>
> >
> > -#endif /* POWER_INTEL_UNCORE_H */
> > +#endif /* INTEL_UNCORE_H */
> > diff --git a/drivers/power/intel_uncore/meson.build
> > b/drivers/power/intel_uncore/meson.build
> > new file mode 100644
> > index 0000000000..876df8ad14
> > --- /dev/null
> > +++ b/drivers/power/intel_uncore/meson.build
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2017 Intel
> > +Corporation # Copyright(c) 2024 Advanced Micro Devices, Inc.
> > +
> > +sources = files('intel_uncore.c')
> > +deps += ['power']
> > diff --git a/drivers/power/meson.build b/drivers/power/meson.build
> > index 8c7215c639..c83047af94 100644
> > --- a/drivers/power/meson.build
> > +++ b/drivers/power/meson.build
> > @@ -6,7 +6,8 @@ drivers = [
> > 'amd_pstate',
> > 'cppc',
> > 'kvm_vm',
> > - 'pstate'
> > + 'pstate',
> > + 'intel_uncore'
> The cppc, amd_pstate and so on belong to cpufreq scope.
> And intel_uncore belongs to uncore dvfs scope.
> They are not the same level. So I proposes that we need to create one
> directory
> called like cpufreq or core.
> This 'intel_uncore' name don't seems appropriate. what do you think the
> following
> directory structure:
> drivers/power/uncore/intel_uncore.c
> drivers/power/uncore/amd_uncore.c (according to the patch[4/4]).
At present, Meson does not support detecting an additional level of
subdirectories within drivers/*.
All the drivers maintain a consistent subdirectory structure.
> > ]
> > std_deps = ['power']
> > diff --git a/lib/power/meson.build b/lib/power/meson.build index
> > f3e3451cdc..9b13d98810 100644
> > --- a/lib/power/meson.build
> > +++ b/lib/power/meson.build
> > @@ -13,7 +13,6 @@ if not is_linux
> > endif
> > sources = files(
> > 'power_common.c',
> > - 'power_intel_uncore.c',
> > 'rte_power.c',
> > 'rte_power_uncore.c',
> > 'rte_power_pmd_mgmt.c',
> > @@ -24,6 +23,7 @@ headers = files(
> > 'rte_power_guest_channel.h',
> > 'rte_power_pmd_mgmt.h',
> > 'rte_power_uncore.h',
> > + 'rte_power_uncore_ops.h',
> > )
> > if cc.has_argument('-Wno-cast-qual')
> > cflags += '-Wno-cast-qual'
> > diff --git a/lib/power/rte_power_uncore.c
> > b/lib/power/rte_power_uncore.c index 48c75a5da0..9f8771224f 100644
> > --- a/lib/power/rte_power_uncore.c
> > +++ b/lib/power/rte_power_uncore.c
> > @@ -1,6 +1,7 @@
> > /* SPDX-License-Identifier: BSD-3-Clause
> > * Copyright(c) 2010-2014 Intel Corporation
> > * Copyright(c) 2023 AMD Corporation
> > + * Copyright(c) 2024 Advanced Micro Devices, Inc.
> > */
> >
> > #include <errno.h>
> > @@ -12,98 +13,50 @@
> > #include "rte_power_uncore.h"
> > #include "power_intel_uncore.h"
> >
> > -enum rte_uncore_power_mgmt_env default_uncore_env =
> > RTE_UNCORE_PM_ENV_NOT_SET;
> > +static enum rte_uncore_power_mgmt_env global_uncore_env =
> > +RTE_UNCORE_PM_ENV_NOT_SET; static struct rte_power_uncore_ops
> > +*global_uncore_ops;
> >
> > static rte_spinlock_t global_env_cfg_lock =
> > RTE_SPINLOCK_INITIALIZER;
> > +static RTE_TAILQ_HEAD(, rte_power_uncore_ops) uncore_ops_list =
> > + TAILQ_HEAD_INITIALIZER(uncore_ops_list);
> >
> > -static uint32_t
> > -power_get_dummy_uncore_freq(unsigned int pkg __rte_unused,
> > - unsigned int die __rte_unused)
> > -{
> > - return 0;
> > -}
> > -
> > -static int
> > -power_set_dummy_uncore_freq(unsigned int pkg __rte_unused,
> > - unsigned int die __rte_unused, uint32_t index __rte_unused)
> > -{
> > - return 0;
> > -}
> > +const char *uncore_env_str[] = {
> > + "not set",
> > + "auto-detect",
> > + "intel-uncore",
> > + "amd-hsmp"
> > +};
> Why open the "auto-detect" mode to user?
> Why not set this automatically at framework initialization?
> After all, the uncore driver is fixed for one platform.
The auto-detection feature has been implemented to enable seamless migration
across platforms
without requiring any changes to the application
> >
> > -static int
> > -power_dummy_uncore_freq_max(unsigned int pkg __rte_unused,
> > - unsigned int die __rte_unused)
> > -{
> > - return 0;
> > -}
> > -
> <...>
> > -static int
> > -power_dummy_uncore_get_num_freqs(unsigned int pkg __rte_unused,
> > - unsigned int die __rte_unused)
> > +/* register the ops struct in rte_power_uncore_ops, return 0 on
> > +success. */ int rte_power_register_uncore_ops(struct
> > +rte_power_uncore_ops *driver_ops)
> > {
> > - return 0;
> > -}
> > + if (!driver_ops->init || !driver_ops->exit ||
> > !driver_ops->get_num_pkgs ||
> > + !driver_ops->get_num_dies || !driver_ops->get_num_freqs ||
> > + !driver_ops->get_avail_freqs || !driver_ops->get_freq ||
> > + !driver_ops->set_freq || !driver_ops->freq_max ||
> > + !driver_ops->freq_min) {
> > + POWER_LOG(ERR, "Missing callbacks while registering power
> > ops");
> > + return -1;
> > + }
> > + if (driver_ops->cb)
> > + driver_ops->cb();
> >
> > -static unsigned int
> > -power_dummy_uncore_get_num_pkgs(void)
> > -{
> > - return 0;
> > -}
> > + TAILQ_INSERT_TAIL(&uncore_ops_list, driver_ops, next);
> >
> > -static unsigned int
> > -power_dummy_uncore_get_num_dies(unsigned int pkg __rte_unused) -{
> > return 0;
> > }
> > -
> > -/* function pointers */
> > -rte_power_get_uncore_freq_t rte_power_get_uncore_freq =
> > power_get_dummy_uncore_freq; -rte_power_set_uncore_freq_t
> > rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
> > -rte_power_uncore_freq_change_t rte_power_uncore_freq_max =
> > power_dummy_uncore_freq_max; -rte_power_uncore_freq_change_t
> > rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
> > -rte_power_uncore_freqs_t rte_power_uncore_freqs =
> > power_dummy_uncore_freqs; -rte_power_uncore_get_num_freqs_t
> > rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
> > -rte_power_uncore_get_num_pkgs_t rte_power_uncore_get_num_pkgs =
> > power_dummy_uncore_get_num_pkgs; -rte_power_uncore_get_num_dies_t
> > rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
> > -
> > -static void
> > -reset_power_uncore_function_ptrs(void)
> > -{
> > - rte_power_get_uncore_freq = power_get_dummy_uncore_freq;
> > - rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
> > - rte_power_uncore_freq_max = power_dummy_uncore_freq_max;
> > - rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
> > - rte_power_uncore_freqs = power_dummy_uncore_freqs;
> > - rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
> > - rte_power_uncore_get_num_pkgs = power_dummy_uncore_get_num_pkgs;
> > - rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
> > -}
> > -
> > int
> > rte_power_set_uncore_env(enum rte_uncore_power_mgmt_env env)
> > {
> > - int ret;
> > + int ret = -1;
> > + struct rte_power_uncore_ops *ops;
> >
> > rte_spinlock_lock(&global_env_cfg_lock);
> >
> > - if (default_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
> > + if (global_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
> > POWER_LOG(ERR, "Uncore Power Management Env already set.");
> > - rte_spinlock_unlock(&global_env_cfg_lock);
> > - return -1;
> > + goto out;
> > }
> >
> <...>
> > + if (env <= RTE_DIM(uncore_env_str)) {
> > + RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next)
> > + if (strncmp(ops->name, uncore_env_str[env],
> > + RTE_POWER_UNCORE_DRIVER_NAMESZ) == 0) {
> > + global_uncore_env = env;
> > + global_uncore_ops = ops;
> > + ret = 0;
> > + goto out;
> > + }
> > + POWER_LOG(ERR, "Power Management (%s) not supported",
> > + uncore_env_str[env]);
> > + } else
> > + POWER_LOG(ERR, "Invalid Power Management Environment");
> >
> > - default_uncore_env = env;
> > out:
> > rte_spinlock_unlock(&global_env_cfg_lock);
> > return ret;
> > @@ -139,15 +89,22 @@ void
> > rte_power_unset_uncore_env(void)
> > {
> > rte_spinlock_lock(&global_env_cfg_lock);
> > - default_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
> > - reset_power_uncore_function_ptrs();
> > + global_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
> > rte_spinlock_unlock(&global_env_cfg_lock);
> > }
> >
>
> How about abstract an ABI interface to intialize or set the uncore driver on
> platform
> by automatical.
>
> And later do power_intel_uncore_init_on_die() for each die on different
> package.
>
> > enum rte_uncore_power_mgmt_env
> > rte_power_get_uncore_env(void)
> > {
> > - return default_uncore_env;
> > + return global_uncore_env;
> > +}
> > +
> > +struct rte_power_uncore_ops *
> > +rte_power_get_uncore_ops(void)
> > +{
> > + RTE_ASSERT(global_uncore_ops != NULL);
> > +
> > + return global_uncore_ops;
> > }
> >
> > int
> > @@ -155,27 +112,29 @@ rte_power_uncore_init(unsigned int pkg, unsigned
> > int die)
> This pkg means the socket id on the platform, right?
> If so, I am not sure that the
> uncore_info[RTE_MAX_NUMA_NODES][MAX_NUMA_DIE] used in uncore lib is
> universal for all uncore driver.
> For example, uncore driver just support do uncore dvfs based on the socket
> unit.
> What shoud we do for this? we may need to think twice.
Yes, pkg represents a socket id. In platforms with a single uncore controller
per socket,
the die ID should be set to '0' for the corresponding socket ID (pkg).
.
> > {
> > int ret = -1;
> >
> <...>
