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

[Mark Pearson <markpearson@lenovo.com>]

Thanks Mario!

On 2022-03-02 21:53, Mario Limonciello wrote:
> On 3/1/22 14:15, Mark Pearson wrote:
>> Looking for feedback on this feature. Whether it is worth
>> pursuing and any concerns with the general implementation.
>>
>> I've recently been working on PSC platform profile mode support
>> for Lenovo AMD platforms (patch proposed upstream last week).
>> One of the interesting pieces with the Lenovo PSC implementation
>> is it supports different profiles for AC (plugged in) vs DC
>> (running from battery).
>>
>> I was thinking of adding this support in the thinkpad_acpi driver,
>> but it seems it would be nicer to make this generally available for
>> all platforms that offer profile support.
>>
>> This implementation allows the user to set one profile for when a
>> system is plugged in, and a different profile for when they are
>> unplugged. I imagine this would be used so that performance mode
>> is used when plugged in and low-power used when unplugged (as an
>> example). The user could configure it to match their preference.
>>
>> If the user doesn't configure a DC profile it behaves the same as
>> previously and any ACPI power events will be ignored. If the user
>> configures a DC profile then when a system is unplugged it will
>> automatically configure this setting.
>>
>> I've added platform_profile_ac and platform_profile_dc sysfs nodes.
>> The platform_profile and platform_profile_ac nodes will behave the
>> same when setting a profile to maintain backwards compatibility.
> 
> To make it more deterministic I would say configure it like this:
> 1) If you write a profile to `platform_profile` and the backend supports
> both DC and AC profiles make it the default profile for both.  This is
> more like "backwards compatibility" mode
> 2) If you write a profile to `platform_profile_dc` and the backend
> supports both then don't do anything in `platform_profile_ac` and vice
> versa.  Require a user to write both of them explicitly.
> 
> That means you have a new state of "unset" for the profiles, but if you
> don't include the state then I think it can lead to confusing behaviors
> if userspace writes one vs the other first.
> 
So I don't think any backend support is particularly needed. If a platform
supports platform profiles, then having an AC vs DC mode doesn't need
anything special - it's just switching modes.

On the Lenovo AMD platforms in the thinkpad_acpi driver this will
trigger some extra niceness to use a separate set of profiles, but even
for the platforms that don't have this it's still nice to auto-switch to
(for example) a low-power mode when you unplug - and that doesn't need
any extra support beyond just supporting platform profiles

I like the suggestion on updating both if platform_profile is update but
I'm worried that it changes the current behaviour:

- on the Lenovo AMD profiles, the battery profile set are all lower
power/performance than the plugged-in profiles (by design)
- with the implicit setting of the dc mode we'd be changing the behavior
so that when you unplug performance will drop.

This is (usually) a good thing, and is I think what Windows users get,
but it's not what currently happens on Linux. To get back to full power mode
you'd have to disable the DC setting - which isn't particularly
intuitive. It's why in this initial implementation I made the dc setting
'opt-in'....but maybe I'm being over cautious.

Once user space has two sliders the whole point is moot - I suspect I'm
overthinking this :)

Ack on the unset state - I'll add that.

>>
>> If you read the platform_profile it will return the currently
>> active profile.
>> If you read the platform_profile_ac or platform_profile_dc node it
>> will return the configured profile. This is something missing from
>> the current implementation that I think is a nice bonus.
> 
> Yeah nice bonus.  Some inline comments on this.
> 
>>
>> User space implementation could potentially be used to do the same
>> idea, but having this available allows users to configure from
>> cmdline or use scripts seemed valuable.
>>
>> Note - I'm aware that I still need to:
>>   1) Update the API documentation file
>>   2) Implement a disable/unconfigure on the profile_dc setting
>> But I figured this was far enough along that it would be good to get
>> comments.
> 
> If backend doesn't support AC/DC I think you should return an error for
> one of them rather than trying to hide the difference.  Think about
> userspace - it might want to have say two sliders and hide one if one of
> them isn't supported.

See above - I don't think there are cases where this wouldn't be
supported. Let me know if I'm missing something

> 
>>
>> Thanks in advance for any feedback.
>>
>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> ---
>>   drivers/acpi/platform_profile.c  | 130 +++++++++++++++++++++++++++++--
>>   include/linux/platform_profile.h |   1 +
>>   2 files changed, 125 insertions(+), 6 deletions(-)
>>
<snip<
>> @@ -117,10 +227,14 @@ static ssize_t platform_profile_store(struct
>> device *dev,
>>     static DEVICE_ATTR_RO(platform_profile_choices);
>>   static DEVICE_ATTR_RW(platform_profile);
>> +static DEVICE_ATTR_RW(platform_profile_ac);
>> +static DEVICE_ATTR_RW(platform_profile_dc);
> 
> My opinion here is that if you are keeping the existing one in place to
> show "current" active profile and make the new ones to show you
> "selected" profile they should have a different naming convention.
> 
> Some ideas:
> - selected_*_profile
> - platform_profile_policy_*
> - *_policy
> 
> Something else that comes to mind is you can rename "platform_profile"
> as "active_profile" (but create a compatibility symlink back to
> platform_profile), but I don't know that's really needed as long as it's
> all well documented.
> 

I like the selected_*_profile - I'll go with that.
I'm less enthusiastic about symlinks - just feels messy.

Thanks for the feedback - very much appreciated
Mark