Message-ID: 
 <CAGwozwEvLUbqUGBvHh18tSb7nh9ttKQFxXG-LcQ+W80WSdktgA@mail.gmail.com>
Date: Fri, 27 Sep 2024 10:44:00 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Mario Limonciello <superm1@...nel.org>
Cc: 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>,
 "Derek J . Clark" <derekjohn.clark@...il.com>,
	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

Hi Mario,

On Thu, 26 Sept 2024 at 20:09, Mario Limonciello <superm1@...nel.org> wrote:
>
> On 9/26/2024 06:00, Antheas Kapenekakis wrote:
> > Hi Shyam,
> >
> >> 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.
> >
> >> 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?

Having a single call that sets everything would be my preference, so I
would support this.

Although looking at [1], seems like it will be separate calls anway.

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

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

Spoken like a true linux user. Users do not know what a kernel or
sysfs is and they will not be touching any of this. It just needs to
be baby-proofed enough so for the 5 users that do it is safe.

Let us focus on the problem here. There are currently around 5
manufacturers shipping products in a space where granular TDP control
is expected and where AMD has not provided them with a solution.

And for this, there are two issues. First, there is no standard for
granular TDP control tuned by the manufacturer. Second, when such a
standard is created, there is a healthy pool of devices in the market
where the manufacturer cannot be expected to provide an updated BIOS
for them.
Therefore, we need a proposal where 1) the manufacturer can provide
granular TDP controls in a fully customizable manner (e.g., with a LUT
that controls everything), and 2) for devices that will not get that
tuning, a custom profile setting that will expose important tuning
parameters to userspace so that we can retrofit it and extend the
their lifespan.

> > 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'd tend to agree since the current mailbox targets that I know of are
STAPM limit (for STAPM) and skin temp limit (for STT). Since you used
the term SPL, I carried that over to the proposal, but it would not
control SPL. Instead it would control both of the former, including
sPPT and fPPT (if that is still supported; unclear in [1]; but
disabling boost will be a requirement).

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

I will have to repeat that as far as the handheld market is concerned,
we are not talking about advanced users. Instead, we are talking for
all users.

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

Sounds like you have your work cut out for you if the custom profile
is supposed to dynamically load.

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

While (in Bazzite) we have a solution that works very reliably and is
safe (not RyzenAdj), we have to begin cleaning up loose ends so that
we can 1) enable TDP control in a stock secureboot kernel with early
lockdown enabled (e.g., Fedora), 2) provide manufacturers with certain
reliability guarantees so they can warranty units running under linux,
3) prepare our solutions for being packaged in upstream distribution
repositories (Debian, Fedora), where using an existing solution is a
blocker as they do not provide or should provide such hardware access
when secure boot is enabled.

Though, since manufacturers like Ayaneo currently use RyzenAdj in
Windows, I might be nitpicking too much.

As for why Secure Boot is important, let add [2], where Rockstar
points the finger to Valve for BattlEye not working. Much of the
anticheat issue is due to the fact that it is trivial to cheat without
having a secureboot enabled kernel with the early lockdown flag
engaged, as it allows both custom drivers and userspace to gain access
to sensitive process memory in a way that is undetectable by
anticheat. Vanguard does not work in Linux for much of the same
reason.

Steam Deck is, for those uninitiated, a device that does not carry
Secureboot keys, and SteamOS is a distribution that does not support
Secure boot. Although both can change (Steam Deck BIOS supports secure
boot). However, Bazzite is secure boot enabled and we encourage our
users to leave it enabled, although for the moment they have to enroll
our MOK key, which most of them do.

Antheas

Link: https://www.pcgamer.com/games/grand-theft-auto/gta-online-is-no-longer-compatible-with-steam-deck-thanks-to-its-new-anti-cheat-software-despite-battleye-having-an-opt-in-system-for-this-sort-of-thing/
[2]

Powered by blists - more mailing lists