RE: [External] Re: [RFC] ACPI: platform-profile: support for AC vs DC modes

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

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

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

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

> So same end-result, but without introducing new userspace API. Note that
> even if we were to go with your RFC userspace would still need to be
> adjusted to use the new uapi, so AFAICT we really win nothing by doing this
> in the kernel.
> 
> > This patch let's that happen for any platform - regardless of if it has
> > separate support.
> 
> Doing this fully in userspace also allows this to happen for any platform.
> 
> > 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.
> 
> Distros usually are quicker in updating userspace bits which are required
> for hw-enablement purposes; and for this to be really useful it
> needs a UI to control it, so it will need userspace support regardless
> of where we solve it.
> 
> > 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.
> 
> Many many hw-enablement things, like thunderbolt authentication, proper
> automatic switching of audio streams, proper handling of various hotkeys
> like vol / brightness up/down, privacy screen on/off toggling, easy use
> of VPNs and wwan modems, are all not or very poorly supported in other
> desktop-environments. I often get bug reports from users using other
> DEs about things like this and there really is nothing which we can
> do about this. In some cases like i3 window-manager the norm
> is actually for users to have to write custom scripts for everything.
> 
> Other DEs do try, but simply lack the manpower, this is an unfortunate
> situation, but not really a good argument for just shoving everything
> in the kernel.
> 
> > 3) The power events happen in the kernel which is perfect. Once I
> > figured that out it seemed a no-brainer to me.
> 
> That same event also gets forwarded to userspace over the power_supply
> class uapi.
> > 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.
> 
> There is nothing which the kernel can do here, which userspace cannot
> also do; and generally speaking in cases like this userspace can do it
> better, because it can add a lot more fancy policy like (random example):
> switch to performance mode when on AC and the battery is fully charged.
> 
> There will be many different usecases here and hardcoding a single
> simple but also dumb policy in the kernel really is not doing anyone
> any favors.
> 
> >>> 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.
> 
> A userspace solution, at least at the power-profile-daemon level will
> be just as universal. And as for unsupported DEs, whether users have
> to manually poke a sysfs file, or have to manually make a dbus call
> (which can also be done from the shell) really does not make much
> of a difference.
> 
> > 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.
> 
> See above, userspace not doing its job properly is really not a good
> argument to put something in the kernel. The kernel generally is
> about resource management/sharing and hardware abstraction,
> in general where possible the kernel should provide mechanism with
> no, or a very minimal policy and leave the rest up to userspace.
> 
> This feels like just shoving something into the kernel just because
> that is convenient, not because it really belongs in the kernel,
> but we want the kernel to be as small as possible (it really already
> is much too big) because:
> 
> 1. Any kernel bugs are fatal if they get triggered, unlike hitting
>    a bug in userdspace code, hitting a bug in kernelspace code will
>    almost always take the entire system down.
> 2. Kernel memory is not swapable
> 
> > 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.
> 
> Above you are talking about changes to the thinkpad_acpi driver
> to possibly use some sort of "hardware" support for ac/dc profile
> switching, that is something which might be sensible to support
> in the kernel, although given that that is Lenovo specific a
> generic userspace solution to offer the same functionality by
> having p-p-d switch profiles on ac/dc changes seems like it
> would work just as well, avoiding the need for anything lenovo
> specific .
> 
> Regards,
> 
> Hans