[v1,13/15] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode

Commit Message

Shyam Sundar S K July 12, 2022, 2:58 p.m. UTC

The transition to auto-mode happens when the PMF driver receives
AMT (Auto Mode transition) event. transition logic will reside in the
PMF driver but the events would come from other supported drivers[1].

The thermal parameters would vary between when a performance "on-lap" mode
is detected and versus when not. The CQL event would get triggered from
other drivers, so that PMF driver would adjust the system thermal config
based on the ACPI inputs.

OEMs can control whether or not to enable AMT or CQL via other supported
drivers[1] but the actual transition logic resides in the AMD PMF driver.
When an AMT event is received the automatic mode transition RAPL algorithm
will run. When a CQL event is received an performance "on-lap" mode will
be enabled and thermal parameters will be adjusted accordingly.

[1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=755b249250df1b612d982f3b702c831b26ecdf73

Signed-off-by: Shyam Sundar S K <[e-mail]>
---
 drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
 drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
 drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
 drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
 drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
 5 files changed, 156 insertions(+), 7 deletions(-)

Comments

Hans de Goede July 27, 2022, 9:33 p.m. UTC | #1

Hi,

On 7/12/22 16:58, Shyam Sundar S K wrote:
> The transition to auto-mode happens when the PMF driver receives
> AMT (Auto Mode transition) event. transition logic will reside in the
> PMF driver but the events would come from other supported drivers[1].
> 
> The thermal parameters would vary between when a performance "on-lap" mode
> is detected and versus when not. The CQL event would get triggered from
> other drivers, so that PMF driver would adjust the system thermal config
> based on the ACPI inputs.
> 
> OEMs can control whether or not to enable AMT or CQL via other supported
> drivers[1] but the actual transition logic resides in the AMD PMF driver.
> When an AMT event is received the automatic mode transition RAPL algorithm
> will run. When a CQL event is received an performance "on-lap" mode will
> be enabled and thermal parameters will be adjusted accordingly.
> 
> [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=755b249250df1b612d982f3b702c831b26ecdf73
> 
> Signed-off-by: Shyam Sundar S K <[e-mail]>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>  drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>  drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>  drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>  drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>  5 files changed, 156 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index e9f33e61659f..4bde86aeafa0 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -12,6 +12,8 @@
>  #include "pmf.h"
>  
>  #define APMF_METHOD "\\_SB_.PMF_.APMF"
> +#define APMF_CQL_NOTIFICATION	2
> +#define APMF_AMT_NOTIFICATION	3
>  
>  static unsigned long supported_func;
>  
> @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>  {
>  	func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>  	func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
> +	func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>  	func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>  	func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>  	func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  	return err;
>  }
>  
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
> +{
> +	union acpi_object *info;
> +	size_t size;
> +	int err = 0;
> +
> +	info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
> +	if (!info)
> +		return -EIO;
> +
> +	size = *(u16 *)info->buffer.pointer;
> +
> +	if (size < sizeof(union acpi_object)) {
> +		pr_err("PMF: buffer too small %zu\n", size);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	size = min(sizeof(*req), size);
> +	memset(req, 0, sizeof(*req));
> +	memcpy(req, info->buffer.pointer, size);
> +
> +	pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
> +		 req->pending_req, req->cql_event, req->amt_event);
> +
> +out:
> +	kfree(info);
> +	return err;
> +}
> +

Please use apmf_if_call_store_buffer() for this.

>  static acpi_handle apmf_if_probe_handle(void)
>  {
>  	acpi_handle handle = NULL;
> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>  	return handle;
>  }
>  
> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
> +{
> +	struct amd_pmf_dev *pmf_dev = data;
> +	struct apmf_if *apmf_if = pmf_dev->apmf_if;
> +	int ret;
> +
> +	if (apmf_if->func.sbios_requests) {
> +		struct apmf_sbios_req req;
> +
> +		ret = apmf_get_sbios_requests(apmf_if, &req);
> +		if (ret) {
> +			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
> +			return;
> +		}
> +		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
> +			pr_debug("PMF: AMT is supported and notifications %s\n",
> +				 req.amt_event ? "Enabled" : "Disabled");
> +			if (req.amt_event)
> +				pmf_dev->is_amt_event = true;
> +			else
> +				pmf_dev->is_amt_event = !!req.amt_event;

Replace the entire if-else with:

			pmf_dev->is_amt_event = !!req.amt_event;

please.


> +		}
> +
> +		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
> +			pr_debug("PMF: CQL is supported and notifications %s\n",
> +				 req.cql_event ? "Enabled" : "Disabled");
> +			if (req.cql_event)
> +				pmf_dev->is_cql_event = true;
> +			else
> +				pmf_dev->is_cql_event = !!req.cql_event;

Replace the entire if-else with:

			pmf_dev->is_cql_event = !!req.cql_event;

please.


> +
> +			/* update the target mode information */
> +			amd_pmf_update_2_cql(pmf_dev);
> +		}
> +	}
> +}
> +
>  void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> +
>  	if (pmf_dev->apmf_if->func.sbios_heartbeat)
>  		cancel_delayed_work_sync(&pmf_dev->heart_beat);
> +
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
> +		acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
> +					   apmf_event_handler);
>  }
>  
>  int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>  	struct apmf_notification_cfg *n;
>  	acpi_handle apmf_if_handle;
>  	struct apmf_if *apmf_if;
> -	int ret;
> +	int ret, status;
>  
>  	apmf_if_handle = apmf_if_probe_handle();
>  	if (!apmf_if_handle)
> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  		schedule_delayed_work(&pmf_dev->heart_beat, 0);
>  	}
>  
> +	/* Install the APMF Notify handler */
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
> +		status = acpi_install_notify_handler(ahandle,
> +						     ACPI_ALL_NOTIFY,
> +						     apmf_event_handler, pmf_dev);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(pmf_dev->dev, "failed to install notify handler\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  out:
>  	return ret;
>  }
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 954fde25e71e..a85ef4b95cdb 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	bool update = false;
>  	int i, j;
>  
> +	if (!dev->amt_running && dev->is_amt_event) {
> +		dev_dbg(dev->dev, "setting AMT thermals\n");
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +		dev->amt_running = true;
> +		return;
> +	}
> +
>  	/* Get the average moving average computed by auto mode algorithm */
>  	avg_power = amd_pmf_get_moving_avg(socket_power);
>  
> @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	}
>  }
>  
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
> +{
> +	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
> +			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
> +	if ((config_store.current_mode == AUTO_PERFORMANCE ||
> +	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
> +	    config_store.current_mode !=
> +	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
> +		config_store.current_mode =
> +				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +	}
> +	dev_dbg(dev->dev, "updated CQL thermals\n");
> +}
> +
>  static void amd_pmf_get_power_threshold(void)
>  {
>  	config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 2b03ae1ad37f..eba8f0d79a62 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>  	}
>  }
>  
> -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>  {
>  	struct cnqf_tran_params *tp;
>  	int src, i, j, index = 0;
> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>  	}
>  
>  	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
> -		config_store.trans_param[src][i].timer += time_lapsed_ms;
> +		config_store.trans_param[src][i].timer += time_elapsed_ms;

This should be in squashed into the patch introducing the type. Each patch should
compile without errors individually (on top of previous patches obviously).

