[PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Shyam Sundar S K]

In this series, support for following features has been added.
- "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
  where the system power can be boosted or throttled independent
  of the selected slider position.
- On the fly, the CnQF can be turned on/off via a sysfs knob.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Shyam Sundar S K (4):
  platform/x86/amd/pmf: Add support for CnQF
  platform/x86/amd/pmf: Add sysfs to toggle CnQF
  Documentation/ABI/testing/sysfs-amd-pmf: Add ABI doc for AMD PMF
  MAINTAINERS: Update ABI doc path for AMD PMF

 Documentation/ABI/testing/sysfs-amd-pmf |  11 +
 MAINTAINERS                             |   1 +
 drivers/platform/x86/amd/pmf/Makefile   |   2 +-
 drivers/platform/x86/amd/pmf/acpi.c     |  10 +
 drivers/platform/x86/amd/pmf/cnqf.c     | 374 ++++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/core.c     |  16 +-
 drivers/platform/x86/amd/pmf/pmf.h      | 100 +++++++
 7 files changed, 512 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-amd-pmf
 create mode 100644 drivers/platform/x86/amd/pmf/cnqf.c

-- 
2.25.1

[PATCH 1/4] platform/x86/amd/pmf: Add support for CnQF

[Shyam Sundar S K]

CnQF (a.k.a Cool and Quiet Framework) extends the static slider concept.
PMF dynamically manages system power limits and fan policy based on system
power trends which is representative of workload trend.

Static slider and CnQF controls are mutually exclusive for system power
budget adjustments. CnQF supports configurable number of modes which can
be unique for AC and DC. Every mode is representative of a system state
characterized by unique steady state and boost behavior.

OEMs can configure the different modes/system states and how the
transition to a mode happens. Whether to have CnQF manage system power
budget dynamically in AC or DC or both is also configurable. Mode changes
due to CnQF don’t result in slider position change.

The default OEM values are obtained after evaluating the PMF ACPI function
idx 11 & 12 for AC and DC respectively. Whether to turn ON/OFF by default
is guided by a "flag" passed by the OEM BIOS.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/Makefile |   2 +-
 drivers/platform/x86/amd/pmf/acpi.c   |  10 +
 drivers/platform/x86/amd/pmf/cnqf.c   | 314 ++++++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/core.c   |  16 +-
 drivers/platform/x86/amd/pmf/pmf.h    | 100 ++++++++
 5 files changed, 440 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/x86/amd/pmf/cnqf.c

diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
index ef2b08c9174d..fdededf54392 100644
--- a/drivers/platform/x86/amd/pmf/Makefile
+++ b/drivers/platform/x86/amd/pmf/Makefile
@@ -6,4 +6,4 @@
 
 obj-$(CONFIG_AMD_PMF) += amd-pmf.o
 amd-pmf-objs := core.o acpi.o sps.o \
-		auto-mode.o
+		auto-mode.o cnqf.o
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index cb46a7252414..05a2b8a056fc 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -233,6 +233,16 @@ static int apmf_get_system_params(struct amd_pmf_dev *dev)
 	return 0;
 }
 
+int apmf_get_dyn_slider_def_ac(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data)
+{
+	return apmf_if_call_store_buffer(pdev, APMF_FUNC_DYN_SLIDER_AC, data, sizeof(*data));
+}
+
+int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data)
+{
+	return apmf_if_call_store_buffer(pdev, APMF_FUNC_DYN_SLIDER_DC, data, sizeof(*data));
+}
+
 void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
 {
 	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
new file mode 100644
index 000000000000..b8abe0f4447a
--- /dev/null
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Platform Management Framework Driver
+ *
+ * Copyright (c) 2022, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ */
+
+#include <linux/workqueue.h>
+#include "pmf.h"
+
+static struct cnqf_config config_store;
+
+static int amd_pmf_handle_cnqf(struct amd_pmf_dev *dev, int src, int idx,
+			       struct cnqf_config *table)
+{
+	struct power_table_control *pc;
+
+	pc = &config_store.mode_set[src][idx].power_control;
+
+	amd_pmf_send_cmd(dev, SET_SPL, false, pc->spl, NULL);
+	amd_pmf_send_cmd(dev, SET_FPPT, false, pc->fppt, NULL);
+	amd_pmf_send_cmd(dev, SET_SPPT, false, pc->sppt, NULL);
+	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pc->sppt_apu_only, NULL);
+	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pc->stt_min, NULL);
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU],
+			 NULL);
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2],
+			 NULL);
+
+	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
+		apmf_update_fan_idx(dev,
+				    config_store.mode_set[src][idx].fan_control.manual,
+				    config_store.mode_set[src][idx].fan_control.fan_id);
+
+	return 0;
+}
+
+static void amd_pmf_update_power_threshold(void)
+{
+	struct cnqf_mode_settings *ts;
+	struct cnqf_tran_params *tp;
+	int i;
+
+	for (i = 0; i < POWER_SOURCE_MAX; i++) {
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_TO_QUIET];
+		ts = &config_store.mode_set[i][CNQF_MODE_BALANCE];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_TO_TURBO];
+		ts = &config_store.mode_set[i][CNQF_MODE_PERFORMANCE];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE];
+		ts = &config_store.mode_set[i][CNQF_MODE_BALANCE];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE];
+		ts = &config_store.mode_set[i][CNQF_MODE_PERFORMANCE];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_QUIET_TO_BALANCE];
+		ts = &config_store.mode_set[i][CNQF_MODE_QUIET];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+
+		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE];
+		ts = &config_store.mode_set[i][CNQF_MODE_TURBO];
+		tp->power_threshold = ts->power_floor - tp->power_delta;
+	}
+}
+
+static const char *state_as_str(unsigned int state)
+{
+	switch (state) {
+	case CNQF_MODE_QUIET:
+		return "QUIET";
+	case CNQF_MODE_BALANCE:
+		return "BALANCED";
+	case CNQF_MODE_TURBO:
+		return "TURBO";
+	case CNQF_MODE_PERFORMANCE:
+		return "PERFORMANCE";
+	default:
+		return "Unknown CnQF mode";
+	}
+}
+
+void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
+{
+	struct cnqf_tran_params *tp;
+	int src, i, j, index = 0;
+	u32 avg_power = 0;
+
+	src = amd_pmf_get_power_source();
+
+	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
+		config_store.trans_param[src][i].timer += time_lapsed_ms;
+		config_store.trans_param[src][i].total_power += socket_power;
+		config_store.trans_param[src][i].count++;
+
+		tp = &config_store.trans_param[src][i];
+		if (tp->timer >= tp->time_constant && tp->count) {
+			avg_power = tp->total_power / tp->count;
+
+			/* Reset the indices */
+			tp->timer = 0;
+			tp->total_power = 0;
+			tp->count = 0;
+
+			if ((tp->shifting_up && avg_power >= tp->power_threshold) ||
+			    (!tp->shifting_up && avg_power <= tp->power_threshold)) {
+				tp->priority = true;
+			} else {
+				tp->priority = false;
+			}
+		}
+	}
+
+	dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n",
+		avg_power, socket_power, state_as_str(config_store.current_mode));
+
+	for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
+		/* apply the highest priority */
+		index = config_store.trans_prio[j];
+		if (config_store.trans_param[src][index].priority) {
+			if (config_store.current_mode !=
+			    config_store.trans_param[src][index].target_mode) {
+				config_store.current_mode =
+						config_store.trans_param[src][index].target_mode;
+				dev_dbg(dev->dev, "Moving to Mode :%s\n",
+					state_as_str(config_store.current_mode));
+				amd_pmf_handle_cnqf(dev, src,
+						    config_store.current_mode, NULL);
+			}
+			break;
+		}
+	}
+}
+
+static void amd_pmf_update_trans_data(int idx, struct apmf_dyn_slider_output out)
+{
+	struct cnqf_tran_params *tp;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_TO_QUIET];
+	tp->time_constant = out.t_balanced_to_quiet;
+	tp->target_mode = CNQF_MODE_QUIET;
+	tp->shifting_up = false;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE];
+	tp->time_constant = out.t_balanced_to_perf;
+	tp->target_mode = CNQF_MODE_PERFORMANCE;
+	tp->shifting_up = true;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_QUIET_TO_BALANCE];
+	tp->time_constant = out.t_quiet_to_balanced;
+	tp->target_mode = CNQF_MODE_BALANCE;
+	tp->shifting_up = true;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE];
+	tp->time_constant = out.t_perf_to_balanced;
+	tp->target_mode = CNQF_MODE_BALANCE;
+	tp->shifting_up = false;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE];
+	tp->time_constant = out.t_turbo_to_perf;
+	tp->target_mode = CNQF_MODE_PERFORMANCE;
+	tp->shifting_up = false;
+
+	tp = &config_store.trans_param[idx][CNQF_TRANSITION_TO_TURBO];
+	tp->time_constant = out.t_perf_to_turbo;
+	tp->target_mode = CNQF_MODE_TURBO;
+	tp->shifting_up = true;
+}
+
+static void amd_pmf_update_mode_set(int idx, struct apmf_dyn_slider_output out)
+{
+	struct cnqf_mode_settings *ms;
+
+	/* Quiet Mode */
+	ms = &config_store.mode_set[idx][CNQF_MODE_QUIET];
+	ms->power_floor = out.ps[APMF_CNQF_QUIET].pfloor;
+	ms->power_control.fppt = out.ps[APMF_CNQF_QUIET].fppt;
+	ms->power_control.sppt = out.ps[APMF_CNQF_QUIET].sppt;
+	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_QUIET].sppt_apu_only;
+	ms->power_control.spl = out.ps[APMF_CNQF_QUIET].spl;
+	ms->power_control.stt_min = out.ps[APMF_CNQF_QUIET].stt_min_limit;
+	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
+		out.ps[APMF_CNQF_QUIET].stt_skintemp[STT_TEMP_APU];
+	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
+		out.ps[APMF_CNQF_QUIET].stt_skintemp[STT_TEMP_HS2];
+	ms->fan_control.fan_id = out.ps[APMF_CNQF_QUIET].fan_id;
+
+	/* Balance Mode */
+	ms = &config_store.mode_set[idx][CNQF_MODE_BALANCE];
+	ms->power_floor = out.ps[APMF_CNQF_BALANCE].pfloor;
+	ms->power_control.fppt = out.ps[APMF_CNQF_BALANCE].fppt;
+	ms->power_control.sppt = out.ps[APMF_CNQF_BALANCE].sppt;
+	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_BALANCE].sppt_apu_only;
+	ms->power_control.spl = out.ps[APMF_CNQF_BALANCE].spl;
+	ms->power_control.stt_min = out.ps[APMF_CNQF_BALANCE].stt_min_limit;
+	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
+		out.ps[APMF_CNQF_BALANCE].stt_skintemp[STT_TEMP_APU];
+	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
+		out.ps[APMF_CNQF_BALANCE].stt_skintemp[STT_TEMP_HS2];
+	ms->fan_control.fan_id = out.ps[APMF_CNQF_BALANCE].fan_id;
+
+	/* Performance Mode */
+	ms = &config_store.mode_set[idx][CNQF_MODE_PERFORMANCE];
+	ms->power_floor = out.ps[APMF_CNQF_PERFORMANCE].pfloor;
+	ms->power_control.fppt = out.ps[APMF_CNQF_PERFORMANCE].fppt;
+	ms->power_control.sppt = out.ps[APMF_CNQF_PERFORMANCE].sppt;
+	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_PERFORMANCE].sppt_apu_only;
+	ms->power_control.spl = out.ps[APMF_CNQF_PERFORMANCE].spl;
+	ms->power_control.stt_min = out.ps[APMF_CNQF_PERFORMANCE].stt_min_limit;
+	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
+		out.ps[APMF_CNQF_PERFORMANCE].stt_skintemp[STT_TEMP_APU];
+	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
+		out.ps[APMF_CNQF_PERFORMANCE].stt_skintemp[STT_TEMP_HS2];
+	ms->fan_control.fan_id = out.ps[APMF_CNQF_PERFORMANCE].fan_id;
+
+	/* Turbo Mode */
+	ms = &config_store.mode_set[idx][CNQF_MODE_TURBO];
+	ms->power_floor = out.ps[APMF_CNQF_TURBO].pfloor;
+	ms->power_control.fppt = out.ps[APMF_CNQF_TURBO].fppt;
+	ms->power_control.sppt = out.ps[APMF_CNQF_TURBO].sppt;
+	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_TURBO].sppt_apu_only;
+	ms->power_control.spl = out.ps[APMF_CNQF_TURBO].spl;
+	ms->power_control.stt_min = out.ps[APMF_CNQF_TURBO].stt_min_limit;
+	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
+		out.ps[APMF_CNQF_TURBO].stt_skintemp[STT_TEMP_APU];
+	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
+		out.ps[APMF_CNQF_TURBO].stt_skintemp[STT_TEMP_HS2];
+	ms->fan_control.fan_id = out.ps[APMF_CNQF_TURBO].fan_id;
+}
+
+static int amd_pmf_check_flags(struct amd_pmf_dev *dev)
+{
+	struct apmf_dyn_slider_output out = {};
+
+	if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC))
+		apmf_get_dyn_slider_def_ac(dev, &out);
+	else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC))
+		apmf_get_dyn_slider_def_dc(dev, &out);
+
+	return out.flags;
+}
+
+void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
+{
+	struct apmf_dyn_slider_output out;
+	int i, j, ret;
+
+	for (i = 0; i < POWER_SOURCE_MAX; i++) {
+		if (i == POWER_SOURCE_AC && is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC)) {
+			ret = apmf_get_dyn_slider_def_ac(dev, &out);
+			if (ret)
+				dev_err(dev->dev,
+					"APMF apmf_get_dyn_slider_def_ac failed :%d\n", ret);
+		} else if (i == POWER_SOURCE_DC &&
+				is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
+			ret = apmf_get_dyn_slider_def_dc(dev, &out);
+			if (ret)
+				dev_err(dev->dev,
+					"APMF apmf_get_dyn_slider_def_dc failed :%d\n", ret);
+		}
+
+		amd_pmf_update_mode_set(i, out);
+		amd_pmf_update_trans_data(i, out);
+
+		for (j = 0; j < CNQF_MODE_MAX; j++) {
+			if (config_store.mode_set[i][j].fan_control.fan_id == FAN_INDEX_AUTO)
+				config_store.mode_set[i][j].fan_control.manual = false;
+			else
+				config_store.mode_set[i][j].fan_control.manual = true;
+		}
+	}
+	amd_pmf_update_power_threshold();
+
+	for (i = 0; i < CNQF_TRANSITION_MAX; i++)
+		config_store.trans_prio[i] = i;
+}
+
+void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev)
+{
+	cancel_delayed_work_sync(&dev->work_buffer);
+}
+
+void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
+{
+	int ret, src;
+
+	ret = amd_pmf_check_flags(dev);
+	if (!ret) {
+		dev_dbg(dev->dev, "CnQF bios default_enable flag not set\n");
+		return;
+	}
+
+	dev->cnqf_enabled = true;
+
+	/* Disable SPS if its advertised as supported, giving clear preference
+	 * to user selection choices
+	 */
+	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
+		amd_pmf_deinit_sps(dev);
+
+	amd_pmf_load_defaults_cnqf(dev);
+	amd_pmf_init_metrics_table(dev);
+
+	/* update the thermal for CnQF */
+	src = amd_pmf_get_power_source();
+	amd_pmf_handle_cnqf(dev, src, config_store.current_mode, NULL);
+}
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index a675ca969331..2cb7793d07cf 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -123,6 +123,11 @@ static void amd_pmf_get_metrics(struct work_struct *work)
 		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
 	}
 
+	if (dev->cnqf_enabled) {
+		/* Apply the CnQF transition */
+		amd_pmf_trans_cnqf(dev, socket_power, time_elapsed_ms);
+	}
+
 	dev->start_time = ktime_to_ms(ktime_get());
 	schedule_delayed_work(&dev->work_buffer, msecs_to_jiffies(metrics_table_loop_ms));
 	mutex_unlock(&dev->update_mutex);
@@ -262,6 +267,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
 	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
 		amd_pmf_init_auto_mode(dev);
 		dev_dbg(dev->dev, "Auto Mode Init done\n");
+	} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
+			  is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
+		/* Enable Cool n Quiet Framework (CnQF) */
+		amd_pmf_init_cnqf(dev);
+		dev_dbg(dev->dev, "CnQF Init done\n");
 	}
 }
 
@@ -270,8 +280,12 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
 	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
 		amd_pmf_deinit_sps(dev);
 
-	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE))
+	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
 		amd_pmf_deinit_auto_mode(dev);
