Message-ID: <5a75b73f-dcb1-4a45-9526-194a3451b5c6@amd.com>
Date: Thu, 26 Sep 2024 15:55:32 +0530
From: Shyam Sundar S K <Shyam-sundar.S-k@....com>
To: Mario Limonciello <superm1@...nel.org>,
 "Rafael J . Wysocki" <rafael@...nel.org>, Hans de Goede
 <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, "Luke D . Jones" <luke@...nes.dev>,
 Mark Pearson <mpearson-lenovo@...ebb.ca>
Cc: "open list:AMD PMF DRIVER" <platform-driver-x86@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:ACPI" <linux-acpi@...r.kernel.org>,
 "Derek J . Clark" <derekjohn.clark@...il.com>,
 Antheas Kapenekakis <lkml@...heas.dev>, me@...egospodneti.ch,
 Denis Benato <benato.denis96@...il.com>,
 Mario Limonciello <mario.limonciello@....com>
Subject: Re: [RFC 2/2] platform/x86/amd: pmf: Add manual control support



On 9/26/2024 08:29, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@....com>
> 
> A number of users resort to using reverse engineered software like
> ryzenadj to manipulate debugging interfaces for modifying APU settings.
> 
> At a glance these tools are useful, but the problem is they break
> state machines in other software such as the PMF driver or the OEM
> EC.
> 
> Offer a knob for PMF to allow 'manual control' which will users can
> directly change things like fPPT and sPPT. As this can be harmful for
> a system to try to push limits outside of a thermal design, taint the
> kernel and show a critical message when in use.

I appreciate the proposal, but giving users this control seems similar
to using tools like Ryzenadj or Ryzen Master, which are primarily for
overclocking. Atleast Ryzen Master has a dedicated mailbox with PMFW.

While some existing PMF mailboxes are being deprecated, and SPL has
been removed starting with Strix[1] due to the APTS method.

It's important to use some settings together rather than individually
(which the users might not be aware of). For instance, updating SPL
requires corresponding updates to STT limits to avoid negative outcomes.

Additionally, altering these parameters can exceed thermal limits and
potentially void warranties.

Considering CnQF, why not let OEMs opt-in and allow the algorithm to
manage power budgets, rather than providing these controls to users
from the kernel when userspace tools already exist?

Please note that on systems with Smart PC enabled, if users manually
adjust the system thermals, it can lead to the thermal controls
becoming unmanageable.

Please consider this perspective.

[1]
https://github.com/torvalds/linux/blob/master/drivers/platform/x86/amd/pmf/sps.c#L193

IMHO, having a 'custom' platform-profile node is fine, but tieing that
to PMF static slider would be a NO from my side, because of the above
said reasons.

Thanks,
Shyam

