Thanks Mario & Hans On 2022-03-08 11:10, Hans de Goede wrote: > Hi, > > On 3/8/22 16:55, Limonciello, Mario wrote: >> [AMD Official Use Only] >> >>>> I don't think that's right for the PSC Thinkpads. They have dedicated >>>> different tunings for each of the slider positions on AC vs DC. >>>> >>>> So "balanced" on AC will not be the same as "balanced" on DC. >>> >>> I see, but it is not like balanced on AC is closer to performance >>> on DC then it is to balanced on DC, right? IOW in the UI we should >>> still call them both balanced ? >> >> I feel that's a gross oversimplification to say balanced on AC is close >> to performance on DC. There are *so many* other (otherwise invisible) >> tuning knobs behind what PSC does that Lenovo has weighed out the benefits >> of using for different circumstances. >> >> You nerf all this by just having one user space facing knob and let userspace >> change to performance mode when you on charger. > > The way I see this there are 2 ways this can work on the kernel to fw/ec > boundary: > > 1. There are actually 6 values we can write to a single slot: > ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc-performance > > 2. There are separate ac-setting + dc-setting slots to which we can > write one of 3 values: low-power, balanced, performance; and the fw/ec > automatically picks which slot to used based on ac vs battery status > > If 1 is the case for PSC then I agree that the kernel should indeed get involved > and it should automatically write either the ac or dc variant of the last > userspace requested value so that things behave as expected. > > If 2 however is the case then I think all that is necessary is for the > driver to just write the last userspace selected value to both slots. > > Note that neither case requires a userspace API change when solved > as I suggest. 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. 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. This patch let's that happen for any platform - regardless of if it has separate support. If a vendor wants to handle plugged in vs battery to a more nuanced degree they can, but that's at the individual driver level. I originally thought that maybe this should be done in user space but: 1) It takes a lot longer for user space changes to get rolled out and make it into distros. 2) Not all users will get to use it - sure Gnome and KDE might get the feature but chances of other desktops picking it up are small. I could look at releasing a utility to do it but....urgh. Nobody gets a good experience that way. Linux packaging is a minefield. 3) The power events happen in the kernel which is perfect. Once I figured that out it seemed a no-brainer to me. I think user space should add the ability to have a nice GUI to toggle a unplugged profile setting. But the guts of it seem to me to belong better in the kernel. > >> At least the way Windows does this is that it offers "one" UI slider but you >> have last selected values based on if you're plugged in or on battery. >> >> 1) So on battery I might have balanced selected to start out. >> 2) Then I plug in a charger, and balanced is still selected but this has >> different characteristics from balanced on battery. >> 3) Now I change to performance while on charger. >> 4) Then I unplug charger and it goes back to my selection for battery: "balanced". > > The above is more about policy then it is about mechanism, userspace > can easily remember 2 separate settings for ac vs battery and restore > the last set value for ac or battery when changing between the 2. > > Since this mostly about the policy which profile to set when this > really belongs in userspace IMHO and solving this in userspace means that > we will have a single universal solution for all the different > platform_profile implementations, and we seem to have quite a lot of > those (at least one per laptop vendor, Lenovo currently has 2) I disagree here. This is more universal by design. I was surprised at how many vendors are using platform-profiles (I think it's awesome!) but now they can all get this too. The intention here is very strongly not supposed to be Lenovo specific. The follow on patch that I could do in thinkpad_acpi to use a different setting in unplugged/plugged mode - that will be Lenovo specific and taking advantage of the functionality the Lenovo FW is offering. That doesn't seem unreasonable to me. >>> If that is right then I think my point still stands, if PSC >>> has 2 separate slots (one AC one DC) for the performance >>> setting, then we can just set both when userspace selects a >>> performance level and have the actual e.g. balanced -> performance >>> change be done by userspace when userspace select the machine >>> has been connected to a charger. >> >> But you *don't want to* actually select performance when you switch to a >> charger. If you have 3 value slots for AC and 3 value slots for DC you >> should only be swapping between what is in those 3 values slots. > > That only works if all implementation have separate AC and DC profile > slots, which most won't have. If we just sync the 2 slots for implementations > which do have 2 slots and then always "fake" 2 slots in userspace we > have a universal implementation which will work well everywhere, without > any significant downside to the implementations which do have 2 slots. > I'm missing something in this bit. If a vendor is providing platform profiles all we're doing is letting a user choose the profile they want depending on their power situation. I don't think there are empty slots particularly. I've got a feeling I'm missing something obvious - but my experience of user space is it's really hard to get a consistent experience for all Linux users reliably - everybody is running something different. If nothing else I think that should be a big factor for adding this support to the kernel. Obviously if this feature isn't wanted I'll drop it - but I think it's something useful that users will appreciate on any HW. Mark
