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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux