[discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[Hans de Goede]

Hi All,

AMD based ThinkPads (1) have separate ac (connected to an external
powersource) and on battery tuned versions of the low-power, balanced and
performance profiles. So in essence they have six profiles: low-power-ac,
low-power-battery, balanced-ac, balanced-battery, performance-ac and
performance-battery.

The question is how to deal with this. There is a previous discussion
about this here:
https://lore.kernel.org/platform-driver-x86/20220301201554.4417-1-markpearson@lenovo.com/

From that previous discussions 3 possible solutions come to mind:

1. Simply treat this as 6 different profiles, maybe with documenting
   -ac/-battery postfixes for the profile-names and let userspace
   sort it out.

Pro:    -Simple from the kernel side
Contra: -Does not work with existing userspace code
        -This is quite ugly IMHO / does not feel right

2. Only export three profiles to userspace, while going "all in" on this
   concept and change drivers/acpi/platform_profile.c to add new:
   /sys/firmware/acpi/platform_profile_ac
   /sys/firmware/acpi/platform_profile_battery
   files which can contain different desired settings for the ac/battery
   case and have the kernel automatically switch between the two and
   also have it pass the ac/battery state to
   platform_profile_handler.profile_set so that for hw which has
   different ac/battery flavors of the profile the driver knows which
   one to select (without needing to detect this itself)

Pro:    -This matches well with the behavior which we want for the user
         (which is for the system to save the profile as 2 separate settings
          for ac/battery and switch the profile to the last selected setting
          for ac/battery when the state changes)
        -Solve the ac/battery state listening/notification only once
         instead of having all platform_profile drivers DIY
Contra: -This means deprecating /sys/firmware/acpi/platform_profile and
         defining how it maps to the 2 new files, e.g. if it is written
         does that only set the current active profile, or both ?
        -Userspace needs to be adjusted to use the new uapi and once it
         has been adjusted it also still needs to work on the older
         kernels which will be tricky/nasty to implement and also is
         a problem from CI / testing pov.

3. Only export three profiles to userspace and have the
   drivers/acpi/platform_profile.c "core" code pass the ac/battery state
   to platform_profile_handler.profile_set so that for hw which has
   different ac/battery flavors of the profile the driver knows which
   one to select (without needing to detect this itself)

Pro:    -Solve the ac/battery state listening/notification only once
         instead of having all platform_profile drivers DIY
        -Leaves the existing userspace API 100% unchanged and leaves
         existing userspace code working without it requiring any
         changes
Contra: -As part of the discussion on this the RFE to "have separate
         "last selected" ac/battery profile settings and automatically
         switch when the state changes" has surfaced; and that is
         not solved


Writing it down like this, to me 3. is the clear winner. The only
downside of 3. I can come up with arguably is better solved in
userspace (2), esp. since this will likely also require some UI design
work to somehow make it clear to the user that there are two different
settings (3).

Also even if no UI changes are deemed necessary this will still require
userspace changes to save+restore the two separate "last selected"
profile settings over reboots.

Please let me know what you think of this, and of course another
completely different approach is welcome too.

Regards,

Hans


1) Although AMD based ThinkPads are the trigger for this discussion,
   this applies to more new AMD based laptops, so this is not ThinkPad
   specific

2) IMHO it would be good to file a RFE issue for this against p-p-d:
   https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/new

3) AFAICT Windows does the 2 separate "last selected" ac/battery profile
   settings thing while just showing a single slider in its UI, but that
   really is a whole other discussion which the userspace/UI folks can have.