>  		config_store.trans_param[src][i].total_power += socket_power;
>  		config_store.trans_param[src][i].count++;
>  
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 674ddf599135..2a3dacfb2005 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	enum platform_profile_option current_profile;
>  	ktime_t time_elapsed_ms;
>  	int socket_power;
> +	u8 mode;
>  
>  	/* Get the current profile information */
>  	platform_profile_get(&current_profile);
>  
> +	if (!dev->is_amt_event)
> +		dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
> +			dev->is_amt_event ? "Enabled" : "Disabled");
> +
>  	/* Transfer table contents */
>  	memset(&dev->m_table, 0, sizeof(dev->m_table));
>  	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>  
>  	if (current_profile == PLATFORM_PROFILE_BALANCED) {
> -		/* Apply the Auto Mode transition */
> -		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		if (dev->is_amt_event) {
> +			/* Apply the Auto Mode transition */
> +			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		} else if (!dev->is_amt_event && dev->amt_running) {
> +			pr_debug("resetting AMT thermals\n");
> +			mode = amd_pmf_get_pprof_modes(dev);
> +			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
> +			dev->amt_running = false;
> +		}
> +	} else {
> +		dev->amt_running = false;
>  	}
>  
>  	if (dev->cnqf_feat) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 0532f49e9613..9ae9812353cd 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -17,6 +17,7 @@
>  /* APMF Functions */
>  #define APMF_FUNC_VERIFY_INTERFACE			0
>  #define APMF_FUNC_GET_SYS_PARAMS			1
> +#define APMF_FUNC_SBIOS_REQUESTS			2
>  #define APMF_FUNC_SBIOS_HEARTBEAT			4
>  #define APMF_FUNC_AUTO_MODE					5
>  #define APMF_FUNC_SET_FAN_IDX				7
> @@ -51,6 +52,7 @@
>  /* AMD PMF BIOS interfaces */
>  struct apmf_if_functions {
>  	bool system_params;
> +	bool sbios_requests;
>  	bool sbios_heartbeat;
>  	bool auto_mode_def;
>  	bool fan_table_idx;
> @@ -84,6 +86,24 @@ struct apmf_system_params {
>  	u32 heartbeat_int;
>  } __packed;
>  
> +struct apmf_sbios_req {
> +	u16 size;
> +	u32 pending_req;
> +	u8 rsd;
> +	u8 cql_event;
> +	u8 amt_event;
> +	u32 fppt;
> +	u32 sppt;
> +	u32 fppt_apu_only;
> +	u32 spl;
> +	u32 stt_min_limit;
> +	u8 skin_temp_apu;
> +	u8 skin_temp_hs2;
> +	u8 dps_enable;
> +	u32 custom_policy_1;
> +	u32 custom_policy_2;
> +} __packed;
> +
>  struct apmf_fan_idx {
>  	u16 size;
>  	u8 fan_ctl_mode;
> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>  #endif /* CONFIG_DEBUG_FS */
>  	bool cnqf_feat;
>  	bool cnqf_running;
> +	bool is_amt_event;
> +	bool is_cql_event;
> +	bool amt_running;
>  };
>  
>  struct apmf_sps_prop_granular {
> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>  /* Auto Mode Layer */
>  void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>  int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>  void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>  void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  /* CnQF Layer */
>  int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  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);
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  #endif /* PMF_H */

Regards,

Hans

Hans de Goede July 27, 2022, 9:44 p.m. UTC | #2

Hi,

On 7/12/22 16:58, Shyam Sundar S K wrote:
> The transition to auto-mode happens when the PMF driver receives
> AMT (Auto Mode transition) event. transition logic will reside in the
> PMF driver but the events would come from other supported drivers[1].
> 
> The thermal parameters would vary between when a performance "on-lap" mode
> is detected and versus when not. The CQL event would get triggered from
> other drivers, so that PMF driver would adjust the system thermal config
> based on the ACPI inputs.
> 
> OEMs can control whether or not to enable AMT or CQL via other supported
> drivers[1] but the actual transition logic resides in the AMD PMF driver.
> When an AMT event is received the automatic mode transition RAPL algorithm
> will run. When a CQL event is received an performance "on-lap" mode will
> be enabled and thermal parameters will be adjusted accordingly.
> 
> [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=755b249250df1b612d982f3b702c831b26ecdf73
> 
> Signed-off-by: Shyam Sundar S K <[e-mail]>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>  drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>  drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>  drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>  drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>  5 files changed, 156 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index e9f33e61659f..4bde86aeafa0 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -12,6 +12,8 @@
>  #include "pmf.h"
>  
>  #define APMF_METHOD "\\_SB_.PMF_.APMF"
> +#define APMF_CQL_NOTIFICATION	2
> +#define APMF_AMT_NOTIFICATION	3
>  
>  static unsigned long supported_func;
>  
> @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>  {
>  	func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>  	func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
> +	func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>  	func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>  	func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>  	func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  	return err;
>  }
>  
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
> +{
> +	union acpi_object *info;
> +	size_t size;
> +	int err = 0;
> +
> +	info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
> +	if (!info)
> +		return -EIO;
> +
> +	size = *(u16 *)info->buffer.pointer;
> +
> +	if (size < sizeof(union acpi_object)) {
> +		pr_err("PMF: buffer too small %zu\n", size);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	size = min(sizeof(*req), size);
> +	memset(req, 0, sizeof(*req));
> +	memcpy(req, info->buffer.pointer, size);
> +
> +	pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
> +		 req->pending_req, req->cql_event, req->amt_event);
> +
> +out:
> +	kfree(info);
> +	return err;
> +}
> +
>  static acpi_handle apmf_if_probe_handle(void)
>  {
>  	acpi_handle handle = NULL;
> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>  	return handle;
>  }
>  
> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
> +{
> +	struct amd_pmf_dev *pmf_dev = data;
> +	struct apmf_if *apmf_if = pmf_dev->apmf_if;
> +	int ret;
> +
> +	if (apmf_if->func.sbios_requests) {
> +		struct apmf_sbios_req req;
> +
> +		ret = apmf_get_sbios_requests(apmf_if, &req);
> +		if (ret) {
> +			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
> +			return;
> +		}
> +		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
> +			pr_debug("PMF: AMT is supported and notifications %s\n",
> +				 req.amt_event ? "Enabled" : "Disabled");
> +			if (req.amt_event)
> +				pmf_dev->is_amt_event = true;
> +			else
> +				pmf_dev->is_amt_event = !!req.amt_event;
> +		}
> +
> +		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
> +			pr_debug("PMF: CQL is supported and notifications %s\n",
> +				 req.cql_event ? "Enabled" : "Disabled");
> +			if (req.cql_event)
> +				pmf_dev->is_cql_event = true;
> +			else
> +				pmf_dev->is_cql_event = !!req.cql_event;
> +
> +			/* update the target mode information */
> +			amd_pmf_update_2_cql(pmf_dev);
> +		}
> +	}
> +}
> +
>  void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> +
>  	if (pmf_dev->apmf_if->func.sbios_heartbeat)
>  		cancel_delayed_work_sync(&pmf_dev->heart_beat);
> +
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
> +		acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
> +					   apmf_event_handler);
>  }
>  
>  int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>  	struct apmf_notification_cfg *n;
>  	acpi_handle apmf_if_handle;
>  	struct apmf_if *apmf_if;
> -	int ret;
> +	int ret, status;
>  
>  	apmf_if_handle = apmf_if_probe_handle();
>  	if (!apmf_if_handle)
> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  		schedule_delayed_work(&pmf_dev->heart_beat, 0);
>  	}
>  
> +	/* Install the APMF Notify handler */
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
> +		status = acpi_install_notify_handler(ahandle,
> +						     ACPI_ALL_NOTIFY,
> +						     apmf_event_handler, pmf_dev);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(pmf_dev->dev, "failed to install notify handler\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  out:
>  	return ret;
>  }
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 954fde25e71e..a85ef4b95cdb 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	bool update = false;
>  	int i, j;
>  
> +	if (!dev->amt_running && dev->is_amt_event) {
> +		dev_dbg(dev->dev, "setting AMT thermals\n");
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +		dev->amt_running = true;
> +		return;
> +	}
> +
>  	/* Get the average moving average computed by auto mode algorithm */
>  	avg_power = amd_pmf_get_moving_avg(socket_power);
>  
> @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	}
>  }
>  
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
> +{
> +	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
> +			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
> +	if ((config_store.current_mode == AUTO_PERFORMANCE ||
> +	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
> +	    config_store.current_mode !=
> +	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
> +		config_store.current_mode =
> +				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +	}
> +	dev_dbg(dev->dev, "updated CQL thermals\n");
> +}
> +
>  static void amd_pmf_get_power_threshold(void)
>  {
>  	config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 2b03ae1ad37f..eba8f0d79a62 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>  	}
>  }
>  
> -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>  {
>  	struct cnqf_tran_params *tp;
>  	int src, i, j, index = 0;
> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>  	}
>  
>  	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
> -		config_store.trans_param[src][i].timer += time_lapsed_ms;
> +		config_store.trans_param[src][i].timer += time_elapsed_ms;
>  		config_store.trans_param[src][i].total_power += socket_power;
>  		config_store.trans_param[src][i].count++;
>  
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 674ddf599135..2a3dacfb2005 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	enum platform_profile_option current_profile;
>  	ktime_t time_elapsed_ms;
>  	int socket_power;
> +	u8 mode;
>  
>  	/* Get the current profile information */
>  	platform_profile_get(&current_profile);
>  
> +	if (!dev->is_amt_event)
> +		dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
> +			dev->is_amt_event ? "Enabled" : "Disabled");
> +
>  	/* Transfer table contents */
>  	memset(&dev->m_table, 0, sizeof(dev->m_table));
>  	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>  
>  	if (current_profile == PLATFORM_PROFILE_BALANCED) {

Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?

Can you please explain the expected interactions between thinkpad_acpi and
this code here a bit more ?

Since we now only call amd_pmf_trans_automode() based on the AMT flag and
that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
to be cleared when the profile is not balanced and can we thus not just drop
the current_profile == PLATFORM_PROFILE_BALANCED check all together?

It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
then enable AMT, else disable it, logic belongs inside thinkpad_acpi
and not here?

Regards,

Hans

> -		/* Apply the Auto Mode transition */
> -		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		if (dev->is_amt_event) {
> +			/* Apply the Auto Mode transition */
> +			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		} else if (!dev->is_amt_event && dev->amt_running) {
> +			pr_debug("resetting AMT thermals\n");
> +			mode = amd_pmf_get_pprof_modes(dev);
> +			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
> +			dev->amt_running = false;
> +		}
> +	} else {
> +		dev->amt_running = false;
>  	}
>  
>  	if (dev->cnqf_feat) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 0532f49e9613..9ae9812353cd 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -17,6 +17,7 @@
>  /* APMF Functions */
>  #define APMF_FUNC_VERIFY_INTERFACE			0
>  #define APMF_FUNC_GET_SYS_PARAMS			1
> +#define APMF_FUNC_SBIOS_REQUESTS			2
>  #define APMF_FUNC_SBIOS_HEARTBEAT			4
>  #define APMF_FUNC_AUTO_MODE					5
>  #define APMF_FUNC_SET_FAN_IDX				7
> @@ -51,6 +52,7 @@
>  /* AMD PMF BIOS interfaces */
>  struct apmf_if_functions {
>  	bool system_params;
> +	bool sbios_requests;
>  	bool sbios_heartbeat;
>  	bool auto_mode_def;
>  	bool fan_table_idx;
> @@ -84,6 +86,24 @@ struct apmf_system_params {
>  	u32 heartbeat_int;
>  } __packed;
>  
> +struct apmf_sbios_req {
> +	u16 size;
> +	u32 pending_req;
> +	u8 rsd;
> +	u8 cql_event;
> +	u8 amt_event;
> +	u32 fppt;
> +	u32 sppt;
> +	u32 fppt_apu_only;
> +	u32 spl;
> +	u32 stt_min_limit;
> +	u8 skin_temp_apu;
> +	u8 skin_temp_hs2;
> +	u8 dps_enable;
> +	u32 custom_policy_1;
> +	u32 custom_policy_2;
> +} __packed;
> +
>  struct apmf_fan_idx {
>  	u16 size;
>  	u8 fan_ctl_mode;
> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>  #endif /* CONFIG_DEBUG_FS */
>  	bool cnqf_feat;
>  	bool cnqf_running;
> +	bool is_amt_event;
> +	bool is_cql_event;
> +	bool amt_running;
>  };
>  
>  struct apmf_sps_prop_granular {
> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>  /* Auto Mode Layer */
>  void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>  int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>  void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>  void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  /* CnQF Layer */
>  int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  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);
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  #endif /* PMF_H */

Hans de Goede July 27, 2022, 9:46 p.m. UTC | #3

<resend with Cc list fixed>

Hi,

On 7/12/22 16:58, Shyam Sundar S K wrote:
> The transition to auto-mode happens when the PMF driver receives
> AMT (Auto Mode transition) event. transition logic will reside in the
> PMF driver but the events would come from other supported drivers[1].
> 
> The thermal parameters would vary between when a performance "on-lap" mode
> is detected and versus when not. The CQL event would get triggered from
> other drivers, so that PMF driver would adjust the system thermal config
> based on the ACPI inputs.
> 
> OEMs can control whether or not to enable AMT or CQL via other supported
> drivers[1] but the actual transition logic resides in the AMD PMF driver.
> When an AMT event is received the automatic mode transition RAPL algorithm
> will run. When a CQL event is received an performance "on-lap" mode will
> be enabled and thermal parameters will be adjusted accordingly.
> 
> [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=755b249250df1b612d982f3b702c831b26ecdf73
> 
> Signed-off-by: Shyam Sundar S K <[e-mail]>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>  drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>  drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>  drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>  drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>  5 files changed, 156 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index e9f33e61659f..4bde86aeafa0 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -12,6 +12,8 @@
>  #include "pmf.h"
>  
>  #define APMF_METHOD "\\_SB_.PMF_.APMF"
> +#define APMF_CQL_NOTIFICATION	2
> +#define APMF_AMT_NOTIFICATION	3
>  
>  static unsigned long supported_func;
>  
> @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>  {
>  	func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>  	func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
> +	func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>  	func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>  	func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>  	func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  	return err;
>  }
>  
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
> +{
> +	union acpi_object *info;
> +	size_t size;
> +	int err = 0;
> +
> +	info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
> +	if (!info)
> +		return -EIO;
> +
> +	size = *(u16 *)info->buffer.pointer;
> +
> +	if (size < sizeof(union acpi_object)) {
> +		pr_err("PMF: buffer too small %zu\n", size);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	size = min(sizeof(*req), size);
> +	memset(req, 0, sizeof(*req));
> +	memcpy(req, info->buffer.pointer, size);
> +
> +	pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
> +		 req->pending_req, req->cql_event, req->amt_event);
> +
> +out:
> +	kfree(info);
> +	return err;
> +}
> +
>  static acpi_handle apmf_if_probe_handle(void)
>  {
>  	acpi_handle handle = NULL;
> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>  	return handle;
>  }
>  
> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
> +{
> +	struct amd_pmf_dev *pmf_dev = data;
> +	struct apmf_if *apmf_if = pmf_dev->apmf_if;
> +	int ret;
> +
> +	if (apmf_if->func.sbios_requests) {
> +		struct apmf_sbios_req req;
> +
> +		ret = apmf_get_sbios_requests(apmf_if, &req);
> +		if (ret) {
> +			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
> +			return;
> +		}
> +		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
> +			pr_debug("PMF: AMT is supported and notifications %s\n",
> +				 req.amt_event ? "Enabled" : "Disabled");
> +			if (req.amt_event)
> +				pmf_dev->is_amt_event = true;
> +			else
> +				pmf_dev->is_amt_event = !!req.amt_event;
> +		}
> +
> +		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
> +			pr_debug("PMF: CQL is supported and notifications %s\n",
> +				 req.cql_event ? "Enabled" : "Disabled");
> +			if (req.cql_event)
> +				pmf_dev->is_cql_event = true;
> +			else
> +				pmf_dev->is_cql_event = !!req.cql_event;
> +
> +			/* update the target mode information */
> +			amd_pmf_update_2_cql(pmf_dev);
> +		}
> +	}
> +}
> +
>  void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> +
>  	if (pmf_dev->apmf_if->func.sbios_heartbeat)
>  		cancel_delayed_work_sync(&pmf_dev->heart_beat);
> +
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
> +		acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
> +					   apmf_event_handler);
>  }
>  
>  int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  {
> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>  	struct apmf_notification_cfg *n;
>  	acpi_handle apmf_if_handle;
>  	struct apmf_if *apmf_if;
> -	int ret;
> +	int ret, status;
>  
>  	apmf_if_handle = apmf_if_probe_handle();
>  	if (!apmf_if_handle)
> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>  		schedule_delayed_work(&pmf_dev->heart_beat, 0);
>  	}
>  
> +	/* Install the APMF Notify handler */
> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
> +		status = acpi_install_notify_handler(ahandle,
> +						     ACPI_ALL_NOTIFY,
> +						     apmf_event_handler, pmf_dev);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(pmf_dev->dev, "failed to install notify handler\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  out:
>  	return ret;
>  }
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 954fde25e71e..a85ef4b95cdb 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	bool update = false;
>  	int i, j;
>  
> +	if (!dev->amt_running && dev->is_amt_event) {
> +		dev_dbg(dev->dev, "setting AMT thermals\n");
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +		dev->amt_running = true;
> +		return;
> +	}
> +
>  	/* Get the average moving average computed by auto mode algorithm */
>  	avg_power = amd_pmf_get_moving_avg(socket_power);
>  
> @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  	}
>  }
>  
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
> +{
> +	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
> +			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
> +	if ((config_store.current_mode == AUTO_PERFORMANCE ||
> +	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
> +	    config_store.current_mode !=
> +	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
> +		config_store.current_mode =
> +				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
> +	}
> +	dev_dbg(dev->dev, "updated CQL thermals\n");
> +}
> +
>  static void amd_pmf_get_power_threshold(void)
>  {
>  	config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 2b03ae1ad37f..eba8f0d79a62 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>  	}
>  }
>  
> -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>  {
>  	struct cnqf_tran_params *tp;
>  	int src, i, j, index = 0;
> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>  	}
>  
>  	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
> -		config_store.trans_param[src][i].timer += time_lapsed_ms;
> +		config_store.trans_param[src][i].timer += time_elapsed_ms;
>  		config_store.trans_param[src][i].total_power += socket_power;
>  		config_store.trans_param[src][i].count++;
>  
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 674ddf599135..2a3dacfb2005 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	enum platform_profile_option current_profile;
>  	ktime_t time_elapsed_ms;
>  	int socket_power;
> +	u8 mode;
>  
>  	/* Get the current profile information */
>  	platform_profile_get(&current_profile);
>  
> +	if (!dev->is_amt_event)
> +		dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
> +			dev->is_amt_event ? "Enabled" : "Disabled");
> +
>  	/* Transfer table contents */
>  	memset(&dev->m_table, 0, sizeof(dev->m_table));
>  	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>  	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>  
>  	if (current_profile == PLATFORM_PROFILE_BALANCED) {

Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?

Can you please explain the expected interactions between thinkpad_acpi and
this code here a bit more ?

Since we now only call amd_pmf_trans_automode() based on the AMT flag and
that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
to be cleared when the profile is not balanced and can we thus not just drop
the current_profile == PLATFORM_PROFILE_BALANCED check all together?

It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
then enable AMT, else disable it, logic belongs inside thinkpad_acpi
and not here?

Regards,

Hans

> -		/* Apply the Auto Mode transition */
> -		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		if (dev->is_amt_event) {
> +			/* Apply the Auto Mode transition */
> +			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		} else if (!dev->is_amt_event && dev->amt_running) {
> +			pr_debug("resetting AMT thermals\n");
> +			mode = amd_pmf_get_pprof_modes(dev);
> +			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
> +			dev->amt_running = false;
> +		}
> +	} else {
> +		dev->amt_running = false;
>  	}
>  
>  	if (dev->cnqf_feat) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 0532f49e9613..9ae9812353cd 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -17,6 +17,7 @@
>  /* APMF Functions */
>  #define APMF_FUNC_VERIFY_INTERFACE			0
>  #define APMF_FUNC_GET_SYS_PARAMS			1
> +#define APMF_FUNC_SBIOS_REQUESTS			2
>  #define APMF_FUNC_SBIOS_HEARTBEAT			4
>  #define APMF_FUNC_AUTO_MODE					5
>  #define APMF_FUNC_SET_FAN_IDX				7
> @@ -51,6 +52,7 @@
>  /* AMD PMF BIOS interfaces */
>  struct apmf_if_functions {
>  	bool system_params;
> +	bool sbios_requests;
>  	bool sbios_heartbeat;
>  	bool auto_mode_def;
>  	bool fan_table_idx;
> @@ -84,6 +86,24 @@ struct apmf_system_params {
>  	u32 heartbeat_int;
>  } __packed;
>  
> +struct apmf_sbios_req {
> +	u16 size;
> +	u32 pending_req;
> +	u8 rsd;
> +	u8 cql_event;
> +	u8 amt_event;
> +	u32 fppt;
> +	u32 sppt;
> +	u32 fppt_apu_only;
> +	u32 spl;
> +	u32 stt_min_limit;
> +	u8 skin_temp_apu;
> +	u8 skin_temp_hs2;
> +	u8 dps_enable;
> +	u32 custom_policy_1;
> +	u32 custom_policy_2;
> +} __packed;
> +
>  struct apmf_fan_idx {
>  	u16 size;
>  	u8 fan_ctl_mode;
> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>  #endif /* CONFIG_DEBUG_FS */
>  	bool cnqf_feat;
>  	bool cnqf_running;
> +	bool is_amt_event;
> +	bool is_cql_event;
> +	bool amt_running;
>  };
>  
>  struct apmf_sps_prop_granular {
> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>  /* Auto Mode Layer */
>  void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>  int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>  void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>  void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  /* CnQF Layer */
>  int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>  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);
> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>  
>  #endif /* PMF_H */

Mario Limonciello July 27, 2022, 11:52 p.m. UTC | #4

On 7/27/2022 16:46, Hans de Goede wrote:
> <resend with Cc list fixed>
> 
> Hi,
> 
> On 7/12/22 16:58, Shyam Sundar S K wrote:
>> The transition to auto-mode happens when the PMF driver receives
>> AMT (Auto Mode transition) event. transition logic will reside in the
>> PMF driver but the events would come from other supported drivers[1].
>>
>> The thermal parameters would vary between when a performance "on-lap" mode
>> is detected and versus when not. The CQL event would get triggered from
>> other drivers, so that PMF driver would adjust the system thermal config
>> based on the ACPI inputs.
>>
>> OEMs can control whether or not to enable AMT or CQL via other supported
>> drivers[1] but the actual transition logic resides in the AMD PMF driver.
>> When an AMT event is received the automatic mode transition RAPL algorithm
>> will run. When a CQL event is received an performance "on-lap" mode will
>> be enabled and thermal parameters will be adjusted accordingly.
>>
>> [1]
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Fcommit%2F%3Fh%3Dreview-hans%26id%3D755b249250df1b612d982f3b702c831b26ecdf73&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cd72d8627f7ad4088aff308da7019afbf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637945552890705546%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=4gLIOjMo%2FYO%2BkKdqtk4pcLHlsBrLiUb41cTKvuYrcHQ%3D&amp;reserved=0
>>
>> Signed-off-by: Shyam Sundar S K <[e-mail]>
>> ---
>>   drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>>   drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>>   drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>>   drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>>   drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>>   5 files changed, 156 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index e9f33e61659f..4bde86aeafa0 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -12,6 +12,8 @@
>>   #include "pmf.h"
>>   
>>   #define APMF_METHOD "\\_SB_.PMF_.APMF"
>> +#define APMF_CQL_NOTIFICATION	2
>> +#define APMF_AMT_NOTIFICATION	3
>>   
>>   static unsigned long supported_func;
>>   
>> @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>>   {
>>   	func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>>   	func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
>> +	func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>>   	func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>>   	func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>>   	func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
>> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>   	return err;
>>   }
>>   
>> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
>> +{
>> +	union acpi_object *info;
>> +	size_t size;
>> +	int err = 0;
>> +
>> +	info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
>> +	if (!info)
>> +		return -EIO;
>> +
>> +	size = *(u16 *)info->buffer.pointer;
>> +
>> +	if (size < sizeof(union acpi_object)) {
>> +		pr_err("PMF: buffer too small %zu\n", size);
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	size = min(sizeof(*req), size);
>> +	memset(req, 0, sizeof(*req));
>> +	memcpy(req, info->buffer.pointer, size);
>> +
>> +	pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
>> +		 req->pending_req, req->cql_event, req->amt_event);
>> +
>> +out:
>> +	kfree(info);
>> +	return err;
>> +}
>> +
>>   static acpi_handle apmf_if_probe_handle(void)
>>   {
>>   	acpi_handle handle = NULL;
>> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>>   	return handle;
>>   }
>>   
>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>> +{
>> +	struct amd_pmf_dev *pmf_dev = data;
>> +	struct apmf_if *apmf_if = pmf_dev->apmf_if;
>> +	int ret;
>> +
>> +	if (apmf_if->func.sbios_requests) {
>> +		struct apmf_sbios_req req;
>> +
>> +		ret = apmf_get_sbios_requests(apmf_if, &req);
>> +		if (ret) {
>> +			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>> +			return;
>> +		}
>> +		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>> +			pr_debug("PMF: AMT is supported and notifications %s\n",
>> +				 req.amt_event ? "Enabled" : "Disabled");
>> +			if (req.amt_event)
>> +				pmf_dev->is_amt_event = true;
>> +			else
>> +				pmf_dev->is_amt_event = !!req.amt_event;
>> +		}
>> +
>> +		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>> +			pr_debug("PMF: CQL is supported and notifications %s\n",
>> +				 req.cql_event ? "Enabled" : "Disabled");
>> +			if (req.cql_event)
>> +				pmf_dev->is_cql_event = true;
>> +			else
>> +				pmf_dev->is_cql_event = !!req.cql_event;
>> +
>> +			/* update the target mode information */
>> +			amd_pmf_update_2_cql(pmf_dev);
>> +		}
>> +	}
>> +}
>> +
>>   void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>>   {
>> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>> +
>>   	if (pmf_dev->apmf_if->func.sbios_heartbeat)
>>   		cancel_delayed_work_sync(&pmf_dev->heart_beat);
>> +
>> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
>> +		acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>> +					   apmf_event_handler);
>>   }
>>   
>>   int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>   {
>> +	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>   	struct apmf_notification_cfg *n;
>>   	acpi_handle apmf_if_handle;
>>   	struct apmf_if *apmf_if;
>> -	int ret;
>> +	int ret, status;
>>   
>>   	apmf_if_handle = apmf_if_probe_handle();
>>   	if (!apmf_if_handle)
>> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>   		schedule_delayed_work(&pmf_dev->heart_beat, 0);
>>   	}
>>   
>> +	/* Install the APMF Notify handler */
>> +	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
>> +		status = acpi_install_notify_handler(ahandle,
>> +						     ACPI_ALL_NOTIFY,
>> +						     apmf_event_handler, pmf_dev);
>> +		if (ACPI_FAILURE(status)) {
>> +			dev_err(pmf_dev->dev, "failed to install notify handler\n");
>> +			return -ENODEV;
>> +		}
>> +	}
>> +
>>   out:
>>   	return ret;
>>   }
>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
>> index 954fde25e71e..a85ef4b95cdb 100644
>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>   	bool update = false;
>>   	int i, j;
>>   
>> +	if (!dev->amt_running && dev->is_amt_event) {
>> +		dev_dbg(dev->dev, "setting AMT thermals\n");
>> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>> +		dev->amt_running = true;
>> +		return;
>> +	}
>> +
>>   	/* Get the average moving average computed by auto mode algorithm */
>>   	avg_power = amd_pmf_get_moving_avg(socket_power);
>>   
>> @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>   	}
>>   }
>>   
>> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
>> +{
>> +	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
>> +			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
>> +	if ((config_store.current_mode == AUTO_PERFORMANCE ||
>> +	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
>> +	    config_store.current_mode !=
>> +	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
>> +		config_store.current_mode =
>> +				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
>> +		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>> +	}
>> +	dev_dbg(dev->dev, "updated CQL thermals\n");
>> +}
>> +
>>   static void amd_pmf_get_power_threshold(void)
>>   {
>>   	config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
>> index 2b03ae1ad37f..eba8f0d79a62 100644
>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>>   	}
>>   }
>>   
>> -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>>   {
>>   	struct cnqf_tran_params *tp;
>>   	int src, i, j, index = 0;
>> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>>   	}
>>   
>>   	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
>> -		config_store.trans_param[src][i].timer += time_lapsed_ms;
>> +		config_store.trans_param[src][i].timer += time_elapsed_ms;
>>   		config_store.trans_param[src][i].total_power += socket_power;
>>   		config_store.trans_param[src][i].count++;
>>   
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 674ddf599135..2a3dacfb2005 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>   	enum platform_profile_option current_profile;
>>   	ktime_t time_elapsed_ms;
>>   	int socket_power;
>> +	u8 mode;
>>   
>>   	/* Get the current profile information */
>>   	platform_profile_get(&current_profile);
>>   
>> +	if (!dev->is_amt_event)
>> +		dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
>> +			dev->is_amt_event ? "Enabled" : "Disabled");
>> +
>>   	/* Transfer table contents */
>>   	memset(&dev->m_table, 0, sizeof(dev->m_table));
>>   	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
>> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>   	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>>   
>>   	if (current_profile == PLATFORM_PROFILE_BALANCED) {
> 
> Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
> on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
> then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?
> 
> Can you please explain the expected interactions between thinkpad_acpi and
> this code here a bit more ?
> 
> Since we now only call amd_pmf_trans_automode() based on the AMT flag and
> that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
> to be cleared when the profile is not balanced and can we thus not just drop
> the current_profile == PLATFORM_PROFILE_BALANCED check all together?
> 
> It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
> then enable AMT, else disable it, logic belongs inside thinkpad_acpi
> and not here?
> 

It actually already lives in thinkpad_acpi.

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/tree/drivers/platform/x86/thinkpad_acpi.c?h=review-hans#n10489

By the control point from thinkpad_acpi BIOS events will be emitted 
controlling whether AMT is running in a given mode.

So yes; there is no need for this here anymore.

> Regards,
> 
> Hans
> 
>> -		/* Apply the Auto Mode transition */
>> -		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>> +		if (dev->is_amt_event) {
>> +			/* Apply the Auto Mode transition */
>> +			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>> +		} else if (!dev->is_amt_event && dev->amt_running) {
>> +			pr_debug("resetting AMT thermals\n");
>> +			mode = amd_pmf_get_pprof_modes(dev);
>> +			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
>> +			dev->amt_running = false;
>> +		}
>> +	} else {
>> +		dev->amt_running = false;
>>   	}
>>   
>>   	if (dev->cnqf_feat) {
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 0532f49e9613..9ae9812353cd 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -17,6 +17,7 @@
>>   /* APMF Functions */
>>   #define APMF_FUNC_VERIFY_INTERFACE			0
>>   #define APMF_FUNC_GET_SYS_PARAMS			1
>> +#define APMF_FUNC_SBIOS_REQUESTS			2
>>   #define APMF_FUNC_SBIOS_HEARTBEAT			4
>>   #define APMF_FUNC_AUTO_MODE					5
>>   #define APMF_FUNC_SET_FAN_IDX				7
>> @@ -51,6 +52,7 @@
>>   /* AMD PMF BIOS interfaces */
>>   struct apmf_if_functions {
>>   	bool system_params;
>> +	bool sbios_requests;
>>   	bool sbios_heartbeat;
>>   	bool auto_mode_def;
>>   	bool fan_table_idx;
>> @@ -84,6 +86,24 @@ struct apmf_system_params {
>>   	u32 heartbeat_int;
>>   } __packed;
>>   
>> +struct apmf_sbios_req {
>> +	u16 size;
>> +	u32 pending_req;
>> +	u8 rsd;
>> +	u8 cql_event;
>> +	u8 amt_event;
>> +	u32 fppt;
>> +	u32 sppt;
>> +	u32 fppt_apu_only;
>> +	u32 spl;
>> +	u32 stt_min_limit;
>> +	u8 skin_temp_apu;
>> +	u8 skin_temp_hs2;
>> +	u8 dps_enable;
>> +	u32 custom_policy_1;
>> +	u32 custom_policy_2;
>> +} __packed;
>> +
>>   struct apmf_fan_idx {
>>   	u16 size;
>>   	u8 fan_ctl_mode;
>> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>>   #endif /* CONFIG_DEBUG_FS */
>>   	bool cnqf_feat;
>>   	bool cnqf_running;
>> +	bool is_amt_event;
>> +	bool is_cql_event;
>> +	bool amt_running;
>>   };
>>   
>>   struct apmf_sps_prop_granular {
>> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>>   /* Auto Mode Layer */
>>   void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>>   int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
>> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>>   void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>>   void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
>> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
>> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>   
>>   /* CnQF Layer */
>>   int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
>> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>   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);
>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>   
>>   #endif /* PMF_H */
>

Hans de Goede July 28, 2022, 1:03 p.m. UTC | #5

Hi,

On 7/28/22 01:52, Limonciello, Mario wrote:
> On 7/27/2022 16:46, Hans de Goede wrote:
>> <resend with Cc list fixed>
>>
>> Hi,
>>
>> On 7/12/22 16:58, Shyam Sundar S K wrote:
>>> The transition to auto-mode happens when the PMF driver receives
>>> AMT (Auto Mode transition) event. transition logic will reside in the
>>> PMF driver but the events would come from other supported drivers[1].
>>>
>>> The thermal parameters would vary between when a performance "on-lap" mode
>>> is detected and versus when not. The CQL event would get triggered from
>>> other drivers, so that PMF driver would adjust the system thermal config
>>> based on the ACPI inputs.
>>>
>>> OEMs can control whether or not to enable AMT or CQL via other supported
>>> drivers[1] but the actual transition logic resides in the AMD PMF driver.
>>> When an AMT event is received the automatic mode transition RAPL algorithm
>>> will run. When a CQL event is received an performance "on-lap" mode will
>>> be enabled and thermal parameters will be adjusted accordingly.
>>>
>>> [1]
>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Fcommit%2F%3Fh%3Dreview-hans%26id%3D755b249250df1b612d982f3b702c831b26ecdf73&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cd72d8627f7ad4088aff308da7019afbf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637945552890705546%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=4gLIOjMo%2FYO%2BkKdqtk4pcLHlsBrLiUb41cTKvuYrcHQ%3D&amp;reserved=0
>>>
>>> Signed-off-by: Shyam Sundar S K <[e-mail]>
>>> ---
>>>   drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>>>   drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>>>   drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>>>   drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>>>   drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>>>   5 files changed, 156 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>> index e9f33e61659f..4bde86aeafa0 100644
>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>> @@ -12,6 +12,8 @@
>>>   #include "pmf.h"
>>>     #define APMF_METHOD "\\_SB_.PMF_.APMF"
>>> +#define APMF_CQL_NOTIFICATION    2
>>> +#define APMF_AMT_NOTIFICATION    3
>>>     static unsigned long supported_func;
>>>   @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>>>   {
>>>       func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>>>       func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
>>> +    func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>>>       func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>>>       func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>>>       func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
>>> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>       return err;
>>>   }
>>>   +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
>>> +{
>>> +    union acpi_object *info;
>>> +    size_t size;
>>> +    int err = 0;
>>> +
>>> +    info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
>>> +    if (!info)
>>> +        return -EIO;
>>> +
>>> +    size = *(u16 *)info->buffer.pointer;
>>> +
>>> +    if (size < sizeof(union acpi_object)) {
>>> +        pr_err("PMF: buffer too small %zu\n", size);
>>> +        err = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    size = min(sizeof(*req), size);
>>> +    memset(req, 0, sizeof(*req));
>>> +    memcpy(req, info->buffer.pointer, size);
>>> +
>>> +    pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
>>> +         req->pending_req, req->cql_event, req->amt_event);
>>> +
>>> +out:
>>> +    kfree(info);
>>> +    return err;
>>> +}
>>> +
>>>   static acpi_handle apmf_if_probe_handle(void)
>>>   {
>>>       acpi_handle handle = NULL;
>>> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>>>       return handle;
>>>   }
>>>   +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>> +{
>>> +    struct amd_pmf_dev *pmf_dev = data;
>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>> +    int ret;
>>> +
>>> +    if (apmf_if->func.sbios_requests) {
>>> +        struct apmf_sbios_req req;
>>> +
>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>> +        if (ret) {
>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>> +            return;
>>> +        }
>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>> +            if (req.amt_event)
>>> +                pmf_dev->is_amt_event = true;
>>> +            else
>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>> +        }
>>> +
>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>> +            if (req.cql_event)
>>> +                pmf_dev->is_cql_event = true;
>>> +            else
>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>> +
>>> +            /* update the target mode information */
>>> +            amd_pmf_update_2_cql(pmf_dev);
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>>>   {
>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>> +
>>>       if (pmf_dev->apmf_if->func.sbios_heartbeat)
>>>           cancel_delayed_work_sync(&pmf_dev->heart_beat);
>>> +
>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
>>> +        acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>>> +                       apmf_event_handler);
>>>   }
>>>     int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>   {
>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>       struct apmf_notification_cfg *n;
>>>       acpi_handle apmf_if_handle;
>>>       struct apmf_if *apmf_if;
>>> -    int ret;
>>> +    int ret, status;
>>>         apmf_if_handle = apmf_if_probe_handle();
>>>       if (!apmf_if_handle)
>>> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>           schedule_delayed_work(&pmf_dev->heart_beat, 0);
>>>       }
>>>   +    /* Install the APMF Notify handler */
>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
>>> +        status = acpi_install_notify_handler(ahandle,
>>> +                             ACPI_ALL_NOTIFY,
>>> +                             apmf_event_handler, pmf_dev);
>>> +        if (ACPI_FAILURE(status)) {
>>> +            dev_err(pmf_dev->dev, "failed to install notify handler\n");
>>> +            return -ENODEV;
>>> +        }
>>> +    }
>>> +
>>>   out:
>>>       return ret;
>>>   }
>>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
>>> index 954fde25e71e..a85ef4b95cdb 100644
>>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>>> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>       bool update = false;
>>>       int i, j;
>>>   +    if (!dev->amt_running && dev->is_amt_event) {
>>> +        dev_dbg(dev->dev, "setting AMT thermals\n");
>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>> +        dev->amt_running = true;
>>> +        return;
>>> +    }
>>> +
>>>       /* Get the average moving average computed by auto mode algorithm */
>>>       avg_power = amd_pmf_get_moving_avg(socket_power);
>>>   @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>       }
>>>   }
>>>   +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
>>> +{
>>> +    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
>>> +            dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
>>> +    if ((config_store.current_mode == AUTO_PERFORMANCE ||
>>> +         config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
>>> +        config_store.current_mode !=
>>> +        config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
>>> +        config_store.current_mode =
>>> +                config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>> +    }
>>> +    dev_dbg(dev->dev, "updated CQL thermals\n");
>>> +}
>>> +
>>>   static void amd_pmf_get_power_threshold(void)
>>>   {
>>>       config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
>>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
>>> index 2b03ae1ad37f..eba8f0d79a62 100644
>>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>>> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>>>       }
>>>   }
>>>   -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>>>   {
>>>       struct cnqf_tran_params *tp;
>>>       int src, i, j, index = 0;
>>> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>>>       }
>>>         for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
>>> -        config_store.trans_param[src][i].timer += time_lapsed_ms;
>>> +        config_store.trans_param[src][i].timer += time_elapsed_ms;
>>>           config_store.trans_param[src][i].total_power += socket_power;
>>>           config_store.trans_param[src][i].count++;
>>>   diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>>> index 674ddf599135..2a3dacfb2005 100644
>>> --- a/drivers/platform/x86/amd/pmf/core.c
>>> +++ b/drivers/platform/x86/amd/pmf/core.c
>>> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>       enum platform_profile_option current_profile;
>>>       ktime_t time_elapsed_ms;
>>>       int socket_power;
>>> +    u8 mode;
>>>         /* Get the current profile information */
>>>       platform_profile_get(&current_profile);
>>>   +    if (!dev->is_amt_event)
>>> +        dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
>>> +            dev->is_amt_event ? "Enabled" : "Disabled");
>>> +
>>>       /* Transfer table contents */
>>>       memset(&dev->m_table, 0, sizeof(dev->m_table));
>>>       amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
>>> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>       socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>>>         if (current_profile == PLATFORM_PROFILE_BALANCED) {
>>
>> Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
>> on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
>> then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?
>>
>> Can you please explain the expected interactions between thinkpad_acpi and
>> this code here a bit more ?
>>
>> Since we now only call amd_pmf_trans_automode() based on the AMT flag and
>> that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
>> to be cleared when the profile is not balanced and can we thus not just drop
>> the current_profile == PLATFORM_PROFILE_BALANCED check all together?
>>
>> It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
>> then enable AMT, else disable it, logic belongs inside thinkpad_acpi
>> and not here?
>>
> 
> It actually already lives in thinkpad_acpi.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/tree/drivers/platform/x86/thinkpad_acpi.c?h=review-hans#n10489
> 
> By the control point from thinkpad_acpi BIOS events will be emitted controlling whether AMT is running in a given mode.
> 
> So yes; there is no need for this here anymore.

Right. There still are some open questions / things which need fixing here though:

1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
   will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
   stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
   the amd-pmf module gets loaded first. So if the intend is for it to always be owned
   by thinkpad_acpi then the amd-pmf code must check for this and not even try to
   register its platform_profile support. We cannot rely on module ordering ensuring
   that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
   since there are no module load ordering guarantees.

2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
   enable AMT and then the periodically run workqueue function from amd-pmf
   will do its AMT thing. But what when the thinkpad_acpi platform_profile is
   set to low-power or performance. Should the amd-pmf code then apply the static
   slider settings for low-power/performance which it has read from the ACPI
   tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?

3. If the answer to 2. is "Yes the amd-pmf code should apply the static-slider
   settings" then we will still need patch 1/15 to allow the amd-pmd code to
   read the platform-profile setting from the thinkpad_acpi platform-profile
   provider;
   And if the answer is "No, the thinkpad ACPI/EC will take care of this"
   then we should probably make sure that the static slider code never runs
   at all on thinkpads.

Regards,

Hans


  



> 
>> Regards,
>>
>> Hans
>>
>>> -        /* Apply the Auto Mode transition */
>>> -        amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>> +        if (dev->is_amt_event) {
>>> +            /* Apply the Auto Mode transition */
>>> +            amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>> +        } else if (!dev->is_amt_event && dev->amt_running) {
>>> +            pr_debug("resetting AMT thermals\n");
>>> +            mode = amd_pmf_get_pprof_modes(dev);
>>> +            amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
>>> +            dev->amt_running = false;
>>> +        }
>>> +    } else {
>>> +        dev->amt_running = false;
>>>       }
>>>         if (dev->cnqf_feat) {
>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>> index 0532f49e9613..9ae9812353cd 100644
>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>> @@ -17,6 +17,7 @@
>>>   /* APMF Functions */
>>>   #define APMF_FUNC_VERIFY_INTERFACE            0
>>>   #define APMF_FUNC_GET_SYS_PARAMS            1
>>> +#define APMF_FUNC_SBIOS_REQUESTS            2
>>>   #define APMF_FUNC_SBIOS_HEARTBEAT            4
>>>   #define APMF_FUNC_AUTO_MODE                    5
>>>   #define APMF_FUNC_SET_FAN_IDX                7
>>> @@ -51,6 +52,7 @@
>>>   /* AMD PMF BIOS interfaces */
>>>   struct apmf_if_functions {
>>>       bool system_params;
>>> +    bool sbios_requests;
>>>       bool sbios_heartbeat;
>>>       bool auto_mode_def;
>>>       bool fan_table_idx;
>>> @@ -84,6 +86,24 @@ struct apmf_system_params {
>>>       u32 heartbeat_int;
>>>   } __packed;
>>>   +struct apmf_sbios_req {
>>> +    u16 size;
>>> +    u32 pending_req;
>>> +    u8 rsd;
>>> +    u8 cql_event;
>>> +    u8 amt_event;
>>> +    u32 fppt;
>>> +    u32 sppt;
>>> +    u32 fppt_apu_only;
>>> +    u32 spl;
>>> +    u32 stt_min_limit;
>>> +    u8 skin_temp_apu;
>>> +    u8 skin_temp_hs2;
>>> +    u8 dps_enable;
>>> +    u32 custom_policy_1;
>>> +    u32 custom_policy_2;
>>> +} __packed;
>>> +
>>>   struct apmf_fan_idx {
>>>       u16 size;
>>>       u8 fan_ctl_mode;
>>> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>>>   #endif /* CONFIG_DEBUG_FS */
>>>       bool cnqf_feat;
>>>       bool cnqf_running;
>>> +    bool is_amt_event;
>>> +    bool is_cql_event;
>>> +    bool amt_running;
>>>   };
>>>     struct apmf_sps_prop_granular {
>>> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>>>   /* Auto Mode Layer */
>>>   void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>>>   int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
>>> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>>>   void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>>>   void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
>>> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>>> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
>>> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>     /* CnQF Layer */
>>>   int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
>>> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>   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);
>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>     #endif /* PMF_H */
>>
>

Mario Limonciello July 28, 2022, 1:43 p.m. UTC | #6

On 7/28/2022 08:03, Hans de Goede wrote:
> Hi,
> 
> On 7/28/22 01:52, Limonciello, Mario wrote:
>> On 7/27/2022 16:46, Hans de Goede wrote:
>>> <resend with Cc list fixed>
>>>
>>> Hi,
>>>
>>> On 7/12/22 16:58, Shyam Sundar S K wrote:
>>>> The transition to auto-mode happens when the PMF driver receives
>>>> AMT (Auto Mode transition) event. transition logic will reside in the
>>>> PMF driver but the events would come from other supported drivers[1].
>>>>
>>>> The thermal parameters would vary between when a performance "on-lap" mode
>>>> is detected and versus when not. The CQL event would get triggered from
>>>> other drivers, so that PMF driver would adjust the system thermal config
>>>> based on the ACPI inputs.
>>>>
>>>> OEMs can control whether or not to enable AMT or CQL via other supported
>>>> drivers[1] but the actual transition logic resides in the AMD PMF driver.
>>>> When an AMT event is received the automatic mode transition RAPL algorithm
>>>> will run. When a CQL event is received an performance "on-lap" mode will
>>>> be enabled and thermal parameters will be adjusted accordingly.
>>>>
>>>> [1]
>>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Fcommit%2F%3Fh%3Dreview-hans%26id%3D755b249250df1b612d982f3b702c831b26ecdf73&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf7d0a10b43444af391fa08da709993b3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637946102171795208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=GRSR4jEbQu7yaY6CS%2BKienSw7majkwcazo8xoKHd2pA%3D&amp;reserved=0
>>>>
>>>> Signed-off-by: Shyam Sundar S K <[e-mail]>
>>>> ---
>>>>    drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>>>>    drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>>>>    drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>>>>    drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>>>>    drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>>>>    5 files changed, 156 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index e9f33e61659f..4bde86aeafa0 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -12,6 +12,8 @@
>>>>    #include "pmf.h"
>>>>      #define APMF_METHOD "\\_SB_.PMF_.APMF"
>>>> +#define APMF_CQL_NOTIFICATION    2
>>>> +#define APMF_AMT_NOTIFICATION    3
>>>>      static unsigned long supported_func;
>>>>    @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>>>>    {
>>>>        func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>>>>        func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
>>>> +    func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>>>>        func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>>>>        func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>>>>        func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
>>>> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>>        return err;
>>>>    }
>>>>    +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
>>>> +{
>>>> +    union acpi_object *info;
>>>> +    size_t size;
>>>> +    int err = 0;
>>>> +
>>>> +    info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
>>>> +    if (!info)
>>>> +        return -EIO;
>>>> +
>>>> +    size = *(u16 *)info->buffer.pointer;
>>>> +
>>>> +    if (size < sizeof(union acpi_object)) {
>>>> +        pr_err("PMF: buffer too small %zu\n", size);
>>>> +        err = -EINVAL;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    size = min(sizeof(*req), size);
>>>> +    memset(req, 0, sizeof(*req));
>>>> +    memcpy(req, info->buffer.pointer, size);
>>>> +
>>>> +    pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
>>>> +         req->pending_req, req->cql_event, req->amt_event);
>>>> +
>>>> +out:
>>>> +    kfree(info);
>>>> +    return err;
>>>> +}
>>>> +
>>>>    static acpi_handle apmf_if_probe_handle(void)
>>>>    {
>>>>        acpi_handle handle = NULL;
>>>> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>>>>        return handle;
>>>>    }
>>>>    +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>> +{
>>>> +    struct amd_pmf_dev *pmf_dev = data;
>>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>>> +    int ret;
>>>> +
>>>> +    if (apmf_if->func.sbios_requests) {
>>>> +        struct apmf_sbios_req req;
>>>> +
>>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>>> +        if (ret) {
>>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>>> +            return;
>>>> +        }
>>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>>> +            if (req.amt_event)
>>>> +                pmf_dev->is_amt_event = true;
>>>> +            else
>>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>>> +        }
>>>> +
>>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>>> +            if (req.cql_event)
>>>> +                pmf_dev->is_cql_event = true;
>>>> +            else
>>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>>> +
>>>> +            /* update the target mode information */
>>>> +            amd_pmf_update_2_cql(pmf_dev);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>    void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>>>>    {
>>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>> +
>>>>        if (pmf_dev->apmf_if->func.sbios_heartbeat)
>>>>            cancel_delayed_work_sync(&pmf_dev->heart_beat);
>>>> +
>>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
>>>> +        acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>>>> +                       apmf_event_handler);
>>>>    }
>>>>      int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>>    {
>>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>>        struct apmf_notification_cfg *n;
>>>>        acpi_handle apmf_if_handle;
>>>>        struct apmf_if *apmf_if;
>>>> -    int ret;
>>>> +    int ret, status;
>>>>          apmf_if_handle = apmf_if_probe_handle();
>>>>        if (!apmf_if_handle)
>>>> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>>            schedule_delayed_work(&pmf_dev->heart_beat, 0);
>>>>        }
>>>>    +    /* Install the APMF Notify handler */
>>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
>>>> +        status = acpi_install_notify_handler(ahandle,
>>>> +                             ACPI_ALL_NOTIFY,
>>>> +                             apmf_event_handler, pmf_dev);
>>>> +        if (ACPI_FAILURE(status)) {
>>>> +            dev_err(pmf_dev->dev, "failed to install notify handler\n");
>>>> +            return -ENODEV;
>>>> +        }
>>>> +    }
>>>> +
>>>>    out:
>>>>        return ret;
>>>>    }
>>>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
>>>> index 954fde25e71e..a85ef4b95cdb 100644
>>>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>>>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>>>> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>>        bool update = false;
>>>>        int i, j;
>>>>    +    if (!dev->amt_running && dev->is_amt_event) {
>>>> +        dev_dbg(dev->dev, "setting AMT thermals\n");
>>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>>> +        dev->amt_running = true;
>>>> +        return;
>>>> +    }
>>>> +
>>>>        /* Get the average moving average computed by auto mode algorithm */
>>>>        avg_power = amd_pmf_get_moving_avg(socket_power);
>>>>    @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>>        }
>>>>    }
>>>>    +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
>>>> +{
>>>> +    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
>>>> +            dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
>>>> +    if ((config_store.current_mode == AUTO_PERFORMANCE ||
>>>> +         config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
>>>> +        config_store.current_mode !=
>>>> +        config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
>>>> +        config_store.current_mode =
>>>> +                config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
>>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>>> +    }
>>>> +    dev_dbg(dev->dev, "updated CQL thermals\n");
>>>> +}
>>>> +
>>>>    static void amd_pmf_get_power_threshold(void)
>>>>    {
>>>>        config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
>>>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
>>>> index 2b03ae1ad37f..eba8f0d79a62 100644
>>>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>>>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>>>> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>>>>        }
>>>>    }
>>>>    -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
>>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>>>>    {
>>>>        struct cnqf_tran_params *tp;
>>>>        int src, i, j, index = 0;
>>>> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>>>>        }
>>>>          for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
>>>> -        config_store.trans_param[src][i].timer += time_lapsed_ms;
>>>> +        config_store.trans_param[src][i].timer += time_elapsed_ms;
>>>>            config_store.trans_param[src][i].total_power += socket_power;
>>>>            config_store.trans_param[src][i].count++;
>>>>    diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>>>> index 674ddf599135..2a3dacfb2005 100644
>>>> --- a/drivers/platform/x86/amd/pmf/core.c
>>>> +++ b/drivers/platform/x86/amd/pmf/core.c
>>>> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>>        enum platform_profile_option current_profile;
>>>>        ktime_t time_elapsed_ms;
>>>>        int socket_power;
>>>> +    u8 mode;
>>>>          /* Get the current profile information */
>>>>        platform_profile_get(&current_profile);
>>>>    +    if (!dev->is_amt_event)
>>>> +        dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
>>>> +            dev->is_amt_event ? "Enabled" : "Disabled");
>>>> +
>>>>        /* Transfer table contents */
>>>>        memset(&dev->m_table, 0, sizeof(dev->m_table));
>>>>        amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
>>>> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>>        socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>>>>          if (current_profile == PLATFORM_PROFILE_BALANCED) {
>>>
>>> Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
>>> on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
>>> then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?
>>>
>>> Can you please explain the expected interactions between thinkpad_acpi and
>>> this code here a bit more ?
>>>
>>> Since we now only call amd_pmf_trans_automode() based on the AMT flag and
>>> that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
>>> to be cleared when the profile is not balanced and can we thus not just drop
>>> the current_profile == PLATFORM_PROFILE_BALANCED check all together?
>>>
>>> It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
>>> then enable AMT, else disable it, logic belongs inside thinkpad_acpi
>>> and not here?
>>>
>>
>> It actually already lives in thinkpad_acpi.
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Ftree%2Fdrivers%2Fplatform%2Fx86%2Fthinkpad_acpi.c%3Fh%3Dreview-hans%23n10489&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf7d0a10b43444af391fa08da709993b3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637946102171795208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=cGxN8GG6LXnk%2FPDi%2BS1zNAkWRH4AiTKqF0eebcfcTRA%3D&amp;reserved=0
>>
>> By the control point from thinkpad_acpi BIOS events will be emitted controlling whether AMT is running in a given mode.
>>
>> So yes; there is no need for this here anymore.
> 
> Right. There still are some open questions / things which need fixing here though:
> 
> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>     will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>     stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>     the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>     by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>     register its platform_profile support. We cannot rely on module ordering ensuring
>     that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>     since there are no module load ordering guarantees.

This was my thought initially too while this was being developed, but 
actually there is some nuance here that is non-obvious.  The platform 
profile registering code in amd-pmf will examine bits set in the BIOS to 
decide whether or not to export platform profile support.  In Lenovo 
platforms that support thinkpad_acpi these bits are not set.  So 
platform profile support ONLY comes from thinkpad-acpi in those platforms.

> 
> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>     enable AMT and then the periodically run workqueue function from amd-pmf
>     will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>     set to low-power or performance. Should the amd-pmf code then apply the static
>     slider settings for low-power/performance which it has read from the ACPI
>     tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
> 

When thinkpad_acpi changes platform profile then a BIOS event goes 
through and amd-pmf receives that and will run based on the event.


> 3. If the answer to 2. is "Yes the amd-pmf code should apply the static-slider
>     settings" then we will still need patch 1/15 to allow the amd-pmd code to
>     read the platform-profile setting from the thinkpad_acpi platform-profile
>     provider;
>     And if the answer is "No, the thinkpad ACPI/EC will take care of this"
>     then we should probably make sure that the static slider code never runs
>     at all on thinkpads.

Yup, already handled.

> 
> Regards,
> 
> Hans
> 
> 
>    
> 
> 
> 
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>> -        /* Apply the Auto Mode transition */
>>>> -        amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>>> +        if (dev->is_amt_event) {
>>>> +            /* Apply the Auto Mode transition */
>>>> +            amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>>> +        } else if (!dev->is_amt_event && dev->amt_running) {
>>>> +            pr_debug("resetting AMT thermals\n");
>>>> +            mode = amd_pmf_get_pprof_modes(dev);
>>>> +            amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
>>>> +            dev->amt_running = false;
>>>> +        }
>>>> +    } else {
>>>> +        dev->amt_running = false;
>>>>        }
>>>>          if (dev->cnqf_feat) {
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 0532f49e9613..9ae9812353cd 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -17,6 +17,7 @@
>>>>    /* APMF Functions */
>>>>    #define APMF_FUNC_VERIFY_INTERFACE            0
>>>>    #define APMF_FUNC_GET_SYS_PARAMS            1
>>>> +#define APMF_FUNC_SBIOS_REQUESTS            2
>>>>    #define APMF_FUNC_SBIOS_HEARTBEAT            4
>>>>    #define APMF_FUNC_AUTO_MODE                    5
>>>>    #define APMF_FUNC_SET_FAN_IDX                7
>>>> @@ -51,6 +52,7 @@
>>>>    /* AMD PMF BIOS interfaces */
>>>>    struct apmf_if_functions {
>>>>        bool system_params;
>>>> +    bool sbios_requests;
>>>>        bool sbios_heartbeat;
>>>>        bool auto_mode_def;
>>>>        bool fan_table_idx;
>>>> @@ -84,6 +86,24 @@ struct apmf_system_params {
>>>>        u32 heartbeat_int;
>>>>    } __packed;
>>>>    +struct apmf_sbios_req {
>>>> +    u16 size;
>>>> +    u32 pending_req;
>>>> +    u8 rsd;
>>>> +    u8 cql_event;
>>>> +    u8 amt_event;
>>>> +    u32 fppt;
>>>> +    u32 sppt;
>>>> +    u32 fppt_apu_only;
>>>> +    u32 spl;
>>>> +    u32 stt_min_limit;
>>>> +    u8 skin_temp_apu;
>>>> +    u8 skin_temp_hs2;
>>>> +    u8 dps_enable;
>>>> +    u32 custom_policy_1;
>>>> +    u32 custom_policy_2;
>>>> +} __packed;
>>>> +
>>>>    struct apmf_fan_idx {
>>>>        u16 size;
>>>>        u8 fan_ctl_mode;
>>>> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>>>>    #endif /* CONFIG_DEBUG_FS */
>>>>        bool cnqf_feat;
>>>>        bool cnqf_running;
>>>> +    bool is_amt_event;
>>>> +    bool is_cql_event;
>>>> +    bool amt_running;
>>>>    };
>>>>      struct apmf_sps_prop_granular {
>>>> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>>>>    /* Auto Mode Layer */
>>>>    void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>>>>    int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
>>>> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>>>>    void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>>>>    void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
>>>> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>>>> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
>>>> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>>      /* CnQF Layer */
>>>>    int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
>>>> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>>    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);
>>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>>      #endif /* PMF_H */
>>>
>>
>

Hans de Goede July 28, 2022, 2:09 p.m. UTC | #7

Hi,

On 7/28/22 15:43, Limonciello, Mario wrote:
> On 7/28/2022 08:03, Hans de Goede wrote:
>> Hi,
>>
>> On 7/28/22 01:52, Limonciello, Mario wrote:
>>> On 7/27/2022 16:46, Hans de Goede wrote:
>>>> <resend with Cc list fixed>
>>>>
>>>> Hi,
>>>>
>>>> On 7/12/22 16:58, Shyam Sundar S K wrote:
>>>>> The transition to auto-mode happens when the PMF driver receives
>>>>> AMT (Auto Mode transition) event. transition logic will reside in the
>>>>> PMF driver but the events would come from other supported drivers[1].
>>>>>
>>>>> The thermal parameters would vary between when a performance "on-lap" mode
>>>>> is detected and versus when not. The CQL event would get triggered from
>>>>> other drivers, so that PMF driver would adjust the system thermal config
>>>>> based on the ACPI inputs.
>>>>>
>>>>> OEMs can control whether or not to enable AMT or CQL via other supported
>>>>> drivers[1] but the actual transition logic resides in the AMD PMF driver.
>>>>> When an AMT event is received the automatic mode transition RAPL algorithm
>>>>> will run. When a CQL event is received an performance "on-lap" mode will
>>>>> be enabled and thermal parameters will be adjusted accordingly.
>>>>>
>>>>> [1]
>>>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Fcommit%2F%3Fh%3Dreview-hans%26id%3D755b249250df1b612d982f3b702c831b26ecdf73&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf7d0a10b43444af391fa08da709993b3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637946102171795208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=GRSR4jEbQu7yaY6CS%2BKienSw7majkwcazo8xoKHd2pA%3D&amp;reserved=0
>>>>>
>>>>> Signed-off-by: Shyam Sundar S K <[e-mail]>
>>>>> ---
>>>>>    drivers/platform/x86/amd/pmf/acpi.c      | 90 +++++++++++++++++++++++-
>>>>>    drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++
>>>>>    drivers/platform/x86/amd/pmf/cnqf.c      |  4 +-
>>>>>    drivers/platform/x86/amd/pmf/core.c      | 18 ++++-
>>>>>    drivers/platform/x86/amd/pmf/pmf.h       | 29 +++++++-
>>>>>    5 files changed, 156 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>>> index e9f33e61659f..4bde86aeafa0 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>>> @@ -12,6 +12,8 @@
>>>>>    #include "pmf.h"
>>>>>      #define APMF_METHOD "\\_SB_.PMF_.APMF"
>>>>> +#define APMF_CQL_NOTIFICATION    2
>>>>> +#define APMF_AMT_NOTIFICATION    3
>>>>>      static unsigned long supported_func;
>>>>>    @@ -55,6 +57,7 @@ static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
>>>>>    {
>>>>>        func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
>>>>>        func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
>>>>> +    func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
>>>>>        func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
>>>>>        func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
>>>>>        func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
>>>>> @@ -292,6 +295,36 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>>>        return err;
>>>>>    }
>>>>>    +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
>>>>> +{
>>>>> +    union acpi_object *info;
>>>>> +    size_t size;
>>>>> +    int err = 0;
>>>>> +
>>>>> +    info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
>>>>> +    if (!info)
>>>>> +        return -EIO;
>>>>> +
>>>>> +    size = *(u16 *)info->buffer.pointer;
>>>>> +
>>>>> +    if (size < sizeof(union acpi_object)) {
>>>>> +        pr_err("PMF: buffer too small %zu\n", size);
>>>>> +        err = -EINVAL;
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    size = min(sizeof(*req), size);
>>>>> +    memset(req, 0, sizeof(*req));
>>>>> +    memcpy(req, info->buffer.pointer, size);
>>>>> +
>>>>> +    pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
>>>>> +         req->pending_req, req->cql_event, req->amt_event);
>>>>> +
>>>>> +out:
>>>>> +    kfree(info);
>>>>> +    return err;
>>>>> +}
>>>>> +
>>>>>    static acpi_handle apmf_if_probe_handle(void)
>>>>>    {
>>>>>        acpi_handle handle = NULL;
>>>>> @@ -312,18 +345,62 @@ static acpi_handle apmf_if_probe_handle(void)
>>>>>        return handle;
>>>>>    }
>>>>>    +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>>> +{
>>>>> +    struct amd_pmf_dev *pmf_dev = data;
>>>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (apmf_if->func.sbios_requests) {
>>>>> +        struct apmf_sbios_req req;
>>>>> +
>>>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>>>> +        if (ret) {
>>>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>>>> +            return;
>>>>> +        }
>>>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>>>> +            if (req.amt_event)
>>>>> +                pmf_dev->is_amt_event = true;
>>>>> +            else
>>>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>>>> +        }
>>>>> +
>>>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>>>> +            if (req.cql_event)
>>>>> +                pmf_dev->is_cql_event = true;
>>>>> +            else
>>>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>>>> +
>>>>> +            /* update the target mode information */
>>>>> +            amd_pmf_update_2_cql(pmf_dev);
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>>>>>    {
>>>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>>> +
>>>>>        if (pmf_dev->apmf_if->func.sbios_heartbeat)
>>>>>            cancel_delayed_work_sync(&pmf_dev->heart_beat);
>>>>> +
>>>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
>>>>> +        acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
>>>>> +                       apmf_event_handler);
>>>>>    }
>>>>>      int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>>>    {
>>>>> +    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>>>        struct apmf_notification_cfg *n;
>>>>>        acpi_handle apmf_if_handle;
>>>>>        struct apmf_if *apmf_if;
>>>>> -    int ret;
>>>>> +    int ret, status;
>>>>>          apmf_if_handle = apmf_if_probe_handle();
>>>>>        if (!apmf_if_handle)
>>>>> @@ -360,6 +437,17 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
>>>>>            schedule_delayed_work(&pmf_dev->heart_beat, 0);
>>>>>        }
>>>>>    +    /* Install the APMF Notify handler */
>>>>> +    if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
>>>>> +        status = acpi_install_notify_handler(ahandle,
>>>>> +                             ACPI_ALL_NOTIFY,
>>>>> +                             apmf_event_handler, pmf_dev);
>>>>> +        if (ACPI_FAILURE(status)) {
>>>>> +            dev_err(pmf_dev->dev, "failed to install notify handler\n");
>>>>> +            return -ENODEV;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>>    out:
>>>>>        return ret;
>>>>>    }
>>>>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
>>>>> index 954fde25e71e..a85ef4b95cdb 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>>>>> @@ -111,6 +111,13 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>>>        bool update = false;
>>>>>        int i, j;
>>>>>    +    if (!dev->amt_running && dev->is_amt_event) {
>>>>> +        dev_dbg(dev->dev, "setting AMT thermals\n");
>>>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>>>> +        dev->amt_running = true;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>        /* Get the average moving average computed by auto mode algorithm */
>>>>>        avg_power = amd_pmf_get_moving_avg(socket_power);
>>>>>    @@ -161,6 +168,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>>>>>        }
>>>>>    }
>>>>>    +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
>>>>> +{
>>>>> +    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
>>>>> +            dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
>>>>> +    if ((config_store.current_mode == AUTO_PERFORMANCE ||
>>>>> +         config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
>>>>> +        config_store.current_mode !=
>>>>> +        config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
>>>>> +        config_store.current_mode =
>>>>> +                config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
>>>>> +        amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
>>>>> +    }
>>>>> +    dev_dbg(dev->dev, "updated CQL thermals\n");
>>>>> +}
>>>>> +
>>>>>    static void amd_pmf_get_power_threshold(void)
>>>>>    {
>>>>>        config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
>>>>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
>>>>> index 2b03ae1ad37f..eba8f0d79a62 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>>>>> @@ -101,7 +101,7 @@ static const char *state_as_str(unsigned int state)
>>>>>        }
>>>>>    }
>>>>>    -void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
>>>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
>>>>>    {
>>>>>        struct cnqf_tran_params *tp;
>>>>>        int src, i, j, index = 0;
>>>>> @@ -117,7 +117,7 @@ void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
>>>>>        }
>>>>>          for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
>>>>> -        config_store.trans_param[src][i].timer += time_lapsed_ms;
>>>>> +        config_store.trans_param[src][i].timer += time_elapsed_ms;
>>>>>            config_store.trans_param[src][i].total_power += socket_power;
>>>>>            config_store.trans_param[src][i].count++;
>>>>>    diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>>>>> index 674ddf599135..2a3dacfb2005 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/core.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/core.c
>>>>> @@ -109,10 +109,15 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>>>        enum platform_profile_option current_profile;
>>>>>        ktime_t time_elapsed_ms;
>>>>>        int socket_power;
>>>>> +    u8 mode;
>>>>>          /* Get the current profile information */
>>>>>        platform_profile_get(&current_profile);
>>>>>    +    if (!dev->is_amt_event)
>>>>> +        dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
>>>>> +            dev->is_amt_event ? "Enabled" : "Disabled");
>>>>> +
>>>>>        /* Transfer table contents */
>>>>>        memset(&dev->m_table, 0, sizeof(dev->m_table));
>>>>>        amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
>>>>> @@ -123,8 +128,17 @@ static void amd_pmf_get_metrics(struct work_struct *work)
>>>>>        socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
>>>>>          if (current_profile == PLATFORM_PROFILE_BALANCED) {
>>>>
>>>> Hmm, I guess this is also why the platform_profile_get() is necessary ? Because
>>>> on Thinkpads thinkpad_acpi is expected to be the platform_profile provider and
>>>> then the PMF code wants to know the platform_profile setting from thinkpad_acpi ?
>>>>
>>>> Can you please explain the expected interactions between thinkpad_acpi and
>>>> this code here a bit more ?
>>>>
>>>> Since we now only call amd_pmf_trans_automode() based on the AMT flag and
>>>> that flag is controlled by the thinkpad BIOS/EC can we not expect that flag
>>>> to be cleared when the profile is not balanced and can we thus not just drop
>>>> the current_profile == PLATFORM_PROFILE_BALANCED check all together?
>>>>
>>>> It seems to me that if current_profile == PLATFORM_PROFILE_BALANCED
>>>> then enable AMT, else disable it, logic belongs inside thinkpad_acpi
>>>> and not here?
>>>>
>>>
>>> It actually already lives in thinkpad_acpi.
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Ftree%2Fdrivers%2Fplatform%2Fx86%2Fthinkpad_acpi.c%3Fh%3Dreview-hans%23n10489&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf7d0a10b43444af391fa08da709993b3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637946102171795208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=cGxN8GG6LXnk%2FPDi%2BS1zNAkWRH4AiTKqF0eebcfcTRA%3D&amp;reserved=0
>>>
>>> By the control point from thinkpad_acpi BIOS events will be emitted controlling whether AMT is running in a given mode.
>>>
>>> So yes; there is no need for this here anymore.
>>
>> Right. There still are some open questions / things which need fixing here though:
>>
>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>     will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>     stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>     the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>     by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>     register its platform_profile support. We cannot rely on module ordering ensuring
>>     that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>     since there are no module load ordering guarantees.
> 
> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.

Right, Shyam mentioned this in another part of the thread. As I
mentioned there IHMO it would still be good to check this in the driver
though. To catch cases where a BIOS for some reasons advertises an
unexpected combination of features.

>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>     enable AMT and then the periodically run workqueue function from amd-pmf
>>     will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>     set to low-power or performance. Should the amd-pmf code then apply the static
>>     slider settings for low-power/performance which it has read from the ACPI
>>     tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>
> 
> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.

Hmm, I don't remember seeing anything for this in the patches. Actually this
reminds me that the code should probably reschedule (using mod_delayed_work)
the work to run immediately after a BIOS event, rather then waiting for
the next normally scheduled run.

But even then I don't remember seeing any code related to catching
platform-profile changes done outside amd-pmf... ?

There is this bit:

 	if (current_profile == PLATFORM_PROFILE_BALANCED) {
-		/* Apply the Auto Mode transition */
-		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+		if (dev->is_amt_event) {
+			/* Apply the Auto Mode transition */
+			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+		} else if (!dev->is_amt_event && dev->amt_running) {
+			pr_debug("resetting AMT thermals\n");
+			mode = amd_pmf_get_pprof_modes(dev);
+			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
+			dev->amt_running = false;
+		}
+	} else {
+		dev->amt_running = false;
 	}

But the new code here only applies the static slider settings on
is_amt_event edges (going from 1->0) and if the static slider support
bits are supposed to not be set then amd_pmf_load_defaults_sps() will
not have run because
is_apmf_func_supported(APMF_FUNC_STATIC_SLIDER_GRANULAR) will return
false.

So the values being set by amd_pmf_update_slider() will not have been
initialized and it will be setting everything to 0.

Also amd_pmf_get_pprof_modes() will always return POWER_MODE_POWER_SAVER
since pmf->current_profile is left at its 0 value (from kzalloc) in
this case.

So it seems that the code path where AMT is being disabled here is
buggy and it still is not clear to me where the limits get set
when thinkpad_acpi's platform_profile gets set to low-power
or performance.

Regards,

Hans


> 
> 
>> 3. If the answer to 2. is "Yes the amd-pmf code should apply the static-slider
>>     settings" then we will still need patch 1/15 to allow the amd-pmd code to
>>     read the platform-profile setting from the thinkpad_acpi platform-profile
>>     provider;
>>     And if the answer is "No, the thinkpad ACPI/EC will take care of this"
>>     then we should probably make sure that the static slider code never runs
>>     at all on thinkpads.
> 
> Yup, already handled.
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>   
>>
>>
>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>> -        /* Apply the Auto Mode transition */
>>>>> -        amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>>>> +        if (dev->is_amt_event) {
>>>>> +            /* Apply the Auto Mode transition */
>>>>> +            amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
>>>>> +        } else if (!dev->is_amt_event && dev->amt_running) {
>>>>> +            pr_debug("resetting AMT thermals\n");
>>>>> +            mode = amd_pmf_get_pprof_modes(dev);
>>>>> +            amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
>>>>> +            dev->amt_running = false;
>>>>> +        }
>>>>> +    } else {
>>>>> +        dev->amt_running = false;
>>>>>        }
>>>>>          if (dev->cnqf_feat) {
>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>>> index 0532f49e9613..9ae9812353cd 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>>> @@ -17,6 +17,7 @@
>>>>>    /* APMF Functions */
>>>>>    #define APMF_FUNC_VERIFY_INTERFACE            0
>>>>>    #define APMF_FUNC_GET_SYS_PARAMS            1
>>>>> +#define APMF_FUNC_SBIOS_REQUESTS            2
>>>>>    #define APMF_FUNC_SBIOS_HEARTBEAT            4
>>>>>    #define APMF_FUNC_AUTO_MODE                    5
>>>>>    #define APMF_FUNC_SET_FAN_IDX                7
>>>>> @@ -51,6 +52,7 @@
>>>>>    /* AMD PMF BIOS interfaces */
>>>>>    struct apmf_if_functions {
>>>>>        bool system_params;
>>>>> +    bool sbios_requests;
>>>>>        bool sbios_heartbeat;
>>>>>        bool auto_mode_def;
>>>>>        bool fan_table_idx;
>>>>> @@ -84,6 +86,24 @@ struct apmf_system_params {
>>>>>        u32 heartbeat_int;
>>>>>    } __packed;
>>>>>    +struct apmf_sbios_req {
>>>>> +    u16 size;
>>>>> +    u32 pending_req;
>>>>> +    u8 rsd;
>>>>> +    u8 cql_event;
>>>>> +    u8 amt_event;
>>>>> +    u32 fppt;
>>>>> +    u32 sppt;
>>>>> +    u32 fppt_apu_only;
>>>>> +    u32 spl;
>>>>> +    u32 stt_min_limit;
>>>>> +    u8 skin_temp_apu;
>>>>> +    u8 skin_temp_hs2;
>>>>> +    u8 dps_enable;
>>>>> +    u32 custom_policy_1;
>>>>> +    u32 custom_policy_2;
>>>>> +} __packed;
>>>>> +
>>>>>    struct apmf_fan_idx {
>>>>>        u16 size;
>>>>>        u8 fan_ctl_mode;
>>>>> @@ -171,6 +191,9 @@ struct amd_pmf_dev {
>>>>>    #endif /* CONFIG_DEBUG_FS */
>>>>>        bool cnqf_feat;
>>>>>        bool cnqf_running;
>>>>> +    bool is_amt_event;
>>>>> +    bool is_cql_event;
>>>>> +    bool amt_running;
>>>>>    };
>>>>>      struct apmf_sps_prop_granular {
>>>>> @@ -417,9 +440,11 @@ int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
>>>>>    /* Auto Mode Layer */
>>>>>    void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
>>>>>    int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
>>>>> +int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
>>>>>    void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
>>>>>    void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
>>>>> -void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>>>>> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
>>>>> +void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>>>      /* CnQF Layer */
>>>>>    int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
>>>>> @@ -427,6 +452,6 @@ int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
>>>>>    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);
>>>>> +void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
>>>>>      #endif /* PMF_H */
>>>>
>>>
>>
>

Mario Limonciello July 28, 2022, 2:38 p.m. UTC | #8

>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>      will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>      stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>      the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>      by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>      register its platform_profile support. We cannot rely on module ordering ensuring
>>>      that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>      since there are no module load ordering guarantees.
>>
>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
> 
> Right, Shyam mentioned this in another part of the thread. As I
> mentioned there IHMO it would still be good to check this in the driver
> though. To catch cases where a BIOS for some reasons advertises an
> unexpected combination of features.
> 
>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>      enable AMT and then the periodically run workqueue function from amd-pmf
>>>      will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>      set to low-power or performance. Should the amd-pmf code then apply the static
>>>      slider settings for low-power/performance which it has read from the ACPI
>>>      tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>
>>
>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
> 
> Hmm, I don't remember seeing anything for this in the patches. Actually this
> reminds me that the code should probably reschedule (using mod_delayed_work)
> the work to run immediately after a BIOS event, rather then waiting for
> the next normally scheduled run.
> 
> But even then I don't remember seeing any code related to catching
> platform-profile changes done outside amd-pmf... ?

It's not a platform profile change - it's an ACPI event.

When a user changes a platform profile then thinkpad_acpi will see 
whether it's balanced or not.  When changing to/from balanced 
thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.

This is the code you're looking for (in this specific patch):

+static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
+{
+	struct amd_pmf_dev *pmf_dev = data;
+	struct apmf_if *apmf_if = pmf_dev->apmf_if;
+	int ret;
+
+	if (apmf_if->func.sbios_requests) {
+		struct apmf_sbios_req req;
+
+		ret = apmf_get_sbios_requests(apmf_if, &req);
+		if (ret) {
+			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
+			return;
+		}
+		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
+			pr_debug("PMF: AMT is supported and notifications %s\n",
+				 req.amt_event ? "Enabled" : "Disabled");
+			if (req.amt_event)
+				pmf_dev->is_amt_event = true;
+			else
+				pmf_dev->is_amt_event = !!req.amt_event;
+		}
+
+		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
+			pr_debug("PMF: CQL is supported and notifications %s\n",
+				 req.cql_event ? "Enabled" : "Disabled");
+			if (req.cql_event)
+				pmf_dev->is_cql_event = true;
+			else
+				pmf_dev->is_cql_event = !!req.cql_event;
+
+			/* update the target mode information */
+			amd_pmf_update_2_cql(pmf_dev);
+		}
+	}
+}
+

> 
> There is this bit:
> 
>   	if (current_profile == PLATFORM_PROFILE_BALANCED) {
> -		/* Apply the Auto Mode transition */
> -		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		if (dev->is_amt_event) {
> +			/* Apply the Auto Mode transition */
> +			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
> +		} else if (!dev->is_amt_event && dev->amt_running) {
> +			pr_debug("resetting AMT thermals\n");
> +			mode = amd_pmf_get_pprof_modes(dev);
> +			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
> +			dev->amt_running = false;
> +		}
> +	} else {
> +		dev->amt_running = false;
>   	}
> 
> But the new code here only applies the static slider settings on
> is_amt_event edges (going from 1->0) and if the static slider support
> bits are supposed to not be set then amd_pmf_load_defaults_sps() will
> not have run because
> is_apmf_func_supported(APMF_FUNC_STATIC_SLIDER_GRANULAR) will return
> false.
> 
> So the values being set by amd_pmf_update_slider() will not have been
> initialized and it will be setting everything to 0.

> 
> Also amd_pmf_get_pprof_modes() will always return POWER_MODE_POWER_SAVER
> since pmf->current_profile is left at its 0 value (from kzalloc) in
> this case.
> 
> So it seems that the code path where AMT is being disabled here is
> buggy and it still is not clear to me where the limits get set
> when thinkpad_acpi's platform_profile gets set to low-power
> or performance.
> 

I think you're right an extra check should end up in 
amd_pmf_update_slider that only runs code when the static slider returns 
true.

Hans de Goede July 28, 2022, 5:46 p.m. UTC | #9

Hi,

On 7/28/22 16:38, Limonciello, Mario wrote:
> 
>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>      will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>      stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>      the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>      by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>      register its platform_profile support. We cannot rely on module ordering ensuring
>>>>      that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>      since there are no module load ordering guarantees.
>>>
>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>
>> Right, Shyam mentioned this in another part of the thread. As I
>> mentioned there IHMO it would still be good to check this in the driver
>> though. To catch cases where a BIOS for some reasons advertises an
>> unexpected combination of features.
>>
>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>      enable AMT and then the periodically run workqueue function from amd-pmf
>>>>      will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>      set to low-power or performance. Should the amd-pmf code then apply the static
>>>>      slider settings for low-power/performance which it has read from the ACPI
>>>>      tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>
>>>
>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>
>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>> reminds me that the code should probably reschedule (using mod_delayed_work)
>> the work to run immediately after a BIOS event, rather then waiting for
>> the next normally scheduled run.
>>
>> But even then I don't remember seeing any code related to catching
>> platform-profile changes done outside amd-pmf... ?
> 
> It's not a platform profile change - it's an ACPI event.
> 
> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
> 
> This is the code you're looking for (in this specific patch):
> 
> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
> +{
> +    struct amd_pmf_dev *pmf_dev = data;
> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
> +    int ret;
> +
> +    if (apmf_if->func.sbios_requests) {
> +        struct apmf_sbios_req req;
> +
> +        ret = apmf_get_sbios_requests(apmf_if, &req);
> +        if (ret) {
> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
> +            return;
> +        }
> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
> +            pr_debug("PMF: AMT is supported and notifications %s\n",
> +                 req.amt_event ? "Enabled" : "Disabled");
> +            if (req.amt_event)
> +                pmf_dev->is_amt_event = true;
> +            else
> +                pmf_dev->is_amt_event = !!req.amt_event;
> +        }
> +
> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
> +            pr_debug("PMF: CQL is supported and notifications %s\n",
> +                 req.cql_event ? "Enabled" : "Disabled");
> +            if (req.cql_event)
> +                pmf_dev->is_cql_event = true;
> +            else
> +                pmf_dev->is_cql_event = !!req.cql_event;
> +
> +            /* update the target mode information */
> +            amd_pmf_update_2_cql(pmf_dev);
> +        }
> +    }
> +}
> +

