Message-ID: <e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de>
Date: Mon, 24 Feb 2025 23:42:27 +0100
From: Armin Wolf <W_Armin@....de>
To: Luke Jones <luke@...nes.dev>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
 Antheas Kapenekakis <lkml@...heas.dev>,
 "Limonciello, Mario" <mario.limonciello@....com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Len Brown <lenb@...nel.org>,
 "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
 linux-kernel@...r.kernel.org,
 "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Hans de Goede
 <hdegoede@...hat.com>, me@...egospodneti.ch
Subject: Re: [PATCH 0/3] ACPI: platform_profile: fix legacy sysfs with
 multiple handlers

Am 24.02.25 um 22:51 schrieb Luke Jones:

> On Mon, 2025-02-24 at 15:52 -0500, Mark Pearson wrote:
>> Hi Antheas,
>>
>> On Mon, Feb 24, 2025, at 2:50 PM, Antheas Kapenekakis wrote:
>>> On the Asus Z13 (2025), a device that would need the amd-pmf quirk
>>> that
>>> was removed on the platform_profile refactor, we see the following
>>> output
>>> from the sysfs platform profile:
>>>
>>> $ cat /sys/firmware/acpi/platform_profile_choices
>>> balanced performance
>>>
>>> I.e., the quiet profile is missing. Which is a major regression in
>>> terms of
>>> power efficiency and affects both tuned, and ppd (it also affected
>>> my
>>> software but I fixed that on Saturday). This would affect any
>>> laptop that
>>> loads both amd-pmf and asus-wmi (around 15 models give or take?).
>>>
>>> The problem stems from the fact that asus-wmi uses quiet, and amd-
>>> pmf uses
>>> low-power. While it is not clear to me what the amd-pmf module is
>>> supposed
>>> to do here, and perhaps some autodetection should be done and make
>>> it bail,
>>> if we assume it should be kept, then there is a small refactor that
>>> is
>>> needed to maintain the existing ABI interface.
>>>
>>> This is the subject of this patch series.
>>>
>>> Essentially, we introduce the concept of a "secondary" handler.
>>> Secondary
>>> handlers work exactly the same, except for the fact they are able
>>> to
>>> receive all profile names through the sysfs interface. The
>>> expectation
>>> here would be that the handlers choose the closest appropriate
>>> profile
>>> they have, and this is what I did for the amd-pmf handler.
>>>
>>> In their own platform_profile namespace, these handlers still work
>>> normally
>>> and only accept the profiles from their probe functions, with -
>>> ENOSUP for
>>> the rest.
>>>
>>> In the absence of a primary handler, the options of all secondary
>>> handlers
>>> are unioned in the legacy sysfs, which prevents them from hiding
>>> each
>>> other's options.
>>>
>>> With this patch series applied, the sysfs interface will look like
>>> this:
>>>
>>> $ cat /sys/firmware/acpi/platform_profile_choices
>>> quiet balanced performance
>>>
>>> And writing quiet to it results in the profile being applied to
>>> both
>>> platform profile handlers.
>>>
>>> $ echo low-power > /sys/firmware/acpi/platform_profile
>>> bash: echo: write error: Operation not supported
>>> $ echo quiet > /sys/firmware/acpi/platform_profile
>>> $ cat /sys/class/platform-profile/platform-profile-*/{name,profile}
>>> asus-wmi
>>> amd-pmf
>>> quiet
>>> quiet
>>>
>>> Agreed ABI still works:
>>> $ echo quiet > /sys/class/platform-profile/platform-profile-
>>> 0/profile
>>> $ echo quiet > /sys/class/platform-profile/platform-profile-
>>> 1/profile
>>> bash: echo: write error: Operation not supported
>>> $ echo low-power > /sys/class/platform-profile/platform-profile-
>>> 0/profile
>>> bash: echo: write error: Operation not supported
>>> $ echo low-power > /sys/class/platform-profile/platform-profile-
>>> 1/profile
>>>
>> I understand where you're coming from with this implementation but my
>> concern is this is making profiles more complicated - and they're
>> already becoming hard to understand (and debug) for users.
>>
>> I'm not a huge fan of multiple profile handlers, but can see why some
>> people might want them and that they're a valid tool to have
>> (especially given some of the limitations of what platform vendors
>> themselves implement).
>>
>> In patch #3 it states that 'It is the expectation that secondary
>> handlers will pick the closest profile they have to what was sent'.
>> I'm not convinced that is true, or desired.
>>
>> e.g. Quiet and low-power are different things and can have different
>> implementations. One is giving you as much power as possible with the
>> fans running below a certain audible level; and one is giving you a
>> system with as low-power consumption as possible, but still be
>> usable. They're admittedly not very different in practice - but they
>> can be different.
>>
>> Would it be better here to ask AMD to implement a quiet profile
>> (maybe it can be based on low-power, at least initially)?
>> I think that would solve the ASUS issue and not introduce another
>> layer of complexity.
>>
>> Mark
> Hi Mark,
>
> I've supported over 80 different ASUS laptops in the last 6 years or
> so, I can offer some insight.
>
> Across the entire range (TUF, ROG, Vivobook, Zen) which implements some
> form of "thermal throttle" as it is called in asus-wmi (which is what
> is used by platform_profile) the difference between low-power and quiet
> is very much nil - the "quiet" profile is only a name, and the TDP is
> limited along with fans to match - so the result is "low-power".
>
> As Mario suggests in his reply perhaps an alias would be best, or, as I
> was going to do, simply rename the "quiet" profile in asus-wmi to "low-
> power" as I already did but have not submitted yet due to a large train
> of patches in progress. It's a single line change and nullifies the
> entire issue and this series.
>
> In any case asus handling of platform profile is something I have been
> steadily working on for the last few months for both laptops and
> handhelds and I will have a new patch series coming soon (version 7 of
> previously submitted dealing with this).
>
> This submitted series is a NACK from me.
>
> Cheers,
> Luke.

I agree with you here, changing asus-wmi to using "low-power" would be an
appropriate quick fix here.

But i think the real solution to this is to extend tuned and platform-profiles-daemon
so that they use the new platform-profile class interface when available. This would
prevent such issues in the future.

Maybe someone can open an issue for both projects to notify them that an improved API
for controlling the platform-profiles is available?

Thanks,
Armin Wolf

Powered by blists - more mailing lists