[AMD Official Use Only - AMD Internal Distribution Only]
Hi Huisong,
> -----Original Message-----
> From: lihuisong (C) <lihuis...@huawei.com>
> Sent: Saturday, October 26, 2024 8:37 AM
> To: Tummala, Sivaprasad <sivaprasad.tumm...@amd.com>
> Cc: dev@dpdk.org; david.h...@intel.com; anatoly.bura...@intel.com;
> jer...@marvell.com; radu.nico...@intel.com; cristian.dumitre...@intel.com;
> konstantin.anan...@huawei.com; Yigit, Ferruh <ferruh.yi...@amd.com>;
> gak...@marvell.com
> Subject: Re: [PATCH v9 1/6] power: refactor core power management library
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Sivaprasad,
>
> LGTM except for some trivial comments inline, With belows to change, you can
> add
> Acked-by: Huisong Li <lihuis...@huawei.com>
>
> /Huisong
> 在 2024/10/23 13:11, Sivaprasad Tummala 写道:
> > 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.
> >
> > v8:
> > - marked rte_power_logtype as internal
> > - removed c++ guards for internal header files
> > - renamed rte_power_cpufreq_api.h for naming convention
> > - renamed rte_power_register_ops for naming convention
> >
> > v6:
> > - fixed compilation error with symbol export in API
> > - exported power_get_lcore_mapped_cpu_id as internal API to be
> > used in drivers/power/*
> >
> > v5:
> > - fixed code style warning
> >
> > v4:
> > - fixed build error with RTE_ASSERT
> >
> > v3:
> > - renamed rte_power_core_ops.h as rte_power_cpufreq_api.h
> > - re-worked on auto detection logic
> >
> > v2:
> > - added NULL check for global_core_ops in rte_power_get_core_ops
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tumm...@amd.com>
> > ---
> <snip>
> > +
> > +/**
> > + * Register power cpu 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.
> Not the index in the table, need to fix it.
> BTW, this API always success now. so no return value.
API now consistently returns 0 on success, rather than an index in the table.
It will return a negative value on error.
I'll update the documentation to reflect this change and avoid any confusion.
> > + */
> > +__rte_internal
> > +int rte_power_register_cpufreq_ops(struct rte_power_cpufreq_ops
> > +*ops);
> > +
> > +/**
> > + * Macro to statically register the ops of a cpufreq driver.
> > + */
> > +#define RTE_POWER_REGISTER_CPUFREQ_OPS(ops) \
> > +RTE_INIT(power_hdlr_init_##ops) \
> > +{ \
> > + rte_power_register_cpufreq_ops(&ops); \ }
> > +
> > +#endif
> > diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c index
> > 36c3f3da98..3168b6d301 100644
> > --- a/lib/power/rte_power.c
> > +++ b/lib/power/rte_power.c
> > @@ -6,155 +6,88 @@
> >
> > #include <rte_errno.h>
> > #include <rte_spinlock.h>
> > +#include <rte_debug.h>
> >
> > #include "rte_power.h"
> > -#include "power_acpi_cpufreq.h"
> > -#include "power_cppc_cpufreq.h"
> > #include "power_common.h"
> > -#include "power_kvm_vm.h"
> > -#include "power_pstate_cpufreq.h"
> > -#include "power_amd_pstate_cpufreq.h"
> >
> > -enum power_management_env global_default_env = PM_ENV_NOT_SET;
> > +static enum power_management_env global_default_env =
> PM_ENV_NOT_SET;
> > +static struct rte_power_cpufreq_ops *global_cpufreq_ops;
> >
> > static rte_spinlock_t global_env_cfg_lock =
> > RTE_SPINLOCK_INITIALIZER;
> > -
> > -/* function pointers */
> > -rte_power_freqs_t rte_power_freqs = NULL; -rte_power_get_freq_t
> > rte_power_get_freq = NULL; -rte_power_set_freq_t rte_power_set_freq =
> > NULL; -rte_power_freq_change_t rte_power_freq_up = NULL;
> > -rte_power_freq_change_t rte_power_freq_down = NULL;
> > -rte_power_freq_change_t rte_power_freq_max = NULL;
> > -rte_power_freq_change_t rte_power_freq_min = NULL;
> > -rte_power_freq_change_t rte_power_turbo_status;
> > -rte_power_freq_change_t rte_power_freq_enable_turbo;
> > -rte_power_freq_change_t rte_power_freq_disable_turbo;
> > -rte_power_get_capabilities_t rte_power_get_capabilities;
> > -
> > -static void
> > -reset_power_function_ptrs(void)
> > +static RTE_TAILQ_HEAD(, rte_power_cpufreq_ops) cpufreq_ops_list =
> > + TAILQ_HEAD_INITIALIZER(cpufreq_ops_list);
> > +
> > +const char *power_env_str[] = {
> > + "not set",
> > + "acpi",
> > + "kvm-vm",
> > + "pstate",
> > + "cppc",
> > + "amd-pstate"
> > +};
>
> How use the "not set"? I don't know what its usage is. Do we need to consider
> removing it later?
The "not set" is default state and indicates no specific cpufreq management
driver is active.
If the specific driver (located in drivers/power/*) is disabled during the
build process,
the API will fail to configure the environment, leaving it in the "not set"
state.
>
> > +
> > +/* register the ops struct in rte_power_cpufreq_ops, return 0 on
> > +success. */ int rte_power_register_cpufreq_ops(struct
> > +rte_power_cpufreq_ops *driver_ops)
> > {
> > - rte_power_freqs = NULL;
> > - rte_power_get_freq = NULL;
> > - rte_power_set_freq = NULL;
> > - rte_power_freq_up = NULL;
> > - rte_power_freq_down = NULL;
> > - rte_power_freq_max = NULL;
> > - rte_power_freq_min = NULL;
> > - rte_power_turbo_status = NULL;
> > - rte_power_freq_enable_turbo = NULL;
> > - rte_power_freq_disable_turbo = NULL;
> > - rte_power_get_capabilities = NULL;
> > + if (!driver_ops->init || !driver_ops->exit ||
> > + !driver_ops->check_env_support ||
> > !driver_ops->get_avail_freqs ||
> > + !driver_ops->get_freq || !driver_ops->set_freq ||
> > + !driver_ops->freq_up || !driver_ops->freq_down ||
> > + !driver_ops->freq_max || !driver_ops->freq_min ||
> > + !driver_ops->turbo_status || !driver_ops->enable_turbo ||
> > + !driver_ops->disable_turbo || !driver_ops->get_caps) {
> > + POWER_LOG(ERR, "Missing callbacks while registering cpufreq
> > ops");
> > + return -EINVAL;
> > + }
> > +
> > + TAILQ_INSERT_TAIL(&cpufreq_ops_list, driver_ops, next);
> > +
> > + return 0;
> > }
> suggest that change function return value as above mention.
Same as above.
> >
> > int
> > rte_power_check_env_supported(enum power_management_env env)
> > {
> > - switch (env) {
> > - case PM_ENV_ACPI_CPUFREQ:
> > - return power_acpi_cpufreq_check_supported();
> > - case PM_ENV_PSTATE_CPUFREQ:
> > - return power_pstate_cpufreq_check_supported();
> > - case PM_ENV_KVM_VM:
> > - return power_kvm_vm_check_supported();
> > - case PM_ENV_CPPC_CPUFREQ:
> > - return power_cppc_cpufreq_check_supported();
> > - case PM_ENV_AMD_PSTATE_CPUFREQ:
> > - return power_amd_pstate_cpufreq_check_supported();
> > - default:
> > - rte_errno = EINVAL;
> > - return -1;
> > - }
> > + struct rte_power_cpufreq_ops *ops;
> > +
> > + if (env >= RTE_DIM(power_env_str))
> > + return 0;
> > +
> > + RTE_TAILQ_FOREACH(ops, &cpufreq_ops_list, next)
> > + if (strncmp(ops->name, power_env_str[env],
> > + RTE_POWER_DRIVER_NAMESZ) == 0)
> > + return ops->check_env_support();
> > +
> > + return 0;
> > }
> >
> <snip>
