Hi Sivaprasad,

Some comments inline.

在 2024/10/21 12:07, 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.

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>
---
  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     |  10 +-
  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              |  14 +
  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                      |  18 +-
  lib/power/rte_power.c                         | 355 ++++++++----------
  lib/power/rte_power.h                         | 116 +++---
  lib/power/rte_power_cpufreq_api.h             | 206 ++++++++++
  lib/power/version.map                         |  15 +
  26 files changed, 665 insertions(+), 269 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 (96%)
  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_cpufreq_api.h

diff --git a/drivers/meson.build b/drivers/meson.build
index 2733306698..7ef4f581a0 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -29,6 +29,7 @@ subdirs = [
          'event',          # depends on common, bus, mempool and net.
          'baseband',       # depends on common and bus.
          'gpu',            # depends on common and bus.
+        'power',          # depends on common (in future).
  ]
if meson.is_cross_build()
diff --git a/lib/power/power_acpi_cpufreq.c b/drivers/power/acpi/acpi_cpufreq.c
similarity index 95%
rename from lib/power/power_acpi_cpufreq.c
rename to drivers/power/acpi/acpi_cpufreq.c
index ae809fbb60..974fbb7ba8 100644
--- a/lib/power/power_acpi_cpufreq.c
+++ b/drivers/power/acpi/acpi_cpufreq.c
@@ -10,7 +10,7 @@
  #include <rte_stdatomic.h>
  #include <rte_string_fns.h>
-#include "power_acpi_cpufreq.h"
+#include "acpi_cpufreq.h"
  #include "power_common.h"
<...>
diff --git a/lib/power/power_common.c b/lib/power/power_common.c
index b47c63a5f1..e482f71c64 100644
--- a/lib/power/power_common.c
+++ b/lib/power/power_common.c
@@ -13,7 +13,7 @@
#include "power_common.h" -RTE_LOG_REGISTER_DEFAULT(power_logtype, INFO);
+RTE_LOG_REGISTER_DEFAULT(rte_power_logtype, INFO);
#define POWER_SYSFILE_SCALING_DRIVER \
                "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
diff --git a/lib/power/power_common.h b/lib/power/power_common.h
index 82fb94d0c0..c294f561bb 100644
--- a/lib/power/power_common.h
+++ b/lib/power/power_common.h
@@ -6,12 +6,13 @@
  #define _POWER_COMMON_H_
#include <rte_common.h>
+#include <rte_compat.h>
  #include <rte_log.h>
#define RTE_POWER_INVALID_FREQ_INDEX (~0) -extern int power_logtype;
-#define RTE_LOGTYPE_POWER power_logtype
+extern int rte_power_logtype;
+#define RTE_LOGTYPE_POWER rte_power_logtype
  #define POWER_LOG(level, ...) \
        RTE_LOG_LINE(level, POWER, "" __VA_ARGS__)
@@ -23,14 +24,27 @@ extern int power_logtype;
  #endif
/* check if scaling driver matches one we want */
+__rte_internal
  int cpufreq_check_scaling_driver(const char *driver);
+
+__rte_internal
  int power_set_governor(unsigned int lcore_id, const char *new_governor,
                char *orig_governor, size_t orig_governor_len);
cpufreq_check_scaling_driver and power_set_governor are just used for cpufreq, they shouldn't be put in this common header file.
We've come to an aggrement in patch V2 1/4.
I guess you forget it😁
suggest that move these two APIs to rte_power_cpufreq_api.h.
+
+__rte_internal
  int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
                __rte_format_printf(3, 4);
+
+__rte_internal
  int read_core_sysfs_u32(FILE *f, uint32_t *val);
+
+__rte_internal
  int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
+
+__rte_internal
  int write_core_sysfs_s(FILE *f, const char *str);
+
+__rte_internal
  int power_get_lcore_mapped_cpu_id(uint32_t lcore_id, uint32_t *cpu_id);
#endif /* _POWER_COMMON_H_ */
diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c
index 36c3f3da98..416f0148a3 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_core_ops *global_power_core_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_core_ops) core_ops_list =
+                       TAILQ_HEAD_INITIALIZER(core_ops_list);
+
+const char *power_env_str[] = {
+       "not set",
+       "acpi",
+       "kvm-vm",
+       "pstate",
+       "cppc",
+       "amd-pstate"
+};
+

<...>
+uint32_t
+rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->get_avail_freqs(lcore_id, freqs, n);
+}
+
+uint32_t
+rte_power_get_freq(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->get_freq(lcore_id);
+}
+
+uint32_t
+rte_power_set_freq(unsigned int lcore_id, uint32_t index)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->set_freq(lcore_id, index);
+}
+
+int
+rte_power_freq_up(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->freq_up(lcore_id);
+}
+
+int
+rte_power_freq_down(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->freq_down(lcore_id);
+}
+
+int
+rte_power_freq_max(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->freq_max(lcore_id);
+}
+
+int
+rte_power_freq_min(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->freq_min(lcore_id);
+}
+int
+rte_power_turbo_status(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->turbo_status(lcore_id);
+}
+
+int
+rte_power_freq_enable_turbo(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->enable_turbo(lcore_id);
+}
+
+int
+rte_power_freq_disable_turbo(unsigned int lcore_id)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->disable_turbo(lcore_id);
+}
+
+int
+rte_power_get_capabilities(unsigned int lcore_id,
+               struct rte_power_core_capabilities *caps)
+{
+       RTE_ASSERT(global_power_core_ops != NULL);
+       return global_power_core_ops->get_caps(lcore_id, caps);
  }
diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h
index 4fa4afe399..e9a72b92ad 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_cpufreq_api.h"
From the name of rte_power.c and rte_power.h, they are supposed to work for all power libraries I also proposed in previous version. But rte_power.* currently just work for cpufreq lib. If we need to put all power components togeter and create it.
Now that the rte_power_cpufreq_api.h has been created for cpufreq library.
How about directly rename rte_power.c to rte_poer_cpufreq_api.c and rte_power.h to rte_power_cpufreq_api.h? There will be ABI changes, but it is allowed in this 24.11. If we plan to do it later, we'll have to wait another year.
+
  #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
+};
<...>

Reply via email to