Hi Sivaprasad, A couple of comments below: On 20/07/2024 17:50, 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 > --- > drivers/meson.build | 1 + > .../power/acpi/acpi_cpufreq.c | 22 +- > .../power/acpi/acpi_cpufreq.h | 6 +- > drivers/power/acpi/meson.build | 10 + > .../power/amd_pstate/amd_pstate_cpufreq.c | 24 +- > .../power/amd_pstate/amd_pstate_cpufreq.h | 8 +- > drivers/power/amd_pstate/meson.build | 10 + > .../power/cppc/cppc_cpufreq.c | 22 +- > .../power/cppc/cppc_cpufreq.h | 8 +- > drivers/power/cppc/meson.build | 10 + > .../power/kvm_vm}/guest_channel.c | 0 > .../power/kvm_vm}/guest_channel.h | 0 > .../power/kvm_vm/kvm_vm.c | 22 +- > .../power/kvm_vm/kvm_vm.h | 6 +- > drivers/power/kvm_vm/meson.build | 16 + > drivers/power/meson.build | 12 + > drivers/power/pstate/meson.build | 10 + > .../power/pstate/pstate_cpufreq.c | 22 +- > .../power/pstate/pstate_cpufreq.h | 6 +- > lib/power/meson.build | 7 +- > lib/power/power_common.c | 2 +- > lib/power/power_common.h | 16 +- > lib/power/rte_power.c | 287 ++++++------------ > lib/power/rte_power.h | 139 ++++++--- > lib/power/rte_power_core_ops.h | 208 +++++++++++++ > lib/power/version.map | 14 + > 26 files changed, 618 insertions(+), 270 deletions(-) > rename lib/power/power_acpi_cpufreq.c => drivers/power/acpi/acpi_cpufreq.c (95%) > rename lib/power/power_acpi_cpufreq.h => drivers/power/acpi/acpi_cpufreq.h (98%) > create mode 100644 drivers/power/acpi/meson.build > rename lib/power/power_amd_pstate_cpufreq.c => drivers/power/amd_pstate/amd_pstate_cpufreq.c (95%) > rename lib/power/power_amd_pstate_cpufreq.h => drivers/power/amd_pstate/amd_pstate_cpufreq.h (97%) > create mode 100644 drivers/power/amd_pstate/meson.build > rename lib/power/power_cppc_cpufreq.c => drivers/power/cppc/cppc_cpufreq.c (95%) > rename lib/power/power_cppc_cpufreq.h => drivers/power/cppc/cppc_cpufreq.h (97%) > create mode 100644 drivers/power/cppc/meson.build > rename {lib/power => drivers/power/kvm_vm}/guest_channel.c (100%) > rename {lib/power => drivers/power/kvm_vm}/guest_channel.h (100%) > rename lib/power/power_kvm_vm.c => drivers/power/kvm_vm/kvm_vm.c (82%) > rename lib/power/power_kvm_vm.h => drivers/power/kvm_vm/kvm_vm.h (98%) > create mode 100644 drivers/power/kvm_vm/meson.build > create mode 100644 drivers/power/meson.build > create mode 100644 drivers/power/pstate/meson.build > rename lib/power/power_pstate_cpufreq.c => drivers/power/pstate/pstate_cpufreq.c (96%) > rename lib/power/power_pstate_cpufreq.h => drivers/power/pstate/pstate_cpufreq.h (98%) > create mode 100644 lib/power/rte_power_core_ops.h --snip-- > diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c > index 36c3f3da98..8afb5949b9 100644 > --- a/lib/power/rte_power.c > +++ b/lib/power/rte_power.c > @@ -8,153 +8,86 @@ > #include > > #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_core_ops *global_power_core_ops; Suggest initialising this to NULL so we can check in rte_power_get_core_ops if it's null and throw an error. --snip-- > +struct rte_power_core_ops * > +rte_power_get_core_ops(void) > +{ Need a check here to see if rte_power_get_core_ops is NULL. If it is, then the developer has probably called a frequency change API before the relevant init function, so throw an error. Also, all the functions that call this need to check if it returns NULL so as to avoid a segfault when they attempts to call the op function. > + return global_power_core_ops; > +} > + --snip-- > diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h > index 4fa4afe399..5e4aacf08b 100644 > --- a/lib/power/rte_power.h > +++ b/lib/power/rte_power.h > @@ -1,5 +1,6 @@ > /* SPDX-License-Identifier: BSD-3-Clause > * Copyright(c) 2010-2014 Intel Corporation > + * Copyright(c) 2024 Advanced Micro Devices, Inc. > */ > > #ifndef _RTE_POWER_H > @@ -14,14 +15,21 @@ > #include > #include > > +#include "rte_power_core_ops.h" > + > #ifdef __cplusplus > extern "C" { > #endif > > /* 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}; > +enum power_management_env { > + PM_ENV_NOT_SET = 0, > + PM_ENV_ACPI_CPUFREQ, > + PM_ENV_KVM_VM, > + PM_ENV_PSTATE_CPUFREQ, > + PM_ENV_CPPC_CPUFREQ, > + PM_ENV_AMD_PSTATE_CPUFREQ > +}; > > /** > * Check if a specific power management environment type is supported on a > @@ -66,6 +74,15 @@ void rte_power_unset_env(void); > */ > enum power_management_env rte_power_get_env(void); > > +/** > + * @internal Get the power ops struct from its index. > + * > + * @return > + * The pointer to the ops struct in the table if registered. > + */ > +struct rte_power_core_ops * > +rte_power_get_core_ops(void); > + > /** > * 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 +125,13 @@ 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_core_ops *ops = rte_power_get_core_ops(); > > -extern rte_power_freqs_t rte_power_freqs; > + return ops->get_avail_freqs(lcore_id, freqs, n); This function will segfault if is called before the appropriate init is performed. See comments above on global_power_core_ops. Same for all the functions below that call global_power_core_ops(). > +} > > /** > * Return the current index of available frequencies of a specific lcore. > @@ -124,9 +144,13 @@ 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_core_ops *ops = rte_power_get_core_ops(); > > -extern rte_power_get_freq_t rte_power_get_freq; > + return ops->get_freq(lcore_id); > +} > > /** > * Set the new frequency for a specific lcore by indicating the index of > @@ -144,82 +168,101 @@ extern rte_power_get_freq_t rte_power_get_freq; > * - 0 on success without frequency changed. > * - Negative on error. > */ > -typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t index); > - > -extern rte_power_set_freq_t rte_power_set_freq; > +static inline uint32_t > +rte_power_set_freq(unsigned int lcore_id, uint32_t index) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > > -/** > - * Function pointer definition for generic frequency change functions. Review > - * each environments specific documentation for usage. > - * > - * @param lcore_id > - * lcore id. > - * > - * @return > - * - 1 on success with frequency changed. > - * - 0 on success without frequency changed. > - * - Negative on error. > - */ > -typedef int (*rte_power_freq_change_t)(unsigned int lcore_id); > + return ops->set_freq(lcore_id, index); > +} > > /** > * Scale up the frequency of a specific lcore according to the available > * frequencies. > * Review each environments specific documentation for usage. > */ > -extern rte_power_freq_change_t rte_power_freq_up; > +static inline int > +rte_power_freq_up(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->freq_up(lcore_id); > +} > > /** > * Scale down the frequency of a specific lcore according to the available > * frequencies. > * Review each environments specific documentation for usage. > */ > -extern rte_power_freq_change_t rte_power_freq_down; > +static inline int > +rte_power_freq_down(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->freq_down(lcore_id); > +} > > /** > * Scale up the frequency of a specific lcore to the highest according to the > * available frequencies. > * Review each environments specific documentation for usage. > */ > -extern rte_power_freq_change_t rte_power_freq_max; > +static inline int > +rte_power_freq_max(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->freq_max(lcore_id); > +} > > /** > * Scale down the frequency of a specific lcore to the lowest according to the > * available frequencies. > * Review each environments specific documentation for usage.. > */ > -extern rte_power_freq_change_t rte_power_freq_min; > +static inline int > +rte_power_freq_min(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->freq_min(lcore_id); > +} > > /** > * Query the Turbo Boost status of a specific lcore. > * Review each environments specific documentation for usage.. > */ > -extern rte_power_freq_change_t rte_power_turbo_status; > +static inline int > +rte_power_turbo_status(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->turbo_status(lcore_id); > +} > > /** > * Enable Turbo Boost for this lcore. > * Review each environments specific documentation for usage.. > */ > -extern rte_power_freq_change_t rte_power_freq_enable_turbo; > +static inline int > +rte_power_freq_enable_turbo(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > + > + return ops->enable_turbo(lcore_id); > +} > > /** > * Disable Turbo Boost for this lcore. > * Review each environments specific documentation for usage.. > */ > -extern rte_power_freq_change_t rte_power_freq_disable_turbo; > +static inline int > +rte_power_freq_disable_turbo(unsigned int lcore_id) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > > -/** > - * Power capabilities summary. > - */ > -struct rte_power_core_capabilities { > - union { > - uint64_t capabilities; > - struct { > - uint64_t turbo:1; /**< Turbo can be enabled. */ > - uint64_t priority:1; /**< SST-BF high freq core */ > - }; > - }; > -}; > + return ops->disable_turbo(lcore_id); > +} > > /** > * Returns power capabilities for a specific lcore. > @@ -235,10 +278,14 @@ struct rte_power_core_capabilities { > * - 0 on success. > * - Negative on error. > */ > -typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id, > - struct rte_power_core_capabilities *caps); > +static inline int > +rte_power_get_capabilities(unsigned int lcore_id, > + struct rte_power_core_capabilities *caps) > +{ > + struct rte_power_core_ops *ops = rte_power_get_core_ops(); > > -extern rte_power_get_capabilities_t rte_power_get_capabilities; > + return ops->get_caps(lcore_id, caps); > +} > > #ifdef __cplusplus > } --snip--