Right this is the AMT on/off path that bit I understand.
This happens when switching to / away from balanced mode.

My question is what does the equivalent of these lines:

+		amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
+		amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
+		amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
+		amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
+				 config_store.prop[src][idx].sppt_apu_only, NULL);
+		amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
+				 config_store.prop[src][idx].stt_min, NULL);
+		amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
+				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
+		amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
+				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);

When the profile is switched (by userspace, or through the hotkeys on
the laptop) to low-power or to performance mode ?

Regards,

Hans

Mario Limonciello July 28, 2022, 6:06 p.m. UTC | #10

On 7/28/2022 12:46, Hans de Goede wrote:
> Hi,
> 
> On 7/28/22 16:38, Limonciello, Mario wrote:
>>
>>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>>       will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>>       stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>>       the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>>       by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>>       register its platform_profile support. We cannot rely on module ordering ensuring
>>>>>       that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>>       since there are no module load ordering guarantees.
>>>>
>>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>>
>>> Right, Shyam mentioned this in another part of the thread. As I
>>> mentioned there IHMO it would still be good to check this in the driver
>>> though. To catch cases where a BIOS for some reasons advertises an
>>> unexpected combination of features.
>>>
>>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>>       enable AMT and then the periodically run workqueue function from amd-pmf
>>>>>       will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>>       set to low-power or performance. Should the amd-pmf code then apply the static
>>>>>       slider settings for low-power/performance which it has read from the ACPI
>>>>>       tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>>
>>>>
>>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>>
>>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>>> reminds me that the code should probably reschedule (using mod_delayed_work)
>>> the work to run immediately after a BIOS event, rather then waiting for
>>> the next normally scheduled run.
>>>
>>> But even then I don't remember seeing any code related to catching
>>> platform-profile changes done outside amd-pmf... ?
>>
>> It's not a platform profile change - it's an ACPI event.
>>
>> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
>>
>> This is the code you're looking for (in this specific patch):
>>
>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>> +{
>> +    struct amd_pmf_dev *pmf_dev = data;
>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>> +    int ret;
>> +
>> +    if (apmf_if->func.sbios_requests) {
>> +        struct apmf_sbios_req req;
>> +
>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>> +        if (ret) {
>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>> +            return;
>> +        }
>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>> +                 req.amt_event ? "Enabled" : "Disabled");
>> +            if (req.amt_event)
>> +                pmf_dev->is_amt_event = true;
>> +            else
>> +                pmf_dev->is_amt_event = !!req.amt_event;
>> +        }
>> +
>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>> +                 req.cql_event ? "Enabled" : "Disabled");
>> +            if (req.cql_event)
>> +                pmf_dev->is_cql_event = true;
>> +            else
>> +                pmf_dev->is_cql_event = !!req.cql_event;
>> +
>> +            /* update the target mode information */
>> +            amd_pmf_update_2_cql(pmf_dev);
>> +        }
>> +    }
>> +}
>> +
> 
> Right this is the AMT on/off path that bit I understand.
> This happens when switching to / away from balanced mode.
> 
> My question is what does the equivalent of these lines:
> 
> +		amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
> +		amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
> +		amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
> +		amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
> +				 config_store.prop[src][idx].sppt_apu_only, NULL);
> +		amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
> +				 config_store.prop[src][idx].stt_min, NULL);
> +		amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
> +		amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
> 
> When the profile is switched (by userspace, or through the hotkeys on
> the laptop) to low-power or to performance mode ?

