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