+	} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
+			  is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
+		amd_pmf_deinit_cnqf(dev);
+	}
 }
 
 static const struct acpi_device_id amd_pmf_acpi_ids[] = {
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 0a72a395c2ef..db3c86c2e86f 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -22,6 +22,8 @@
 #define APMF_FUNC_AUTO_MODE					5
 #define APMF_FUNC_SET_FAN_IDX				7
 #define APMF_FUNC_STATIC_SLIDER_GRANULAR       9
+#define APMF_FUNC_DYN_SLIDER_AC				11
+#define APMF_FUNC_DYN_SLIDER_DC				12
 
 /* Message Definitions */
 #define SET_SPL				0x03 /* SPL: Sustained Power Limit */
@@ -165,6 +167,7 @@ struct amd_pmf_dev {
 	int socket_power_history_idx;
 	bool amt_enabled;
 	struct mutex update_mutex; /* protects race between ACPI handler and metrics thread */
+	bool cnqf_enabled;
 };
 
 struct apmf_sps_prop_granular {
@@ -294,6 +297,94 @@ struct apmf_auto_mode {
 	u32 fan_id_quiet;
 } __packed;
 
+/* CnQF Layer */
+enum cnqf_trans_priority {
+	CNQF_TRANSITION_TO_TURBO, /* Any other mode to Turbo Mode */
+	CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE, /* quiet/balance to Performance Mode */
+	CNQF_TRANSITION_FROM_QUIET_TO_BALANCE, /* Quiet Mode to Balance Mode */
+	CNQF_TRANSITION_TO_QUIET, /* Any other mode to Quiet Mode */
+	CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE, /* Performance/Turbo to Balance Mode */
+	CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE, /* Turbo mode to Performance Mode */
+	CNQF_TRANSITION_MAX,
+};
+
+enum cnqf_mode {
+	CNQF_MODE_QUIET,
+	CNQF_MODE_BALANCE,
+	CNQF_MODE_PERFORMANCE,
+	CNQF_MODE_TURBO,
+	CNQF_MODE_MAX,
+};
+
+enum apmf_cnqf_pos {
+	APMF_CNQF_TURBO,
+	APMF_CNQF_PERFORMANCE,
+	APMF_CNQF_BALANCE,
+	APMF_CNQF_QUIET,
+	APMF_CNQF_MAX,
+};
+
+struct cnqf_mode_settings {
+	struct power_table_control power_control;
+	struct fan_table_control fan_control;
+	u32 power_floor;
+};
+
+struct cnqf_tran_params {
+	u32 time_constant; /* minimum time required to switch to next mode */
+	u32 power_delta; /* minimum power required to switch to next mode */
+	u32 power_threshold;
+	u32 timer; /* elapsed time. if timer > timethreshold, it will move to next mode */
+	u32 total_power;
+	u32 count;
+	bool priority;
+	bool shifting_up;
+	enum cnqf_mode target_mode;
+};
+
+struct cnqf_power_delta {
+	u32 to_turbo;
+	u32 balance_to_perf;
+	u32 quiet_to_balance;
+	u32 to_quiet;
+	u32 perf_to_balance;
+	u32 turbo_to_perf;
+};
+
+struct cnqf_config {
+	struct cnqf_tran_params trans_param[POWER_SOURCE_MAX][CNQF_TRANSITION_MAX];
+	struct cnqf_mode_settings mode_set[POWER_SOURCE_MAX][CNQF_MODE_MAX];
+	struct power_table_control defaults;
+	enum cnqf_mode current_mode;
+	struct cnqf_power_delta power_delta[POWER_SOURCE_MAX];
+	u32 power_src;
+	u32 avg_power;
+	enum cnqf_trans_priority trans_prio[CNQF_TRANSITION_MAX];
+};
+
+struct apmf_cnqf_power_set {
+	u32 pfloor;
+	u32 fppt;
+	u32 sppt;
+	u32 sppt_apu_only;
+	u32 spl;
+	u32 stt_min_limit;
+	u8 stt_skintemp[STT_TEMP_COUNT];
+	u32 fan_id;
+} __packed;
+
+struct apmf_dyn_slider_output {
+	u16 size;
+	u16 flags;
+	u32 t_perf_to_turbo;
+	u32 t_balanced_to_perf;
+	u32 t_quiet_to_balanced;
+	u32 t_balanced_to_quiet;
+	u32 t_perf_to_balanced;
+	u32 t_turbo_to_perf;
+	struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
+} __packed;
+
 /* Core Layer */
 int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
 void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
@@ -324,4 +415,13 @@ int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req
 void amd_pmf_update_2_cql(struct amd_pmf_dev *dev, bool is_cql_event);
 int amd_pmf_reset_amt(struct amd_pmf_dev *dev);
 void amd_pmf_handle_amt(struct amd_pmf_dev *dev);
+
+/* CnQF Layer */
+int apmf_get_dyn_slider_def_ac(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data);
+int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data);
+void amd_pmf_init_cnqf(struct amd_pmf_dev *dev);
+void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
+void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev);
+void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
+
 #endif /* PMF_H */
-- 
2.25.1

Re: [PATCH 1/4] platform/x86/amd/pmf: Add support for CnQF

[Limonciello, Mario]

On 8/23/2022 05:29, Shyam Sundar S K wrote:
> CnQF (a.k.a Cool and Quiet Framework) extends the static slider concept.
> PMF dynamically manages system power limits and fan policy based on system
> power trends which is representative of workload trend.
> 
> Static slider and CnQF controls are mutually exclusive for system power
> budget adjustments. CnQF supports configurable number of modes which can
> be unique for AC and DC. Every mode is representative of a system state
> characterized by unique steady state and boost behavior.
> 
> OEMs can configure the different modes/system states and how the
> transition to a mode happens. Whether to have CnQF manage system power
> budget dynamically in AC or DC or both is also configurable. Mode changes
> due to CnQF don’t result in slider position change.
> 
> The default OEM values are obtained after evaluating the PMF ACPI function
> idx 11 & 12 for AC and DC respectively. Whether to turn ON/OFF by default
> is guided by a "flag" passed by the OEM BIOS.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/Makefile |   2 +-
>   drivers/platform/x86/amd/pmf/acpi.c   |  10 +
>   drivers/platform/x86/amd/pmf/cnqf.c   | 314 ++++++++++++++++++++++++++
>   drivers/platform/x86/amd/pmf/core.c   |  16 +-
>   drivers/platform/x86/amd/pmf/pmf.h    | 100 ++++++++
>   5 files changed, 440 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/platform/x86/amd/pmf/cnqf.c
> 
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index ef2b08c9174d..fdededf54392 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -6,4 +6,4 @@
>   
>   obj-$(CONFIG_AMD_PMF) += amd-pmf.o
>   amd-pmf-objs := core.o acpi.o sps.o \
> -		auto-mode.o
> +		auto-mode.o cnqf.o
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index cb46a7252414..05a2b8a056fc 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -233,6 +233,16 @@ static int apmf_get_system_params(struct amd_pmf_dev *dev)
>   	return 0;
>   }
>   
> +int apmf_get_dyn_slider_def_ac(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data)
> +{
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_DYN_SLIDER_AC, data, sizeof(*data));
> +}
> +
> +int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data)
> +{
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_DYN_SLIDER_DC, data, sizeof(*data));
> +}
> +
>   void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>   {
>   	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> new file mode 100644
> index 000000000000..b8abe0f4447a
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver
> + *
> + * Copyright (c) 2022, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#include <linux/workqueue.h>
> +#include "pmf.h"
> +
> +static struct cnqf_config config_store;
> +
> +static int amd_pmf_handle_cnqf(struct amd_pmf_dev *dev, int src, int idx,
> +			       struct cnqf_config *table)
> +{
> +	struct power_table_control *pc;
> +
> +	pc = &config_store.mode_set[src][idx].power_control;
> +
> +	amd_pmf_send_cmd(dev, SET_SPL, false, pc->spl, NULL);
> +	amd_pmf_send_cmd(dev, SET_FPPT, false, pc->fppt, NULL);
> +	amd_pmf_send_cmd(dev, SET_SPPT, false, pc->sppt, NULL);
> +	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pc->sppt_apu_only, NULL);
> +	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pc->stt_min, NULL);
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU],
> +			 NULL);
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2],
> +			 NULL);
> +
> +	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
> +		apmf_update_fan_idx(dev,
> +				    config_store.mode_set[src][idx].fan_control.manual,
> +				    config_store.mode_set[src][idx].fan_control.fan_id);
> +
> +	return 0;
> +}
> +
> +static void amd_pmf_update_power_threshold(void)
> +{
> +	struct cnqf_mode_settings *ts;
> +	struct cnqf_tran_params *tp;
> +	int i;
> +
> +	for (i = 0; i < POWER_SOURCE_MAX; i++) {
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_TO_QUIET];
> +		ts = &config_store.mode_set[i][CNQF_MODE_BALANCE];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_TO_TURBO];
> +		ts = &config_store.mode_set[i][CNQF_MODE_PERFORMANCE];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE];
> +		ts = &config_store.mode_set[i][CNQF_MODE_BALANCE];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE];
> +		ts = &config_store.mode_set[i][CNQF_MODE_PERFORMANCE];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_QUIET_TO_BALANCE];
> +		ts = &config_store.mode_set[i][CNQF_MODE_QUIET];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +
> +		tp = &config_store.trans_param[i][CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE];
> +		ts = &config_store.mode_set[i][CNQF_MODE_TURBO];
> +		tp->power_threshold = ts->power_floor - tp->power_delta;
> +	}
> +}
> +
> +static const char *state_as_str(unsigned int state)
> +{
> +	switch (state) {
> +	case CNQF_MODE_QUIET:
> +		return "QUIET";
> +	case CNQF_MODE_BALANCE:
> +		return "BALANCED";
> +	case CNQF_MODE_TURBO:
> +		return "TURBO";
> +	case CNQF_MODE_PERFORMANCE:
> +		return "PERFORMANCE";
> +	default:
> +		return "Unknown CnQF mode";
> +	}
> +}
> +
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
> +{
> +	struct cnqf_tran_params *tp;
> +	int src, i, j, index = 0;
> +	u32 avg_power = 0;
> +
> +	src = amd_pmf_get_power_source();
> +
> +	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
> +		config_store.trans_param[src][i].timer += time_lapsed_ms;
> +		config_store.trans_param[src][i].total_power += socket_power;
> +		config_store.trans_param[src][i].count++;
> +
> +		tp = &config_store.trans_param[src][i];
> +		if (tp->timer >= tp->time_constant && tp->count) {
> +			avg_power = tp->total_power / tp->count;
> +
> +			/* Reset the indices */
> +			tp->timer = 0;
> +			tp->total_power = 0;
> +			tp->count = 0;
> +
> +			if ((tp->shifting_up && avg_power >= tp->power_threshold) ||
> +			    (!tp->shifting_up && avg_power <= tp->power_threshold)) {
> +				tp->priority = true;
> +			} else {
> +				tp->priority = false;
> +			}
> +		}
> +	}
> +
> +	dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n",
> +		avg_power, socket_power, state_as_str(config_store.current_mode));
> +
> +	for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
> +		/* apply the highest priority */
> +		index = config_store.trans_prio[j];
> +		if (config_store.trans_param[src][index].priority) {
> +			if (config_store.current_mode !=
> +			    config_store.trans_param[src][index].target_mode) {
> +				config_store.current_mode =
> +						config_store.trans_param[src][index].target_mode;
> +				dev_dbg(dev->dev, "Moving to Mode :%s\n",
> +					state_as_str(config_store.current_mode));
> +				amd_pmf_handle_cnqf(dev, src,
> +						    config_store.current_mode, NULL);
> +			}
> +			break;
> +		}
> +	}
> +}
> +
> +static void amd_pmf_update_trans_data(int idx, struct apmf_dyn_slider_output out)
> +{
> +	struct cnqf_tran_params *tp;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_TO_QUIET];
> +	tp->time_constant = out.t_balanced_to_quiet;
> +	tp->target_mode = CNQF_MODE_QUIET;
> +	tp->shifting_up = false;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE];
> +	tp->time_constant = out.t_balanced_to_perf;
> +	tp->target_mode = CNQF_MODE_PERFORMANCE;
> +	tp->shifting_up = true;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_QUIET_TO_BALANCE];
> +	tp->time_constant = out.t_quiet_to_balanced;
> +	tp->target_mode = CNQF_MODE_BALANCE;
> +	tp->shifting_up = true;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE];
> +	tp->time_constant = out.t_perf_to_balanced;
> +	tp->target_mode = CNQF_MODE_BALANCE;
> +	tp->shifting_up = false;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE];
> +	tp->time_constant = out.t_turbo_to_perf;
> +	tp->target_mode = CNQF_MODE_PERFORMANCE;
> +	tp->shifting_up = false;
> +
> +	tp = &config_store.trans_param[idx][CNQF_TRANSITION_TO_TURBO];
> +	tp->time_constant = out.t_perf_to_turbo;
> +	tp->target_mode = CNQF_MODE_TURBO;
> +	tp->shifting_up = true;
> +}
> +
> +static void amd_pmf_update_mode_set(int idx, struct apmf_dyn_slider_output out)
> +{
> +	struct cnqf_mode_settings *ms;
> +
> +	/* Quiet Mode */
> +	ms = &config_store.mode_set[idx][CNQF_MODE_QUIET];
> +	ms->power_floor = out.ps[APMF_CNQF_QUIET].pfloor;
> +	ms->power_control.fppt = out.ps[APMF_CNQF_QUIET].fppt;
> +	ms->power_control.sppt = out.ps[APMF_CNQF_QUIET].sppt;
> +	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_QUIET].sppt_apu_only;
> +	ms->power_control.spl = out.ps[APMF_CNQF_QUIET].spl;
> +	ms->power_control.stt_min = out.ps[APMF_CNQF_QUIET].stt_min_limit;
> +	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
> +		out.ps[APMF_CNQF_QUIET].stt_skintemp[STT_TEMP_APU];
> +	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
> +		out.ps[APMF_CNQF_QUIET].stt_skintemp[STT_TEMP_HS2];
> +	ms->fan_control.fan_id = out.ps[APMF_CNQF_QUIET].fan_id;
> +
> +	/* Balance Mode */
> +	ms = &config_store.mode_set[idx][CNQF_MODE_BALANCE];
> +	ms->power_floor = out.ps[APMF_CNQF_BALANCE].pfloor;
> +	ms->power_control.fppt = out.ps[APMF_CNQF_BALANCE].fppt;
> +	ms->power_control.sppt = out.ps[APMF_CNQF_BALANCE].sppt;
> +	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_BALANCE].sppt_apu_only;
> +	ms->power_control.spl = out.ps[APMF_CNQF_BALANCE].spl;
> +	ms->power_control.stt_min = out.ps[APMF_CNQF_BALANCE].stt_min_limit;
> +	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
> +		out.ps[APMF_CNQF_BALANCE].stt_skintemp[STT_TEMP_APU];
> +	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
> +		out.ps[APMF_CNQF_BALANCE].stt_skintemp[STT_TEMP_HS2];
> +	ms->fan_control.fan_id = out.ps[APMF_CNQF_BALANCE].fan_id;
> +
> +	/* Performance Mode */
> +	ms = &config_store.mode_set[idx][CNQF_MODE_PERFORMANCE];
> +	ms->power_floor = out.ps[APMF_CNQF_PERFORMANCE].pfloor;
> +	ms->power_control.fppt = out.ps[APMF_CNQF_PERFORMANCE].fppt;
> +	ms->power_control.sppt = out.ps[APMF_CNQF_PERFORMANCE].sppt;
> +	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_PERFORMANCE].sppt_apu_only;
> +	ms->power_control.spl = out.ps[APMF_CNQF_PERFORMANCE].spl;
> +	ms->power_control.stt_min = out.ps[APMF_CNQF_PERFORMANCE].stt_min_limit;
> +	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
> +		out.ps[APMF_CNQF_PERFORMANCE].stt_skintemp[STT_TEMP_APU];
> +	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
> +		out.ps[APMF_CNQF_PERFORMANCE].stt_skintemp[STT_TEMP_HS2];
> +	ms->fan_control.fan_id = out.ps[APMF_CNQF_PERFORMANCE].fan_id;
> +
> +	/* Turbo Mode */
> +	ms = &config_store.mode_set[idx][CNQF_MODE_TURBO];
> +	ms->power_floor = out.ps[APMF_CNQF_TURBO].pfloor;
> +	ms->power_control.fppt = out.ps[APMF_CNQF_TURBO].fppt;
> +	ms->power_control.sppt = out.ps[APMF_CNQF_TURBO].sppt;
> +	ms->power_control.sppt_apu_only = out.ps[APMF_CNQF_TURBO].sppt_apu_only;
> +	ms->power_control.spl = out.ps[APMF_CNQF_TURBO].spl;
> +	ms->power_control.stt_min = out.ps[APMF_CNQF_TURBO].stt_min_limit;
> +	ms->power_control.stt_skin_temp[STT_TEMP_APU] =
> +		out.ps[APMF_CNQF_TURBO].stt_skintemp[STT_TEMP_APU];
> +	ms->power_control.stt_skin_temp[STT_TEMP_HS2] =
> +		out.ps[APMF_CNQF_TURBO].stt_skintemp[STT_TEMP_HS2];
> +	ms->fan_control.fan_id = out.ps[APMF_CNQF_TURBO].fan_id;
> +}
> +
> +static int amd_pmf_check_flags(struct amd_pmf_dev *dev)
> +{
> +	struct apmf_dyn_slider_output out = {};
> +
> +	if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC))
> +		apmf_get_dyn_slider_def_ac(dev, &out);
> +	else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC))
> +		apmf_get_dyn_slider_def_dc(dev, &out);
> +
> +	return out.flags;
> +}
> +
> +void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
> +{
> +	struct apmf_dyn_slider_output out;
> +	int i, j, ret;
> +
> +	for (i = 0; i < POWER_SOURCE_MAX; i++) {
> +		if (i == POWER_SOURCE_AC && is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC)) {
> +			ret = apmf_get_dyn_slider_def_ac(dev, &out);
> +			if (ret)
> +				dev_err(dev->dev,
> +					"APMF apmf_get_dyn_slider_def_ac failed :%d\n", ret);
> +		} else if (i == POWER_SOURCE_DC &&
> +				is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> +			ret = apmf_get_dyn_slider_def_dc(dev, &out);
> +			if (ret)
> +				dev_err(dev->dev,
> +					"APMF apmf_get_dyn_slider_def_dc failed :%d\n", ret);
> +		}
> +
> +		amd_pmf_update_mode_set(i, out);
> +		amd_pmf_update_trans_data(i, out);
> +
> +		for (j = 0; j < CNQF_MODE_MAX; j++) {
> +			if (config_store.mode_set[i][j].fan_control.fan_id == FAN_INDEX_AUTO)
> +				config_store.mode_set[i][j].fan_control.manual = false;
> +			else
> +				config_store.mode_set[i][j].fan_control.manual = true;
> +		}
> +	}
> +	amd_pmf_update_power_threshold();
> +
> +	for (i = 0; i < CNQF_TRANSITION_MAX; i++)
> +		config_store.trans_prio[i] = i;
> +}
> +
> +void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev)
> +{
> +	cancel_delayed_work_sync(&dev->work_buffer);
> +}
> +
> +void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
> +{
> +	int ret, src;
> +
> +	ret = amd_pmf_check_flags(dev);
> +	if (!ret) {
> +		dev_dbg(dev->dev, "CnQF bios default_enable flag not set\n");
> +		return;
> +	}
> +
> +	dev->cnqf_enabled = true;
> +
> +	/* Disable SPS if its advertised as supported, giving clear preference
> +	 * to user selection choices
> +	 */
> +	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
> +		amd_pmf_deinit_sps(dev);

I wonder if it makes more sense to explicitly init cnqf and "never" init 
sps if the case of APMF_FUNC_DYN_SLIDER_AC or APMF_FUNC_DYN_SLIDER_DC 
and APMF_FUNC_STATIC_SLIDER_GRANULAR existing.

My reasoning is that if the BIOS advertises both running init for SPS 
will cause a platform profile attribute to be created but then running 
init for CNQF will cause it to be removed.  This will then be a few 
uevents to userspace to create the files and remove them that could be 
completely avoided if CNQF init runs first.

> +
> +	amd_pmf_load_defaults_cnqf(dev);
> +	amd_pmf_init_metrics_table(dev);
> +
> +	/* update the thermal for CnQF */
> +	src = amd_pmf_get_power_source();
> +	amd_pmf_handle_cnqf(dev, src, config_store.current_mode, NULL);
> +}
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index a675ca969331..2cb7793d07cf 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -123,6 +123,11 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>   		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>   	}
>   
> +	if (dev->cnqf_enabled) {
> +		/* Apply the CnQF transition */
> +		amd_pmf_trans_cnqf(dev, socket_power, time_elapsed_ms);
> +	}
> +
>   	dev->start_time = ktime_to_ms(ktime_get());
>   	schedule_delayed_work(&dev->work_buffer, msecs_to_jiffies(metrics_table_loop_ms));
>   	mutex_unlock(&dev->update_mutex);
> @@ -262,6 +267,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
>   	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>   		amd_pmf_init_auto_mode(dev);
>   		dev_dbg(dev->dev, "Auto Mode Init done\n");
> +	} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> +			  is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> +		/* Enable Cool n Quiet Framework (CnQF) */
> +		amd_pmf_init_cnqf(dev);
> +		dev_dbg(dev->dev, "CnQF Init done\n");
>   	}
>   }
>   
> @@ -270,8 +280,12 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
>   	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
>   		amd_pmf_deinit_sps(dev);
>   
> -	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE))
> +	if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>   		amd_pmf_deinit_auto_mode(dev);
> +	} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> +			  is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> +		amd_pmf_deinit_cnqf(dev);
> +	}
>   }
>   
>   static const struct acpi_device_id amd_pmf_acpi_ids[] = {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 0a72a395c2ef..db3c86c2e86f 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -22,6 +22,8 @@
>   #define APMF_FUNC_AUTO_MODE					5
>   #define APMF_FUNC_SET_FAN_IDX				7
>   #define APMF_FUNC_STATIC_SLIDER_GRANULAR       9
> +#define APMF_FUNC_DYN_SLIDER_AC				11
> +#define APMF_FUNC_DYN_SLIDER_DC				12
>   
>   /* Message Definitions */
>   #define SET_SPL				0x03 /* SPL: Sustained Power Limit */
> @@ -165,6 +167,7 @@ struct amd_pmf_dev {
>   	int socket_power_history_idx;
>   	bool amt_enabled;
>   	struct mutex update_mutex; /* protects race between ACPI handler and metrics thread */
> +	bool cnqf_enabled;
>   };
>   
>   struct apmf_sps_prop_granular {
> @@ -294,6 +297,94 @@ struct apmf_auto_mode {
>   	u32 fan_id_quiet;
>   } __packed;
>   
> +/* CnQF Layer */
> +enum cnqf_trans_priority {
> +	CNQF_TRANSITION_TO_TURBO, /* Any other mode to Turbo Mode */
> +	CNQF_TRANSITION_FROM_BALANCE_TO_PERFORMANCE, /* quiet/balance to Performance Mode */
> +	CNQF_TRANSITION_FROM_QUIET_TO_BALANCE, /* Quiet Mode to Balance Mode */
> +	CNQF_TRANSITION_TO_QUIET, /* Any other mode to Quiet Mode */
> +	CNQF_TRANSITION_FROM_PERFORMANCE_TO_BALANCE, /* Performance/Turbo to Balance Mode */
> +	CNQF_TRANSITION_FROM_TURBO_TO_PERFORMANCE, /* Turbo mode to Performance Mode */
> +	CNQF_TRANSITION_MAX,
> +};
> +
> +enum cnqf_mode {
> +	CNQF_MODE_QUIET,
> +	CNQF_MODE_BALANCE,
> +	CNQF_MODE_PERFORMANCE,
> +	CNQF_MODE_TURBO,
> +	CNQF_MODE_MAX,
> +};
> +
> +enum apmf_cnqf_pos {
> +	APMF_CNQF_TURBO,
> +	APMF_CNQF_PERFORMANCE,
> +	APMF_CNQF_BALANCE,
> +	APMF_CNQF_QUIET,
> +	APMF_CNQF_MAX,
> +};
> +
> +struct cnqf_mode_settings {
> +	struct power_table_control power_control;
> +	struct fan_table_control fan_control;
> +	u32 power_floor;
> +};
> +
> +struct cnqf_tran_params {
> +	u32 time_constant; /* minimum time required to switch to next mode */
> +	u32 power_delta; /* minimum power required to switch to next mode */
> +	u32 power_threshold;
> +	u32 timer; /* elapsed time. if timer > timethreshold, it will move to next mode */
> +	u32 total_power;
> +	u32 count;
> +	bool priority;
> +	bool shifting_up;
> +	enum cnqf_mode target_mode;
> +};
> +
> +struct cnqf_power_delta {
> +	u32 to_turbo;
> +	u32 balance_to_perf;
> +	u32 quiet_to_balance;
> +	u32 to_quiet;
> +	u32 perf_to_balance;
> +	u32 turbo_to_perf;
> +};
> +
> +struct cnqf_config {
> +	struct cnqf_tran_params trans_param[POWER_SOURCE_MAX][CNQF_TRANSITION_MAX];
> +	struct cnqf_mode_settings mode_set[POWER_SOURCE_MAX][CNQF_MODE_MAX];
> +	struct power_table_control defaults;
> +	enum cnqf_mode current_mode;
> +	struct cnqf_power_delta power_delta[POWER_SOURCE_MAX];
> +	u32 power_src;
> +	u32 avg_power;
> +	enum cnqf_trans_priority trans_prio[CNQF_TRANSITION_MAX];
> +};
> +
> +struct apmf_cnqf_power_set {
> +	u32 pfloor;
> +	u32 fppt;
> +	u32 sppt;
> +	u32 sppt_apu_only;
> +	u32 spl;
> +	u32 stt_min_limit;
> +	u8 stt_skintemp[STT_TEMP_COUNT];
> +	u32 fan_id;
> +} __packed;
> +
> +struct apmf_dyn_slider_output {
> +	u16 size;
> +	u16 flags;
> +	u32 t_perf_to_turbo;
> +	u32 t_balanced_to_perf;
> +	u32 t_quiet_to_balanced;
> +	u32 t_balanced_to_quiet;
> +	u32 t_perf_to_balanced;
> +	u32 t_turbo_to_perf;
> +	struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> +} __packed;
> +
>   /* Core Layer */
>   int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
>   void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
> @@ -324,4 +415,13 @@ int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req
>   void amd_pmf_update_2_cql(struct amd_pmf_dev *dev, bool is_cql_event);
>   int amd_pmf_reset_amt(struct amd_pmf_dev *dev);
>   void amd_pmf_handle_amt(struct amd_pmf_dev *dev);
> +
> +/* CnQF Layer */
> +int apmf_get_dyn_slider_def_ac(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data);
> +int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_output *data);
> +void amd_pmf_init_cnqf(struct amd_pmf_dev *dev);
> +void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
> +void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev);
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> +
>   #endif /* PMF_H */

