On Thu, 19 Dec 2024 at 15:50, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
>
> On 12/19/2024 07:12, Antheas Kapenekakis wrote:
> > Hi Mario,
> > given that there is a Legion Go driver in the works, and Asus already
> > has a driver, the only thing that would be left for locking down ACPI
> > access is manufacturers w/o vendor APIs.
> >
> > So, can we restart the conversation about this driver? It would be
> > nice to get to a place where we can lock down /dev/mem and ACPI by
> > spring.
>
> As Shyam mentioned we don't have control for limits by the PMF driver
> for this on PMF v2 (Strix) or later platforms.
>
> So if we were to revive this custom discussion it would only be for
> Phoenix and Hawk Point platforms.
That's unfortunate.
> >
> > Moreover, since the other two proposed drivers use the
> > firmware_attributes API, should this be used here as well?
>
> I do feel that if we revive this conversation specifically for Phoenix
> and Hawk Point platforms yes we should use the same API to expose it to
> userspace as those other two drivers do.
>
> I'd like Shyam's temperature on this idea though before anyone spends
> time on it. If he's amenable would you want to work on it?
We currently expect the 2025 lineup to include a lot of Strix Point
handhelds, so I'd like a solution that works with that. OneXPlayer
released a model already, and GPD is getting ready to ship as well.
Yeah, I could throw some hours to it after I go through some overdue stuff.
> >
> > By the way, you were right about needing a taint for this. Strix Point
> > fails to enter a lower power state during sleep if you set it to lower
> > than 10W. This is not ideal, as hawk point could go down to 5 while
> > still showing a power difference, but I am unsure where this bug
> > should be reported. This is both through ryzenadj/ALIB
>
> Who is to say this is a bug? Abusing a debugging interface with a
> reverse engineered tool means you might be able to configure a platform
> out of specifications.
The spec being 10+W would be very undesirable for handhelds with Strix
Point, so I'd hope somebody looks into it, esp. if it can be fixed
with a BIOS fw update before more handhelds come out. I can raise the
minimum TDP to 10W, with some user complaints.
Asus and Lenovo use the same mailbox so they'd share the issue too.
FYI for a typical handheld with e.g., a 60Wh battery, a 10W envelope
results in around 20-22W total consumption which is around 2.5 hours.
Hawk Point can be TDP limited down to 16W total consumption (TDP ~7W)
and can go down to 8W with frame limiting etc. I do not have numbers
for Strix Point yet, but to match Hawk Point it has to allow TDP to go
down to 7W. I think for 2025, customer expectation will be 6-8 hours+
at low wattages.
Antheas
> >
> > Antheas
> >
> > On Thu, 26 Sept 2024 at 05:00, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>
> >> 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.
> >>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >> ---
> >> 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@xxxxxxx>
> >> +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@xxxxxxx>
> >> + */
> >> +
> >> +#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);
> >> --
> >> 2.43.0
> >>
>
