在 2024/10/26 13:22, Tummala, Sivaprasad 写道:
[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.
Ack
+ */
+__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.
ok
+
+/* 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>