[PATCH 2/4] platform/x86/amd/pmf: Add sysfs to toggle CnQF

[Shyam Sundar S K]

Whether to turn CnQF on/off by default upon driver load would be decided
by a BIOS flag. Add a sysfs node to provide a way to the user whether to
use static slider or CnQF .

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/cnqf.c | 60 +++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
index b8abe0f4447a..1a8ea6eb6c2f 100644
--- a/drivers/platform/x86/amd/pmf/cnqf.c
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -282,9 +282,67 @@ void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
 		config_store.trans_prio[i] = i;
 }
 
+static ssize_t feat_store(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
+	int mode = amd_pmf_get_pprof_modes(pdev);
+	int result, src;
+	bool input;
+
+	result = kstrtobool(buf, &input);
+	if (result)
+		return result;
+
+	src = amd_pmf_get_power_source();
+	pdev->cnqf_enabled = input;
+
+	if (mode < 0)
+		return mode;
+
+	if (pdev->cnqf_enabled) {
+		if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
+			amd_pmf_deinit_sps(pdev);
+
+		amd_pmf_handle_cnqf(pdev, src, config_store.current_mode, NULL);
+	} else {
+		if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) {
+			amd_pmf_init_sps(pdev);
+			amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL);
+		}
+	}
+
+	dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off");
+	return count;
+}
+
+static ssize_t feat_show(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off");
+}
+
+static DEVICE_ATTR_RW(feat);
+
+static struct attribute *cnqf_feature_attrs[] = {
+	&dev_attr_feat.attr,
+	NULL
+};
+
+static const struct attribute_group cnqf_feature_attribute_group = {
+	.attrs = cnqf_feature_attrs,
+	.name = "cnqf"
+};
+
 void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev)
 {
 	cancel_delayed_work_sync(&dev->work_buffer);
+	sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group);
+	kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE);
 }
 
 void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
@@ -311,4 +369,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
 	/* update the thermal for CnQF */
 	src = amd_pmf_get_power_source();
 	amd_pmf_handle_cnqf(dev, src, config_store.current_mode, NULL);
+	ret = sysfs_create_group(&dev->dev->kobj, &cnqf_feature_attribute_group);
+	kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE);
 }
-- 
2.25.1

[PATCH 3/4] Documentation/ABI/testing/sysfs-amd-pmf: Add ABI doc for AMD PMF

[Shyam Sundar S K]

AMD PMF driver provides the flexibility to turn "on" or "off"
CnQF feature (introduced in the earlier patch).

Add corresponding ABI documentation for the new sysfs node.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 Documentation/ABI/testing/sysfs-amd-pmf | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-amd-pmf

diff --git a/Documentation/ABI/testing/sysfs-amd-pmf b/Documentation/ABI/testing/sysfs-amd-pmf
new file mode 100644
index 000000000000..5935dc549185
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-amd-pmf
@@ -0,0 +1,11 @@
+What:		/sys/devices/platform/AMDI0102\:00/cnqf/feat
+Date:		July 2022
+Contact:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+Description:	Reading this file tells if the AMD Platform Management(PMF)
+		Cool n Quiet Framework(CnQF) feature is enabled or not.
+
+		This feature is not enabled by default and gets only turned on
+		if OEM BIOS passes a "flag" to PMF ACPI function (index 11 or 12)
+		or in case the user writes "on".
+
+		To turn off CnQF user can write "off" to the sysfs node.
-- 
2.25.1

[PATCH 4/4] MAINTAINERS: Update ABI doc path for AMD PMF

[Shyam Sundar S K]

Update the MAINTAINERS file with ABI doc path for AMD PMF driver.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d74bf90f5056..255527be7e24 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1026,6 +1026,7 @@ AMD PMF DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-amd-pmf
 F:	drivers/platform/x86/amd/pmf/
 
 AMD HSMP DRIVER
-- 
2.25.1

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 8/23/22 12:29, Shyam Sundar S K wrote:
> In this series, support for following features has been added.
> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>   where the system power can be boosted or throttled independent
>   of the selected slider position.
> - On the fly, the CnQF can be turned on/off via a sysfs knob.

Thank you. I think that before doing a more in detail review
we first need to agree on the userspace interactions here.

I've added Bastien, the power-profiles-daemon maintainer
to the Cc for this.

From a quick peek at the patches I see that currently they do
the following:

Probe time:
-----------

1. If static slider (classic /sys/firmware/acpi/platform_profile)
is available register as a platform_profile provider

2. Query if the BIOS tells us that CnQF should be enable by
default if yes then unregister the platform_profile provider
and enable CnQF


Run time:
---------

Allow turning CnQF on/off by writing a sysfs attribute for this.

1. When CnQF gets enabled unregister the platform_profile provider

2. When CnQF gets disabled restore the last set profile and
register the platform_profile provider


Questions/remarks:

1. If you look at 1. and 2. under "Probe time", you will see that
when the BIOS requests to have CnQF enabled by default that
userspace will then still shortly see a platform_profile
provider. This must be fixed IMHO by checking whether to do
CnQF by default or not before the initial register call.

2. What about low-power scenarios ? Currently power-profiles-daemon
will always advertise a low-power mode even when there is no
platform-profile support, since this is also a hint for other
parts of the system to try and conserve power. But when this
mode is enabled we really want the system to also behave as
if the old static slider mode is active and set to low-power.

Some ideas:
a) maybe still have the amd-pmf code register a (different)
platform_profile provider whn in CnQF mode and have it only
advertise low-power

b) teach power-profiles-daemon about CnQF and have it
disable CnQF when entering low-power mode?

c) make the CnQF code in PMF take the charge level into
account and have it not go "full throttle" when the chare
is below say 25% ?

3. Bastien, can power-profiles-daemon deal with
/sys/firmware/acpi/platform_profile disappearing or
appearing while it is running? 

Regards,

Hans



> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> Shyam Sundar S K (4):
>   platform/x86/amd/pmf: Add support for CnQF
>   platform/x86/amd/pmf: Add sysfs to toggle CnQF
>   Documentation/ABI/testing/sysfs-amd-pmf: Add ABI doc for AMD PMF
>   MAINTAINERS: Update ABI doc path for AMD PMF
> 
>  Documentation/ABI/testing/sysfs-amd-pmf |  11 +
>  MAINTAINERS                             |   1 +
>  drivers/platform/x86/amd/pmf/Makefile   |   2 +-
>  drivers/platform/x86/amd/pmf/acpi.c     |  10 +
>  drivers/platform/x86/amd/pmf/cnqf.c     | 374 ++++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/core.c     |  16 +-
>  drivers/platform/x86/amd/pmf/pmf.h      | 100 +++++++
>  7 files changed, 512 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-amd-pmf
>  create mode 100644 drivers/platform/x86/amd/pmf/cnqf.c
> 

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Bastien Nocera]

On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 8/23/22 12:29, Shyam Sundar S K wrote:
> > In this series, support for following features has been added.
> > - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
> >   where the system power can be boosted or throttled independent
> >   of the selected slider position.
> > - On the fly, the CnQF can be turned on/off via a sysfs knob.
>
> Thank you. I think that before doing a more in detail review
> we first need to agree on the userspace interactions here.
>
> I've added Bastien, the power-profiles-daemon maintainer
> to the Cc for this.
>
> From a quick peek at the patches I see that currently they do
> the following:
>
> Probe time:
> -----------
>
> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
> is available register as a platform_profile provider
>
> 2. Query if the BIOS tells us that CnQF should be enable by
> default if yes then unregister the platform_profile provider
> and enable CnQF
>
>
> Run time:
> ---------
>
> Allow turning CnQF on/off by writing a sysfs attribute for this.
>
> 1. When CnQF gets enabled unregister the platform_profile provider
>
> 2. When CnQF gets disabled restore the last set profile and
> register the platform_profile provider
>
>
> Questions/remarks:
>
> 1. If you look at 1. and 2. under "Probe time", you will see that
> when the BIOS requests to have CnQF enabled by default that
> userspace will then still shortly see a platform_profile
> provider. This must be fixed IMHO by checking whether to do
> CnQF by default or not before the initial register call.
>
> 2. What about low-power scenarios ? Currently power-profiles-daemon
> will always advertise a low-power mode even when there is no
> platform-profile support, since this is also a hint for other
> parts of the system to try and conserve power. But when this
> mode is enabled we really want the system to also behave as
> if the old static slider mode is active and set to low-power.
>
> Some ideas:
> a) maybe still have the amd-pmf code register a (different)
> platform_profile provider whn in CnQF mode and have it only
> advertise low-power
>
> b) teach power-profiles-daemon about CnQF and have it
> disable CnQF when entering low-power mode?
>
> c) make the CnQF code in PMF take the charge level into
> account and have it not go "full throttle" when the chare
> is below say 25% ?
>
> 3. Bastien, can power-profiles-daemon deal with
> /sys/firmware/acpi/platform_profile disappearing or
> appearing while it is running?

