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