Lenovo's firmware will handle the equivalent of changing relevant values 
for their platform through a BIOS interface in this case when they 
change ACPI platform profiles.  You will see in their driver something 
call "PSC" mode, and this is exactly that type of stuff.

> 
> Regards,
> 
> Hans
>

Hans de Goede July 28, 2022, 6:17 p.m. UTC | #11

Hi,

On 7/28/22 20:06, Limonciello, Mario wrote:
> On 7/28/2022 12:46, Hans de Goede wrote:
>> Hi,
>>
>> On 7/28/22 16:38, Limonciello, Mario wrote:
>>>
>>>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>>>       will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>>>       stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>>>       the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>>>       by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>>>       register its platform_profile support. We cannot rely on module ordering ensuring
>>>>>>       that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>>>       since there are no module load ordering guarantees.
>>>>>
>>>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>>>
>>>> Right, Shyam mentioned this in another part of the thread. As I
>>>> mentioned there IHMO it would still be good to check this in the driver
>>>> though. To catch cases where a BIOS for some reasons advertises an
>>>> unexpected combination of features.
>>>>
>>>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>>>       enable AMT and then the periodically run workqueue function from amd-pmf
>>>>>>       will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>>>       set to low-power or performance. Should the amd-pmf code then apply the static
>>>>>>       slider settings for low-power/performance which it has read from the ACPI
>>>>>>       tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>>>
>>>>>
>>>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>>>
>>>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>>>> reminds me that the code should probably reschedule (using mod_delayed_work)
>>>> the work to run immediately after a BIOS event, rather then waiting for
>>>> the next normally scheduled run.
>>>>
>>>> But even then I don't remember seeing any code related to catching
>>>> platform-profile changes done outside amd-pmf... ?
>>>
>>> It's not a platform profile change - it's an ACPI event.
>>>
>>> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
>>>
>>> This is the code you're looking for (in this specific patch):
>>>
>>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>> +{
>>> +    struct amd_pmf_dev *pmf_dev = data;
>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>> +    int ret;
>>> +
>>> +    if (apmf_if->func.sbios_requests) {
>>> +        struct apmf_sbios_req req;
>>> +
>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>> +        if (ret) {
>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>> +            return;
>>> +        }
>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>> +            if (req.amt_event)
>>> +                pmf_dev->is_amt_event = true;
>>> +            else
>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>> +        }
>>> +
>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>> +            if (req.cql_event)
>>> +                pmf_dev->is_cql_event = true;
>>> +            else
>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>> +
>>> +            /* update the target mode information */
>>> +            amd_pmf_update_2_cql(pmf_dev);
>>> +        }
>>> +    }
>>> +}
>>> +
>>
>> Right this is the AMT on/off path that bit I understand.
>> This happens when switching to / away from balanced mode.
>>
>> My question is what does the equivalent of these lines:
>>
>> +        amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
>> +        amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
>> +        amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
>> +        amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
>> +                 config_store.prop[src][idx].sppt_apu_only, NULL);
>> +        amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>> +                 config_store.prop[src][idx].stt_min, NULL);
>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
>>
>> When the profile is switched (by userspace, or through the hotkeys on
>> the laptop) to low-power or to performance mode ?
> 
> Lenovo's firmware will handle the equivalent of changing relevant values for their platform through a BIOS interface in this case when they change ACPI platform profiles.  You will see in their driver something call "PSC" mode, and this is exactly that type of stuff.

