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
