Message-ID: <CAFqHKTk8U1DoMjeZFYhm_abQ7mxWd6e6Phbm9kGCHPJHfOteHw@mail.gmail.com>
Date: Thu, 26 Sep 2024 21:01:20 -0700
From: Derek John Clark <derekjohn.clark@...il.com>
To: Mario Limonciello <superm1@...nel.org>
Cc: Antheas Kapenekakis <lkml@...heas.dev>, Shyam Sundar S K <Shyam-sundar.S-k@....com>, 
	"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>, 
	"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>, 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

Hello all,

>>> 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.

> In the laptop market I agree with you. However, in the handheld
> market, users expect to be able to lower the power envelope of the
> device on demand in a granular fashion. As the battery drop is
> measured in Watts, tying a slider to Watts is a natural solution.
>
> Most of the time, when those controls are used it is to limit the
> thermal envelope of the device, not exceed it. We want to remove the
> use of these tools and allow manufacturers the ability to customise
> the power envelope they offer to users.

I agree with Mario here. Due to the use case and battery size, handheld users
intent is to minimize power draw while maintaining performance. The typical use
case in "per-watt" control is to manage this. It is usually done
per-game (either
manually by the user or recalled with userspace tools). We have found that the
'performance' profile alone won't always limit the power enough, which decreases
the playtime, and 'balanced' will limit it too much, decreasing performance.
Users want to be able to tune a "sweet spot" to get maximal performance from
both metrics.

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

>Hmm, what do you think about about offering a wrapper for this for
>people to manipulate?

>>> 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.

>The tough part about striking the balance here is how would an end user
>know what values to set in tandem.  I think a lot of people just assume
>they can "just change SPL" and that's it and have a good experience.

I'm unsure of the generalized case here, but if we are using this to limit SPL
rather than raise it over design spec, then it would seem to me that STT would
be set to match or exceed SPL and the cooling solution would be able to
compensate for that. I'm happy to be corrected here if this is not a correct
assumption. I think there may be some variation on how manufacturers implement
this in the BIOS. For example, the Legion Go uses STT to push TDP to the
thermal limit (between SPL and SPPT) when using the ACPI profiles. Their
"custom" profile changes the behavior to fully respect the user set SPL/SPPT/
FPPT. I'm not sure if/how others handle this differently. In any case,
I would expect
the driver could handle this.

>> This suggestion was referring to a combined slider, much like the
>> suggestion below. So STT limits would be modified in tandem,
>> respecting manufacturer profiles. See comments below.
>>
>> If you find the name SPL disagreeable, it could be named {tdp,
>> tdp_min, tdp_max}. This is the solution used by Valve on the Steam
>> Deck (power1_cap{+min,max}, power2_cap{+min,max}).

>It's not so much that it's disagreeable term but Shyam is pointing out
>that SPL is no longer a valid argument to the platform mailbox.

I think intuitive generic terms would be ideal. [ppt|sppt|fppt]_limit[_min|_max]
are well understood by power users currently. There should be some
terminology that applies generally across different implementations of similar
concepts.

>> In addition, boost is seen as detrimental to handheld devices, with
>> most users disliking and disabling it. Steam Deck does not use boost.
>> It is disabled by Steam (power1_cap == power2_cap). So STT and STAPM
>> are not very relevant. In addition, Steam Deck van gogh has a more
>> linear response so TDP limits are less required.

I find this to be case by case, some games have more sudden/dynamic loads and
need the extra overhead, while others will waste it. Flexibility is important I
think. The Deck also benefits from scale and Steam integration right now so
publishers are able to, and do, tune for that device specifically. I don't know
how far the lessons from that device transfer to other handhelds. Having them
as an option, even if unused, would be a benefit.

>>> 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?

>The problem is all of the RE tools rely upon PCI config space access or
>/dev/mem access to manipulate undocumented register offsets.
>
>When the system is under kernel lockdown (such as with distro kernel
>when UEFI secure boot is turned on) then those interfaces are
>intentionally locked down.
>
>That's why I'm hoping we can strike some sort of balance at the request
>for some advanced users being able to tune values in a predictable
>fashion while also allowing OEMs to configure policies like CNQF or
>Smart PC when users for users that don't tinker.

>>> 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.

>Yeah; that's why as this RFC patch I didn't let CNQF, ITS or Smart PC
>initialize.  Basically if manual control is enabled then "SPS" and
>manual sysfs control is the only thing available.

>> Much like you, we dislike AutoTDP solutions that use e.g., RyzenAdj, as they:
>>   1) Do not respect manufacturer limits
>>   2) Cause system instability such as stutters when setting values
>>   3) Can cause crashes if they access the mailbox at the same time as
>> the AMD drm driver.

>Yes.  Exactly why I feel that if we offer an interface instead people
>can use such an interface instead of these tools.

The general consensus on the userspace development side is that we'd like to
move away from needing to do these hacks to get the most from the hardware. I
would say things like RyzenAdj, ryzen_smu, acpi_call, etc. have provided enough
evidence that there is a gap in the baseline functionality that will ultimately
be filled *somehow*. I'd like to see a move towards this as an acceptable
in-kernel standard to replace those tools. In that same vein I think it's
important that common sense defaults and manufacturers intent are respected. I
have some ideas for how that could be done. If information is not
available for a
given device then the "custom" parameter will not be available in
power_profile_available and attempts to set to it will -EINVAL.
Similarly, have PPT
attrs only show in sysfs if that data is available.

- For legacy devices (and likely many smaller "boutique" manufacturers for the
  foreseeable future) a DMI table with limits for each supported attribute
  could provide this. Limiting this table to handhelds specifically would be
  acceptable to me, I don't see the value for laptops personally. For almost
  everything on the market currently we have this data, provided by the OEM.
- For devices with WMI (Legion Go, ROG Ally) the manufacturer has provided the
  methods to get these limits directly, so that could be handled in the
  appropriate manufacturer WMI drivers.
- For future devices this information should be (is?) included in the
  PMF tables in the BIOS and enabled automatically when detected by the driver,
  which will hopefully reduce the number of necessary kernel patches going
  forward.

Powered by blists - more mailing lists