Ok I see, thank you for clarifying this.

So as for the AMT mode, since that is Lenovo only, I guess that means
that there is no need to do call amd_pmf_update_slider() when AMT
is being disabled since at this point the firmware will have
already set the values.

Actually this seems to mean that we must ensure that the AMD-PMF
code stops touching these settings as soon as the event is received.

Which would imply killing the periodic work when an AMT off event
is received from within the event handling and then restating it
when AMT is on (and making sure the work being queued or not state
matches the AMT on/off state at driver probe time) ?

Regards,

Hans

Mario Limonciello July 28, 2022, 9:01 p.m. UTC | #12

On 7/28/2022 13:17, Hans de Goede wrote:
> Hi,
> 
> On 7/28/22 20:06, Limonciello, Mario wrote:
>> On 7/28/2022 12:46, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 7/28/22 16:38, Limonciello, Mario wrote:
>>>>
>>>>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>>>>        will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>>>>        stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>>>>        the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>>>>        by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>>>>        register its platform_profile support. We cannot rely on module ordering ensuring
>>>>>>>        that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>>>>        since there are no module load ordering guarantees.
>>>>>>
>>>>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>>>>
>>>>> Right, Shyam mentioned this in another part of the thread. As I
>>>>> mentioned there IHMO it would still be good to check this in the driver
>>>>> though. To catch cases where a BIOS for some reasons advertises an
>>>>> unexpected combination of features.
>>>>>
>>>>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>>>>        enable AMT and then the periodically run workqueue function from amd-pmf
>>>>>>>        will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>>>>        set to low-power or performance. Should the amd-pmf code then apply the static
>>>>>>>        slider settings for low-power/performance which it has read from the ACPI
>>>>>>>        tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>>>>
>>>>>>
>>>>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>>>>
>>>>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>>>>> reminds me that the code should probably reschedule (using mod_delayed_work)
>>>>> the work to run immediately after a BIOS event, rather then waiting for
>>>>> the next normally scheduled run.
>>>>>
>>>>> But even then I don't remember seeing any code related to catching
>>>>> platform-profile changes done outside amd-pmf... ?
>>>>
>>>> It's not a platform profile change - it's an ACPI event.
>>>>
>>>> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
>>>>
>>>> This is the code you're looking for (in this specific patch):
>>>>
>>>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>> +{
>>>> +    struct amd_pmf_dev *pmf_dev = data;
>>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>>> +    int ret;
>>>> +
>>>> +    if (apmf_if->func.sbios_requests) {
>>>> +        struct apmf_sbios_req req;
>>>> +
>>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>>> +        if (ret) {
>>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>>> +            return;
>>>> +        }
>>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>>> +            if (req.amt_event)
>>>> +                pmf_dev->is_amt_event = true;
>>>> +            else
>>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>>> +        }
>>>> +
>>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>>> +            if (req.cql_event)
>>>> +                pmf_dev->is_cql_event = true;
>>>> +            else
>>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>>> +
>>>> +            /* update the target mode information */
>>>> +            amd_pmf_update_2_cql(pmf_dev);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>
>>> Right this is the AMT on/off path that bit I understand.
>>> This happens when switching to / away from balanced mode.
>>>
>>> My question is what does the equivalent of these lines:
>>>
>>> +        amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
>>> +                 config_store.prop[src][idx].sppt_apu_only, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>>> +                 config_store.prop[src][idx].stt_min, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
>>>
>>> When the profile is switched (by userspace, or through the hotkeys on
>>> the laptop) to low-power or to performance mode ?
>>
>> Lenovo's firmware will handle the equivalent of changing relevant values for their platform through a BIOS interface in this case when they change ACPI platform profiles.  You will see in their driver something call "PSC" mode, and this is exactly that type of stuff.
> 
> Ok I see, thank you for clarifying this.
> 
> So as for the AMT mode, since that is Lenovo only, I guess that means
> that there is no need to do call amd_pmf_update_slider() when AMT
> is being disabled since at this point the firmware will have
> already set the values.

Yeah, Shyam made this modification for v2 to make sure that code path 
isn't called unless static slider was set in the BIOS.

> 
> Actually this seems to mean that we must ensure that the AMD-PMF
> code stops touching these settings as soon as the event is received.
> 
> Which would imply killing the periodic work when an AMT off event
> is received from within the event handling and then restating it
> when AMT is on (and making sure the work being queued or not state
> matches the AMT on/off state at driver probe time) ?
> 

At first glance this seems plausible, but actually I think it should 
stay as is because CQL thermals can be set at any time (that's like a 
lap mode sensor event from thinkpad_acpi).  Even when AMT is turned off, 
you may want the CQL thermal profile set accordingly.

Hans de Goede July 29, 2022, 11:03 a.m. UTC | #13

Hi,

On 7/28/22 23:01, Limonciello, Mario wrote:
> On 7/28/2022 13:17, Hans de Goede wrote:
>> Hi,
>>
>> On 7/28/22 20:06, Limonciello, Mario wrote:
>>> On 7/28/2022 12:46, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 7/28/22 16:38, Limonciello, Mario wrote:
>>>>>
>>>>>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>>>>>        will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>>>>>        stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>>>>>        the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>>>>>        by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>>>>>        register its platform_profile support. We cannot rely on module ordering ensuring
>>>>>>>>        that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>>>>>        since there are no module load ordering guarantees.
>>>>>>>
>>>>>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>>>>>
>>>>>> Right, Shyam mentioned this in another part of the thread. As I
>>>>>> mentioned there IHMO it would still be good to check this in the driver
>>>>>> though. To catch cases where a BIOS for some reasons advertises an
>>>>>> unexpected combination of features.
>>>>>>
>>>>>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>>>>>        enable AMT and then the periodically run workqueue function from amd-pmf
>>>>>>>>        will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>>>>>        set to low-power or performance. Should the amd-pmf code then apply the static
>>>>>>>>        slider settings for low-power/performance which it has read from the ACPI
>>>>>>>>        tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>>>>>
>>>>>>>
>>>>>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>>>>>
>>>>>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>>>>>> reminds me that the code should probably reschedule (using mod_delayed_work)
>>>>>> the work to run immediately after a BIOS event, rather then waiting for
>>>>>> the next normally scheduled run.
>>>>>>
>>>>>> But even then I don't remember seeing any code related to catching
>>>>>> platform-profile changes done outside amd-pmf... ?
>>>>>
>>>>> It's not a platform profile change - it's an ACPI event.
>>>>>
>>>>> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
>>>>>
>>>>> This is the code you're looking for (in this specific patch):
>>>>>
>>>>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>>> +{
>>>>> +    struct amd_pmf_dev *pmf_dev = data;
>>>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (apmf_if->func.sbios_requests) {
>>>>> +        struct apmf_sbios_req req;
>>>>> +
>>>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>>>> +        if (ret) {
>>>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>>>> +            return;
>>>>> +        }
>>>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>>>> +            if (req.amt_event)
>>>>> +                pmf_dev->is_amt_event = true;
>>>>> +            else
>>>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>>>> +        }
>>>>> +
>>>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>>>> +            if (req.cql_event)
>>>>> +                pmf_dev->is_cql_event = true;
>>>>> +            else
>>>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>>>> +
>>>>> +            /* update the target mode information */
>>>>> +            amd_pmf_update_2_cql(pmf_dev);
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>
>>>> Right this is the AMT on/off path that bit I understand.
>>>> This happens when switching to / away from balanced mode.
>>>>
>>>> My question is what does the equivalent of these lines:
>>>>
>>>> +        amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
>>>> +                 config_store.prop[src][idx].sppt_apu_only, NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>>>> +                 config_store.prop[src][idx].stt_min, NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
>>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
>>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
>>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
>>>>
>>>> When the profile is switched (by userspace, or through the hotkeys on
>>>> the laptop) to low-power or to performance mode ?
>>>
>>> Lenovo's firmware will handle the equivalent of changing relevant values for their platform through a BIOS interface in this case when they change ACPI platform profiles.  You will see in their driver something call "PSC" mode, and this is exactly that type of stuff.
>>
>> Ok I see, thank you for clarifying this.
>>
>> So as for the AMT mode, since that is Lenovo only, I guess that means
>> that there is no need to do call amd_pmf_update_slider() when AMT
>> is being disabled since at this point the firmware will have
>> already set the values.
> 
> Yeah, Shyam made this modification for v2 to make sure that code path isn't called unless static slider was set in the BIOS.

But this code path is only hit when AMT / auto mode is available and
when that is true then the static slider should never be set in the BIOS
so the whole amd_pmf_update_slider() call on AMT disable can simply
be dropped AFAICT.

> 
>>
>> Actually this seems to mean that we must ensure that the AMD-PMF
>> code stops touching these settings as soon as the event is received.
>>
>> Which would imply killing the periodic work when an AMT off event
>> is received from within the event handling and then restating it
>> when AMT is on (and making sure the work being queued or not state
>> matches the AMT on/off state at driver probe time) ?
>>
> 
> At first glance this seems plausible, but actually I think it should stay as is because CQL thermals can be set at any time (that's like a lap mode sensor event from thinkpad_acpi).  Even when AMT is turned off, you may want the CQL thermal profile set accordingly.

So the CQL code is to handle lapmode when AMT is active. But I would
expect the firmware to update the power-limits, etc. for lapmode itself
when in performance mode.

The amd_pmf_update_2_cql() function only does things when
config_store.current_mode == AUTO_PERFORMANCE (or AUTO_PERFORMANCE_ON_LAP)

And that reflects the last mode selected by the auto/AMT mode code, not
the mode actual set by thinkpad_acpi so if the last auto selected mode
was balanced and then AMT gets disabled because thinkpad_acpi switches
to performance mode, then on CQL events after the switch amd_pmf_update_2_cql()
will not do anything.

To me it seems that when AMT is off the AMD-PMF code should not touch
the power-limits, etc. at all and thus it should also always ignore
CQL events when AMT is off.

This assumes that the firmware takes care of udating the limits for
on lap / off lap when thinkpad_acpi's profile is set to performance.

If thinkpad_acpi does not do this then the AMD-PMF code should
check what mode has been selected by the thinkpad_acpi code in
amd_pmf_update_2_cql() when AMT is off.

Regards,

Hans

Mario Limonciello July 29, 2022, 3:43 p.m. UTC | #14

On 7/29/2022 06:03, Hans de Goede wrote:
>>>
>>> So as for the AMT mode, since that is Lenovo only, I guess that means
>>> that there is no need to do call amd_pmf_update_slider() when AMT
>>> is being disabled since at this point the firmware will have
>>> already set the values.
>>
>> Yeah, Shyam made this modification for v2 to make sure that code path isn't called unless static slider was set in the BIOS.
> 
> But this code path is only hit when AMT / auto mode is available and
> when that is true then the static slider should never be set in the BIOS
> so the whole amd_pmf_update_slider() call on AMT disable can simply
> be dropped AFAICT.

The reason to leave it in place but guarded like this is for validation 
of the feature behaves properly from AMD internal systems AMD test BIOS. 
  It can be used to prove out something works properly without needing 
to include extra drivers and software.

> 
>>
>>>
>>> Actually this seems to mean that we must ensure that the AMD-PMF
>>> code stops touching these settings as soon as the event is received.
>>>
>>> Which would imply killing the periodic work when an AMT off event
>>> is received from within the event handling and then restating it
>>> when AMT is on (and making sure the work being queued or not state
>>> matches the AMT on/off state at driver probe time) ?
>>>
>>
>> At first glance this seems plausible, but actually I think it should stay as is because CQL thermals can be set at any time (that's like a lap mode sensor event from thinkpad_acpi).  Even when AMT is turned off, you may want the CQL thermal profile set accordingly.
> 
> So the CQL code is to handle lapmode when AMT is active. But I would
> expect the firmware to update the power-limits, etc. for lapmode itself
> when in performance mode. >
> The amd_pmf_update_2_cql() function only does things when
> config_store.current_mode == AUTO_PERFORMANCE (or AUTO_PERFORMANCE_ON_LAP)
> 
> And that reflects the last mode selected by the auto/AMT mode code, not
> the mode actual set by thinkpad_acpi so if the last auto selected mode
> was balanced and then AMT gets disabled because thinkpad_acpi switches
> to performance mode, then on CQL events after the switch amd_pmf_update_2_cql()
> will not do anything.
> 
> To me it seems that when AMT is off the AMD-PMF code should not touch
> the power-limits, etc. at all and thus it should also always ignore
> CQL events when AMT is off.
> 
> This assumes that the firmware takes care of udating the limits for
> on lap / off lap when thinkpad_acpi's profile is set to performance.

Where does this assumption come from?  I guess that's how it's done on 
Lenovo's Intel systems?

AMT and CQL is a new feature on Lenovo AMD systems, this is the way that 
it's supposed to be done here.

> 
> If thinkpad_acpi does not do this then the AMD-PMF code should
> check what mode has been selected by the thinkpad_acpi code in
> amd_pmf_update_2_cql() when AMT is off.
> 

It is up to the firmware (and thinkpad_acpi) to decide when to send
the CQL events.

Shyam - any comments here?

Shyam Sundar S K July 29, 2022, 5:40 p.m. UTC | #15

Hi Mario,

On 7/29/2022 9:13 PM, Limonciello, Mario wrote:
> On 7/29/2022 06:03, Hans de Goede wrote:
>>>>
>>>> So as for the AMT mode, since that is Lenovo only, I guess that means
>>>> that there is no need to do call amd_pmf_update_slider() when AMT
>>>> is being disabled since at this point the firmware will have
>>>> already set the values.
>>>
>>> Yeah, Shyam made this modification for v2 to make sure that code path
>>> isn't called unless static slider was set in the BIOS.
>>
>> But this code path is only hit when AMT / auto mode is available and
>> when that is true then the static slider should never be set in the BIOS
>> so the whole amd_pmf_update_slider() call on AMT disable can simply
>> be dropped AFAICT.
> 
> The reason to leave it in place but guarded like this is for validation
> of the feature behaves properly from AMD internal systems AMD test BIOS.
>  It can be used to prove out something works properly without needing to
> include extra drivers and software.

Yes. We will need this path to check on the internal CRB system to
validate the 'auto mode'. Whenever the amd-pmf driver gets the AMT
disable event we shall disable the power-settings w.r.t to 'auto mode'.

I moved the handling to amd_pmf_reset_amt() based on Hans review
remarks, and its guarded with a if() check, so that we accidentally
don't land up in updating the static slider.

Also left a note on the same function, so that it provides some
information on why the logic is being done in that way.

> 
>>
>>>
>>>>
>>>> Actually this seems to mean that we must ensure that the AMD-PMF
>>>> code stops touching these settings as soon as the event is received.
>>>>
>>>> Which would imply killing the periodic work when an AMT off event
>>>> is received from within the event handling and then restating it
>>>> when AMT is on (and making sure the work being queued or not state
>>>> matches the AMT on/off state at driver probe time) ?
>>>>
>>>
>>> At first glance this seems plausible, but actually I think it should
>>> stay as is because CQL thermals can be set at any time (that's like a
>>> lap mode sensor event from thinkpad_acpi).  Even when AMT is turned
>>> off, you may want the CQL thermal profile set accordingly.
>>
>> So the CQL code is to handle lapmode when AMT is active. But I would
>> expect the firmware to update the power-limits, etc. for lapmode itself
>> when in performance mode. >
>> The amd_pmf_update_2_cql() function only does things when
>> config_store.current_mode == AUTO_PERFORMANCE (or
>> AUTO_PERFORMANCE_ON_LAP)
>>
>> And that reflects the last mode selected by the auto/AMT mode code, not
>> the mode actual set by thinkpad_acpi so if the last auto selected mode
>> was balanced and then AMT gets disabled because thinkpad_acpi switches
>> to performance mode, then on CQL events after the switch
>> amd_pmf_update_2_cql()
>> will not do anything.
>>
>> To me it seems that when AMT is off the AMD-PMF code should not touch
>> the power-limits, etc. at all and thus it should also always ignore
>> CQL events when AMT is off.
>>
>> This assumes that the firmware takes care of udating the limits for
>> on lap / off lap when thinkpad_acpi's profile is set to performance.
> 
> Where does this assumption come from?  I guess that's how it's done on
> Lenovo's Intel systems?
> 
> AMT and CQL is a new feature on Lenovo AMD systems, this is the way that
> it's supposed to be done here.

Yes, this was newly designed for Lenovo AMD systems. The behavior is
same on windows too (atleast on the RMB laptops today) .

When the system is running in 'auto-mode performance' and the user keeps
the system on his lap, amd-pmf driver receives a 'CQL' event from Lenovo
BIOS. In this case, the amd-pmf driver shall apply thermal limits w.r.t
to 'auto-mode performance-on-lap' and not 'auto-mode performance'.


> 
>>
>> If thinkpad_acpi does not do this then the AMD-PMF code should
>> check what mode has been selected by the thinkpad_acpi code in
>> amd_pmf_update_2_cql() when AMT is off.
>>
> 
> It is up to the firmware (and thinkpad_acpi) to decide when to send
> the CQL events.
> 
> Shyam - any comments here?

Yes, I agree with Mario here.

Thanks,
Shyam

Hans de Goede July 29, 2022, 5:59 p.m. UTC | #16

Hi,

On 7/29/22 19:40, Shyam Sundar S K wrote:
> Hi Mario,
> 
> On 7/29/2022 9:13 PM, Limonciello, Mario wrote:
>> On 7/29/2022 06:03, Hans de Goede wrote:
>>>>>
>>>>> So as for the AMT mode, since that is Lenovo only, I guess that means
>>>>> that there is no need to do call amd_pmf_update_slider() when AMT
>>>>> is being disabled since at this point the firmware will have
>>>>> already set the values.
>>>>
>>>> Yeah, Shyam made this modification for v2 to make sure that code path
>>>> isn't called unless static slider was set in the BIOS.
>>>
>>> But this code path is only hit when AMT / auto mode is available and
>>> when that is true then the static slider should never be set in the BIOS
>>> so the whole amd_pmf_update_slider() call on AMT disable can simply
>>> be dropped AFAICT.
>>
>> The reason to leave it in place but guarded like this is for validation
>> of the feature behaves properly from AMD internal systems AMD test BIOS.
>>  It can be used to prove out something works properly without needing to
>> include extra drivers and software.
> 
> Yes. We will need this path to check on the internal CRB system to
> validate the 'auto mode'. Whenever the amd-pmf driver gets the AMT
> disable event we shall disable the power-settings w.r.t to 'auto mode'.
> 
> I moved the handling to amd_pmf_reset_amt() based on Hans review
> remarks, and its guarded with a if() check, so that we accidentally
> don't land up in updating the static slider.
> 
> Also left a note on the same function, so that it provides some
> information on why the logic is being done in that way.
> 
>>
>>>
>>>>
>>>>>
>>>>> Actually this seems to mean that we must ensure that the AMD-PMF
>>>>> code stops touching these settings as soon as the event is received.
>>>>>
>>>>> Which would imply killing the periodic work when an AMT off event
>>>>> is received from within the event handling and then restating it
>>>>> when AMT is on (and making sure the work being queued or not state
>>>>> matches the AMT on/off state at driver probe time) ?
>>>>>
>>>>
>>>> At first glance this seems plausible, but actually I think it should
>>>> stay as is because CQL thermals can be set at any time (that's like a
>>>> lap mode sensor event from thinkpad_acpi).  Even when AMT is turned
>>>> off, you may want the CQL thermal profile set accordingly.
>>>
>>> So the CQL code is to handle lapmode when AMT is active. But I would
>>> expect the firmware to update the power-limits, etc. for lapmode itself
>>> when in performance mode. >
>>> The amd_pmf_update_2_cql() function only does things when
>>> config_store.current_mode == AUTO_PERFORMANCE (or
>>> AUTO_PERFORMANCE_ON_LAP)
>>>
>>> And that reflects the last mode selected by the auto/AMT mode code, not
>>> the mode actual set by thinkpad_acpi so if the last auto selected mode
>>> was balanced and then AMT gets disabled because thinkpad_acpi switches
>>> to performance mode, then on CQL events after the switch
>>> amd_pmf_update_2_cql()
>>> will not do anything.
>>>
>>> To me it seems that when AMT is off the AMD-PMF code should not touch
>>> the power-limits, etc. at all and thus it should also always ignore
>>> CQL events when AMT is off.
>>>
>>> This assumes that the firmware takes care of udating the limits for
>>> on lap / off lap when thinkpad_acpi's profile is set to performance.
>>
>> Where does this assumption come from?  I guess that's how it's done on
>> Lenovo's Intel systems?
>>
>> AMT and CQL is a new feature on Lenovo AMD systems, this is the way that
>> it's supposed to be done here.
> 
> Yes, this was newly designed for Lenovo AMD systems. The behavior is
> same on windows too (atleast on the RMB laptops today) .
> 
> When the system is running in 'auto-mode performance' and the user keeps
> the system on his lap, amd-pmf driver receives a 'CQL' event from Lenovo
> BIOS. In this case, the amd-pmf driver shall apply thermal limits w.r.t
> to 'auto-mode performance-on-lap' and not 'auto-mode performance'.

The question here is not about the 'auto-mode performance' mode
but what to do when AMT / 'auto-mode performance' is disabled.

What should the behavior of the AMD-PMf code be when it receives
a CQL event when AMT is disabled ?

>>> If thinkpad_acpi does not do this then the AMD-PMF code should
>>> check what mode has been selected by the thinkpad_acpi code in
>>> amd_pmf_update_2_cql() when AMT is off.

Regards,

Hans

Shyam Sundar S K Aug. 1, 2022, 10:29 a.m. UTC | #17

Hi Hans,

On 7/29/2022 11:29 PM, Hans de Goede wrote:
> Hi,
> 
> On 7/29/22 19:40, Shyam Sundar S K wrote:
>> Hi Mario,
>>
>> On 7/29/2022 9:13 PM, Limonciello, Mario wrote:
>>> On 7/29/2022 06:03, Hans de Goede wrote:
>>>>>>
>>>>>> So as for the AMT mode, since that is Lenovo only, I guess that means
>>>>>> that there is no need to do call amd_pmf_update_slider() when AMT
>>>>>> is being disabled since at this point the firmware will have
>>>>>> already set the values.
>>>>>
>>>>> Yeah, Shyam made this modification for v2 to make sure that code path
>>>>> isn't called unless static slider was set in the BIOS.
>>>>
>>>> But this code path is only hit when AMT / auto mode is available and
>>>> when that is true then the static slider should never be set in the BIOS
>>>> so the whole amd_pmf_update_slider() call on AMT disable can simply
>>>> be dropped AFAICT.
>>>
>>> The reason to leave it in place but guarded like this is for validation
>>> of the feature behaves properly from AMD internal systems AMD test BIOS.
>>>  It can be used to prove out something works properly without needing to
>>> include extra drivers and software.
>>
>> Yes. We will need this path to check on the internal CRB system to
>> validate the 'auto mode'. Whenever the amd-pmf driver gets the AMT
>> disable event we shall disable the power-settings w.r.t to 'auto mode'.
>>
>> I moved the handling to amd_pmf_reset_amt() based on Hans review
>> remarks, and its guarded with a if() check, so that we accidentally
>> don't land up in updating the static slider.
>>
>> Also left a note on the same function, so that it provides some
>> information on why the logic is being done in that way.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Actually this seems to mean that we must ensure that the AMD-PMF
>>>>>> code stops touching these settings as soon as the event is received.
>>>>>>
>>>>>> Which would imply killing the periodic work when an AMT off event
>>>>>> is received from within the event handling and then restating it
>>>>>> when AMT is on (and making sure the work being queued or not state
>>>>>> matches the AMT on/off state at driver probe time) ?
>>>>>>
>>>>>
>>>>> At first glance this seems plausible, but actually I think it should
>>>>> stay as is because CQL thermals can be set at any time (that's like a
>>>>> lap mode sensor event from thinkpad_acpi).  Even when AMT is turned
>>>>> off, you may want the CQL thermal profile set accordingly.
>>>>
>>>> So the CQL code is to handle lapmode when AMT is active. But I would
>>>> expect the firmware to update the power-limits, etc. for lapmode itself
>>>> when in performance mode. >
>>>> The amd_pmf_update_2_cql() function only does things when
>>>> config_store.current_mode == AUTO_PERFORMANCE (or
>>>> AUTO_PERFORMANCE_ON_LAP)
>>>>
>>>> And that reflects the last mode selected by the auto/AMT mode code, not
>>>> the mode actual set by thinkpad_acpi so if the last auto selected mode
>>>> was balanced and then AMT gets disabled because thinkpad_acpi switches
>>>> to performance mode, then on CQL events after the switch
>>>> amd_pmf_update_2_cql()
>>>> will not do anything.
>>>>
>>>> To me it seems that when AMT is off the AMD-PMF code should not touch
>>>> the power-limits, etc. at all and thus it should also always ignore
>>>> CQL events when AMT is off.
>>>>
>>>> This assumes that the firmware takes care of udating the limits for
>>>> on lap / off lap when thinkpad_acpi's profile is set to performance.
>>>
>>> Where does this assumption come from?  I guess that's how it's done on
>>> Lenovo's Intel systems?
>>>
>>> AMT and CQL is a new feature on Lenovo AMD systems, this is the way that
>>> it's supposed to be done here.
>>
>> Yes, this was newly designed for Lenovo AMD systems. The behavior is
>> same on windows too (atleast on the RMB laptops today) .
>>
>> When the system is running in 'auto-mode performance' and the user keeps
>> the system on his lap, amd-pmf driver receives a 'CQL' event from Lenovo
>> BIOS. In this case, the amd-pmf driver shall apply thermal limits w.r.t
>> to 'auto-mode performance-on-lap' and not 'auto-mode performance'.
> 
> The question here is not about the 'auto-mode performance' mode
> but what to do when AMT / 'auto-mode performance' is disabled.
> 
> What should the behavior of the AMD-PMf code be when it receives
> a CQL event when AMT is disabled ?

When:
1. AMT is disabled and we get a CQL event, it becomes a no-op to the
amd-pmf driver.
2. AMT is enabled:
  - Avg. SoC power is higher than a selected measure, the amd-pmf driver
tries to move to 'auto-mode performance' and apply the thermals set in
the BIOS for 'auto-mode peformance' but in this scenario, when we are in
'auto-mode performance' and user moves the laptop from desk to lap, we
receive a 'on-lap' event. In this case we apply thermals w.r.t to
'auto-mode performance-on-lap' and not 'auto-mode performance'.

That is what is being done in amd_pmf_update_2_cql() with a check:
	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;

Update of CQL happens only when AMT is active.

Thanks,
Shyam

> 
>>>> If thinkpad_acpi does not do this then the AMD-PMF code should
>>>> check what mode has been selected by the thinkpad_acpi code in
>>>> amd_pmf_update_2_cql() when AMT is off.
> 
> Regards,
> 
> Hans
>

Hans de Goede Aug. 1, 2022, 11:08 a.m. UTC | #18

Hi,

On 8/1/22 12:29, Shyam Sundar S K wrote:
> Hi Hans,
> 
> On 7/29/2022 11:29 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 7/29/22 19:40, Shyam Sundar S K wrote:
>>> Hi Mario,
>>>
>>> On 7/29/2022 9:13 PM, Limonciello, Mario wrote:
>>>> On 7/29/2022 06:03, Hans de Goede wrote:
>>>>>>>
>>>>>>> So as for the AMT mode, since that is Lenovo only, I guess that means
>>>>>>> that there is no need to do call amd_pmf_update_slider() when AMT
>>>>>>> is being disabled since at this point the firmware will have
>>>>>>> already set the values.
>>>>>>
>>>>>> Yeah, Shyam made this modification for v2 to make sure that code path
>>>>>> isn't called unless static slider was set in the BIOS.
>>>>>
>>>>> But this code path is only hit when AMT / auto mode is available and
>>>>> when that is true then the static slider should never be set in the BIOS
>>>>> so the whole amd_pmf_update_slider() call on AMT disable can simply
>>>>> be dropped AFAICT.
>>>>
>>>> The reason to leave it in place but guarded like this is for validation
>>>> of the feature behaves properly from AMD internal systems AMD test BIOS.
>>>>  It can be used to prove out something works properly without needing to
>>>> include extra drivers and software.
>>>
>>> Yes. We will need this path to check on the internal CRB system to
>>> validate the 'auto mode'. Whenever the amd-pmf driver gets the AMT
>>> disable event we shall disable the power-settings w.r.t to 'auto mode'.
>>>
>>> I moved the handling to amd_pmf_reset_amt() based on Hans review
>>> remarks, and its guarded with a if() check, so that we accidentally
>>> don't land up in updating the static slider.
>>>
>>> Also left a note on the same function, so that it provides some
>>> information on why the logic is being done in that way.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Actually this seems to mean that we must ensure that the AMD-PMF
>>>>>>> code stops touching these settings as soon as the event is received.
>>>>>>>
>>>>>>> Which would imply killing the periodic work when an AMT off event
>>>>>>> is received from within the event handling and then restating it
>>>>>>> when AMT is on (and making sure the work being queued or not state
>>>>>>> matches the AMT on/off state at driver probe time) ?
>>>>>>>
>>>>>>
>>>>>> At first glance this seems plausible, but actually I think it should
>>>>>> stay as is because CQL thermals can be set at any time (that's like a
>>>>>> lap mode sensor event from thinkpad_acpi).  Even when AMT is turned
>>>>>> off, you may want the CQL thermal profile set accordingly.
>>>>>
>>>>> So the CQL code is to handle lapmode when AMT is active. But I would
>>>>> expect the firmware to update the power-limits, etc. for lapmode itself
>>>>> when in performance mode. >
>>>>> The amd_pmf_update_2_cql() function only does things when
>>>>> config_store.current_mode == AUTO_PERFORMANCE (or
>>>>> AUTO_PERFORMANCE_ON_LAP)
>>>>>
>>>>> And that reflects the last mode selected by the auto/AMT mode code, not
>>>>> the mode actual set by thinkpad_acpi so if the last auto selected mode
>>>>> was balanced and then AMT gets disabled because thinkpad_acpi switches
>>>>> to performance mode, then on CQL events after the switch
>>>>> amd_pmf_update_2_cql()
>>>>> will not do anything.
>>>>>
>>>>> To me it seems that when AMT is off the AMD-PMF code should not touch
>>>>> the power-limits, etc. at all and thus it should also always ignore
>>>>> CQL events when AMT is off.
>>>>>
>>>>> This assumes that the firmware takes care of udating the limits for
>>>>> on lap / off lap when thinkpad_acpi's profile is set to performance.
>>>>
>>>> Where does this assumption come from?  I guess that's how it's done on
>>>> Lenovo's Intel systems?
>>>>
>>>> AMT and CQL is a new feature on Lenovo AMD systems, this is the way that
>>>> it's supposed to be done here.
>>>
>>> Yes, this was newly designed for Lenovo AMD systems. The behavior is
>>> same on windows too (atleast on the RMB laptops today) .
>>>
>>> When the system is running in 'auto-mode performance' and the user keeps
>>> the system on his lap, amd-pmf driver receives a 'CQL' event from Lenovo
>>> BIOS. In this case, the amd-pmf driver shall apply thermal limits w.r.t
>>> to 'auto-mode performance-on-lap' and not 'auto-mode performance'.
>>
>> The question here is not about the 'auto-mode performance' mode
>> but what to do when AMT / 'auto-mode performance' is disabled.
>>
>> What should the behavior of the AMD-PMf code be when it receives
>> a CQL event when AMT is disabled ?
> 
> When:
> 1. AMT is disabled and we get a CQL event, it becomes a no-op to the
> amd-pmf driver.

But that is not what happens in the current (v2) code:

1. The apmf_event_handler() is always registered as long as the driver is bound
   (which it must be to catch AMT being re-enabled)

2. apmf_event_handler() does:

+		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
+			pr_debug("PMF: CQL is supported and notifications %s\n",
+				 req.cql_event ? "Enabled" : "Disabled");
+			pmf_dev->is_cql_event = !!req.cql_event;
+
+			/* update the target mode information */
+			amd_pmf_update_2_cql(pmf_dev);
+		}

3. amd_pmf_update_2_cql() does:

+void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
+{
+	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
+			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
+	if ((config_store.current_mode == AUTO_PERFORMANCE ||
+	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
+	    config_store.current_mode !=
+	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
+		config_store.current_mode =
+				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
+		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
+	}
+	dev_dbg(dev->dev, "updated CQL thermals\n");
+}

Note this does not check dev->is_amt_event at all so CQL events
may lead to amd_pmf_handle_automode() getting called, which sets
the limits even when AMT is disabled.

 
> 2. AMT is enabled:
>   - Avg. SoC power is higher than a selected measure, the amd-pmf driver
> tries to move to 'auto-mode performance' and apply the thermals set in
> the BIOS for 'auto-mode peformance' but in this scenario, when we are in
> 'auto-mode performance' and user moves the laptop from desk to lap, we
> receive a 'on-lap' event. In this case we apply thermals w.r.t to
> 'auto-mode performance-on-lap' and not 'auto-mode performance'.
> 
> That is what is being done in amd_pmf_update_2_cql() with a check:
> 	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
> 			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;

Right this much I understand.

> Update of CQL happens only when AMT is active.

So you mean the SBIOS is expected to only send CQL events when it has
set AMT to be enabled ?  AFAIK the CQL events are basically on-lap /
not-on-lap events, these can easily also happen when not in AMT mode,
so the SBIOS would need to explicitly not send these events when
lap-mode changes when AMT is disabled.

Since firmware bugs do happen and can sometimes be hard to fix /
have long times to get published IMHO it would be best if 
amd_pmf_update_2_cql() would check dev->is_amt_event and return
early from the function if that is not set.

Regards,

Hans

Patch

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index e9f33e61659f..4bde86aeafa0 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -12,6 +12,8 @@ 
 #include "pmf.h"
 
 #define APMF_METHOD "\\_SB_.PMF_.APMF"
+#define APMF_CQL_NOTIFICATION	2
+#define APMF_AMT_NOTIFICATION	3
 
 static unsigned long supported_func;
 
@@ -55,6 +57,7 @@  static void apmf_if_parse_functions(struct apmf_if_functions *func, u32 mask)
 {
 	func->system_params = mask & APMF_FUNC_GET_SYS_PARAMS;
 	func->static_slider_granular = mask & APMF_FUNC_STATIC_SLIDER_GRANULAR;
+	func->sbios_requests = mask & APMF_FUNC_SBIOS_REQUESTS;
 	func->auto_mode_def = mask & APMF_FUNC_AUTO_MODE;
 	func->fan_table_idx = mask & APMF_FUNC_SET_FAN_IDX;
 	func->dyn_slider_ac = mask & APMF_FUNC_DYN_SLIDER_GRANULAR_AC;
@@ -292,6 +295,36 @@  int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
 	return err;
 }
 
+int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req)
+{
+	union acpi_object *info;
+	size_t size;
+	int err = 0;
+
+	info = apmf_if_call(ampf_if, APMF_FUNC_SBIOS_REQUESTS, NULL);
+	if (!info)
+		return -EIO;
+
+	size = *(u16 *)info->buffer.pointer;
+
+	if (size < sizeof(union acpi_object)) {
+		pr_err("PMF: buffer too small %zu\n", size);
+		err = -EINVAL;
+		goto out;
+	}
+
+	size = min(sizeof(*req), size);
+	memset(req, 0, sizeof(*req));
+	memcpy(req, info->buffer.pointer, size);
+
+	pr_debug("PMF: %s: pending_req:%d cql:%d amt:%d\n", __func__,
+		 req->pending_req, req->cql_event, req->amt_event);
+
+out:
+	kfree(info);
+	return err;
+}
+
 static acpi_handle apmf_if_probe_handle(void)
 {
 	acpi_handle handle = NULL;
@@ -312,18 +345,62 @@  static acpi_handle apmf_if_probe_handle(void)
 	return handle;
 }
 
+static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
+{
+	struct amd_pmf_dev *pmf_dev = data;
+	struct apmf_if *apmf_if = pmf_dev->apmf_if;
+	int ret;
+
+	if (apmf_if->func.sbios_requests) {
+		struct apmf_sbios_req req;
+
+		ret = apmf_get_sbios_requests(apmf_if, &req);
+		if (ret) {
+			dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
+			return;
+		}
+		if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
+			pr_debug("PMF: AMT is supported and notifications %s\n",
+				 req.amt_event ? "Enabled" : "Disabled");
+			if (req.amt_event)
+				pmf_dev->is_amt_event = true;
+			else
+				pmf_dev->is_amt_event = !!req.amt_event;
+		}
+
+		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
+			pr_debug("PMF: CQL is supported and notifications %s\n",
+				 req.cql_event ? "Enabled" : "Disabled");
+			if (req.cql_event)
+				pmf_dev->is_cql_event = true;
+			else
+				pmf_dev->is_cql_event = !!req.cql_event;
+
+			/* update the target mode information */
+			amd_pmf_update_2_cql(pmf_dev);
+		}
+	}
+}
+
 void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
 {
+	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
+
 	if (pmf_dev->apmf_if->func.sbios_heartbeat)
 		cancel_delayed_work_sync(&pmf_dev->heart_beat);
+
+	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE))
+		acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY,
+					   apmf_event_handler);
 }
 
 int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
 {
+	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
 	struct apmf_notification_cfg *n;
 	acpi_handle apmf_if_handle;
 	struct apmf_if *apmf_if;
-	int ret;
+	int ret, status;
 
 	apmf_if_handle = apmf_if_probe_handle();
 	if (!apmf_if_handle)
@@ -360,6 +437,17 @@  int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
 		schedule_delayed_work(&pmf_dev->heart_beat, 0);
 	}
 