No, it doesn't.

It expects the platform_profile file to be available on startup, at
worse with the choices not yet filled in. It doesn't handle the
platform_profile file going away, it doesn't handle the
platform_profile_choices file changing after it's been initially
filled in, and it doesn't support less than one power profile being
made available, and only supports hiding the performance profile if
the platform doesn't support it.

Some of those things we could change/fix, some other things will not.
If the platform_profile_choices file only contained a single item,
then power-profiles-daemon would just export the "low-power" and
"balanced" profiles to user-space, as it does on unsupported hardware.

The profiles in power-profiles-daemon are supposed to show the user
intent, which having a single setting would effectively nullify.
It's unclear to me how CnQF takes user intent into account (it's also
unclear to me how that's a low-power setting rather than a combination
of the existing cool and quiet settings).

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 9/1/22 14:24, Bastien Nocera wrote:
> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>> In this series, support for following features has been added.
>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>   where the system power can be boosted or throttled independent
>>>   of the selected slider position.
>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>
>> Thank you. I think that before doing a more in detail review
>> we first need to agree on the userspace interactions here.
>>
>> I've added Bastien, the power-profiles-daemon maintainer
>> to the Cc for this.
>>
>> From a quick peek at the patches I see that currently they do
>> the following:
>>
>> Probe time:
>> -----------
>>
>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>> is available register as a platform_profile provider
>>
>> 2. Query if the BIOS tells us that CnQF should be enable by
>> default if yes then unregister the platform_profile provider
>> and enable CnQF
>>
>>
>> Run time:
>> ---------
>>
>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>
>> 1. When CnQF gets enabled unregister the platform_profile provider
>>
>> 2. When CnQF gets disabled restore the last set profile and
>> register the platform_profile provider
>>
>>
>> Questions/remarks:
>>
>> 1. If you look at 1. and 2. under "Probe time", you will see that
>> when the BIOS requests to have CnQF enabled by default that
>> userspace will then still shortly see a platform_profile
>> provider. This must be fixed IMHO by checking whether to do
>> CnQF by default or not before the initial register call.
>>
>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>> will always advertise a low-power mode even when there is no
>> platform-profile support, since this is also a hint for other
>> parts of the system to try and conserve power. But when this
>> mode is enabled we really want the system to also behave as
>> if the old static slider mode is active and set to low-power.
>>
>> Some ideas:
>> a) maybe still have the amd-pmf code register a (different)
>> platform_profile provider whn in CnQF mode and have it only
>> advertise low-power
>>
>> b) teach power-profiles-daemon about CnQF and have it
>> disable CnQF when entering low-power mode?
>>
>> c) make the CnQF code in PMF take the charge level into
>> account and have it not go "full throttle" when the chare
>> is below say 25% ?
>>
>> 3. Bastien, can power-profiles-daemon deal with
>> /sys/firmware/acpi/platform_profile disappearing or
>> appearing while it is running?
> 
> No, it doesn't.
> 
> It expects the platform_profile file to be available on startup, at
> worse with the choices not yet filled in. It doesn't handle the
> platform_profile file going away, it doesn't handle the
> platform_profile_choices file changing after it's been initially
> filled in, and it doesn't support less than one power profile being
> made available, and only supports hiding the performance profile if
> the platform doesn't support it.

Ok, so this means that if we go with these changes as currently
proposed that if a user uses the sysfs file to turn CnQF on/off
they will need to restart power-profile-daemon.

I think that that is acceptable given that the user needs to manually
poke things anyway. We should probably document this in the documentation
for the sysfs attribute (as well as in newer versions of the p-p-d
docs/README).

> Some of those things we could change/fix, some other things will not.
> If the platform_profile_choices file only contained a single item,
> then power-profiles-daemon would just export the "low-power" and
> "balanced" profiles to user-space, as it does on unsupported hardware.

Right.

> The profiles in power-profiles-daemon are supposed to show the user
> intent, which having a single setting would effectively nullify.

Yes that was my understanding too.

> It's unclear to me how CnQF takes user intent into account (it's also
> unclear to me how that's a low-power setting rather than a combination
> of the existing cool and quiet settings).

AMD folks, please correct me if any of the below is wrong:

AFAIK even though it is called CnQF it is more like auto-profile
selection and as such does not take user intent into account
at all.

It looks at the workload over a somewhat longer time period (say
5 minutes or so I guess?) and then if that consistently has been
quite high, it will select something similar to performance.

Where as for a more mixed workload it will select balanced and for
a mostly idle machine it will select low-power.

I guess this auto feature is best treated the same as unsupported hw.

> (it's also
> unclear to me how that's a low-power setting rather than a combination
> of the existing cool and quiet settings).

Even though it is called cool and quiet AFAIK it won't be all that
cool and quiet when running a heavy workload. Which is why I wonder
how to re-conciliate this with showing low-power in e.g. the
GNOME shell system men. Because in essence even if the battery
is low the system will still go full-throttle when confronted
with a heavy workload.

So selecting low-power would result in the screen-dimming which
I think is part of that, but the CPU's max power-consumption won't
get limited as it would when platform-profiles are supported.

So I guess this is indeed very much like how p-p-d behaves
on unsupported hw...

###

As mentioned I guess one option would be for CnQF to
still register a platform_profile provider and then in
balanced mode do its CnQF thing and in low-power mode
disable CnQF and apply the static-slider low-power settings
I think that that would work best from things actual
working in a way I would expect the avarage end-user to
expect things to work.

So p-p-d would then still see platform-profile support
in CnQF mode but with only low-power + balanced advertised.

Bastien would that work for you?

AMD folks would that also work for you ?

###

I'm also wondering if we are going to still export
balanced + low-power modes to userspace in CnQF mode
and disable CnQF in low-power mode then if we
even need a sysfs knob to turn it on/off at all.

I guess the sysfs knob would then still be useful
to turn it on on systems where it defaults to off
in the BIOS.  Might be better to do this as
a kernel-cmdline (module-param) then though, then we
also avoid the problem of platform_profile support
all of a sudden changing underneath's p-p-d's feet.

Regards,

Hans






and for toggling ooff through sysfs

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Bastien Nocera]

On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/1/22 14:24, Bastien Nocera wrote:
> > On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 8/23/22 12:29, Shyam Sundar S K wrote:
> >>> In this series, support for following features has been added.
> >>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
> >>>   where the system power can be boosted or throttled independent
> >>>   of the selected slider position.
> >>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
> >>
> >> Thank you. I think that before doing a more in detail review
> >> we first need to agree on the userspace interactions here.
> >>
> >> I've added Bastien, the power-profiles-daemon maintainer
> >> to the Cc for this.
> >>
> >> From a quick peek at the patches I see that currently they do
> >> the following:
> >>
> >> Probe time:
> >> -----------
> >>
> >> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
> >> is available register as a platform_profile provider
> >>
> >> 2. Query if the BIOS tells us that CnQF should be enable by
> >> default if yes then unregister the platform_profile provider
> >> and enable CnQF
> >>
> >>
> >> Run time:
> >> ---------
> >>
> >> Allow turning CnQF on/off by writing a sysfs attribute for this.
> >>
> >> 1. When CnQF gets enabled unregister the platform_profile provider
> >>
> >> 2. When CnQF gets disabled restore the last set profile and
> >> register the platform_profile provider
> >>
> >>
> >> Questions/remarks:
> >>
> >> 1. If you look at 1. and 2. under "Probe time", you will see that
> >> when the BIOS requests to have CnQF enabled by default that
> >> userspace will then still shortly see a platform_profile
> >> provider. This must be fixed IMHO by checking whether to do
> >> CnQF by default or not before the initial register call.
> >>
> >> 2. What about low-power scenarios ? Currently power-profiles-daemon
> >> will always advertise a low-power mode even when there is no
> >> platform-profile support, since this is also a hint for other
> >> parts of the system to try and conserve power. But when this
> >> mode is enabled we really want the system to also behave as
> >> if the old static slider mode is active and set to low-power.
> >>
> >> Some ideas:
> >> a) maybe still have the amd-pmf code register a (different)
> >> platform_profile provider whn in CnQF mode and have it only
> >> advertise low-power
> >>
> >> b) teach power-profiles-daemon about CnQF and have it
> >> disable CnQF when entering low-power mode?
> >>
> >> c) make the CnQF code in PMF take the charge level into
> >> account and have it not go "full throttle" when the chare
> >> is below say 25% ?
> >>
> >> 3. Bastien, can power-profiles-daemon deal with
> >> /sys/firmware/acpi/platform_profile disappearing or
> >> appearing while it is running?
> >
> > No, it doesn't.
> >
> > It expects the platform_profile file to be available on startup, at
> > worse with the choices not yet filled in. It doesn't handle the
> > platform_profile file going away, it doesn't handle the
> > platform_profile_choices file changing after it's been initially
> > filled in, and it doesn't support less than one power profile being
> > made available, and only supports hiding the performance profile if
> > the platform doesn't support it.
>
> Ok, so this means that if we go with these changes as currently
> proposed that if a user uses the sysfs file to turn CnQF on/off
> they will need to restart power-profile-daemon.
>
> I think that that is acceptable given that the user needs to manually
> poke things anyway. We should probably document this in the documentation
> for the sysfs attribute (as well as in newer versions of the p-p-d
> docs/README).
>
> > Some of those things we could change/fix, some other things will not.
> > If the platform_profile_choices file only contained a single item,
> > then power-profiles-daemon would just export the "low-power" and
> > "balanced" profiles to user-space, as it does on unsupported hardware.
>
> Right.
>
> > The profiles in power-profiles-daemon are supposed to show the user
> > intent, which having a single setting would effectively nullify.
>
> Yes that was my understanding too.
>
> > It's unclear to me how CnQF takes user intent into account (it's also
> > unclear to me how that's a low-power setting rather than a combination
> > of the existing cool and quiet settings).
>
> AMD folks, please correct me if any of the below is wrong:
>
> AFAIK even though it is called CnQF it is more like auto-profile
> selection and as such does not take user intent into account
> at all.
>
> It looks at the workload over a somewhat longer time period (say
> 5 minutes or so I guess?) and then if that consistently has been
> quite high, it will select something similar to performance.
>
> Where as for a more mixed workload it will select balanced and for
> a mostly idle machine it will select low-power.
>
> I guess this auto feature is best treated the same as unsupported hw.
>
> > (it's also
> > unclear to me how that's a low-power setting rather than a combination
> > of the existing cool and quiet settings).
>
> Even though it is called cool and quiet AFAIK it won't be all that
> cool and quiet when running a heavy workload. Which is why I wonder
> how to re-conciliate this with showing low-power in e.g. the
> GNOME shell system men. Because in essence even if the battery
> is low the system will still go full-throttle when confronted
> with a heavy workload.
>
> So selecting low-power would result in the screen-dimming which
> I think is part of that, but the CPU's max power-consumption won't
> get limited as it would when platform-profiles are supported.
>
> So I guess this is indeed very much like how p-p-d behaves
> on unsupported hw...
>
> ###
>
> As mentioned I guess one option would be for CnQF to
> still register a platform_profile provider and then in
> balanced mode do its CnQF thing and in low-power mode
> disable CnQF and apply the static-slider low-power settings
> I think that that would work best from things actual
> working in a way I would expect the avarage end-user to
> expect things to work.
>
> So p-p-d would then still see platform-profile support
> in CnQF mode but with only low-power + balanced advertised.
>
> Bastien would that work for you?

That's something I can make work, yes.

> AMD folks would that also work for you ?
>
> ###
>
> I'm also wondering if we are going to still export
> balanced + low-power modes to userspace in CnQF mode
> and disable CnQF in low-power mode then if we
> even need a sysfs knob to turn it on/off at all.
>
> I guess the sysfs knob would then still be useful
> to turn it on on systems where it defaults to off
> in the BIOS.  Might be better to do this as
> a kernel-cmdline (module-param) then though, then we
> also avoid the problem of platform_profile support
> all of a sudden changing underneath's p-p-d's feet.

I would say that, you could probably have CnQF transparently replacing
the more static "balanced" profile if it is available, and have a
separate setting to force enable/disable it as a kernel command-line
for debugging or if the BIOS menu doesn't offer it as an option.

That way the balanced mode would work like a more refined automatic
profile switcher, and make the default experience better, without the
disappearing profiles, or the user-space API headaches.

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Shyam Sundar S K]

Hi Bastien, Hans

On 9/1/2022 7:04 PM, Bastien Nocera wrote:
> On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/1/22 14:24, Bastien Nocera wrote:
>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>> In this series, support for following features has been added.
>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>   where the system power can be boosted or throttled independent
>>>>>   of the selected slider position.
>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>
>>>> Thank you. I think that before doing a more in detail review
>>>> we first need to agree on the userspace interactions here.
>>>>
>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>> to the Cc for this.
>>>>
>>>> From a quick peek at the patches I see that currently they do
>>>> the following:
>>>>
>>>> Probe time:
>>>> -----------
>>>>
>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>> is available register as a platform_profile provider
>>>>
>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>> default if yes then unregister the platform_profile provider
>>>> and enable CnQF
>>>>
>>>>
>>>> Run time:
>>>> ---------
>>>>
>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>
>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>
>>>> 2. When CnQF gets disabled restore the last set profile and
>>>> register the platform_profile provider
>>>>
>>>>
>>>> Questions/remarks:
>>>>
>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>> when the BIOS requests to have CnQF enabled by default that
>>>> userspace will then still shortly see a platform_profile
>>>> provider. This must be fixed IMHO by checking whether to do
>>>> CnQF by default or not before the initial register call.
>>>>
>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>> will always advertise a low-power mode even when there is no
>>>> platform-profile support, since this is also a hint for other
>>>> parts of the system to try and conserve power. But when this
>>>> mode is enabled we really want the system to also behave as
>>>> if the old static slider mode is active and set to low-power.
>>>>
>>>> Some ideas:
>>>> a) maybe still have the amd-pmf code register a (different)
>>>> platform_profile provider whn in CnQF mode and have it only
>>>> advertise low-power
>>>>
>>>> b) teach power-profiles-daemon about CnQF and have it
>>>> disable CnQF when entering low-power mode?
>>>>
>>>> c) make the CnQF code in PMF take the charge level into
>>>> account and have it not go "full throttle" when the chare
>>>> is below say 25% ?
>>>>
>>>> 3. Bastien, can power-profiles-daemon deal with
>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>> appearing while it is running?
>>>
>>> No, it doesn't.
>>>
>>> It expects the platform_profile file to be available on startup, at
>>> worse with the choices not yet filled in. It doesn't handle the
>>> platform_profile file going away, it doesn't handle the
>>> platform_profile_choices file changing after it's been initially
>>> filled in, and it doesn't support less than one power profile being
>>> made available, and only supports hiding the performance profile if
>>> the platform doesn't support it.
>>
>> Ok, so this means that if we go with these changes as currently
>> proposed that if a user uses the sysfs file to turn CnQF on/off
>> they will need to restart power-profile-daemon.
>>
>> I think that that is acceptable given that the user needs to manually
>> poke things anyway. We should probably document this in the documentation
>> for the sysfs attribute (as well as in newer versions of the p-p-d
>> docs/README).
>>
>>> Some of those things we could change/fix, some other things will not.
>>> If the platform_profile_choices file only contained a single item,
>>> then power-profiles-daemon would just export the "low-power" and
>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>
>> Right.
>>
>>> The profiles in power-profiles-daemon are supposed to show the user
>>> intent, which having a single setting would effectively nullify.
>>
>> Yes that was my understanding too.
>>
>>> It's unclear to me how CnQF takes user intent into account (it's also
>>> unclear to me how that's a low-power setting rather than a combination
>>> of the existing cool and quiet settings).
>>
>> AMD folks, please correct me if any of the below is wrong:
>>
>> AFAIK even though it is called CnQF it is more like auto-profile
>> selection and as such does not take user intent into account
>> at all.
>>
>> It looks at the workload over a somewhat longer time period (say
>> 5 minutes or so I guess?) and then if that consistently has been
>> quite high, it will select something similar to performance.
>>
>> Where as for a more mixed workload it will select balanced and for
>> a mostly idle machine it will select low-power.
>>
>> I guess this auto feature is best treated the same as unsupported hw.
>>
>>> (it's also
>>> unclear to me how that's a low-power setting rather than a combination
>>> of the existing cool and quiet settings).
>>
>> Even though it is called cool and quiet AFAIK it won't be all that
>> cool and quiet when running a heavy workload. Which is why I wonder
>> how to re-conciliate this with showing low-power in e.g. the
>> GNOME shell system men. Because in essence even if the battery
>> is low the system will still go full-throttle when confronted
>> with a heavy workload.
>>
>> So selecting low-power would result in the screen-dimming which
>> I think is part of that, but the CPU's max power-consumption won't
>> get limited as it would when platform-profiles are supported.
>>
>> So I guess this is indeed very much like how p-p-d behaves
>> on unsupported hw...
>>
>> ###
>>
>> As mentioned I guess one option would be for CnQF to
>> still register a platform_profile provider and then in
>> balanced mode do its CnQF thing and in low-power mode
>> disable CnQF and apply the static-slider low-power settings
>> I think that that would work best from things actual
>> working in a way I would expect the avarage end-user to
>> expect things to work.
>>
>> So p-p-d would then still see platform-profile support
>> in CnQF mode but with only low-power + balanced advertised.
>>
>> Bastien would that work for you?
> 
> That's something I can make work, yes.
> 
>> AMD folks would that also work for you ?
>>
>> ###
>>
>> I'm also wondering if we are going to still export
>> balanced + low-power modes to userspace in CnQF mode
>> and disable CnQF in low-power mode then if we
>> even need a sysfs knob to turn it on/off at all.
>>
>> I guess the sysfs knob would then still be useful
>> to turn it on on systems where it defaults to off
>> in the BIOS.  Might be better to do this as
>> a kernel-cmdline (module-param) then though, then we
>> also avoid the problem of platform_profile support
>> all of a sudden changing underneath's p-p-d's feet.
> 
> I would say that, you could probably have CnQF transparently replacing
> the more static "balanced" profile if it is available, and have a
> separate setting to force enable/disable it as a kernel command-line
> for debugging or if the BIOS menu doesn't offer it as an option.
> 
> That way the balanced mode would work like a more refined automatic
> profile switcher, and make the default experience better, without the
> disappearing profiles, or the user-space API headaches.
> 