Re: [discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[Bastien Nocera]

On Tue, 2022-03-15 at 10:16 +0100, Hans de Goede wrote:
> Hi All,
> 
> AMD based ThinkPads (1) have separate ac (connected to an external
> powersource) and on battery tuned versions of the low-power, balanced
> and
> performance profiles. So in essence they have six profiles: low-
> power-ac,
> low-power-battery, balanced-ac, balanced-battery, performance-ac and
> performance-battery.
> 
<snip>
> Also even if no UI changes are deemed necessary this will still
> require
> userspace changes to save+restore the two separate "last selected"
> profile settings over reboots.
> 
> Please let me know what you think of this, and of course another
> completely different approach is welcome too.

From the power-profiles-daemon, and the GNOME UI side, we're unlikely
to expose those separate profiles as that's not how we envisioned the
profiles being used (there's an API to force a power-saving, or
performance profile temporarily, and the desktop implements power-
saving policies based upon that).

We have priors when it comes to specific profiles behaving slightly
differently depending on external factors, for example the performance
profile on some Intel Lenovo laptops being throttled down when a lap is
detected, or our attempt at using the "balance_power" intel pstate
preference when on battery (later reverted because this preference is
incredibly aggressive...).

I think that keeping the external API would be best for user-space,
we'd just want to have a way to know how it's behaving internally, eg.
exposing that it has separate profiles for AC and battery, and whether
an AC or battery variant is selected at this time.

Cheers

Re: [External] [discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[Mark Pearson]

Thanks Hans

On 2022-03-15 05:16, Hans de Goede wrote:
> Hi All,
> 
> AMD based ThinkPads (1) have separate ac (connected to an external 
> powersource) and on battery tuned versions of the low-power, balanced
> and performance profiles. So in essence they have six profiles:
> low-power-ac, low-power-battery, balanced-ac, balanced-battery,
> performance-ac and performance-battery.
> 
> The question is how to deal with this. There is a previous
> discussion about this here: 
> https://lore.kernel.org/platform-driver-x86/20220301201554.4417-1-markpearson@lenovo.com/>>
>  From that previous discussions 3 possible solutions come to mind:
> 
> 1. Simply treat this as 6 different profiles, maybe with documenting 
> -ac/-battery postfixes for the profile-names and let userspace sort
> it out.
> 
> Pro:    -Simple from the kernel side Contra: -Does not work with
> existing userspace code -This is quite ugly IMHO / does not feel
> right
Agreed - I initially looked at implementing it this way and it's not
good. I don't recommend it

> 
> 2. Only export three profiles to userspace, while going "all in" on
> this concept and change drivers/acpi/platform_profile.c to add new: 
> /sys/firmware/acpi/platform_profile_ac 
> /sys/firmware/acpi/platform_profile_battery files which can contain
> different desired settings for the ac/battery case and have the
> kernel automatically switch between the two and also have it pass the
> ac/battery state to platform_profile_handler.profile_set so that for
> hw which has different ac/battery flavors of the profile the driver
> knows which one to select (without needing to detect this itself)
> 
> Pro:    -This matches well with the behavior which we want for the
> user (which is for the system to save the profile as 2 separate
> settings for ac/battery and switch the profile to the last selected
> setting for ac/battery when the state changes) -Solve the ac/battery
> state listening/notification only once instead of having all
> platform_profile drivers DIY Contra: -This means deprecating
> /sys/firmware/acpi/platform_profile and defining how it maps to the 2
> new files, e.g. if it is written does that only set the current
> active profile, or both ? -Userspace needs to be adjusted to use the
> new uapi and once it has been adjusted it also still needs to work on
> the older kernels which will be tricky/nasty to implement and also
> is a problem from CI / testing pov.

I don't think the contra here is that complicated or causing backwards
compatibility issues.

For the platform_profile attribute
 - If you read it then you get the current active configured profile
regardless of whether you are plugged in or not.
 - If you set it then it sets both ac and battery profile and they
stay in sync

For the platform_profile_ac and platform_profile_dc
 - If you read it you get the configured setting for that power mode
 - If you set it, then that mode is active for the appropriate power mode

I think that keeps this backwards compatible and user space can decide
on their own schedule/preferences/design if they implement something extra.
Am I missing something?

> 
> 3. Only export three profiles to userspace and have the 
> drivers/acpi/platform_profile.c "core" code pass the ac/battery
> state to platform_profile_handler.profile_set so that for hw which
> has different ac/battery flavors of the profile the driver knows
> which one to select (without needing to detect this itself)
> 
> Pro:    -Solve the ac/battery state listening/notification only once 
> instead of having all platform_profile drivers DIY -Leaves the
> existing userspace API 100% unchanged and leaves existing userspace
> code working without it requiring any changes Contra: -As part of the
> discussion on this the RFE to "have separate "last selected"
> ac/battery profile settings and automatically switch when the state
> changes" has surfaced; and that is not solved
> 
All looks reasonable to me.

> 
> Writing it down like this, to me 3. is the clear winner. The only 
> downside of 3. I can come up with arguably is better solved in 
> userspace (2), esp. since this will likely also require some UI
> design work to somehow make it clear to the user that there are two
> different settings (3).

For me I went with #2 in the RFC implementation as I thought it was a
nice feature to have generally which is it's big plus - but I'm
genuinely fairly split as to whether #2 or #3 is best. I could easily go
either way.

One Q I have is that I would like our platforms to have the ability to
auto-switch between AC and battery profile for their configured mode
automatically. So if we go with option #3 I want to implement an event
handler in the thinkpad_acpi driver for these platforms. Does that raise
any flags or concerns? I prototyped this before I wrote the RFC and it's
pretty simple.

> 
> Also even if no UI changes are deemed necessary this will still
> require userspace changes to save+restore the two separate "last
> selected" profile settings over reboots.
> 
> Please let me know what you think of this, and of course another 
> completely different approach is welcome too.
> 
> Regards,
> 
> Hans
> 
> 
> 1) Although AMD based ThinkPads are the trigger for this discussion, 
> this applies to more new AMD based laptops, so this is not ThinkPad 
> specific
It's not even really AMD specific - doing support for them was just want
got the idea started.
> 
> 2) IMHO it would be good to file a RFE issue for this against p-p-d: 
> https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/new>>
Once we reach conclusion if this is a ack or nack I can do that.
I'd love for my team to get more involved with development at that level
if possible.

