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.