module param would be fine to force load CnQF if the BIOS does not
advertise it.

But I still think, having a sysfs node would still help to give an
option to the user to "opt-out" of the scenarios where he thinks that
battery can drain out if CnQF is turned on? Or in any case to turn
on/off CnQF on the fly, so that he can still switch back to the
traditional "static slider" based power optimizations.

Please let me know your thoughts on this?

Thanks,
Shyam

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Bastien Nocera]

Hey Shyam,

I misunderstood that CnQF was a single setting, but it looks like it
has 4 different levels, right?
Unless there's a major malfunction, I don't think that offering to
switch between 2 different policies where the difference is how
"static" the performance boosts are is very useful, or comprehensible,
to end-users.

If CnQF only has a single "on" setting, then this could replace the
balanced mode for what you call "static slider", so the end-user can
still make a choice and have agency on whether the system tries to
save power, or increase performance.

If CnQF has multiple levels (Turbo, Performance, Balanced and Quiet,
right?), then I don't think it's useful to have a sysfs setting to
switch it at runtime, which only confuses user-space and the users.
BIOS setting and/or kernel command-line option are the way to go.

Did I understand this correctly?

On Tue, 6 Sept 2022 at 12:00, Shyam Sundar S K <Shyam-sundar.S-k@amd.com> wrote:
>
> Hi Bastien, Hans
>
> On 9/1/2022 7:04 PM, Bastien Nocera wrote:
> > On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 9/1/22 14:24, Bastien Nocera wrote:
> >>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
> >>>>> In this series, support for following features has been added.
> >>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
> >>>>>   where the system power can be boosted or throttled independent
> >>>>>   of the selected slider position.
> >>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
> >>>>
> >>>> Thank you. I think that before doing a more in detail review
> >>>> we first need to agree on the userspace interactions here.
> >>>>
> >>>> I've added Bastien, the power-profiles-daemon maintainer
> >>>> to the Cc for this.
> >>>>
> >>>> From a quick peek at the patches I see that currently they do
> >>>> the following:
> >>>>
> >>>> Probe time:
> >>>> -----------
> >>>>
> >>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
> >>>> is available register as a platform_profile provider
> >>>>
> >>>> 2. Query if the BIOS tells us that CnQF should be enable by
> >>>> default if yes then unregister the platform_profile provider
> >>>> and enable CnQF
> >>>>
> >>>>
> >>>> Run time:
> >>>> ---------
> >>>>
> >>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
> >>>>
> >>>> 1. When CnQF gets enabled unregister the platform_profile provider
> >>>>
> >>>> 2. When CnQF gets disabled restore the last set profile and
> >>>> register the platform_profile provider
> >>>>
> >>>>
> >>>> Questions/remarks:
> >>>>
> >>>> 1. If you look at 1. and 2. under "Probe time", you will see that
> >>>> when the BIOS requests to have CnQF enabled by default that
> >>>> userspace will then still shortly see a platform_profile
> >>>> provider. This must be fixed IMHO by checking whether to do
> >>>> CnQF by default or not before the initial register call.
> >>>>
> >>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
> >>>> will always advertise a low-power mode even when there is no
> >>>> platform-profile support, since this is also a hint for other
> >>>> parts of the system to try and conserve power. But when this
> >>>> mode is enabled we really want the system to also behave as
> >>>> if the old static slider mode is active and set to low-power.
> >>>>
> >>>> Some ideas:
> >>>> a) maybe still have the amd-pmf code register a (different)
> >>>> platform_profile provider whn in CnQF mode and have it only
> >>>> advertise low-power
> >>>>
> >>>> b) teach power-profiles-daemon about CnQF and have it
> >>>> disable CnQF when entering low-power mode?
> >>>>
> >>>> c) make the CnQF code in PMF take the charge level into
> >>>> account and have it not go "full throttle" when the chare
> >>>> is below say 25% ?
> >>>>
> >>>> 3. Bastien, can power-profiles-daemon deal with
> >>>> /sys/firmware/acpi/platform_profile disappearing or
> >>>> appearing while it is running?
> >>>
> >>> No, it doesn't.
> >>>
> >>> It expects the platform_profile file to be available on startup, at
> >>> worse with the choices not yet filled in. It doesn't handle the
> >>> platform_profile file going away, it doesn't handle the
> >>> platform_profile_choices file changing after it's been initially
> >>> filled in, and it doesn't support less than one power profile being
> >>> made available, and only supports hiding the performance profile if
> >>> the platform doesn't support it.
> >>
> >> Ok, so this means that if we go with these changes as currently
> >> proposed that if a user uses the sysfs file to turn CnQF on/off
> >> they will need to restart power-profile-daemon.
> >>
> >> I think that that is acceptable given that the user needs to manually
> >> poke things anyway. We should probably document this in the documentation
> >> for the sysfs attribute (as well as in newer versions of the p-p-d
> >> docs/README).
> >>
> >>> Some of those things we could change/fix, some other things will not.
> >>> If the platform_profile_choices file only contained a single item,
> >>> then power-profiles-daemon would just export the "low-power" and
> >>> "balanced" profiles to user-space, as it does on unsupported hardware.
> >>
> >> Right.
> >>
> >>> The profiles in power-profiles-daemon are supposed to show the user
> >>> intent, which having a single setting would effectively nullify.
> >>
> >> Yes that was my understanding too.
> >>
> >>> It's unclear to me how CnQF takes user intent into account (it's also
> >>> unclear to me how that's a low-power setting rather than a combination
> >>> of the existing cool and quiet settings).
> >>
> >> AMD folks, please correct me if any of the below is wrong:
> >>
> >> AFAIK even though it is called CnQF it is more like auto-profile
> >> selection and as such does not take user intent into account
> >> at all.
> >>
> >> It looks at the workload over a somewhat longer time period (say
> >> 5 minutes or so I guess?) and then if that consistently has been
> >> quite high, it will select something similar to performance.
> >>
> >> Where as for a more mixed workload it will select balanced and for
> >> a mostly idle machine it will select low-power.
> >>
> >> I guess this auto feature is best treated the same as unsupported hw.
> >>
> >>> (it's also
> >>> unclear to me how that's a low-power setting rather than a combination
> >>> of the existing cool and quiet settings).
> >>
> >> Even though it is called cool and quiet AFAIK it won't be all that
> >> cool and quiet when running a heavy workload. Which is why I wonder
> >> how to re-conciliate this with showing low-power in e.g. the
> >> GNOME shell system men. Because in essence even if the battery
> >> is low the system will still go full-throttle when confronted
> >> with a heavy workload.
> >>
> >> So selecting low-power would result in the screen-dimming which
> >> I think is part of that, but the CPU's max power-consumption won't
> >> get limited as it would when platform-profiles are supported.
> >>
> >> So I guess this is indeed very much like how p-p-d behaves
> >> on unsupported hw...
> >>
> >> ###
> >>
> >> As mentioned I guess one option would be for CnQF to
> >> still register a platform_profile provider and then in
> >> balanced mode do its CnQF thing and in low-power mode
> >> disable CnQF and apply the static-slider low-power settings
> >> I think that that would work best from things actual
> >> working in a way I would expect the avarage end-user to
> >> expect things to work.
> >>
> >> So p-p-d would then still see platform-profile support
> >> in CnQF mode but with only low-power + balanced advertised.
> >>
> >> Bastien would that work for you?
> >
> > That's something I can make work, yes.
> >
> >> AMD folks would that also work for you ?
> >>
> >> ###
> >>
> >> I'm also wondering if we are going to still export
> >> balanced + low-power modes to userspace in CnQF mode
> >> and disable CnQF in low-power mode then if we
> >> even need a sysfs knob to turn it on/off at all.
> >>
> >> I guess the sysfs knob would then still be useful
> >> to turn it on on systems where it defaults to off
> >> in the BIOS.  Might be better to do this as
> >> a kernel-cmdline (module-param) then though, then we
> >> also avoid the problem of platform_profile support
> >> all of a sudden changing underneath's p-p-d's feet.
> >
> > I would say that, you could probably have CnQF transparently replacing
> > the more static "balanced" profile if it is available, and have a
> > separate setting to force enable/disable it as a kernel command-line
> > for debugging or if the BIOS menu doesn't offer it as an option.
> >
> > That way the balanced mode would work like a more refined automatic
> > profile switcher, and make the default experience better, without the
> > disappearing profiles, or the user-space API headaches.
> >
>
> module param would be fine to force load CnQF if the BIOS does not
> advertise it.
>
> But I still think, having a sysfs node would still help to give an
> option to the user to "opt-out" of the scenarios where he thinks that
> battery can drain out if CnQF is turned on? Or in any case to turn
> on/off CnQF on the fly, so that he can still switch back to the
> traditional "static slider" based power optimizations.
>
> Please let me know your thoughts on this?
>
> Thanks,
> Shyam
>

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi Bastien,

On 9/7/22 16:24, Bastien Nocera wrote:
> Hey Shyam,
> 
> I misunderstood that CnQF was a single setting, but it looks like it
> has 4 different levels, right?
> Unless there's a major malfunction, I don't think that offering to
> switch between 2 different policies where the difference is how
> "static" the performance boosts are is very useful, or comprehensible,
> to end-users.
> 
> If CnQF only has a single "on" setting, then this could replace the
> balanced mode for what you call "static slider", so the end-user can
> still make a choice and have agency on whether the system tries to
> save power, or increase performance.
> 
> If CnQF has multiple levels (Turbo, Performance, Balanced and Quiet,
> right?), then I don't think it's useful to have a sysfs setting to
> switch it at runtime, which only confuses user-space and the users.
> BIOS setting and/or kernel command-line option are the way to go.
> 
> Did I understand this correctly?

Let me try clarify things:

CnQF has 4 levels internally, between which it switches automatically
based on the workload of the last 5 minutes.

So from a userspace pov CnQF is a single setting which can be toggled
on / off.

Basically it is a more dynamic balanced mode, so I think it makes
sense for the amd-pmf code to always export a platform_profile
interface and when CnQF is on then use CnQF for balanced
and the static slider settings for low-power / performance.

And when CnQF is off, then just use what AMD calls the static slider
balanced setting.

This way for performance-profile-daemon nothing really changes.

This can then be combined with allowing the user to turn CnQF
on/off through sysfs as an extra option which p-p-d can ignore
(this sysfs file then choses between CnQF and static balanced
mode when balance is set through the platform interface).

Regards,

Hans


p.s.

Shyam I will reply to your emails in a couple of minutes.





> 
> On Tue, 6 Sept 2022 at 12:00, Shyam Sundar S K <Shyam-sundar.S-k@amd.com> wrote:
>>
>> Hi Bastien, Hans
>>
>> On 9/1/2022 7:04 PM, Bastien Nocera wrote:
>>> On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 9/1/22 14:24, Bastien Nocera wrote:
>>>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>>>> In this series, support for following features has been added.
>>>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>>>   where the system power can be boosted or throttled independent
>>>>>>>   of the selected slider position.
>>>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>>>
>>>>>> Thank you. I think that before doing a more in detail review
>>>>>> we first need to agree on the userspace interactions here.
>>>>>>
>>>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>>>> to the Cc for this.
>>>>>>
>>>>>> From a quick peek at the patches I see that currently they do
>>>>>> the following:
>>>>>>
>>>>>> Probe time:
>>>>>> -----------
>>>>>>
>>>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>>>> is available register as a platform_profile provider
>>>>>>
>>>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>>>> default if yes then unregister the platform_profile provider
>>>>>> and enable CnQF
>>>>>>
>>>>>>
>>>>>> Run time:
>>>>>> ---------
>>>>>>
>>>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>>>
>>>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>>>
>>>>>> 2. When CnQF gets disabled restore the last set profile and
>>>>>> register the platform_profile provider
>>>>>>
>>>>>>
>>>>>> Questions/remarks:
>>>>>>
>>>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>>>> when the BIOS requests to have CnQF enabled by default that
>>>>>> userspace will then still shortly see a platform_profile
>>>>>> provider. This must be fixed IMHO by checking whether to do
>>>>>> CnQF by default or not before the initial register call.
>>>>>>
>>>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>>>> will always advertise a low-power mode even when there is no
>>>>>> platform-profile support, since this is also a hint for other
>>>>>> parts of the system to try and conserve power. But when this
>>>>>> mode is enabled we really want the system to also behave as
>>>>>> if the old static slider mode is active and set to low-power.
>>>>>>
>>>>>> Some ideas:
>>>>>> a) maybe still have the amd-pmf code register a (different)
>>>>>> platform_profile provider whn in CnQF mode and have it only
>>>>>> advertise low-power
>>>>>>
>>>>>> b) teach power-profiles-daemon about CnQF and have it
>>>>>> disable CnQF when entering low-power mode?
>>>>>>
>>>>>> c) make the CnQF code in PMF take the charge level into
>>>>>> account and have it not go "full throttle" when the chare
>>>>>> is below say 25% ?
>>>>>>
>>>>>> 3. Bastien, can power-profiles-daemon deal with
>>>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>>>> appearing while it is running?
>>>>>
>>>>> No, it doesn't.
>>>>>
>>>>> It expects the platform_profile file to be available on startup, at
>>>>> worse with the choices not yet filled in. It doesn't handle the
>>>>> platform_profile file going away, it doesn't handle the
>>>>> platform_profile_choices file changing after it's been initially
>>>>> filled in, and it doesn't support less than one power profile being
>>>>> made available, and only supports hiding the performance profile if
>>>>> the platform doesn't support it.
>>>>
>>>> Ok, so this means that if we go with these changes as currently
>>>> proposed that if a user uses the sysfs file to turn CnQF on/off
>>>> they will need to restart power-profile-daemon.
>>>>
>>>> I think that that is acceptable given that the user needs to manually
>>>> poke things anyway. We should probably document this in the documentation
>>>> for the sysfs attribute (as well as in newer versions of the p-p-d
>>>> docs/README).
>>>>
>>>>> Some of those things we could change/fix, some other things will not.
>>>>> If the platform_profile_choices file only contained a single item,
>>>>> then power-profiles-daemon would just export the "low-power" and
>>>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>>>
>>>> Right.
>>>>
>>>>> The profiles in power-profiles-daemon are supposed to show the user
>>>>> intent, which having a single setting would effectively nullify.
>>>>
>>>> Yes that was my understanding too.
>>>>
>>>>> It's unclear to me how CnQF takes user intent into account (it's also
>>>>> unclear to me how that's a low-power setting rather than a combination
>>>>> of the existing cool and quiet settings).
>>>>
>>>> AMD folks, please correct me if any of the below is wrong:
>>>>
>>>> AFAIK even though it is called CnQF it is more like auto-profile
>>>> selection and as such does not take user intent into account
>>>> at all.
>>>>
>>>> It looks at the workload over a somewhat longer time period (say
>>>> 5 minutes or so I guess?) and then if that consistently has been
>>>> quite high, it will select something similar to performance.
>>>>
>>>> Where as for a more mixed workload it will select balanced and for
>>>> a mostly idle machine it will select low-power.
>>>>
>>>> I guess this auto feature is best treated the same as unsupported hw.
>>>>
>>>>> (it's also
>>>>> unclear to me how that's a low-power setting rather than a combination
>>>>> of the existing cool and quiet settings).
>>>>
>>>> Even though it is called cool and quiet AFAIK it won't be all that
>>>> cool and quiet when running a heavy workload. Which is why I wonder
>>>> how to re-conciliate this with showing low-power in e.g. the
>>>> GNOME shell system men. Because in essence even if the battery
>>>> is low the system will still go full-throttle when confronted
>>>> with a heavy workload.
>>>>
>>>> So selecting low-power would result in the screen-dimming which
>>>> I think is part of that, but the CPU's max power-consumption won't
>>>> get limited as it would when platform-profiles are supported.
>>>>
>>>> So I guess this is indeed very much like how p-p-d behaves
>>>> on unsupported hw...
>>>>
>>>> ###
>>>>
>>>> As mentioned I guess one option would be for CnQF to
>>>> still register a platform_profile provider and then in
>>>> balanced mode do its CnQF thing and in low-power mode
>>>> disable CnQF and apply the static-slider low-power settings
>>>> I think that that would work best from things actual
>>>> working in a way I would expect the avarage end-user to
>>>> expect things to work.
>>>>
>>>> So p-p-d would then still see platform-profile support
>>>> in CnQF mode but with only low-power + balanced advertised.
>>>>
>>>> Bastien would that work for you?
>>>
>>> That's something I can make work, yes.
>>>
>>>> AMD folks would that also work for you ?
>>>>
>>>> ###
>>>>
>>>> I'm also wondering if we are going to still export
>>>> balanced + low-power modes to userspace in CnQF mode
>>>> and disable CnQF in low-power mode then if we
>>>> even need a sysfs knob to turn it on/off at all.
>>>>
>>>> I guess the sysfs knob would then still be useful
>>>> to turn it on on systems where it defaults to off
>>>> in the BIOS.  Might be better to do this as
>>>> a kernel-cmdline (module-param) then though, then we
>>>> also avoid the problem of platform_profile support
>>>> all of a sudden changing underneath's p-p-d's feet.
>>>
>>> I would say that, you could probably have CnQF transparently replacing
>>> the more static "balanced" profile if it is available, and have a
>>> separate setting to force enable/disable it as a kernel command-line
>>> for debugging or if the BIOS menu doesn't offer it as an option.
>>>
>>> That way the balanced mode would work like a more refined automatic
>>> profile switcher, and make the default experience better, without the
>>> disappearing profiles, or the user-space API headaches.
>>>
>>
>> module param would be fine to force load CnQF if the BIOS does not
>> advertise it.
>>
>> But I still think, having a sysfs node would still help to give an
>> option to the user to "opt-out" of the scenarios where he thinks that
>> battery can drain out if CnQF is turned on? Or in any case to turn
>> on/off CnQF on the fly, so that he can still switch back to the
>> traditional "static slider" based power optimizations.
>>
>> Please let me know your thoughts on this?
>>
>> Thanks,
>> Shyam
>>
> 

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Bastien Nocera]