>  3) AFAICT Windows does the 2 separate "last selected" ac/battery
> profile settings thing while just showing a single slider in its UI,
> but that really is a whole other discussion which the userspace/UI
> folks can have.

Thanks
Mark

Re: [External] [discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[Mark Pearson]

Hi

On 3/21/22 18:29, Mark Pearson wrote:
> 
> Thanks Hans
> 
> On 2022-03-15 05:16, Hans de Goede wrote:
>> Hi All,
>>
>> AMD based ThinkPads (1) have separate ac (connected to an external 
>> powersource) and on battery tuned versions of the low-power, balanced
>> and performance profiles. So in essence they have six profiles:
>> low-power-ac, low-power-battery, balanced-ac, balanced-battery,
>> performance-ac and performance-battery.
>>
>> The question is how to deal with this. There is a previous
>> discussion about this here: 
>> https://lore.kernel.org/platform-driver-x86/20220301201554.4417-1-markpearson@lenovo.com/>>>  From that previous discussions 3 possible solutions come to mind:
>>
>> 1. Simply treat this as 6 different profiles, maybe with documenting 
>> -ac/-battery postfixes for the profile-names and let userspace sort
>> it out.
>>
>> Pro:    -Simple from the kernel side Contra: -Does not work with
>> existing userspace code -This is quite ugly IMHO / does not feel
>> right
> Agreed - I initially looked at implementing it this way and it's not
> good. I don't recommend it
> 
>>
>> 2. Only export three profiles to userspace, while going "all in" on
>> this concept and change drivers/acpi/platform_profile.c to add new: 
>> /sys/firmware/acpi/platform_profile_ac 
>> /sys/firmware/acpi/platform_profile_battery files which can contain
>> different desired settings for the ac/battery case and have the
>> kernel automatically switch between the two and also have it pass the
>> ac/battery state to platform_profile_handler.profile_set so that for
>> hw which has different ac/battery flavors of the profile the driver
>> knows which one to select (without needing to detect this itself)
>>
>> Pro:    -This matches well with the behavior which we want for the
>> user (which is for the system to save the profile as 2 separate
>> settings for ac/battery and switch the profile to the last selected
>> setting for ac/battery when the state changes) -Solve the ac/battery
>> state listening/notification only once instead of having all
>> platform_profile drivers DIY Contra: -This means deprecating
>> /sys/firmware/acpi/platform_profile and defining how it maps to the 2
>> new files, e.g. if it is written does that only set the current
>> active profile, or both ? -Userspace needs to be adjusted to use the
>> new uapi and once it has been adjusted it also still needs to work on
>> the older kernels which will be tricky/nasty to implement and also
>> is a problem from CI / testing pov.
> 
> I don't think the contra here is that complicated or causing backwards
> compatibility issues.
> 
> For the platform_profile attribute
>  - If you read it then you get the current active configured profile
> regardless of whether you are plugged in or not.
>  - If you set it then it sets both ac and battery profile and they
> stay in sync
> 
> For the platform_profile_ac and platform_profile_dc
>  - If you read it you get the configured setting for that power mode
>  - If you set it, then that mode is active for the appropriate power mode
> 
> I think that keeps this backwards compatible and user space can decide
> on their own schedule/preferences/design if they implement something extra.
> Am I missing something?
> 
>>
>> 3. Only export three profiles to userspace and have the 
>> drivers/acpi/platform_profile.c "core" code pass the ac/battery
>> state to platform_profile_handler.profile_set so that for hw which
>> has different ac/battery flavors of the profile the driver knows
>> which one to select (without needing to detect this itself)
>>
>> Pro:    -Solve the ac/battery state listening/notification only once 
>> instead of having all platform_profile drivers DIY -Leaves the
>> existing userspace API 100% unchanged and leaves existing userspace
>> code working without it requiring any changes Contra: -As part of the
>> discussion on this the RFE to "have separate "last selected"
>> ac/battery profile settings and automatically switch when the state
>> changes" has surfaced; and that is not solved
>>
> All looks reasonable to me.
> 
>>
>> Writing it down like this, to me 3. is the clear winner. The only 
>> downside of 3. I can come up with arguably is better solved in 
>> userspace (2), esp. since this will likely also require some UI
>> design work to somehow make it clear to the user that there are two
>> different settings (3).
> 
> For me I went with #2 in the RFC implementation as I thought it was a
> nice feature to have generally which is it's big plus - but I'm
> genuinely fairly split as to whether #2 or #3 is best. I could easily go
> either way.
> 
> One Q I have is that I would like our platforms to have the ability to
> auto-switch between AC and battery profile for their configured mode
> automatically. So if we go with option #3 I want to implement an event
> handler in the thinkpad_acpi driver for these platforms. Does that raise
> any flags or concerns? I prototyped this before I wrote the RFC and it's
> pretty simple.
> 
>>
>> Also even if no UI changes are deemed necessary this will still
>> require userspace changes to save+restore the two separate "last
>> selected" profile settings over reboots.
>>
>> Please let me know what you think of this, and of course another 
>> completely different approach is welcome too.
>>
>> Regards,
>>
>> Hans
>>
>>
>> 1) Although AMD based ThinkPads are the trigger for this discussion, 
>> this applies to more new AMD based laptops, so this is not ThinkPad 
>> specific
> It's not even really AMD specific - doing support for them was just want
> got the idea started.
>>
>> 2) IMHO it would be good to file a RFE issue for this against p-p-d: 
>> https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/new>> Once we reach conclusion if this is a ack or nack I can do that.
> I'd love for my team to get more involved with development at that level
> if possible.
> 
>>  3) AFAICT Windows does the 2 separate "last selected" ac/battery
>> profile settings thing while just showing a single slider in its UI,
>> but that really is a whole other discussion which the userspace/UI
>> folks can have.
> 
> Thanks
> Mark