> 
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
>  Documentation/ABI/testing/sysfs-amd-pmf | 10 +++
>  drivers/platform/x86/amd/pmf/Makefile   |  1 +
>  drivers/platform/x86/amd/pmf/core.c     |  9 +++
>  drivers/platform/x86/amd/pmf/manual.c   | 88 +++++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/pmf.h      |  5 ++
>  drivers/platform/x86/amd/pmf/sps.c      |  4 ++
>  6 files changed, 117 insertions(+)
>  create mode 100644 drivers/platform/x86/amd/pmf/manual.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-amd-pmf b/Documentation/ABI/testing/sysfs-amd-pmf
> index 7fc0e1c2b76b..6f3d5cbf443f 100644
> --- a/Documentation/ABI/testing/sysfs-amd-pmf
> +++ b/Documentation/ABI/testing/sysfs-amd-pmf
> @@ -11,3 +11,13 @@ Description:	Reading this file tells if the AMD Platform Management(PMF)
>  		To turn off CnQF user can write "off" to the sysfs node.
>  		Note: Systems that support auto mode will not have this sysfs file
>  		available.
> +
> +What:		/sys/devices/platform/*/{spl, fppt, sppt, sppt_apu_only, stt_min, stt_limit_apu, stt_skip_temp}
> +Date:		December 2024
> +Contact:	Mario Limonciello <mario.limonciello@....com>
> +Description:	Manual control of AMD PMF APU coefficients
> +		.
> +		These files are used to manually control the APU coefficients.
> +		In order to write to these files the module most be
> +		loaded with manual_control=1 and the user must write "custom"
> +		to the ACPI platform profile.
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index 7d6079b02589..81444d6f4428 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -7,4 +7,5 @@
>  obj-$(CONFIG_AMD_PMF) += amd-pmf.o
>  amd-pmf-objs := core.o acpi.o sps.o \
>  		auto-mode.o cnqf.o \
> +		manual.o \
>  		tee-if.o spc.o pmf-quirks.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index d6af0ca036f1..52a68ca094be 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -53,6 +53,10 @@ static bool force_load;
>  module_param(force_load, bool, 0444);
>  MODULE_PARM_DESC(force_load, "Force load this driver on supported older platforms (experimental)");
>  
> +bool pmf_manual_control;
> +module_param_named(manual_control, pmf_manual_control, bool, 0444);
> +MODULE_PARM_DESC(manual_control, "Expose manual control knobs (experimental)");
> +
>  static int amd_pmf_pwr_src_notify_call(struct notifier_block *nb, unsigned long event, void *data)
>  {
>  	struct amd_pmf_dev *pmf = container_of(nb, struct amd_pmf_dev, pwr_src_notifier);
> @@ -349,6 +353,10 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
>  		dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
>  	}
>  
> +	if (pmf_manual_control) {
> +		amd_pmf_init_manual_control(dev);
> +		return;
> +	}
>  	amd_pmf_init_smart_pc(dev);
>  	if (dev->smart_pc_enabled) {
>  		dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
> @@ -485,6 +493,7 @@ static void amd_pmf_remove(struct platform_device *pdev)
>  
>  static const struct attribute_group *amd_pmf_driver_groups[] = {
>  	&cnqf_feature_attribute_group,
> +	&manual_attribute_group,
>  	NULL,
>  };
>  
> diff --git a/drivers/platform/x86/amd/pmf/manual.c b/drivers/platform/x86/amd/pmf/manual.c
> new file mode 100644
> index 000000000000..b33fc3cd8d61
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/manual.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AMD Platform Management Framework Driver
> + *
> + * Copyright (c) 2024, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Mario Limonciello <mario.limonciello@....com>
> + */
> +
> +#include "pmf.h"
> +
> +#define pmf_manual_attribute(_name, _set_command, _get_command)		\
> +static ssize_t _name##_store(struct device *d,				\
> +			     struct device_attribute *attr,		\
> +			     const char *buf, size_t count)		\
> +{									\
> +	struct amd_pmf_dev *dev = dev_get_drvdata(d);			\
> +	uint val;							\
> +									\
> +	if (dev->current_profile != PLATFORM_PROFILE_CUSTOM) {		\
> +		dev_warn_once(dev->dev,					\
> +			      "Manual control is disabled, please set "	\
> +			      "platform profile to custom.\n");		\
> +		return -EINVAL;						\
> +	}								\
> +									\
> +	if (kstrtouint(buf, 10, &val) < 0)				\
> +		return -EINVAL;						\
> +									\
> +	amd_pmf_send_cmd(dev, _set_command, false, val, NULL);		\
> +									\
> +	return count;							\
> +}									\
> +static ssize_t _name##_show(struct device *d,				\
> +			   struct device_attribute *attr,		\
> +			   char *buf)					\
> +{									\
> +	struct amd_pmf_dev *dev = dev_get_drvdata(d);			\
> +	uint val;							\
> +									\
> +	amd_pmf_send_cmd(dev, _get_command, true, ARG_NONE, &val);	\
> +									\
> +	return sysfs_emit(buf, "%u\n", val);				\
> +}
> +
> +pmf_manual_attribute(spl, SET_SPL, GET_SPL);
> +static DEVICE_ATTR_RW(spl);
> +pmf_manual_attribute(fppt, SET_FPPT, GET_FPPT);
> +static DEVICE_ATTR_RW(fppt);
> +pmf_manual_attribute(sppt, SET_SPPT, GET_SPPT);
> +static DEVICE_ATTR_RW(sppt);
> +pmf_manual_attribute(sppt_apu_only, SET_SPPT_APU_ONLY, GET_SPPT_APU_ONLY);
> +static DEVICE_ATTR_RW(sppt_apu_only);
> +pmf_manual_attribute(stt_min, SET_STT_MIN_LIMIT, GET_STT_MIN_LIMIT);
> +static DEVICE_ATTR_RW(stt_min);
> +pmf_manual_attribute(stt_limit_apu, SET_STT_LIMIT_APU, GET_STT_LIMIT_APU);
> +static DEVICE_ATTR_RW(stt_limit_apu);
> +pmf_manual_attribute(stt_skin_temp, SET_STT_LIMIT_HS2, GET_STT_LIMIT_HS2);
> +static DEVICE_ATTR_RW(stt_skin_temp);
> +
> +static umode_t manual_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +{
> +	return pmf_manual_control ? 0660 : 0;
> +}
> +
> +static struct attribute *manual_attrs[] = {
> +	&dev_attr_spl.attr,
> +	&dev_attr_fppt.attr,
> +	&dev_attr_sppt.attr,
> +	&dev_attr_sppt_apu_only.attr,
> +	&dev_attr_stt_min.attr,
> +	&dev_attr_stt_limit_apu.attr,
> +	&dev_attr_stt_skin_temp.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group manual_attribute_group = {
> +	.attrs = manual_attrs,
> +	.is_visible = manual_attr_is_visible,
> +};
> +
> +void amd_pmf_init_manual_control(struct amd_pmf_dev *dev)
> +{
> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> +	pr_crit("Manual PMF control is enabled, please disable it before "
> +		"reporting any bugs unrelated to PMF.\n");
> +}
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 8ce8816da9c1..ca3df63cf190 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -798,4 +798,9 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
>  /* Quirk infrastructure */
>  void amd_pmf_quirks_init(struct amd_pmf_dev *dev);
>  
> +/* Manual configuration */
> +extern bool pmf_manual_control;
> +extern const struct attribute_group manual_attribute_group;
> +void amd_pmf_init_manual_control(struct amd_pmf_dev *dev);
> +
>  #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index 92f7fb22277d..6db88e523a86 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -305,6 +305,8 @@ int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
>  	case PLATFORM_PROFILE_LOW_POWER:
>  		mode = POWER_MODE_POWER_SAVER;
>  		break;
> +	case PLATFORM_PROFILE_CUSTOM:
> +		return 0;
>  	default:
>  		dev_err(pmf->dev, "Unknown Platform Profile.\n");
>  		return -EOPNOTSUPP;
> @@ -412,6 +414,8 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
>  	set_bit(PLATFORM_PROFILE_LOW_POWER, dev->pprof.choices);
>  	set_bit(PLATFORM_PROFILE_BALANCED, dev->pprof.choices);
>  	set_bit(PLATFORM_PROFILE_PERFORMANCE, dev->pprof.choices);
> +	if (pmf_manual_control)
> +		set_bit(PLATFORM_PROFILE_CUSTOM, dev->pprof.choices);
>  
>  	/* Create platform_profile structure and register */
>  	err = platform_profile_register(&dev->pprof);

Powered by blists - more mailing lists