On Wed, 7 Sept 2022 at 16:35, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Bastien,
>
> On 9/7/22 16:24, Bastien Nocera wrote:
> > Hey Shyam,
> >
> > I misunderstood that CnQF was a single setting, but it looks like it
> > has 4 different levels, right?
> > Unless there's a major malfunction, I don't think that offering to
> > switch between 2 different policies where the difference is how
> > "static" the performance boosts are is very useful, or comprehensible,
> > to end-users.
> >
> > If CnQF only has a single "on" setting, then this could replace the
> > balanced mode for what you call "static slider", so the end-user can
> > still make a choice and have agency on whether the system tries to
> > save power, or increase performance.
> >
> > If CnQF has multiple levels (Turbo, Performance, Balanced and Quiet,
> > right?), then I don't think it's useful to have a sysfs setting to
> > switch it at runtime, which only confuses user-space and the users.
> > BIOS setting and/or kernel command-line option are the way to go.
> >
> > Did I understand this correctly?
>
> Let me try clarify things:
>
> CnQF has 4 levels internally, between which it switches automatically
> based on the workload of the last 5 minutes.

Oh, those profiles are internal only, OK. Do those automated levels
behave like the "static slider" ones, to the point of being
indistinguishable? So for example, does the static slider
"performance" behave like "CnQF" if the machine was heavily loaded
machine for 5 minutes?

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 9/7/22 17:35, Bastien Nocera wrote:
> On Wed, 7 Sept 2022 at 16:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Bastien,
>>
>> On 9/7/22 16:24, Bastien Nocera wrote:
>>> Hey Shyam,
>>>
>>> I misunderstood that CnQF was a single setting, but it looks like it
>>> has 4 different levels, right?
>>> Unless there's a major malfunction, I don't think that offering to
>>> switch between 2 different policies where the difference is how
>>> "static" the performance boosts are is very useful, or comprehensible,
>>> to end-users.
>>>
>>> If CnQF only has a single "on" setting, then this could replace the
>>> balanced mode for what you call "static slider", so the end-user can
>>> still make a choice and have agency on whether the system tries to
>>> save power, or increase performance.
>>>
>>> If CnQF has multiple levels (Turbo, Performance, Balanced and Quiet,
>>> right?), then I don't think it's useful to have a sysfs setting to
>>> switch it at runtime, which only confuses user-space and the users.
>>> BIOS setting and/or kernel command-line option are the way to go.
>>>
>>> Did I understand this correctly?
>>
>> Let me try clarify things:
>>
>> CnQF has 4 levels internally, between which it switches automatically
>> based on the workload of the last 5 minutes.
> 
> Oh, those profiles are internal only, OK. Do those automated levels
> behave like the "static slider" ones, to the point of being
> indistinguishable? So for example, does the static slider
> "performance" behave like "CnQF" if the machine was heavily loaded
> machine for 5 minutes?

This is more of a question for AMD to answer. But yes I believe that
the CnQF internal performance mode which it boosts to if the machine
is heavily loaded for 5 minutes is similar to the static slider
performance setting.

Regards,

Hans

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Shyam Sundar S K]

Hi Bastien,

On 9/8/2022 2:38 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/7/22 17:35, Bastien Nocera wrote:
>> On Wed, 7 Sept 2022 at 16:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Bastien,
>>>
>>> On 9/7/22 16:24, Bastien Nocera wrote:
>>>> Hey Shyam,
>>>>
>>>> I misunderstood that CnQF was a single setting, but it looks like it
>>>> has 4 different levels, right?
>>>> Unless there's a major malfunction, I don't think that offering to
>>>> switch between 2 different policies where the difference is how
>>>> "static" the performance boosts are is very useful, or comprehensible,
>>>> to end-users.
>>>>
>>>> If CnQF only has a single "on" setting, then this could replace the
>>>> balanced mode for what you call "static slider", so the end-user can
>>>> still make a choice and have agency on whether the system tries to
>>>> save power, or increase performance.
>>>>
>>>> If CnQF has multiple levels (Turbo, Performance, Balanced and Quiet,
>>>> right?), then I don't think it's useful to have a sysfs setting to
>>>> switch it at runtime, which only confuses user-space and the users.
>>>> BIOS setting and/or kernel command-line option are the way to go.
>>>>
>>>> Did I understand this correctly?
>>>
>>> Let me try clarify things:
>>>
>>> CnQF has 4 levels internally, between which it switches automatically
>>> based on the workload of the last 5 minutes.
>>
>> Oh, those profiles are internal only, OK. Do those automated levels
>> behave like the "static slider" ones, to the point of being
>> indistinguishable? So for example, does the static slider
>> "performance" behave like "CnQF" if the machine was heavily loaded
>> machine for 5 minutes?
> 
> This is more of a question for AMD to answer. But yes I believe that
> the CnQF internal performance mode which it boosts to if the machine
> is heavily loaded for 5 minutes is similar to the static slider
> performance setting.

Its a kind of "yes". But its still dependent on how the OEMs have tuned
the power profiling values and mapped it to the relavant CnQF modes.

Thanks,
Shyam

> 
> Regards,
> 
> Hans
> 

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 9/6/22 11:59, Shyam Sundar S K wrote:
> Hi Bastien, Hans
> 
> On 9/1/2022 7:04 PM, Bastien Nocera wrote:
>> On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 9/1/22 14:24, Bastien Nocera wrote:
>>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>>> In this series, support for following features has been added.
>>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>>   where the system power can be boosted or throttled independent
>>>>>>   of the selected slider position.
>>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>>
>>>>> Thank you. I think that before doing a more in detail review
>>>>> we first need to agree on the userspace interactions here.
>>>>>
>>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>>> to the Cc for this.
>>>>>
>>>>> From a quick peek at the patches I see that currently they do
>>>>> the following:
>>>>>
>>>>> Probe time:
>>>>> -----------
>>>>>
>>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>>> is available register as a platform_profile provider
>>>>>
>>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>>> default if yes then unregister the platform_profile provider
>>>>> and enable CnQF
>>>>>
>>>>>
>>>>> Run time:
>>>>> ---------
>>>>>
>>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>>
>>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>>
>>>>> 2. When CnQF gets disabled restore the last set profile and
>>>>> register the platform_profile provider
>>>>>
>>>>>
>>>>> Questions/remarks:
>>>>>
>>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>>> when the BIOS requests to have CnQF enabled by default that
>>>>> userspace will then still shortly see a platform_profile
>>>>> provider. This must be fixed IMHO by checking whether to do
>>>>> CnQF by default or not before the initial register call.
>>>>>
>>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>>> will always advertise a low-power mode even when there is no
>>>>> platform-profile support, since this is also a hint for other
>>>>> parts of the system to try and conserve power. But when this
>>>>> mode is enabled we really want the system to also behave as
>>>>> if the old static slider mode is active and set to low-power.
>>>>>
>>>>> Some ideas:
>>>>> a) maybe still have the amd-pmf code register a (different)
>>>>> platform_profile provider whn in CnQF mode and have it only
>>>>> advertise low-power
>>>>>
>>>>> b) teach power-profiles-daemon about CnQF and have it
>>>>> disable CnQF when entering low-power mode?
>>>>>
>>>>> c) make the CnQF code in PMF take the charge level into
>>>>> account and have it not go "full throttle" when the chare
>>>>> is below say 25% ?
>>>>>
>>>>> 3. Bastien, can power-profiles-daemon deal with
>>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>>> appearing while it is running?
>>>>
>>>> No, it doesn't.
>>>>
>>>> It expects the platform_profile file to be available on startup, at
>>>> worse with the choices not yet filled in. It doesn't handle the
>>>> platform_profile file going away, it doesn't handle the
>>>> platform_profile_choices file changing after it's been initially
>>>> filled in, and it doesn't support less than one power profile being
>>>> made available, and only supports hiding the performance profile if
>>>> the platform doesn't support it.
>>>
>>> Ok, so this means that if we go with these changes as currently
>>> proposed that if a user uses the sysfs file to turn CnQF on/off
>>> they will need to restart power-profile-daemon.
>>>
>>> I think that that is acceptable given that the user needs to manually
>>> poke things anyway. We should probably document this in the documentation
>>> for the sysfs attribute (as well as in newer versions of the p-p-d
>>> docs/README).
>>>
>>>> Some of those things we could change/fix, some other things will not.
>>>> If the platform_profile_choices file only contained a single item,
>>>> then power-profiles-daemon would just export the "low-power" and
>>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>>
>>> Right.
>>>
>>>> The profiles in power-profiles-daemon are supposed to show the user
>>>> intent, which having a single setting would effectively nullify.
>>>
>>> Yes that was my understanding too.
>>>
>>>> It's unclear to me how CnQF takes user intent into account (it's also
>>>> unclear to me how that's a low-power setting rather than a combination
>>>> of the existing cool and quiet settings).
>>>
>>> AMD folks, please correct me if any of the below is wrong:
>>>
>>> AFAIK even though it is called CnQF it is more like auto-profile
>>> selection and as such does not take user intent into account
>>> at all.
>>>
>>> It looks at the workload over a somewhat longer time period (say
>>> 5 minutes or so I guess?) and then if that consistently has been
>>> quite high, it will select something similar to performance.
>>>
>>> Where as for a more mixed workload it will select balanced and for
>>> a mostly idle machine it will select low-power.
>>>
>>> I guess this auto feature is best treated the same as unsupported hw.
>>>
>>>> (it's also
>>>> unclear to me how that's a low-power setting rather than a combination
>>>> of the existing cool and quiet settings).
>>>
>>> Even though it is called cool and quiet AFAIK it won't be all that
>>> cool and quiet when running a heavy workload. Which is why I wonder
>>> how to re-conciliate this with showing low-power in e.g. the
>>> GNOME shell system men. Because in essence even if the battery
>>> is low the system will still go full-throttle when confronted
>>> with a heavy workload.
>>>
>>> So selecting low-power would result in the screen-dimming which
>>> I think is part of that, but the CPU's max power-consumption won't
>>> get limited as it would when platform-profiles are supported.
>>>
>>> So I guess this is indeed very much like how p-p-d behaves
>>> on unsupported hw...
>>>
>>> ###
>>>
>>> As mentioned I guess one option would be for CnQF to
>>> still register a platform_profile provider and then in
>>> balanced mode do its CnQF thing and in low-power mode
>>> disable CnQF and apply the static-slider low-power settings
>>> I think that that would work best from things actual
>>> working in a way I would expect the avarage end-user to
>>> expect things to work.
>>>
>>> So p-p-d would then still see platform-profile support
>>> in CnQF mode but with only low-power + balanced advertised.
>>>
>>> Bastien would that work for you?
>>
>> That's something I can make work, yes.
>>
>>> AMD folks would that also work for you ?
>>>
>>> ###
>>>
>>> I'm also wondering if we are going to still export
>>> balanced + low-power modes to userspace in CnQF mode
>>> and disable CnQF in low-power mode then if we
>>> even need a sysfs knob to turn it on/off at all.
>>>
>>> I guess the sysfs knob would then still be useful
>>> to turn it on on systems where it defaults to off
>>> in the BIOS.  Might be better to do this as
>>> a kernel-cmdline (module-param) then though, then we
>>> also avoid the problem of platform_profile support
>>> all of a sudden changing underneath's p-p-d's feet.
>>
>> I would say that, you could probably have CnQF transparently replacing
>> the more static "balanced" profile if it is available, and have a
>> separate setting to force enable/disable it as a kernel command-line
>> for debugging or if the BIOS menu doesn't offer it as an option.
>>
>> That way the balanced mode would work like a more refined automatic
>> profile switcher, and make the default experience better, without the
>> disappearing profiles, or the user-space API headaches.
>>
> 
> module param would be fine to force load CnQF if the BIOS does not
> advertise it.
> 
> But I still think, having a sysfs node would still help to give an
> option to the user to "opt-out" of the scenarios where he thinks that
> battery can drain out if CnQF is turned on? Or in any case to turn
> on/off CnQF on the fly, so that he can still switch back to the
> traditional "static slider" based power optimizations.
> 
> Please let me know your thoughts on this?

Having a sysfs file to turn CnQF on/off seems best to me. Users who
want to override the BIOS default at boot can then always do so
with a little script or udev rule writing to the sysfs file.

Regards,

Hans

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Shyam Sundar S K]

Hi Hans,

Apologies for the delay in responding to this thread. Some responses below.

On 9/1/2022 6:14 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/1/22 14:24, Bastien Nocera wrote:
>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>> In this series, support for following features has been added.
>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>   where the system power can be boosted or throttled independent
>>>>   of the selected slider position.
>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>
>>> Thank you. I think that before doing a more in detail review
>>> we first need to agree on the userspace interactions here.
>>>
>>> I've added Bastien, the power-profiles-daemon maintainer
>>> to the Cc for this.
>>>
>>> From a quick peek at the patches I see that currently they do
>>> the following:
>>>
>>> Probe time:
>>> -----------
>>>
>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>> is available register as a platform_profile provider
>>>
>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>> default if yes then unregister the platform_profile provider
>>> and enable CnQF
>>>
>>>
>>> Run time:
>>> ---------
>>>
>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>
>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>
>>> 2. When CnQF gets disabled restore the last set profile and
>>> register the platform_profile provider
>>>
>>>
>>> Questions/remarks:
>>>
>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>> when the BIOS requests to have CnQF enabled by default that
>>> userspace will then still shortly see a platform_profile
>>> provider. This must be fixed IMHO by checking whether to do
>>> CnQF by default or not before the initial register call.
>>>
>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>> will always advertise a low-power mode even when there is no
>>> platform-profile support, since this is also a hint for other
>>> parts of the system to try and conserve power. But when this
>>> mode is enabled we really want the system to also behave as
>>> if the old static slider mode is active and set to low-power.
>>>
>>> Some ideas:
>>> a) maybe still have the amd-pmf code register a (different)
>>> platform_profile provider whn in CnQF mode and have it only
>>> advertise low-power
>>>
>>> b) teach power-profiles-daemon about CnQF and have it
>>> disable CnQF when entering low-power mode?
>>>
>>> c) make the CnQF code in PMF take the charge level into
>>> account and have it not go "full throttle" when the chare
>>> is below say 25% ?
>>>
>>> 3. Bastien, can power-profiles-daemon deal with
>>> /sys/firmware/acpi/platform_profile disappearing or
>>> appearing while it is running?
>>
>> No, it doesn't.
>>
>> It expects the platform_profile file to be available on startup, at
>> worse with the choices not yet filled in. It doesn't handle the
>> platform_profile file going away, it doesn't handle the
>> platform_profile_choices file changing after it's been initially
>> filled in, and it doesn't support less than one power profile being
>> made available, and only supports hiding the performance profile if
>> the platform doesn't support it.
> 
> Ok, so this means that if we go with these changes as currently
> proposed that if a user uses the sysfs file to turn CnQF on/off
> they will need to restart power-profile-daemon.
> 
> I think that that is acceptable given that the user needs to manually
> poke things anyway. We should probably document this in the documentation
> for the sysfs attribute (as well as in newer versions of the p-p-d
> docs/README).
> 
>> Some of those things we could change/fix, some other things will not.
>> If the platform_profile_choices file only contained a single item,
>> then power-profiles-daemon would just export the "low-power" and
>> "balanced" profiles to user-space, as it does on unsupported hardware.
> 
> Right.
> 
>> The profiles in power-profiles-daemon are supposed to show the user
>> intent, which having a single setting would effectively nullify.
> 
> Yes that was my understanding too.
> 
>> It's unclear to me how CnQF takes user intent into account (it's also
>> unclear to me how that's a low-power setting rather than a combination
>> of the existing cool and quiet settings).
> 
> AMD folks, please correct me if any of the below is wrong:
> 
> AFAIK even though it is called CnQF it is more like auto-profile
> selection and as such does not take user intent into account
> at all.

Yes, You are right. Below is a brief note on how CnQF was designed.

1)CnQF – Cool And Quiet Framework - It’s an extension of the static
slider concept wherein PMF dynamically manages system power limits and
fan policy based on system power trends.

2)OEM can opt into the feature by defining the CnQF BIOS ACPI method.

3)Static slider control and CnQF are mutually exclusive.

4)CnQF supports up to 4 modes of operation – Turbo, Performance,
Balanced and Quiet.

- It can be configured for AC and DC distinctly.
- PMF driver calculates the moving average of system power and switches
the mode of operation.
    *If system power is limited to the threshold of the current mode,
