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

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

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

Oh - right good point.  It didn't occur to me that if you only have one 
set of profiles that you could still assign one of the profiles in that 
pool to AC and another to DC.

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

Yes that's correct.

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

It might be jarring the first time, but it's the right behavior in my 
mind.  And if someone doesn't like it they can always just write 
something to the other sysfs file to avoid it.

The other thing is thinkpad_acpi AMD support is new.  Define how it 
works now!

I don't think it affects any other platform profile provider any 
differently.

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

Yup you're right.

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

Sounds good, thanks.