+	/* Install the APMF Notify handler */
+	if (is_apmf_func_supported(APMF_FUNC_AUTO_MODE)) {
+		status = acpi_install_notify_handler(ahandle,
+						     ACPI_ALL_NOTIFY,
+						     apmf_event_handler, pmf_dev);
+		if (ACPI_FAILURE(status)) {
+			dev_err(pmf_dev->dev, "failed to install notify handler\n");
+			return -ENODEV;
+		}
+	}
+
 out:
 	return ret;
 }
diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
index 954fde25e71e..a85ef4b95cdb 100644
--- a/drivers/platform/x86/amd/pmf/auto-mode.c
+++ b/drivers/platform/x86/amd/pmf/auto-mode.c
@@ -111,6 +111,13 @@  void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
 	bool update = false;
 	int i, j;
 
+	if (!dev->amt_running && dev->is_amt_event) {
+		dev_dbg(dev->dev, "setting AMT thermals\n");
+		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
+		dev->amt_running = true;
+		return;
+	}
+
 	/* Get the average moving average computed by auto mode algorithm */
 	avg_power = amd_pmf_get_moving_avg(socket_power);
 
@@ -161,6 +168,21 @@  void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
 	}
 }
 
+void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
+{
+	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
+			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
+	if ((config_store.current_mode == AUTO_PERFORMANCE ||
+	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
+	    config_store.current_mode !=
+	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
+		config_store.current_mode =
+				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
+		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
+	}
+	dev_dbg(dev->dev, "updated CQL thermals\n");
+}
+
 static void amd_pmf_get_power_threshold(void)
 {
 	config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold =
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
index 2b03ae1ad37f..eba8f0d79a62 100644
--- a/drivers/platform/x86/amd/pmf/cnqf.c
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -101,7 +101,7 @@  static const char *state_as_str(unsigned int state)
 	}
 }
 