move to the next higher mode
    *If system power is not limited to the threshold of the current
mode, reduce the power budget by moving to the next lower mode.

5)CnQF feature control is through Radeon SW (a GUI based tool on Windows)

To match the behavior on Windows, we kept a sysfs node to turn on/off
the CnQF on the fly like the way it can be done the windows side with
the Radeon SW tool. If you think that having as a module param makes
more sense, I am open to make the change and send a v2.

Like I mentioned above, on Windows the static slider is absoultely dummy
when the user goes on turns on the CnQF from Radeon SW tool. Based on
the review remarks on the earlier series, we tried to
register/de-register to platform_profile, as per sysfs input (like
setting up and tearing down to platform_profile).

The Difference between Auto-mode (for thinkpads) and CnQF(for others) is
that:

- Automode gets turned on only when the slider position is set to
"balanced" in the platform_profile and
- a corresponding AMT ON event is triggered.
- it has 3 internal modes quiet, balanced, performance

But for CnQF,

- it is independent of the slider position and there are no ACPI events
for it to get kicked in.
- There are two seperate ACPI methods for AC and DC to get the
corresponding thermal values.
- it has 4 internal modes quiet, balanced, performance, turbo


There is already a WIP feature called "policy builder" where the OEMs
can build custom policies, which includes looking at the battery
percentages and making thermal optimizations accordingly. But this was
not taken into consideration while designing the CnQF on windows too. If
we bring in this change for Linux, there maybe differences in the way
the same feature behaves "differently" across OSes.

Like you mentioned the usecase, where just a compilation can end up in
battery drain if not connected to AC power. Can we not have a
documentation update saying it is advised to turn "off" CnQF when
battery % goes below a certain level? That way, the end user experiences
across Linux and Windows remains the same.


> 
> It looks at the workload over a somewhat longer time period (say
> 5 minutes or so I guess?) and then if that consistently has been
> quite high, it will select something similar to performance.

Right. The switch time would be dependent on the "time constant" values
set in the BIOS  which is configurable to the OEMs.

> 
> Where as for a more mixed workload it will select balanced and for
> a mostly idle machine it will select low-power.
> 
> I guess this auto feature is best treated the same as unsupported hw.
> 
>> (it's also
>> unclear to me how that's a low-power setting rather than a combination
>> of the existing cool and quiet settings).
> 
> Even though it is called cool and quiet AFAIK it won't be all that
> cool and quiet when running a heavy workload. Which is why I wonder
> how to re-conciliate this with showing low-power in e.g. the
> GNOME shell system men. Because in essence even if the battery
> is low the system will still go full-throttle when confronted
> with a heavy workload.
> 
> So selecting low-power would result in the screen-dimming which
> I think is part of that, but the CPU's max power-consumption won't
> get limited as it would when platform-profiles are supported.
> 
> So I guess this is indeed very much like how p-p-d behaves
> on unsupported hw...
> 
> ###
> 
> As mentioned I guess one option would be for CnQF to
> still register a platform_profile provider and then in
> balanced mode do its CnQF thing and in low-power mode
> disable CnQF and apply the static-slider low-power settings
> I think that that would work best from things actual
> working in a way I would expect the avarage end-user to
> expect things to work.
> 
> So p-p-d would then still see platform-profile support
> in CnQF mode but with only low-power + balanced advertised.
> 
> Bastien would that work for you?
> 
> AMD folks would that also work for you ?

If we go with the above proposal it would become very identical to what
is being done with automode (expect the extra "turbo" mode and not
having a AMT event). This would need some amount of discussion with our
windows folks also to know what they think about it.

Please let me know your thoughts based on the above notes what I
provided(in the earlier section), so that we can make the code changes
accordingly.

Thanks,
Shyam


> 
> ###
> 
> I'm also wondering if we are going to still export
> balanced + low-power modes to userspace in CnQF mode
> and disable CnQF in low-power mode then if we
> even need a sysfs knob to turn it on/off at all.
> 
> I guess the sysfs knob would then still be useful
> to turn it on on systems where it defaults to off
> in the BIOS.  Might be better to do this as
> a kernel-cmdline (module-param) then though, then we
> also avoid the problem of platform_profile support
> all of a sudden changing underneath's p-p-d's feet.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> and for toggling ooff through sysfs
> 

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 9/6/22 11:53, Shyam Sundar S K wrote:
> Hi Hans,
> 
> Apologies for the delay in responding to this thread. Some responses below.

No worries.

> On 9/1/2022 6:14 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/1/22 14:24, Bastien Nocera wrote:
>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>> In this series, support for following features has been added.
>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>   where the system power can be boosted or throttled independent
>>>>>   of the selected slider position.
>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>
>>>> Thank you. I think that before doing a more in detail review
>>>> we first need to agree on the userspace interactions here.
>>>>
>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>> to the Cc for this.
>>>>
>>>> From a quick peek at the patches I see that currently they do
>>>> the following:
>>>>
>>>> Probe time:
>>>> -----------
>>>>
>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>> is available register as a platform_profile provider
>>>>
>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>> default if yes then unregister the platform_profile provider
>>>> and enable CnQF
>>>>
>>>>
>>>> Run time:
>>>> ---------
>>>>
>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>
>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>
>>>> 2. When CnQF gets disabled restore the last set profile and
>>>> register the platform_profile provider
>>>>
>>>>
>>>> Questions/remarks:
>>>>
>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>> when the BIOS requests to have CnQF enabled by default that
>>>> userspace will then still shortly see a platform_profile
>>>> provider. This must be fixed IMHO by checking whether to do
>>>> CnQF by default or not before the initial register call.
>>>>
>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>> will always advertise a low-power mode even when there is no
>>>> platform-profile support, since this is also a hint for other
>>>> parts of the system to try and conserve power. But when this
>>>> mode is enabled we really want the system to also behave as
>>>> if the old static slider mode is active and set to low-power.
>>>>
>>>> Some ideas:
>>>> a) maybe still have the amd-pmf code register a (different)
>>>> platform_profile provider whn in CnQF mode and have it only
>>>> advertise low-power
>>>>
>>>> b) teach power-profiles-daemon about CnQF and have it
>>>> disable CnQF when entering low-power mode?
>>>>
>>>> c) make the CnQF code in PMF take the charge level into
>>>> account and have it not go "full throttle" when the chare
>>>> is below say 25% ?
>>>>
>>>> 3. Bastien, can power-profiles-daemon deal with
>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>> appearing while it is running?
>>>
>>> No, it doesn't.
>>>
>>> It expects the platform_profile file to be available on startup, at
>>> worse with the choices not yet filled in. It doesn't handle the
>>> platform_profile file going away, it doesn't handle the
>>> platform_profile_choices file changing after it's been initially
>>> filled in, and it doesn't support less than one power profile being
>>> made available, and only supports hiding the performance profile if
>>> the platform doesn't support it.
>>
>> Ok, so this means that if we go with these changes as currently
>> proposed that if a user uses the sysfs file to turn CnQF on/off
>> they will need to restart power-profile-daemon.
>>
>> I think that that is acceptable given that the user needs to manually
>> poke things anyway. We should probably document this in the documentation
>> for the sysfs attribute (as well as in newer versions of the p-p-d
>> docs/README).
>>
>>> Some of those things we could change/fix, some other things will not.
>>> If the platform_profile_choices file only contained a single item,
>>> then power-profiles-daemon would just export the "low-power" and
>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>
>> Right.
>>
>>> The profiles in power-profiles-daemon are supposed to show the user
>>> intent, which having a single setting would effectively nullify.
>>
>> Yes that was my understanding too.
>>
>>> It's unclear to me how CnQF takes user intent into account (it's also
>>> unclear to me how that's a low-power setting rather than a combination
>>> of the existing cool and quiet settings).
>>
>> AMD folks, please correct me if any of the below is wrong:
>>
>> AFAIK even though it is called CnQF it is more like auto-profile
>> selection and as such does not take user intent into account
>> at all.
> 
> Yes, You are right. Below is a brief note on how CnQF was designed.
> 
> 1)CnQF – Cool And Quiet Framework - It’s an extension of the static
> slider concept wherein PMF dynamically manages system power limits and
> fan policy based on system power trends.
> 
> 2)OEM can opt into the feature by defining the CnQF BIOS ACPI method.
> 
> 3)Static slider control and CnQF are mutually exclusive.
> 
> 4)CnQF supports up to 4 modes of operation – Turbo, Performance,
> Balanced and Quiet.
> 
> - It can be configured for AC and DC distinctly.
> - PMF driver calculates the moving average of system power and switches
> the mode of operation.
>     *If system power is limited to the threshold of the current mode,
> move to the next higher mode
>     *If system power is not limited to the threshold of the current
> mode, reduce the power budget by moving to the next lower mode.
> 
> 5)CnQF feature control is through Radeon SW (a GUI based tool on Windows)
> 
> To match the behavior on Windows, we kept a sysfs node to turn on/off
> the CnQF on the fly like the way it can be done the windows side with
> the Radeon SW tool. If you think that having as a module param makes
> more sense, I am open to make the change and send a v2.
> 
> Like I mentioned above, on Windows the static slider is absoultely dummy
> when the user goes on turns on the CnQF from Radeon SW tool. Based on
> the review remarks on the earlier series, we tried to
> register/de-register to platform_profile, as per sysfs input (like
> setting up and tearing down to platform_profile).
> 
> The Difference between Auto-mode (for thinkpads) and CnQF(for others) is
> that:
> 
> - Automode gets turned on only when the slider position is set to
> "balanced" in the platform_profile and
> - a corresponding AMT ON event is triggered.
> - it has 3 internal modes quiet, balanced, performance
> 
> But for CnQF,
> 
> - it is independent of the slider position and there are no ACPI events
> for it to get kicked in.
> - There are two seperate ACPI methods for AC and DC to get the
> corresponding thermal values.
> - it has 4 internal modes quiet, balanced, performance, turbo
> 
> 
> There is already a WIP feature called "policy builder" where the OEMs
> can build custom policies, which includes looking at the battery
> percentages and making thermal optimizations accordingly. But this was
> not taken into consideration while designing the CnQF on windows too. If
> we bring in this change for Linux, there maybe differences in the way
> the same feature behaves "differently" across OSes.
> 
> Like you mentioned the usecase, where just a compilation can end up in
> battery drain if not connected to AC power.

Thanks for the explanation above.

> Can we not have a
> documentation update saying it is advised to turn "off" CnQF when
> battery % goes below a certain level?

So we would need to document that the user needs to poke
the sysfs file when the battery is low ?  That seems very user
unfriendly.

And also don't want power-performance-daemon to need to know about
this AMD specific sysfs knob. That is why we have the standardized
platform_profile userspace API.

> That way, the end user experiences
> across Linux and Windows remains the same.

I can understand that you want to keep things the same. If I've
read the above correctly then currently the user experience under
Windows is that the slider in Windows has been turned into a
dummy slider which does not do anything.

That IMHO is quite a poor user-experience esp. when users want
their battery to last longer because they are going to be e.g.
on the road the entire day.

>> It looks at the workload over a somewhat longer time period (say
>> 5 minutes or so I guess?) and then if that consistently has been
>> quite high, it will select something similar to performance.
> 
> Right. The switch time would be dependent on the "time constant" values
> set in the BIOS  which is configurable to the OEMs.
> 
>>
>> Where as for a more mixed workload it will select balanced and for
>> a mostly idle machine it will select low-power.
>>
>> I guess this auto feature is best treated the same as unsupported hw.
>>
>>> (it's also
>>> unclear to me how that's a low-power setting rather than a combination
>>> of the existing cool and quiet settings).
>>
>> Even though it is called cool and quiet AFAIK it won't be all that
>> cool and quiet when running a heavy workload. Which is why I wonder
>> how to re-conciliate this with showing low-power in e.g. the
>> GNOME shell system men. Because in essence even if the battery
>> is low the system will still go full-throttle when confronted
>> with a heavy workload.
>>
>> So selecting low-power would result in the screen-dimming which
>> I think is part of that, but the CPU's max power-consumption won't
>> get limited as it would when platform-profiles are supported.
>>
>> So I guess this is indeed very much like how p-p-d behaves
>> on unsupported hw...
>>
>> ###
>>
>> As mentioned I guess one option would be for CnQF to
>> still register a platform_profile provider and then in
>> balanced mode do its CnQF thing and in low-power mode
>> disable CnQF and apply the static-slider low-power settings
>> I think that that would work best from things actual
>> working in a way I would expect the avarage end-user to
>> expect things to work.
>>
>> So p-p-d would then still see platform-profile support
>> in CnQF mode but with only low-power + balanced advertised.
>>
>> Bastien would that work for you?
>>
>> AMD folks would that also work for you ?
> 
> If we go with the above proposal it would become very identical to what
> is being done with automode (expect the extra "turbo" mode and not
> having a AMT event).

Yes I think that the AMT mode, where the more dynamic behavior os
only done in balanced mode and low-power is still very much a low
power-mode (and performance is always tuned for permance) makes
a lot more sense from an enduser pov. Then the slider still actually
works as expected and in the default balanced mode users will get
the benefits of the new CnQF behavior / feature.

> This would need some amount of discussion with our
> windows folks also to know what they think about it.

Ok.

Regards,

Hans

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Shyam Sundar S K]