Just wanted to check that, with limited responses, this thread is
effectively closed and the decision is to go with #3 - we'll leave any
implementation up to user space (if they want to take it on). Maybe this
is the prod I needed to go and play there. As a note, if anybody does
take this and needs help testing etc just let me know.

I do plan to follow up with a thinkpad_acpi specific patch for our
platforms using PSC mode.

Mark

Re: [External] [discuss] Split /sys/firmware/acpi/platform_profile into ac and battery profiles?

[Hans de Goede]

Hi Mark,

On 4/1/22 21:24, Mark Pearson wrote:
> Hi
> 
> On 3/21/22 18:29, Mark Pearson wrote:
>>
>> Thanks Hans
>>
>> On 2022-03-15 05:16, Hans de Goede wrote:
>>> Hi All,
>>>
>>> AMD based ThinkPads (1) have separate ac (connected to an external 
>>> powersource) and on battery tuned versions of the low-power, balanced
>>> and performance profiles. So in essence they have six profiles:
>>> low-power-ac, low-power-battery, balanced-ac, balanced-battery,
>>> performance-ac and performance-battery.
>>>
>>> The question is how to deal with this. There is a previous
>>> discussion about this here: 
>>> https://lore.kernel.org/platform-driver-x86/20220301201554.4417-1-markpearson@lenovo.com/>>>  From that previous discussions 3 possible solutions come to mind:
>>>
>>> 1. Simply treat this as 6 different profiles, maybe with documenting 
>>> -ac/-battery postfixes for the profile-names and let userspace sort
>>> it out.
>>>
>>> Pro:    -Simple from the kernel side Contra: -Does not work with
>>> existing userspace code -This is quite ugly IMHO / does not feel
>>> right
>> Agreed - I initially looked at implementing it this way and it's not
>> good. I don't recommend it
>>
>>>
>>> 2. Only export three profiles to userspace, while going "all in" on
>>> this concept and change drivers/acpi/platform_profile.c to add new: 
>>> /sys/firmware/acpi/platform_profile_ac 
>>> /sys/firmware/acpi/platform_profile_battery files which can contain
>>> different desired settings for the ac/battery case and have the
>>> kernel automatically switch between the two and also have it pass the
>>> ac/battery state to platform_profile_handler.profile_set so that for
>>> hw which has different ac/battery flavors of the profile the driver
>>> knows which one to select (without needing to detect this itself)
>>>
>>> Pro:    -This matches well with the behavior which we want for the
>>> user (which is for the system to save the profile as 2 separate
>>> settings for ac/battery and switch the profile to the last selected
>>> setting for ac/battery when the state changes) -Solve the ac/battery
>>> state listening/notification only once instead of having all
>>> platform_profile drivers DIY Contra: -This means deprecating
>>> /sys/firmware/acpi/platform_profile and defining how it maps to the 2
>>> new files, e.g. if it is written does that only set the current
>>> active profile, or both ? -Userspace needs to be adjusted to use the
>>> new uapi and once it has been adjusted it also still needs to work on
>>> the older kernels which will be tricky/nasty to implement and also
>>> is a problem from CI / testing pov.
>>
>> I don't think the contra here is that complicated or causing backwards
>> compatibility issues.
>>
>> For the platform_profile attribute
>>  - If you read it then you get the current active configured profile
>> regardless of whether you are plugged in or not.
>>  - If you set it then it sets both ac and battery profile and they
>> stay in sync
>>
>> For the platform_profile_ac and platform_profile_dc
>>  - If you read it you get the configured setting for that power mode
>>  - If you set it, then that mode is active for the appropriate power mode
>>
>> I think that keeps this backwards compatible and user space can decide
>> on their own schedule/preferences/design if they implement something extra.
>> Am I missing something?
>>
>>>
>>> 3. Only export three profiles to userspace and have the 
>>> drivers/acpi/platform_profile.c "core" code pass the ac/battery
>>> state to platform_profile_handler.profile_set so that for hw which
>>> has different ac/battery flavors of the profile the driver knows
>>> which one to select (without needing to detect this itself)
>>>
>>> Pro:    -Solve the ac/battery state listening/notification only once 
>>> instead of having all platform_profile drivers DIY -Leaves the
>>> existing userspace API 100% unchanged and leaves existing userspace
>>> code working without it requiring any changes Contra: -As part of the
>>> discussion on this the RFE to "have separate "last selected"
>>> ac/battery profile settings and automatically switch when the state
>>> changes" has surfaced; and that is not solved
>>>
>> All looks reasonable to me.
>>
>>>
>>> Writing it down like this, to me 3. is the clear winner. The only 
>>> downside of 3. I can come up with arguably is better solved in 
>>> userspace (2), esp. since this will likely also require some UI
>>> design work to somehow make it clear to the user that there are two
>>> different settings (3).
>>
>> For me I went with #2 in the RFC implementation as I thought it was a
>> nice feature to have generally which is it's big plus - but I'm
>> genuinely fairly split as to whether #2 or #3 is best. I could easily go
>> either way.
>>
>> One Q I have is that I would like our platforms to have the ability to
>> auto-switch between AC and battery profile for their configured mode
>> automatically. So if we go with option #3 I want to implement an event
>> handler in the thinkpad_acpi driver for these platforms. Does that raise
>> any flags or concerns? I prototyped this before I wrote the RFC and it's
>> pretty simple.
>>
>>>
>>> Also even if no UI changes are deemed necessary this will still
>>> require userspace changes to save+restore the two separate "last
>>> selected" profile settings over reboots.
>>>
>>> Please let me know what you think of this, and of course another 
>>> completely different approach is welcome too.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> 1) Although AMD based ThinkPads are the trigger for this discussion, 
>>> this applies to more new AMD based laptops, so this is not ThinkPad 
>>> specific
>> It's not even really AMD specific - doing support for them was just want
>> got the idea started.
>>>
>>> 2) IMHO it would be good to file a RFE issue for this against p-p-d: 
>>> https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/issues/new>> Once we reach conclusion if this is a ack or nack I can do that.
>> I'd love for my team to get more involved with development at that level
>> if possible.
>>
>>>  3) AFAICT Windows does the 2 separate "last selected" ac/battery
>>> profile settings thing while just showing a single slider in its UI,
>>> but that really is a whole other discussion which the userspace/UI
>>> folks can have.
>>
>> Thanks
>> Mark
> 
> Just wanted to check that, with limited responses, this thread is
> effectively closed and the decision is to go with #3 - we'll leave any
> implementation up to user space (if they want to take it on).

Yes I was hoping for some more feedback here from e.g. Rafael, but
I agree that it seems that the conclusion is to go with 3. which
is also the option which I prefer myself.

> Maybe this
> is the prod I needed to go and play there. As a note, if anybody does
> take this and needs help testing etc just let me know.
> 
> I do plan to follow up with a thinkpad_acpi specific patch for our
> platforms using PSC mode.

Sounds good, note as I indicated in the original description of 3.
I think it might be good to do the AC/battery switching listening
inside the core platform_profile code as I expect more drivers to
eventually need this.

Regards,

Hans