[AMD Official Use Only - General]
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yi...@amd.com>
> Sent: Tuesday, February 27, 2024 9:48 PM
> To: Tummala, Sivaprasad <sivaprasad.tumm...@amd.com>;
> david.h...@intel.com; anatoly.bura...@intel.com; jer...@marvell.com;
> radu.nico...@intel.com; gak...@marvell.com; cristian.dumitre...@intel.com;
> konstantin.anan...@huawei.com
> Cc: dev@dpdk.org
> Subject: Re: [RFC PATCH 1/2] power: refactor core power management library
>
> On 2/20/2024 3:33 PM, Sivaprasad Tummala wrote:
> > This patch introduces a comprehensive refactor to the core power
> > management library. The primary focus is on improving modularity and
> > organization by relocating specific driver implementations from the
> > 'lib/power' directory to dedicated directories within
> > 'drivers/power/core/*'. The adjustment of meson.build files enables
> > the selective activation of individual drivers.
> >
> > These changes contribute to a significant enhancement in code
> > organization, providing a clearer structure for driver implementations.
> > The refactor aims to improve overall code clarity and boost
> > maintainability. Additionally, it establishes a foundation for future
> > development, allowing for more focused work on individual drivers and
> > seamless integration of forthcoming enhancements.
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tumm...@amd.com>
> >
>
> +1 to refactor, thanks for the work.
>
> There are multiple power implementations but all are managed withing the power
> library, it is good idea to extract different implementations as drivers.
>
> <...>
>
> > diff --git a/drivers/power/core/acpi/meson.build
> > b/drivers/power/core/acpi/meson.build
> > new file mode 100644
> > index 0000000000..d10ec8ee94
> > --- /dev/null
> > +++ b/drivers/power/core/acpi/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> >
>
> It should be as following, same for all:
>
> Copyright (C) 2024, Advanced Micro Devices, Inc.
>
ACK
> > +
> > +sources = files('power_acpi_cpufreq.c')
> > +
> > +headers = files('power_acpi_cpufreq.h')
> >
>
> In meson, 'headers' variable is used to install the header, this is required
> for the
> case user needs to include the header but I guess that is not the case for
> power
> libraries.
> Can you please check if the 'header' variable in meson is required?
>
> <...>
>
> > @@ -577,3 +577,22 @@ int power_acpi_get_capabilities(unsigned int
> > lcore_id,
> >
> > return 0;
> > }
> > +
> > +static struct rte_power_ops acpi_ops = {
> > + .init = power_acpi_cpufreq_init,
> > + .exit = power_acpi_cpufreq_exit,
> > + .check_env_support = power_acpi_cpufreq_check_supported,
> > + .get_avail_freqs = power_acpi_cpufreq_freqs,
> > + .get_freq = power_acpi_cpufreq_get_freq,
> > + .set_freq = power_acpi_cpufreq_set_freq,
> > + .freq_down = power_acpi_cpufreq_freq_down,
> > + .freq_up = power_acpi_cpufreq_freq_up,
> > + .freq_max = power_acpi_cpufreq_freq_max,
> > + .freq_min = power_acpi_cpufreq_freq_min,
> > + .turbo_status = power_acpi_turbo_status,
> > + .enable_turbo = power_acpi_enable_turbo,
> > + .disable_turbo = power_acpi_disable_turbo,
> > + .get_caps = power_acpi_get_capabilities };
> > +
>
> With current usage of the ops struct, I guess all can be "static const".
ACK
>
> <...>
>
> > diff --git a/drivers/power/core/kvm-vm/meson.build
> > b/drivers/power/core/kvm-vm/meson.build
> > new file mode 100644
> > index 0000000000..3150c6674b
> > --- /dev/null
> > +++ b/drivers/power/core/kvm-vm/meson.build
> > @@ -0,0 +1,20 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(C) 2024 AMD
> > +Limited.
> > +#
> > +
> > +if not is_linux
> > + build = false
> > + reason = 'only supported on Linux'
> > + subdir_done()
> > +endif
> >
>
> Before refactoring, in lib/power was supported only for Linux, I assume this
> means
> all existing power libraries supported only for Linux.
> If so above check can be added to all drivers.
ACK
>
> <...>
>
> > +/* register the ops struct in rte_power_ops, return 0 on success. */
> > +int rte_power_register_ops(const struct rte_power_ops *op) {
> > + struct rte_power_ops *ops;
> > +
> > + if (op->env >= PM_ENV_MAX) {
> > + POWER_LOG(ERR, "Unsupported power management
> environment\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (op->status != 0) {
> > + POWER_LOG(ERR, "Power management env[%d] ops registered
> already\n",
> > + op->env);
> > + return -EINVAL;
> > + }
> > +
> > + if (!op->init || !op->exit || !op->check_env_support ||
> > + !op->get_avail_freqs || !op->get_freq || !op->set_freq ||
> > + !op->freq_up || !op->freq_down || !op->freq_max ||
> > + !op->freq_min || !op->turbo_status || !op->enable_turbo ||
> > + !op->disable_turbo || !op->get_caps) {
> > + POWER_LOG(ERR, "Missing callbacks while registering power
> ops\n");
> > + return -EINVAL;
> > + }
> > +
> > + ops = &rte_power_ops[op->env];
> >
>
> I don't see all drivers set 'op->env',
>
> This 'rte_power_register_ops()' function copies ops from driver proved struct
> to
> library global 'rte_power_ops[]' array,
>
> it can be possible to store ops pointer provided by driver, instead of
> copying it.
>
> And it can be possible to link the ops in this function, instead of putting
> them to
> specific index, as only one ops can be active in a given time, it can be
> possible to
> store active ops pointer in a global variable which removes the need to have
> index
> accessible array for ops.
Agreed. I will rework this to a new struct which can hold a reference to the
respective ops struct.
>
> <...>
>
> > @@ -177,59 +138,76 @@ int
> > rte_power_init(unsigned int lcore_id) {
> > int ret = -1;
> > + struct rte_power_ops *ops;
> >
> > - switch (global_default_env) {
> > - case PM_ENV_ACPI_CPUFREQ:
> > - return power_acpi_cpufreq_init(lcore_id);
> > - case PM_ENV_KVM_VM:
> > - return power_kvm_vm_init(lcore_id);
> > - case PM_ENV_PSTATE_CPUFREQ:
> > - return power_pstate_cpufreq_init(lcore_id);
> > - case PM_ENV_CPPC_CPUFREQ:
> > - return power_cppc_cpufreq_init(lcore_id);
> > - case PM_ENV_AMD_PSTATE_CPUFREQ:
> > - return power_amd_pstate_cpufreq_init(lcore_id);
> > - default:
> > - POWER_LOG(INFO, "Env isn't set yet!");
> > + if (global_default_env != PM_ENV_NOT_SET) {
> > + ops = &rte_power_ops[global_default_env];
> > + if (!ops->status) {
> > + POWER_LOG(ERR, "Power management env[%d] not"
> > + " supported\n", global_default_env);
> > + goto out;
> > + }
> > + return ops->init(lcore_id);
> > }
> > + POWER_LOG(INFO, POWER, "Env isn't set yet!\n");
> >
> > /* Auto detect Environment */
> > - POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq power
> management...");
> > - ret = power_acpi_cpufreq_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_ACPI_CPUFREQ);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq"
> > + " power management...\n");
> >
>
> Shouldn't break the log, can break the line by keeping message whole:
> POWER_LOG(INFO,
> "Attempting to initialise ACPI cpufreq power management...");
>
> <...>
ACK
>
> > @@ -21,7 +22,7 @@ extern "C" {
> > /* Power Management Environment State */ enum power_management_env
> > {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
> > PM_ENV_PSTATE_CPUFREQ, PM_ENV_CPPC_CPUFREQ,
> > - PM_ENV_AMD_PSTATE_CPUFREQ};
> > + PM_ENV_AMD_PSTATE_CPUFREQ, PM_ENV_MAX};
> >
>
> Syntax. Can we have enum item per line?
ACK
>
> > /**
> > * Check if a specific power management environment type is supported
> > on a @@ -66,6 +67,97 @@ void rte_power_unset_env(void);
> > */
> > enum power_management_env rte_power_get_env(void);
> >
> > +typedef int (*rte_power_cpufreq_init_t)(unsigned int lcore_id);
> > +typedef int (*rte_power_cpufreq_exit_t)(unsigned int lcore_id);
> > +typedef int (*rte_power_check_env_support_t)(void);
> > +
> > +typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, uint32_t
> > *freqs,
> > + uint32_t num);
> > +typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id);
> > +typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t
> > +index); typedef int (*rte_power_freq_change_t)(unsigned int
> > +lcore_id);
> > +
> >
>
> I guess above is not required for users, what do you think to create a driver
> header
> file and move these to driver header file?
>
> <...>
>
> > +
> > +/**
> > + * Macro to statically register the ops of a cpufreq driver.
> > + */
> > +#define RTE_POWER_REGISTER_OPS(ops) \
> > + (RTE_INIT(power_hdlr_init_##ops) \
> > + { \
> > + rte_power_register_ops(&ops); \
> > + })
> >
>
> is () required around RTE_INIT()
This was added to address the checkpatch errors.
>
> > +
> > +/**
> > + * @internal Get the power ops struct from its index.
> > + *
> > + * @param ops_index
> > + * The index of the ops struct in the ops struct table.
> > + * @return
> > + * The pointer to the ops struct in the table if registered.
> > + */
> > +struct rte_power_ops *
> > +rte_power_get_ops(int ops_index);
> > +
> > /**
> > * Initialize power management for a specific lcore. If
> > rte_power_set_env() has
> > * not been called then an auto-detect of the environment will start
> > and @@ -108,10 +200,14 @@ int rte_power_exit(unsigned int lcore_id);
> > * @return
> > * The number of available frequencies.
> > */
> > -typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, uint32_t
> > *freqs,
> > - uint32_t num);
> > +static inline uint32_t
> > +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n) {
> > + struct rte_power_ops *ops;
> >
> > -extern rte_power_freqs_t rte_power_freqs;
> > + ops = rte_power_get_ops(rte_power_get_env());
> > + return ops->get_avail_freqs(lcore_id, freqs, n); }
> >
>
> Why not proper functions but "static inline functions"?
These inline functions are expected to be called from datapath and to avoid
additional cycles with the refactor.
>
> >
> > /**
> > * Return the current index of available frequencies of a specific lcore.
> > @@ -124,9 +220,14 @@ extern rte_power_freqs_t rte_power_freqs;
> > * @return
> > * The current index of available frequencies.
> > */
> > -typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id);
> > +static inline uint32_t
> > +rte_power_get_freq(unsigned int lcore_id) {
> > + struct rte_power_ops *ops;
> >
> > -extern rte_power_get_freq_t rte_power_get_freq;
> > + ops = rte_power_get_ops(rte_power_get_env());
> >
>
> As 'rte_power_get_env()' already returns a global variable, why not set a
> global ops
> pointer and directly access to them, is above abstraction providing any
> benefit?
rte_power_get_ops() internally will check if the respective ops struct is
registered or not.
I will rework it and keep global ops to get populated in rte_power_set_env().
>
>
