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 <sivaprasad.tummala@amd.com>
---
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 <rte_spinlock.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_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.
[Siva] rte_power_core_ops as static global is initialized to NULL at runtime. Not sure, if it’s still required to initialize to NULL.
--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.
[Siva] ACK. Will fix this in next version.
+ 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 <rte_log.h>
#include <rte_power_guest_channel.h>
+#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--