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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx/>>>  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





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux