Hi Hans & Mario Thanks for all the comments. On 2022-03-14 10:43, Hans de Goede wrote: > Hi Mario, > > On 3/14/22 14:39, Limonciello, Mario wrote: >> [Public] >> >>>> >>>> I cycled through a few different implementations but came down on what I >>>> proposed. I considered 6 values - but I don't think that makes sense and >>>> makes it overall more complicated than it needs to be and less flexible. >>> >>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>> because I'm wondering how the firmware API here actually looks like, >>> something which so far is not really clear to me. >>> >>> When you say that you considered using 6 values, then I guess that >>> the firmware API actually offers 6 values which we can write to a single slot: >>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>> performance >>> >>> ? >>> >>> But that is not what the RFC patch that started this thread shows at all, >>> the API to the driver is totally unchanged and does not get passed >>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>> switches between say ac-balanced and dc-balanced internally. >>> >>> IOW there really being 2 differently tuned balance-profiles is not visible to >>> the OS at all, this is handled internally inside the EC, correct ? >>> >> >> No - on Lenovo's platform there are 6 different profiles that can be selected >> from the kernel driver. 3 are intended for use on battery, 3 are intended for >> use on AC. > > Ah, I already got that feeling from the rest of the thread, so I reread > Mark's RFC again before posting my reply today and the RFC looked like > the same 3 profiles were being set and the only functionality added > was auto profile switching when changing between AC/battery. > > Thank you for clarifying this. Having 6 different stories > indeed is a very different story. Apologies if I wasn't clear. I was trying to come up with a design that took advantage of the AMD platforms have 6 settings, but was extensible generally to other situations. I will redo the patches and add the thinkpad_acpi on top - that will help it be clearer. > >>> Otherwise I would expect the kernel internal driver API to also change and >>> to also see a matching thinkpad_acpi patch in the RFC series? >> >> The idea I see from Mark's thread was to send out RFC change for the platform profile >> and based on the direction try to implement the thinkpad-acpi change after that. >> >> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >> modeled on top of this the way that you want. > > I fully agree and since you introduce the concept of being on AC/battery to the > drivers/acpi/platform_profile.c cpde, please change the > profile_set and profile_get function prototypes in struct platform_profile_handler > to also take a "bool on_battery" extra argument and use that in the thinkpad > driver to select either the ac or the battery tuned low/balanced/performance > profile. OK - I was thinking that, but I also figured the thinkpad driver could get the power status directly so it was largely redundant (and saves churn on all the other platform profile drivers - there are quite a few now) > > And please also include an update to Documentation/ABI/testing/sysfs-platform_profile > in the next RFC. Absolutely - that was intended. My aim with this RFC was to get feedback on if it was acceptable or not, and if the design had to change. Really appreciate all the good points. > > Also notice how I've tried to consistently use AC/battery in my last reply, > DC really is not a good term for "on battery". AC also is sort of dubious > for "connected to an external power-supply" but its use for that is sorta > common and it is nice and short. > > Sorry if this seems like bikeshedding, but using DC for "on battery" just > feels wrong to me. Ack - and I'm good with the suggestion. > > >>>> The biggest use case I can think of is that a user wants performance >>>> when plugged in and power-save when unplugged; and they want that to >>>> happen automatically. >>> >>> Right, so this what I have understood all along and I'm not disagreeing >>> that this is a desirable feature, but it _does not belong in the kernel_! >>> >>> Also taking Mario's remark about the EC-firmware using differently >>> tuned balanced profiles based on ac vs dc, here is how I envision this >>> working: >>> >>> 1. Laptop is connected to charger >>> 2. EC notices this and: >>> 2.1 Internally switches from balanced-dc settings to balanced-ac settings >>> 2.2 Sends out an event about the laptop now being on AC, which the kernel >>> picks up and then sends to userspace (this already happens) >>> 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is >>> now an AC and in its settings sees that the user wants to switch to >>> performance mode on AC and uses the platform_api in its current form to >>> ask for a switch to performance mode >>> 4. The EC gets a command telling it to switch to performance mode and >>> switches to the ac-tuned version of performance mode since the laptop is >>> on ac. >>> >> >> None of this happens internally on the EC. > > Ack, I understand now thank you for clarifying this. Sorry for not being clear here > >> Also there is nothing in this design >> that guarantees it needs to be EC driven profile changes. It could be other >> mailboxes, ASL code, SMM etc. >> >> The key point here is that thinkpad acpi has 3 AC and 3 DC profiles to choose from, >> so some level from thinkpad acpi above needs to pick among them. > > Ack. > I think this is what makes having the design in the kernel more important. I understand the keeping the kernel small, but the thinkpad_acpi driver needs to guarantee it knows it will get the notification. Without that I don't think I can implement the feature reliably An alternative to the implementation is for me to do this in just the thinkpad_acpi driver and just for PSC mode, and that's what I started with when I looked at this (it's quite a nice simple implementation FWIW). But I figured having something that was configurable has benefits, and something that is applicable to all platforms is a nice feature as well. If doing thinkpad_acpi only would be preferred and more acceptable let me know - but I think it's more limiting overall Thanks Mark