On 9/7/2022 8:18 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/6/22 11:53, Shyam Sundar S K wrote:
>> Hi Hans,
>>
>> Apologies for the delay in responding to this thread. Some responses below.
> 
> No worries.
> 
>> On 9/1/2022 6:14 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 9/1/22 14:24, Bastien Nocera wrote:
>>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>>> In this series, support for following features has been added.
>>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>>   where the system power can be boosted or throttled independent
>>>>>>   of the selected slider position.
>>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>>
>>>>> Thank you. I think that before doing a more in detail review
>>>>> we first need to agree on the userspace interactions here.
>>>>>
>>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>>> to the Cc for this.
>>>>>
>>>>> From a quick peek at the patches I see that currently they do
>>>>> the following:
>>>>>
>>>>> Probe time:
>>>>> -----------
>>>>>
>>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>>> is available register as a platform_profile provider
>>>>>
>>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>>> default if yes then unregister the platform_profile provider
>>>>> and enable CnQF
>>>>>
>>>>>
>>>>> Run time:
>>>>> ---------
>>>>>
>>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>>
>>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>>
>>>>> 2. When CnQF gets disabled restore the last set profile and
>>>>> register the platform_profile provider
>>>>>
>>>>>
>>>>> Questions/remarks:
>>>>>
>>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>>> when the BIOS requests to have CnQF enabled by default that
>>>>> userspace will then still shortly see a platform_profile
>>>>> provider. This must be fixed IMHO by checking whether to do
>>>>> CnQF by default or not before the initial register call.
>>>>>
>>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>>> will always advertise a low-power mode even when there is no
>>>>> platform-profile support, since this is also a hint for other
>>>>> parts of the system to try and conserve power. But when this
>>>>> mode is enabled we really want the system to also behave as
>>>>> if the old static slider mode is active and set to low-power.
>>>>>
>>>>> Some ideas:
>>>>> a) maybe still have the amd-pmf code register a (different)
>>>>> platform_profile provider whn in CnQF mode and have it only
>>>>> advertise low-power
>>>>>
>>>>> b) teach power-profiles-daemon about CnQF and have it
>>>>> disable CnQF when entering low-power mode?
>>>>>
>>>>> c) make the CnQF code in PMF take the charge level into
>>>>> account and have it not go "full throttle" when the chare
>>>>> is below say 25% ?
>>>>>
>>>>> 3. Bastien, can power-profiles-daemon deal with
>>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>>> appearing while it is running?
>>>>
>>>> No, it doesn't.
>>>>
>>>> It expects the platform_profile file to be available on startup, at
>>>> worse with the choices not yet filled in. It doesn't handle the
>>>> platform_profile file going away, it doesn't handle the
>>>> platform_profile_choices file changing after it's been initially
>>>> filled in, and it doesn't support less than one power profile being
>>>> made available, and only supports hiding the performance profile if
>>>> the platform doesn't support it.
>>>
>>> Ok, so this means that if we go with these changes as currently
>>> proposed that if a user uses the sysfs file to turn CnQF on/off
>>> they will need to restart power-profile-daemon.
>>>
>>> I think that that is acceptable given that the user needs to manually
>>> poke things anyway. We should probably document this in the documentation
>>> for the sysfs attribute (as well as in newer versions of the p-p-d
>>> docs/README).
>>>
>>>> Some of those things we could change/fix, some other things will not.
>>>> If the platform_profile_choices file only contained a single item,
>>>> then power-profiles-daemon would just export the "low-power" and
>>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>>
>>> Right.
>>>
>>>> The profiles in power-profiles-daemon are supposed to show the user
>>>> intent, which having a single setting would effectively nullify.
>>>
>>> Yes that was my understanding too.
>>>
>>>> It's unclear to me how CnQF takes user intent into account (it's also
>>>> unclear to me how that's a low-power setting rather than a combination
>>>> of the existing cool and quiet settings).
>>>
>>> AMD folks, please correct me if any of the below is wrong:
>>>
>>> AFAIK even though it is called CnQF it is more like auto-profile
>>> selection and as such does not take user intent into account
>>> at all.
>>
>> Yes, You are right. Below is a brief note on how CnQF was designed.
>>
>> 1)CnQF – Cool And Quiet Framework - It’s an extension of the static
>> slider concept wherein PMF dynamically manages system power limits and
>> fan policy based on system power trends.
>>
>> 2)OEM can opt into the feature by defining the CnQF BIOS ACPI method.
>>
>> 3)Static slider control and CnQF are mutually exclusive.
>>
>> 4)CnQF supports up to 4 modes of operation – Turbo, Performance,
>> Balanced and Quiet.
>>
>> - It can be configured for AC and DC distinctly.
>> - PMF driver calculates the moving average of system power and switches
>> the mode of operation.
>>     *If system power is limited to the threshold of the current mode,
>> move to the next higher mode
>>     *If system power is not limited to the threshold of the current
>> mode, reduce the power budget by moving to the next lower mode.
>>
>> 5)CnQF feature control is through Radeon SW (a GUI based tool on Windows)
>>
>> To match the behavior on Windows, we kept a sysfs node to turn on/off
>> the CnQF on the fly like the way it can be done the windows side with
>> the Radeon SW tool. If you think that having as a module param makes
>> more sense, I am open to make the change and send a v2.
>>
>> Like I mentioned above, on Windows the static slider is absoultely dummy
>> when the user goes on turns on the CnQF from Radeon SW tool. Based on
>> the review remarks on the earlier series, we tried to
>> register/de-register to platform_profile, as per sysfs input (like
>> setting up and tearing down to platform_profile).
>>
>> The Difference between Auto-mode (for thinkpads) and CnQF(for others) is
>> that:
>>
>> - Automode gets turned on only when the slider position is set to
>> "balanced" in the platform_profile and
>> - a corresponding AMT ON event is triggered.
>> - it has 3 internal modes quiet, balanced, performance
>>
>> But for CnQF,
>>
>> - it is independent of the slider position and there are no ACPI events
>> for it to get kicked in.
>> - There are two seperate ACPI methods for AC and DC to get the
>> corresponding thermal values.
>> - it has 4 internal modes quiet, balanced, performance, turbo
>>
>>
>> There is already a WIP feature called "policy builder" where the OEMs
>> can build custom policies, which includes looking at the battery
>> percentages and making thermal optimizations accordingly. But this was
>> not taken into consideration while designing the CnQF on windows too. If
>> we bring in this change for Linux, there maybe differences in the way
>> the same feature behaves "differently" across OSes.
>>
>> Like you mentioned the usecase, where just a compilation can end up in
>> battery drain if not connected to AC power.
> 
> Thanks for the explanation above.
> 
>> Can we not have a
>> documentation update saying it is advised to turn "off" CnQF when
>> battery % goes below a certain level?
> 
> So we would need to document that the user needs to poke
> the sysfs file when the battery is low ?  That seems very user
> unfriendly.
> 
> And also don't want power-performance-daemon to need to know about
> this AMD specific sysfs knob. That is why we have the standardized
> platform_profile userspace API.
> 
>> That way, the end user experiences
>> across Linux and Windows remains the same.
> 
> I can understand that you want to keep things the same. If I've
> read the above correctly then currently the user experience under
> Windows is that the slider in Windows has been turned into a
> dummy slider which does not do anything.
> 
> That IMHO is quite a poor user-experience esp. when users want
> their battery to last longer because they are going to be e.g.
> on the road the entire day.
> 
>>> It looks at the workload over a somewhat longer time period (say
>>> 5 minutes or so I guess?) and then if that consistently has been
>>> quite high, it will select something similar to performance.
>>
>> Right. The switch time would be dependent on the "time constant" values
>> set in the BIOS  which is configurable to the OEMs.
>>
>>>
>>> Where as for a more mixed workload it will select balanced and for
>>> a mostly idle machine it will select low-power.
>>>
>>> I guess this auto feature is best treated the same as unsupported hw.
>>>
>>>> (it's also
>>>> unclear to me how that's a low-power setting rather than a combination
>>>> of the existing cool and quiet settings).
>>>
>>> Even though it is called cool and quiet AFAIK it won't be all that
>>> cool and quiet when running a heavy workload. Which is why I wonder
>>> how to re-conciliate this with showing low-power in e.g. the
>>> GNOME shell system men. Because in essence even if the battery
>>> is low the system will still go full-throttle when confronted
>>> with a heavy workload.
>>>
>>> So selecting low-power would result in the screen-dimming which
>>> I think is part of that, but the CPU's max power-consumption won't
>>> get limited as it would when platform-profiles are supported.
>>>
>>> So I guess this is indeed very much like how p-p-d behaves
>>> on unsupported hw...
>>>
>>> ###
>>>
>>> As mentioned I guess one option would be for CnQF to
>>> still register a platform_profile provider and then in
>>> balanced mode do its CnQF thing and in low-power mode
>>> disable CnQF and apply the static-slider low-power settings
>>> I think that that would work best from things actual
>>> working in a way I would expect the avarage end-user to
>>> expect things to work.
>>>
>>> So p-p-d would then still see platform-profile support
>>> in CnQF mode but with only low-power + balanced advertised.
>>>
>>> Bastien would that work for you?
>>>
>>> AMD folks would that also work for you ?
>>
>> If we go with the above proposal it would become very identical to what
>> is being done with automode (expect the extra "turbo" mode and not
>> having a AMT event).
> 
> Yes I think that the AMT mode, where the more dynamic behavior os
> only done in balanced mode and low-power is still very much a low
> power-mode (and performance is always tuned for permance) makes
> a lot more sense from an enduser pov. Then the slider still actually
> works as expected and in the default balanced mode users will get
> the benefits of the new CnQF behavior / feature.
> 
>> This would need some amount of discussion with our
>> windows folks also to know what they think about it.
> 
> Ok.
> 

OK. I get it. Your preference is to have CnQF getting ON only when

1. BIOS advertises CnQF is "supported" and/or
2. Sysfs knob is set to ON. and
3. the static-slider (platform_profile) is set to "balanced"

In rest of the cases, (low-power or performance) the control would still
remain with the static-slider so that, user can make his own choices.

If that's the case, let me have a word with the windows team also on how
we can have user experiences same across OSes and come back.

Thank you for your feedback.

Thanks,
Shyam

> Regards,
> 
> Hans
> 

Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede]

Hi,

On 9/8/22 12:08, Shyam Sundar S K wrote:
> 
> 
> On 9/7/2022 8:18 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/6/22 11:53, Shyam Sundar S K wrote:
>>> Hi Hans,
>>>
>>> Apologies for the delay in responding to this thread. Some responses below.
>>
>> No worries.
>>
>>> On 9/1/2022 6:14 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 9/1/22 14:24, Bastien Nocera wrote:
>>>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 8/23/22 12:29, Shyam Sundar S K wrote:
>>>>>>> In this series, support for following features has been added.
>>>>>>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider,
>>>>>>>   where the system power can be boosted or throttled independent
>>>>>>>   of the selected slider position.
>>>>>>> - On the fly, the CnQF can be turned on/off via a sysfs knob.
>>>>>>
>>>>>> Thank you. I think that before doing a more in detail review
>>>>>> we first need to agree on the userspace interactions here.
>>>>>>
>>>>>> I've added Bastien, the power-profiles-daemon maintainer
>>>>>> to the Cc for this.
>>>>>>
>>>>>> From a quick peek at the patches I see that currently they do
>>>>>> the following:
>>>>>>
>>>>>> Probe time:
>>>>>> -----------
>>>>>>
>>>>>> 1. If static slider (classic /sys/firmware/acpi/platform_profile)
>>>>>> is available register as a platform_profile provider
>>>>>>
>>>>>> 2. Query if the BIOS tells us that CnQF should be enable by
>>>>>> default if yes then unregister the platform_profile provider
>>>>>> and enable CnQF
>>>>>>
>>>>>>
>>>>>> Run time:
>>>>>> ---------
>>>>>>
>>>>>> Allow turning CnQF on/off by writing a sysfs attribute for this.
>>>>>>
>>>>>> 1. When CnQF gets enabled unregister the platform_profile provider
>>>>>>
>>>>>> 2. When CnQF gets disabled restore the last set profile and
>>>>>> register the platform_profile provider
>>>>>>
>>>>>>
>>>>>> Questions/remarks:
>>>>>>
>>>>>> 1. If you look at 1. and 2. under "Probe time", you will see that
>>>>>> when the BIOS requests to have CnQF enabled by default that
>>>>>> userspace will then still shortly see a platform_profile
>>>>>> provider. This must be fixed IMHO by checking whether to do
>>>>>> CnQF by default or not before the initial register call.
>>>>>>
>>>>>> 2. What about low-power scenarios ? Currently power-profiles-daemon
>>>>>> will always advertise a low-power mode even when there is no
>>>>>> platform-profile support, since this is also a hint for other
>>>>>> parts of the system to try and conserve power. But when this
>>>>>> mode is enabled we really want the system to also behave as
>>>>>> if the old static slider mode is active and set to low-power.
>>>>>>
>>>>>> Some ideas:
>>>>>> a) maybe still have the amd-pmf code register a (different)
>>>>>> platform_profile provider whn in CnQF mode and have it only
>>>>>> advertise low-power
>>>>>>
>>>>>> b) teach power-profiles-daemon about CnQF and have it
>>>>>> disable CnQF when entering low-power mode?
>>>>>>
>>>>>> c) make the CnQF code in PMF take the charge level into
>>>>>> account and have it not go "full throttle" when the chare
>>>>>> is below say 25% ?
>>>>>>
>>>>>> 3. Bastien, can power-profiles-daemon deal with
>>>>>> /sys/firmware/acpi/platform_profile disappearing or
>>>>>> appearing while it is running?
>>>>>
>>>>> No, it doesn't.
>>>>>
>>>>> It expects the platform_profile file to be available on startup, at
>>>>> worse with the choices not yet filled in. It doesn't handle the
>>>>> platform_profile file going away, it doesn't handle the
>>>>> platform_profile_choices file changing after it's been initially
>>>>> filled in, and it doesn't support less than one power profile being
>>>>> made available, and only supports hiding the performance profile if
>>>>> the platform doesn't support it.
>>>>
>>>> Ok, so this means that if we go with these changes as currently
>>>> proposed that if a user uses the sysfs file to turn CnQF on/off
>>>> they will need to restart power-profile-daemon.
>>>>
>>>> I think that that is acceptable given that the user needs to manually
>>>> poke things anyway. We should probably document this in the documentation
>>>> for the sysfs attribute (as well as in newer versions of the p-p-d
>>>> docs/README).
>>>>
>>>>> Some of those things we could change/fix, some other things will not.
>>>>> If the platform_profile_choices file only contained a single item,
>>>>> then power-profiles-daemon would just export the "low-power" and
>>>>> "balanced" profiles to user-space, as it does on unsupported hardware.
>>>>
>>>> Right.
>>>>
>>>>> The profiles in power-profiles-daemon are supposed to show the user
>>>>> intent, which having a single setting would effectively nullify.
>>>>
>>>> Yes that was my understanding too.
>>>>
>>>>> It's unclear to me how CnQF takes user intent into account (it's also
>>>>> unclear to me how that's a low-power setting rather than a combination
>>>>> of the existing cool and quiet settings).
>>>>
>>>> AMD folks, please correct me if any of the below is wrong:
>>>>
>>>> AFAIK even though it is called CnQF it is more like auto-profile
>>>> selection and as such does not take user intent into account
>>>> at all.
>>>
>>> Yes, You are right. Below is a brief note on how CnQF was designed.
>>>
>>> 1)CnQF – Cool And Quiet Framework - It’s an extension of the static
>>> slider concept wherein PMF dynamically manages system power limits and
>>> fan policy based on system power trends.
>>>
>>> 2)OEM can opt into the feature by defining the CnQF BIOS ACPI method.
>>>
>>> 3)Static slider control and CnQF are mutually exclusive.
>>>
>>> 4)CnQF supports up to 4 modes of operation – Turbo, Performance,
>>> Balanced and Quiet.
>>>
>>> - It can be configured for AC and DC distinctly.
>>> - PMF driver calculates the moving average of system power and switches
>>> the mode of operation.
>>>     *If system power is limited to the threshold of the current mode,
>>> move to the next higher mode
>>>     *If system power is not limited to the threshold of the current
>>> mode, reduce the power budget by moving to the next lower mode.
>>>
>>> 5)CnQF feature control is through Radeon SW (a GUI based tool on Windows)
>>>
>>> To match the behavior on Windows, we kept a sysfs node to turn on/off
>>> the CnQF on the fly like the way it can be done the windows side with
>>> the Radeon SW tool. If you think that having as a module param makes
>>> more sense, I am open to make the change and send a v2.
>>>
>>> Like I mentioned above, on Windows the static slider is absoultely dummy
>>> when the user goes on turns on the CnQF from Radeon SW tool. Based on
>>> the review remarks on the earlier series, we tried to
>>> register/de-register to platform_profile, as per sysfs input (like
>>> setting up and tearing down to platform_profile).
>>>
>>> The Difference between Auto-mode (for thinkpads) and CnQF(for others) is
>>> that:
>>>
>>> - Automode gets turned on only when the slider position is set to
>>> "balanced" in the platform_profile and
>>> - a corresponding AMT ON event is triggered.
>>> - it has 3 internal modes quiet, balanced, performance
>>>
>>> But for CnQF,
>>>
>>> - it is independent of the slider position and there are no ACPI events
>>> for it to get kicked in.
>>> - There are two seperate ACPI methods for AC and DC to get the
>>> corresponding thermal values.
>>> - it has 4 internal modes quiet, balanced, performance, turbo
>>>
>>>
>>> There is already a WIP feature called "policy builder" where the OEMs
>>> can build custom policies, which includes looking at the battery
>>> percentages and making thermal optimizations accordingly. But this was
>>> not taken into consideration while designing the CnQF on windows too. If
>>> we bring in this change for Linux, there maybe differences in the way
>>> the same feature behaves "differently" across OSes.
>>>
>>> Like you mentioned the usecase, where just a compilation can end up in
>>> battery drain if not connected to AC power.
>>
>> Thanks for the explanation above.
>>
>>> Can we not have a
>>> documentation update saying it is advised to turn "off" CnQF when
>>> battery % goes below a certain level?
>>
>> So we would need to document that the user needs to poke
>> the sysfs file when the battery is low ?  That seems very user
>> unfriendly.
>>
>> And also don't want power-performance-daemon to need to know about
>> this AMD specific sysfs knob. That is why we have the standardized
>> platform_profile userspace API.
>>
>>> That way, the end user experiences
>>> across Linux and Windows remains the same.
>>
>> I can understand that you want to keep things the same. If I've
>> read the above correctly then currently the user experience under
>> Windows is that the slider in Windows has been turned into a
>> dummy slider which does not do anything.
>>
>> That IMHO is quite a poor user-experience esp. when users want
>> their battery to last longer because they are going to be e.g.
>> on the road the entire day.
>>
>>>> It looks at the workload over a somewhat longer time period (say
>>>> 5 minutes or so I guess?) and then if that consistently has been
>>>> quite high, it will select something similar to performance.
>>>
>>> Right. The switch time would be dependent on the "time constant" values
>>> set in the BIOS  which is configurable to the OEMs.
>>>
>>>>
>>>> Where as for a more mixed workload it will select balanced and for
>>>> a mostly idle machine it will select low-power.
>>>>
>>>> I guess this auto feature is best treated the same as unsupported hw.
>>>>
>>>>> (it's also
>>>>> unclear to me how that's a low-power setting rather than a combination
>>>>> of the existing cool and quiet settings).
>>>>
>>>> Even though it is called cool and quiet AFAIK it won't be all that
>>>> cool and quiet when running a heavy workload. Which is why I wonder
>>>> how to re-conciliate this with showing low-power in e.g. the
>>>> GNOME shell system men. Because in essence even if the battery
>>>> is low the system will still go full-throttle when confronted
>>>> with a heavy workload.
>>>>
>>>> So selecting low-power would result in the screen-dimming which
>>>> I think is part of that, but the CPU's max power-consumption won't
>>>> get limited as it would when platform-profiles are supported.
>>>>
>>>> So I guess this is indeed very much like how p-p-d behaves
>>>> on unsupported hw...
>>>>
>>>> ###
>>>>
>>>> As mentioned I guess one option would be for CnQF to
>>>> still register a platform_profile provider and then in
>>>> balanced mode do its CnQF thing and in low-power mode
>>>> disable CnQF and apply the static-slider low-power settings
>>>> I think that that would work best from things actual
>>>> working in a way I would expect the avarage end-user to
>>>> expect things to work.
>>>>
>>>> So p-p-d would then still see platform-profile support
>>>> in CnQF mode but with only low-power + balanced advertised.
>>>>
>>>> Bastien would that work for you?
>>>>
>>>> AMD folks would that also work for you ?
>>>
>>> If we go with the above proposal it would become very identical to what
>>> is being done with automode (expect the extra "turbo" mode and not
>>> having a AMT event).
>>
>> Yes I think that the AMT mode, where the more dynamic behavior os
>> only done in balanced mode and low-power is still very much a low
>> power-mode (and performance is always tuned for permance) makes
>> a lot more sense from an enduser pov. Then the slider still actually
>> works as expected and in the default balanced mode users will get
>> the benefits of the new CnQF behavior / feature.
>>
>>> This would need some amount of discussion with our
>>> windows folks also to know what they think about it.
>>
>> Ok.
>>
> 
> OK. I get it. Your preference is to have CnQF getting ON only when
> 
> 1. BIOS advertises CnQF is "supported" and/or
> 2. Sysfs knob is set to ON. and
> 3. the static-slider (platform_profile) is set to "balanced"
> 
> In rest of the cases, (low-power or performance) the control would still
> remain with the static-slider so that, user can make his own choices.

Yes that is correct.

> If that's the case, let me have a word with the windows team also on how
> we can have user experiences same across OSes and come back.

Great, thank you.

Regards,

Hans