| Message ID | 20250224195059.10185-1-lkml@antheas.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | ACPI: platform_profile: fix legacy sysfs with multiple handlers | expand |
On 2/24/2025 13:50, 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?). To me this raises a fundamental question. What is really different between quiet and low-power? quiet just affects fan curves and low-power affects power? One could argue that changing power will indirectly affect fan performance. Because it makes me wonder if we really should just make them permanent aliases. > > 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 > > Antheas Kapenekakis (3): > ACPI: platform_profile: Add support for secondary handlers > ACPI: platform_profile: add all options to amd-pmf as a secondary > handler > ACPI: platform_profile: Do not hide options missing in secondary > handlers > > drivers/acpi/platform_profile.c | 57 +++++++++++++++++++++++++----- > drivers/platform/x86/amd/pmf/spc.c | 3 ++ > drivers/platform/x86/amd/pmf/sps.c | 8 +++++ > include/linux/platform_profile.h | 7 ++++ > 4 files changed, 67 insertions(+), 8 deletions(-) >
That is a very good question, but a summary search along the drivers reveals that some laptops have both low-power and quiet, so it is not that clear cut. If you ask me, three is all you need + custom Antheas
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, My primary focus with this patch series is a bug fix. I imported Mario's series into our Bazzite 6.13 kernel, only to find it broke power handling on asus laptops, and it will also do the same on both legion gos, once a driver exists for those. And 6.14rc4 is the same. This needs to be fixed before it ships. This was my attempt at it. I considered other options, like making amd-pmf implement all profiles. But this seems like too dirty for me. So I settled at this. The primary TDP handler of a device is the WMI handler. When that exists, if one of its options is hidden, that is a regression. It does not matter the option. If we want amd-pmf to be able to load as a secondary handler (where the point of that has not been proven to me), then it (or any other secondary handler) cannot obscure the options of the primary platform. So it either has to implement all of them, or do something like this, where it is in between. Antheas
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 will tell you that compared with other manufacturers, when asus says quiet, they mean quiet and not low power Z13's quiet mode is 40W, not very low-power if you ask me. Ally X uses 13w+boost, a source of many complaints. Other manufacturers use around 8W for low power modes. In any case, any rename might break user scripts and there are actually 3 types of low power profiles: PLATFORM_PROFILE_COOL PLATFORM_PROFILE_QUIET PLATFORM_PROFILE_LOW_POWER Then, there is also: PLATFORM_PROFILE_BALANCED_PERFORMANCE Some ACER laptops implement many of those Obscuring any of those is not ideal. Antheas
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
Am 24.02.25 um 22:58 schrieb Antheas Kapenekakis: > I will tell you that compared with other manufacturers, when asus says > quiet, they mean quiet and not low power > > Z13's quiet mode is 40W, not very low-power if you ask me. Ally X uses > 13w+boost, a source of many complaints. Other manufacturers use around > 8W for low power modes. > > In any case, any rename might break user scripts and there are > actually 3 types of low power profiles: > PLATFORM_PROFILE_COOL > PLATFORM_PROFILE_QUIET > PLATFORM_PROFILE_LOW_POWER > > Then, there is also: > PLATFORM_PROFILE_BALANCED_PERFORMANCE > > Some ACER laptops implement many of those > > Obscuring any of those is not ideal. > > > Antheas > I see. Maybe extending userspace to support the new platform-profile class interface could solve this issue without breaking already existing scripts. However i do not know the amount of work which needs to be done to achieve this. Thanks, Armin Wolf
> I see. Maybe extending userspace to support the new platform-profile class interface > could solve this issue without breaking already existing scripts. > > However i do not know the amount of work which needs to be done to achieve this. The patch series I proposed here restores the behavior of the legacy API 1-1 with 6.13, ensuring that there are no userspace breakages and allows for maintaining the legacy interface's usefulness in the future. Without it, once more power profile handlers are added, it will begin to regress even more. We are also not sure it is just Asus, it is very likely there will be other breakages. Currently, platform profile handlers are split in half between low-power and quiet, and another third has additional options. The legacy interface is still good for most cases, there is no need to start moving non-specialized power managers such as ppd to it just yet. Antheas
Yes, making asus-wmi use low-power is indeed the easiest solution, but if I thought it was good enough, I would have done that already as a downstream consumer of the kernel. I just want to be done with this once and for all, so I spent an extra hour today solving this in a cleaner way. Antheas
On Mon, 2025-02-24 at 22:58 +0100, Antheas Kapenekakis wrote: > I will tell you that compared with other manufacturers, when asus > says > quiet, they mean quiet and not low power > > Z13's quiet mode is 40W, not very low-power if you ask me. Ally X > uses > 13w+boost, a source of many complaints. Other manufacturers use > around > 8W for low power modes. > > In any case, any rename might break user scripts and there are > actually 3 types of low power profiles: > PLATFORM_PROFILE_COOL > PLATFORM_PROFILE_QUIET > PLATFORM_PROFILE_LOW_POWER > > Then, there is also: > PLATFORM_PROFILE_BALANCED_PERFORMANCE > > Some ACER laptops implement many of those > > Obscuring any of those is not ideal. > In the context of their other modes it is low power. If there is confusion over what this is meant to mean for all vendors then it should be clarified in the relevant documentation. low-power Low power consumption cool Cooler operation quiet Quieter operation balanced Balance between low power consumption and performance balanced-performance Balance between performance and low power consumption with a slight bias towards performance performance High performance operation custom Driver defined custom profile "Low power consumption" compared to? If these "scripts" use `platform_profile_choices` to get their selections and verify they are available then there should be zero breakage. If they don't then they should be updated to be correct. In any case I am in the process of finalising an update to use the new platform_profile API including "custom". Please don't begin trying to break things just to be "first". My work has been ongoing for this in my spare time for months.
> If these "scripts" use `platform_profile_choices` to get their > selections and verify they are available then there should be zero > breakage. If they don't then they should be updated to be correct. Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let them spend a few days finding out why kernel 6.14 does not work. They should have written a 300 line bash script instead. > In any case I am in the process of finalising an update to use the new > platform_profile API including "custom". Please don't begin trying to > break things just to be "first". My work has been ongoing for this drivers/acpi/platform_profile.c | 57 +++++++++++++++++++++++++----- drivers/platform/x86/amd/pmf/spc.c | 3 ++ drivers/platform/x86/amd/pmf/sps.c | 8 +++++ include/linux/platform_profile.h | 7 ++++ I do not see the name Asus here. This is a compatibility patch. You should try it before commenting on it further. Looking at my ACPI database, there are at least a few Ayaneos, GPDs, and Legion laptops that have the ACPI bindings for pmf, this is not an Asus issue. By the way, I have merged your patch series on Bazzite (well... a cleaned up version that does not happen to crash your own software...) and it happens to work fine with this patch (I know you said platform profiles are not in yet). I still use the asus-wmi APIs personally. sudo fwupdmgr get-bios-setting Authenticating… [ - ] ppt_pl3_fppt: Setting type: Integer Current Value: 80 Description: Set the CPU slow package limit Read Only: False Minimum value: 5 ... > in my spare time for months. Let me comment on this a bit further. Hobbies are good to have and it is nice you found one you like. However, a lot of people are spending a lot of money on their Asus laptops and are actually starting to depend on Linux. If they cannot depend on you or your hobby for support, you should at least make sure to not interfere with parallel efforts for that support, if not try to be synergistic. I did not make this patch to one up you or rush it. This issue is a blocker for deploying our 6.13 kernel. Since this kernel needs to work for the Z13 and pmf quirks are dead ends now (I also got annoyed by asus users complaining about their fan curves being wrong because pmf blew up), I pulled in Mario's platform profile series early, only to find this issue. Botching the asus-wmi platform handler did not meet my standards, so I had to make this series. Also, since I could not pull in Kurk's series, and his changes were extensive, I had to make this series twice, and test it twice. Good news is this series works and the kernel is on its way to be deployed in a few days. Flatpak fix came in clutch today too with 6.13.4. Antheas
Am 25.02.25 um 03:26 schrieb Antheas Kapenekakis: >> If these "scripts" use `platform_profile_choices` to get their >> selections and verify they are available then there should be zero >> breakage. If they don't then they should be updated to be correct. > Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo > quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let > them spend a few days finding out why kernel 6.14 does not work. They > should have written a 300 line bash script instead. Hi, using "echo quiet | sudo tee /sys/firmware/acpi/platform_profile" is quite brittle, as some hardware will populate the available profiles dynamically. Still breaking userspace is indeed not an option here, so we have to think of something else. >> In any case I am in the process of finalising an update to use the new >> platform_profile API including "custom". Please don't begin trying to >> break things just to be "first". My work has been ongoing for this > drivers/acpi/platform_profile.c | 57 +++++++++++++++++++++++++----- > drivers/platform/x86/amd/pmf/spc.c | 3 ++ > drivers/platform/x86/amd/pmf/sps.c | 8 +++++ > include/linux/platform_profile.h | 7 ++++ > > I do not see the name Asus here. This is a compatibility patch. You > should try it before commenting on it further. Looking at my ACPI > database, there are at least a few Ayaneos, GPDs, and Legion laptops > that have the ACPI bindings for pmf, this is not an Asus issue. > > By the way, I have merged your patch series on Bazzite (well... a > cleaned up version that does not happen to crash your own software...) > and it happens to work fine with this patch (I know you said platform > profiles are not in yet). I still use the asus-wmi APIs personally. > > sudo fwupdmgr get-bios-setting > Authenticating… [ - ] > ppt_pl3_fppt: > Setting type: Integer > Current Value: 80 > Description: Set the CPU slow package limit > Read Only: False > Minimum value: 5 > ... > >> in my spare time for months. > Let me comment on this a bit further. Hobbies are good to have and it > is nice you found one you like. However, a lot of people are spending > a lot of money on their Asus laptops and are actually starting to > depend on Linux. If they cannot depend on you or your hobby for > support, you should at least make sure to not interfere with parallel > efforts for that support, if not try to be synergistic. This whole driver was likely written by someone as a hobby, so you already depend on a hobby here. That being said, i agree that fixes have a priority over new features, and i think everyone agrees on that. > I did not make this patch to one up you or rush it. This issue is a > blocker for deploying our 6.13 kernel. Since this kernel needs to work > for the Z13 and pmf quirks are dead ends now (I also got annoyed by > asus users complaining about their fan curves being wrong because pmf > blew up), I pulled in Mario's platform profile series early, only to > find this issue. Botching the asus-wmi platform handler did not meet > my standards, so I had to make this series. Also, since I could not > pull in Kurk's series, and his changes were extensive, I had to make > this series twice, and test it twice. > > Good news is this series works and the kernel is on its way to be > deployed in a few days. Flatpak fix came in clutch today too with > 6.13.4. > > Antheas > Maybe the current strategy of the legacy platform-profile interface can be extended without introducing the "secondary handler" concept. The current strategy only advertises platform profiles supported by all handlers, and as you pointed out this causes problems for users on certain devices. I was thinking that be can change this strategy to advertise all platform profiles supported by at least one handler can then do something like this: - handler 1: supports low_power, balanced and performance - handler 2: supports quiet, balanced and balanced-performance -> legacy interface advertises low_power, quiet, balanced, balanced-performance and performance When setting low_power, the closes equivalent is picked for handlers which do not support low_power: - handler 1: setting low_power - handler 2: setting quiet When setting quiet, the same happens: - handler 1: setting balanced - handler 2: setting quiet Basically all profiles get treated like a range: low_power <- lower end of the performance range cool, quiet, balanced, balanced-performance, performance <- upper end of the performance range The only problem will be that getting the current platform profile would be more difficult, as the legacy handler has to determine the lowest currently selected platform profile. Would this approach be OK? Thanks, Armin Wolf
On Tue, 25 Feb 2025 at 16:56, Armin Wolf <W_Armin@gmx.de> wrote: > > Am 25.02.25 um 03:26 schrieb Antheas Kapenekakis: > > >> If these "scripts" use `platform_profile_choices` to get their > >> selections and verify they are available then there should be zero > >> breakage. If they don't then they should be updated to be correct. > > Yeah, if any Asus users wrote scripts for their laptops to e.g., "echo > > quiet | sudo tee /sys/firmware/acpi/platform_profile" or used TLP let > > them spend a few days finding out why kernel 6.14 does not work. They > > should have written a 300 line bash script instead. > > Hi, > > using "echo quiet | sudo tee /sys/firmware/acpi/platform_profile" is quite > brittle, as some hardware will populate the available profiles dynamically. > > Still breaking userspace is indeed not an option here, so we have to think > of something else. e.g., for example I had a tdp handler for the ally and had to transplant it to thermal_throttle_profile when the breakage with amd_pmf started happening. > snip > > This whole driver was likely written by someone as a hobby, so you already > depend on a hobby here. > > That being said, i agree that fixes have a priority over new features, and > i think everyone agrees on that. Indeed.In which case, saying people should use your hobby code perhaps is a bit overreaching. Haha. > Maybe the current strategy of the legacy platform-profile interface can be extended > without introducing the "secondary handler" concept. > > The current strategy only advertises platform profiles supported by all handlers, and > as you pointed out this causes problems for users on certain devices. > > I was thinking that be can change this strategy to advertise all platform profiles supported > by at least one handler can then do something like this: > > - handler 1: supports low_power, balanced and performance > > - handler 2: supports quiet, balanced and balanced-performance > > -> legacy interface advertises low_power, quiet, balanced, balanced-performance and performance > > When setting low_power, the closes equivalent is picked for handlers which do not support low_power: > > - handler 1: setting low_power > > - handler 2: setting quiet > > When setting quiet, the same happens: > > - handler 1: setting balanced > > - handler 2: setting quiet > > Basically all profiles get treated like a range: > > low_power <- lower end of the performance range > cool, > quiet, > balanced, > balanced-performance, > performance <- upper end of the performance range > > The only problem will be that getting the current platform profile would be more difficult, as > the legacy handler has to determine the lowest currently selected platform profile. > > Would this approach be OK? > > Thanks, > Armin Wolf > So the way this patch series is designed is that the new /sys/class/platform-profileX works exactly the same. Then, when asus-wmi is loaded, regardless of whether amd-pmf is loaded, you get quiet, balanced, and performance. Like it was before. Setting it to quiet makes amd-pmf use its low power setting. And it will work the same with all WMI drivers, regardless of whether they use cool, low-power, quiet, or balanced-power When asus-wmi is unloaded, you get low-power, balanced, and performance. As you would with amd-pmf on its own. 0 ABI changes. Series is small enough so that if you don't like it, it is easy to refactor out during the 6.15 merge window. $ cat /sys/firmware/acpi/platform_profile_choices quiet balanced performance $ sudo rmmod asus-nb-wmi $ cat /sys/firmware/acpi/platform_profile_choices low-power balanced performance Antheas
On Tue, Feb 25, 2025 at 7:07 AM Antheas Kapenekakis <lkml@antheas.dev> wrote: > > Yes, making asus-wmi use low-power is indeed the easiest solution, but > if I thought it was good enough, I would have done that already as a > downstream consumer of the kernel. > > I just want to be done with this once and for all, so I spent an extra > hour today solving this in a cleaner way. What about adding "quiet" as a "hidden choice" to amd-pmf such that it would allow the test_bit(*bit, handler->choices) check in _store_class_profile() to pass, but it would not cause this "choice" to become visible in the new I/F (or when amd-pmf becomes the only platform-profile driver) and it would be aliased to "low-power" internally?
This is what this patch series essentially does. It makes amd-pmf accept all choices but only show its own in its own handler and when it is the only option On Tue, 25 Feb 2025 at 21:22, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Feb 25, 2025 at 7:07 AM Antheas Kapenekakis <lkml@antheas.dev> wrote: > > > > Yes, making asus-wmi use low-power is indeed the easiest solution, but > > if I thought it was good enough, I would have done that already as a > > downstream consumer of the kernel. > > > > I just want to be done with this once and for all, so I spent an extra > > hour today solving this in a cleaner way. > > What about adding "quiet" as a "hidden choice" to amd-pmf such that it > would allow the test_bit(*bit, handler->choices) check in > _store_class_profile() to pass, but it would not cause this "choice" > to become visible in the new I/F (or when amd-pmf becomes the only > platform-profile driver) and it would be aliased to "low-power" > internally?
Top-posting not welcome. On Wed, Feb 26, 2025 at 8:52 PM Antheas Kapenekakis <lkml@antheas.dev> wrote: > > > > What about adding "quiet" as a "hidden choice" to amd-pmf such that it > > would allow the test_bit(*bit, handler->choices) check in > > _store_class_profile() to pass, but it would not cause this "choice" > > to become visible in the new I/F (or when amd-pmf becomes the only > > platform-profile driver) and it would be aliased to "low-power" > > internally? > > This is what this patch series essentially does. It makes amd-pmf > accept all choices but only show its own in its own handler and when > it is the only option No, it does more than this. For instance, it is not necessary to do anything about PLATFORM_PROFILE_BALANCED_PERFORMANCE in it. The structure of it is questionable either. It really should be two patches, one modifying the ACPI platform-profile driver and the other changing amd-pmf on top of this. Moreover, I'm not entirely convinced that the "secondary" driver concept is needed to address the problem at hand.
On Wed, 26 Feb 2025 at 21:04, Rafael J. Wysocki <rafael@kernel.org> wrote: > > Top-posting not welcome. ? > On Wed, Feb 26, 2025 at 8:52 PM Antheas Kapenekakis <lkml@antheas.dev> wrote: > > > > > > What about adding "quiet" as a "hidden choice" to amd-pmf such that it > > > would allow the test_bit(*bit, handler->choices) check in > > > _store_class_profile() to pass, but it would not cause this "choice" > > > to become visible in the new I/F (or when amd-pmf becomes the only > > > platform-profile driver) and it would be aliased to "low-power" > > > internally? > > > > This is what this patch series essentially does. It makes amd-pmf > > accept all choices but only show its own in its own handler and when > > it is the only option > > No, it does more than this. I would say functionality-wise no. The patch could be minified further. > For instance, it is not necessary to do > anything about PLATFORM_PROFILE_BALANCED_PERFORMANCE in it. I do not see a difference between QUIET and BALANCED_PERFORMANCE, any driver occluding either causes the same issue. Severity is debatably lower on BP though. > The structure of it is questionable either. It really should be two > patches, one modifying the ACPI platform-profile driver and the other > changing amd-pmf on top of this. Ack. I can spin it up as 2 patches. > Moreover, I'm not entirely convinced that the "secondary" driver > concept is needed to address the problem at hand. Any suggestions on that front would be welcome. This is just the way I came up with doing it. Best, Antheas
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 Antheas Kapenekakis (3): ACPI: platform_profile: Add support for secondary handlers ACPI: platform_profile: add all options to amd-pmf as a secondary handler ACPI: platform_profile: Do not hide options missing in secondary handlers drivers/acpi/platform_profile.c | 57 +++++++++++++++++++++++++----- drivers/platform/x86/amd/pmf/spc.c | 3 ++ drivers/platform/x86/amd/pmf/sps.c | 8 +++++ include/linux/platform_profile.h | 7 ++++ 4 files changed, 67 insertions(+), 8 deletions(-)