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