-void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms)
+void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms)
 {
 	struct cnqf_tran_params *tp;
 	int src, i, j, index = 0;
@@ -117,7 +117,7 @@  void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_
 	}
 
 	for (i = 0; i < CNQF_TRANSITION_MAX; i++) {
-		config_store.trans_param[src][i].timer += time_lapsed_ms;
+		config_store.trans_param[src][i].timer += time_elapsed_ms;
 		config_store.trans_param[src][i].total_power += socket_power;
 		config_store.trans_param[src][i].count++;
 
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 674ddf599135..2a3dacfb2005 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -109,10 +109,15 @@  static void amd_pmf_get_metrics(struct work_struct *work)
 	enum platform_profile_option current_profile;
 	ktime_t time_elapsed_ms;
 	int socket_power;
+	u8 mode;
 
 	/* Get the current profile information */
 	platform_profile_get(&current_profile);
 
+	if (!dev->is_amt_event)
+		dev_dbg(dev->dev, "%s amt event: %s\n", __func__,
+			dev->is_amt_event ? "Enabled" : "Disabled");
+
 	/* Transfer table contents */
 	memset(&dev->m_table, 0, sizeof(dev->m_table));
 	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
@@ -123,8 +128,17 @@  static void amd_pmf_get_metrics(struct work_struct *work)
 	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
 
 	if (current_profile == PLATFORM_PROFILE_BALANCED) {
-		/* Apply the Auto Mode transition */
-		amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+		if (dev->is_amt_event) {
+			/* Apply the Auto Mode transition */
+			amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+		} else if (!dev->is_amt_event && dev->amt_running) {
+			pr_debug("resetting AMT thermals\n");
+			mode = amd_pmf_get_pprof_modes(dev);
+			amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
+			dev->amt_running = false;
+		}
+	} else {
+		dev->amt_running = false;
 	}
 
 	if (dev->cnqf_feat) {
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 0532f49e9613..9ae9812353cd 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -17,6 +17,7 @@ 
 /* APMF Functions */
 #define APMF_FUNC_VERIFY_INTERFACE			0
 #define APMF_FUNC_GET_SYS_PARAMS			1
+#define APMF_FUNC_SBIOS_REQUESTS			2
 #define APMF_FUNC_SBIOS_HEARTBEAT			4
 #define APMF_FUNC_AUTO_MODE					5
 #define APMF_FUNC_SET_FAN_IDX				7
@@ -51,6 +52,7 @@ 
 /* AMD PMF BIOS interfaces */
 struct apmf_if_functions {
 	bool system_params;
+	bool sbios_requests;
 	bool sbios_heartbeat;
 	bool auto_mode_def;
 	bool fan_table_idx;
@@ -84,6 +86,24 @@  struct apmf_system_params {
 	u32 heartbeat_int;
 } __packed;
 
+struct apmf_sbios_req {
+	u16 size;
+	u32 pending_req;
+	u8 rsd;
+	u8 cql_event;
+	u8 amt_event;
+	u32 fppt;
+	u32 sppt;
+	u32 fppt_apu_only;
+	u32 spl;
+	u32 stt_min_limit;
+	u8 skin_temp_apu;
+	u8 skin_temp_hs2;
+	u8 dps_enable;
+	u32 custom_policy_1;
+	u32 custom_policy_2;
+} __packed;
+
 struct apmf_fan_idx {
 	u16 size;
 	u8 fan_ctl_mode;
@@ -171,6 +191,9 @@  struct amd_pmf_dev {
 #endif /* CONFIG_DEBUG_FS */
 	bool cnqf_feat;
 	bool cnqf_running;
+	bool is_amt_event;
+	bool is_cql_event;
+	bool amt_running;
 };
 
 struct apmf_sps_prop_granular {
@@ -417,9 +440,11 @@  int apmf_update_fan_idx(struct apmf_if *ampf_if, bool manual, u32 idx);
 /* Auto Mode Layer */
 void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev);
 int apmf_get_auto_mode_def(struct apmf_if *ampf_if, struct apmf_auto_mode *data);
+int apmf_get_sbios_requests(struct apmf_if *ampf_if, struct apmf_sbios_req *req);
 void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
 void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
-void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
+void amd_pmf_update_2_cql(struct amd_pmf_dev *dev);
+void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
 
 /* CnQF Layer */
 int apmf_get_dyn_slider_def_ac(struct apmf_if *ampf_if, struct apmf_dyn_slider_output *data);
@@ -427,6 +452,6 @@  int apmf_get_dyn_slider_def_dc(struct apmf_if *ampf_if, struct apmf_dyn_slider_o
 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);
+void amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
 
 #endif /* PMF_H */