| Message ID | 20220301201554.4417-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] ACPI: platform-profile: support for AC vs DC modes | expand |
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. > > 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. > > 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(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index d418462ab791..e4246e6632cf 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/mutex.h> > #include <linux/platform_profile.h> > +#include <linux/power_supply.h> > #include <linux/sysfs.h> > > static struct platform_profile_handler *cur_profile; > @@ -22,6 +23,51 @@ static const char * const profile_names[] = { > }; > static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); > > +static struct notifier_block ac_nb; > +static int cur_profile_ac; > +static int cur_profile_dc; > + > +static int platform_profile_set(void) > +{ > + int profile, err; > + > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > + profile = cur_profile_ac; > + else { > + if (power_supply_is_system_supplied() > 0) > + profile = cur_profile_ac; > + else > + profile = cur_profile_dc; > + } > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + err = cur_profile->profile_set(cur_profile, profile); > + if (err) > + return err; > + > + sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + mutex_unlock(&profile_lock); > + return 0; > +} > + > +static int platform_profile_acpi_event(struct notifier_block *nb, > + unsigned long val, > + void *data) > +{ > + struct acpi_bus_event *entry = (struct acpi_bus_event *)data; > + > + WARN_ON(cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED); > + > + /* if power supply changed, then update profile */ > + if (strcmp(entry->device_class, "ac_adapter") == 0) > + return platform_profile_set(); > + > + return 0; > +} > + > static ssize_t platform_profile_choices_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -77,9 +123,34 @@ static ssize_t platform_profile_show(struct device *dev, > return sysfs_emit(buf, "%s\n", profile_names[profile]); > } > > -static ssize_t platform_profile_store(struct device *dev, > +static ssize_t configured_profile_show(struct device *dev, > struct device_attribute *attr, > - const char *buf, size_t count) > + char *buf, int profile) > +{ > + if (profile == PLATFORM_PROFILE_UNCONFIGURED) > + return sysfs_emit(buf, "Not-configured\n"); > + > + /* Check that profile is valid index */ > + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > + return -EIO; > + return sysfs_emit(buf, "%s\n", profile_names[profile]); > +} > + > +static ssize_t platform_profile_ac_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return configured_profile_show(dev, attr, buf, cur_profile_ac); > +} > + > +static ssize_t platform_profile_dc_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return configured_profile_show(dev, attr, buf, cur_profile_dc); > +} > + > +static int profile_select(const char *buf) > { > int err, i; > > @@ -105,11 +176,50 @@ static ssize_t platform_profile_store(struct device *dev, > return -EOPNOTSUPP; > } > > - err = cur_profile->profile_set(cur_profile, i); > - if (!err) > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > - > mutex_unlock(&profile_lock); > + return i; > +} > + > +static ssize_t platform_profile_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int profile, err; > + > + profile = profile_select(buf); > + if (profile < 0) > + return profile; > + > + cur_profile_ac = profile; > + err = platform_profile_set(); > + if (err) > + return err; > + return count; > +} > + > +static ssize_t platform_profile_ac_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + return platform_profile_store(dev, attr, buf, count); > +} > + > +static ssize_t platform_profile_dc_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int profile, err; > + > + profile = profile_select(buf); > + if (profile < 0) > + return profile; > + > + /* We need to register for ACPI events now */ > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > + register_acpi_notifier(&ac_nb); > + > + cur_profile_dc = profile; > + err = platform_profile_set(); > if (err) > return err; > return count; > @@ -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. > > static struct attribute *platform_profile_attrs[] = { > &dev_attr_platform_profile_choices.attr, > &dev_attr_platform_profile.attr, > + &dev_attr_platform_profile_ac.attr, > + &dev_attr_platform_profile_dc.attr, > NULL > }; > > @@ -161,7 +275,9 @@ int platform_profile_register(struct platform_profile_handler *pprof) > } > > cur_profile = pprof; > + cur_profile_ac = cur_profile_dc = PLATFORM_PROFILE_UNCONFIGURED; > mutex_unlock(&profile_lock); > + ac_nb.notifier_call = platform_profile_acpi_event; > return 0; > } > EXPORT_SYMBOL_GPL(platform_profile_register); > @@ -169,6 +285,8 @@ EXPORT_SYMBOL_GPL(platform_profile_register); > int platform_profile_remove(void) > { > sysfs_remove_group(acpi_kobj, &platform_profile_group); > + if (cur_profile_dc != PLATFORM_PROFILE_UNCONFIGURED) > + unregister_acpi_notifier(&ac_nb); > > mutex_lock(&profile_lock); > cur_profile = NULL; > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > index e5cbb6841f3a..34566256bb60 100644 > --- a/include/linux/platform_profile.h > +++ b/include/linux/platform_profile.h > @@ -15,6 +15,7 @@ > * If more options are added please update profile_names array in > * platform_profile.c and sysfs-platform_profile documentation. > */ > +#define PLATFORM_PROFILE_UNCONFIGURED -1 > > enum platform_profile_option { > PLATFORM_PROFILE_LOW_POWER,
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
>> 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.
Hi Mark, On 3/1/22 21: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). Nitpick: the power going to the laptop has long been converted to DC when it goes into the laptop and when e.g. charging with a car-lighter-connection-to-type-c convertor it has never not been DC. IMHO external_power vs battery would be better names. > 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. > > 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. > > 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. > > Thanks in advance for any feedback. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> If I understand things correctly, then there is no difference between e.g. performance on AC vs on battery, this is just automatic switching the profiles when connecting/disconnecting the charger, correct? If I got that correct, there is no reason why userspace could not do this itself and implementing this in userspace has the advantage that it will work everywhere including on non PSC ThinkPads The hardest part of implementing something like this would be the userspace UI design and any policy decisions surrounding this, if we spend time on implementing those then making userspace do the actual switching of the profiles is pretty trivial in comparison and as said would be a much more universal way to implement this. So IMHO even though some hardware may offer this functionality, this is best left to an universal userspace implementation. Regards, Hans > --- > drivers/acpi/platform_profile.c | 130 +++++++++++++++++++++++++++++-- > include/linux/platform_profile.h | 1 + > 2 files changed, 125 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index d418462ab791..e4246e6632cf 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/mutex.h> > #include <linux/platform_profile.h> > +#include <linux/power_supply.h> > #include <linux/sysfs.h> > > static struct platform_profile_handler *cur_profile; > @@ -22,6 +23,51 @@ static const char * const profile_names[] = { > }; > static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); > > +static struct notifier_block ac_nb; > +static int cur_profile_ac; > +static int cur_profile_dc; > + > +static int platform_profile_set(void) > +{ > + int profile, err; > + > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > + profile = cur_profile_ac; > + else { > + if (power_supply_is_system_supplied() > 0) > + profile = cur_profile_ac; > + else > + profile = cur_profile_dc; > + } > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + err = cur_profile->profile_set(cur_profile, profile); > + if (err) > + return err; > + > + sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + mutex_unlock(&profile_lock); > + return 0; > +} > + > +static int platform_profile_acpi_event(struct notifier_block *nb, > + unsigned long val, > + void *data) > +{ > + struct acpi_bus_event *entry = (struct acpi_bus_event *)data; > + > + WARN_ON(cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED); > + > + /* if power supply changed, then update profile */ > + if (strcmp(entry->device_class, "ac_adapter") == 0) > + return platform_profile_set(); > + > + return 0; > +} > + > static ssize_t platform_profile_choices_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -77,9 +123,34 @@ static ssize_t platform_profile_show(struct device *dev, > return sysfs_emit(buf, "%s\n", profile_names[profile]); > } > > -static ssize_t platform_profile_store(struct device *dev, > +static ssize_t configured_profile_show(struct device *dev, > struct device_attribute *attr, > - const char *buf, size_t count) > + char *buf, int profile) > +{ > + if (profile == PLATFORM_PROFILE_UNCONFIGURED) > + return sysfs_emit(buf, "Not-configured\n"); > + > + /* Check that profile is valid index */ > + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > + return -EIO; > + return sysfs_emit(buf, "%s\n", profile_names[profile]); > +} > + > +static ssize_t platform_profile_ac_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return configured_profile_show(dev, attr, buf, cur_profile_ac); > +} > + > +static ssize_t platform_profile_dc_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return configured_profile_show(dev, attr, buf, cur_profile_dc); > +} > + > +static int profile_select(const char *buf) > { > int err, i; > > @@ -105,11 +176,50 @@ static ssize_t platform_profile_store(struct device *dev, > return -EOPNOTSUPP; > } > > - err = cur_profile->profile_set(cur_profile, i); > - if (!err) > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > - > mutex_unlock(&profile_lock); > + return i; > +} > + > +static ssize_t platform_profile_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int profile, err; > + > + profile = profile_select(buf); > + if (profile < 0) > + return profile; > + > + cur_profile_ac = profile; > + err = platform_profile_set(); > + if (err) > + return err; > + return count; > +} > + > +static ssize_t platform_profile_ac_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + return platform_profile_store(dev, attr, buf, count); > +} > + > +static ssize_t platform_profile_dc_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int profile, err; > + > + profile = profile_select(buf); > + if (profile < 0) > + return profile; > + > + /* We need to register for ACPI events now */ > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > + register_acpi_notifier(&ac_nb); > + > + cur_profile_dc = profile; > + err = platform_profile_set(); > if (err) > return err; > return count; > @@ -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); > > static struct attribute *platform_profile_attrs[] = { > &dev_attr_platform_profile_choices.attr, > &dev_attr_platform_profile.attr, > + &dev_attr_platform_profile_ac.attr, > + &dev_attr_platform_profile_dc.attr, > NULL > }; > > @@ -161,7 +275,9 @@ int platform_profile_register(struct platform_profile_handler *pprof) > } > > cur_profile = pprof; > + cur_profile_ac = cur_profile_dc = PLATFORM_PROFILE_UNCONFIGURED; > mutex_unlock(&profile_lock); > + ac_nb.notifier_call = platform_profile_acpi_event; > return 0; > } > EXPORT_SYMBOL_GPL(platform_profile_register); > @@ -169,6 +285,8 @@ EXPORT_SYMBOL_GPL(platform_profile_register); > int platform_profile_remove(void) > { > sysfs_remove_group(acpi_kobj, &platform_profile_group); > + if (cur_profile_dc != PLATFORM_PROFILE_UNCONFIGURED) > + unregister_acpi_notifier(&ac_nb); > > mutex_lock(&profile_lock); > cur_profile = NULL; > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > index e5cbb6841f3a..34566256bb60 100644 > --- a/include/linux/platform_profile.h > +++ b/include/linux/platform_profile.h > @@ -15,6 +15,7 @@ > * If more options are added please update profile_names array in > * platform_profile.c and sysfs-platform_profile documentation. > */ > +#define PLATFORM_PROFILE_UNCONFIGURED -1 > > enum platform_profile_option { > PLATFORM_PROFILE_LOW_POWER,
[Public] > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Tuesday, March 8, 2022 08:39 > To: Mark Pearson <markpearson@lenovo.com> > Cc: rafael@kernel.org; linux-acpi@vger.kernel.org; platform-driver- > x86@vger.kernel.org > Subject: Re: [RFC] ACPI: platform-profile: support for AC vs DC modes > > Hi Mark, > > On 3/1/22 21: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). > > Nitpick: the power going to the laptop has long been converted > to DC when it goes into the laptop and when e.g. charging > with a car-lighter-connection-to-type-c convertor it has never > not been DC. > > IMHO external_power vs battery would be better names. > > > 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. > > > > 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. > > > > 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. > > > > Thanks in advance for any feedback. > > > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > > If I understand things correctly, then there is no difference > between e.g. performance on AC vs on battery, this is just > automatic switching the profiles when connecting/disconnecting > the charger, correct? > > If I got that correct, there is no reason why userspace could > not do this itself and implementing this in userspace has the > advantage that it will work everywhere including on non > PSC ThinkPads I don't think that's right for the PSC Thinkpads. They have dedicated different tunings for each of the slider positions on AC vs DC. So "balanced" on AC will not be the same as "balanced" on DC. > > The hardest part of implementing something like this would > be the userspace UI design and any policy decisions surrounding > this, if we spend time on implementing those then making userspace > do the actual switching of the profiles is pretty trivial in > comparison and as said would be a much more universal way > to implement this. > > So IMHO even though some hardware may offer this functionality, > this is best left to an universal userspace implementation. > > Regards, > > Hans > > > > --- > > drivers/acpi/platform_profile.c | 130 +++++++++++++++++++++++++++++-- > > include/linux/platform_profile.h | 1 + > > 2 files changed, 125 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > > index d418462ab791..e4246e6632cf 100644 > > --- a/drivers/acpi/platform_profile.c > > +++ b/drivers/acpi/platform_profile.c > > @@ -7,6 +7,7 @@ > > #include <linux/init.h> > > #include <linux/mutex.h> > > #include <linux/platform_profile.h> > > +#include <linux/power_supply.h> > > #include <linux/sysfs.h> > > > > static struct platform_profile_handler *cur_profile; > > @@ -22,6 +23,51 @@ static const char * const profile_names[] = { > > }; > > static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); > > > > +static struct notifier_block ac_nb; > > +static int cur_profile_ac; > > +static int cur_profile_dc; > > + > > +static int platform_profile_set(void) > > +{ > > + int profile, err; > > + > > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > > + profile = cur_profile_ac; > > + else { > > + if (power_supply_is_system_supplied() > 0) > > + profile = cur_profile_ac; > > + else > > + profile = cur_profile_dc; > > + } > > + > > + err = mutex_lock_interruptible(&profile_lock); > > + if (err) > > + return err; > > + > > + err = cur_profile->profile_set(cur_profile, profile); > > + if (err) > > + return err; > > + > > + sysfs_notify(acpi_kobj, NULL, "platform_profile"); > > + mutex_unlock(&profile_lock); > > + return 0; > > +} > > + > > +static int platform_profile_acpi_event(struct notifier_block *nb, > > + unsigned long val, > > + void *data) > > +{ > > + struct acpi_bus_event *entry = (struct acpi_bus_event *)data; > > + > > + WARN_ON(cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED); > > + > > + /* if power supply changed, then update profile */ > > + if (strcmp(entry->device_class, "ac_adapter") == 0) > > + return platform_profile_set(); > > + > > + return 0; > > +} > > + > > static ssize_t platform_profile_choices_show(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > @@ -77,9 +123,34 @@ static ssize_t platform_profile_show(struct device > *dev, > > return sysfs_emit(buf, "%s\n", profile_names[profile]); > > } > > > > -static ssize_t platform_profile_store(struct device *dev, > > +static ssize_t configured_profile_show(struct device *dev, > > struct device_attribute *attr, > > - const char *buf, size_t count) > > + char *buf, int profile) > > +{ > > + if (profile == PLATFORM_PROFILE_UNCONFIGURED) > > + return sysfs_emit(buf, "Not-configured\n"); > > + > > + /* Check that profile is valid index */ > > + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > > + return -EIO; > > + return sysfs_emit(buf, "%s\n", profile_names[profile]); > > +} > > + > > +static ssize_t platform_profile_ac_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + return configured_profile_show(dev, attr, buf, cur_profile_ac); > > +} > > + > > +static ssize_t platform_profile_dc_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + return configured_profile_show(dev, attr, buf, cur_profile_dc); > > +} > > + > > +static int profile_select(const char *buf) > > { > > int err, i; > > > > @@ -105,11 +176,50 @@ static ssize_t platform_profile_store(struct device > *dev, > > return -EOPNOTSUPP; > > } > > > > - err = cur_profile->profile_set(cur_profile, i); > > - if (!err) > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > > - > > mutex_unlock(&profile_lock); > > + return i; > > +} > > + > > +static ssize_t platform_profile_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int profile, err; > > + > > + profile = profile_select(buf); > > + if (profile < 0) > > + return profile; > > + > > + cur_profile_ac = profile; > > + err = platform_profile_set(); > > + if (err) > > + return err; > > + return count; > > +} > > + > > +static ssize_t platform_profile_ac_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + return platform_profile_store(dev, attr, buf, count); > > +} > > + > > +static ssize_t platform_profile_dc_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int profile, err; > > + > > + profile = profile_select(buf); > > + if (profile < 0) > > + return profile; > > + > > + /* We need to register for ACPI events now */ > > + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) > > + register_acpi_notifier(&ac_nb); > > + > > + cur_profile_dc = profile; > > + err = platform_profile_set(); > > if (err) > > return err; > > return count; > > @@ -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); > > > > static struct attribute *platform_profile_attrs[] = { > > &dev_attr_platform_profile_choices.attr, > > &dev_attr_platform_profile.attr, > > + &dev_attr_platform_profile_ac.attr, > > + &dev_attr_platform_profile_dc.attr, > > NULL > > }; > > > > @@ -161,7 +275,9 @@ int platform_profile_register(struct > platform_profile_handler *pprof) > > } > > > > cur_profile = pprof; > > + cur_profile_ac = cur_profile_dc = > PLATFORM_PROFILE_UNCONFIGURED; > > mutex_unlock(&profile_lock); > > + ac_nb.notifier_call = platform_profile_acpi_event; > > return 0; > > } > > EXPORT_SYMBOL_GPL(platform_profile_register); > > @@ -169,6 +285,8 @@ EXPORT_SYMBOL_GPL(platform_profile_register); > > int platform_profile_remove(void) > > { > > sysfs_remove_group(acpi_kobj, &platform_profile_group); > > + if (cur_profile_dc != PLATFORM_PROFILE_UNCONFIGURED) > > + unregister_acpi_notifier(&ac_nb); > > > > mutex_lock(&profile_lock); > > cur_profile = NULL; > > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > > index e5cbb6841f3a..34566256bb60 100644 > > --- a/include/linux/platform_profile.h > > +++ b/include/linux/platform_profile.h > > @@ -15,6 +15,7 @@ > > * If more options are added please update profile_names array in > > * platform_profile.c and sysfs-platform_profile documentation. > > */ > > +#define PLATFORM_PROFILE_UNCONFIGURED -1 > > > > enum platform_profile_option { > > PLATFORM_PROFILE_LOW_POWER,
Hi, On 3/8/22 15:50, Limonciello, Mario wrote: > [Public] > > > >> -----Original Message----- >> From: Hans de Goede <hdegoede@redhat.com> >> Sent: Tuesday, March 8, 2022 08:39 >> To: Mark Pearson <markpearson@lenovo.com> >> Cc: rafael@kernel.org; linux-acpi@vger.kernel.org; platform-driver- >> x86@vger.kernel.org >> Subject: Re: [RFC] ACPI: platform-profile: support for AC vs DC modes >> >> Hi Mark, >> >> On 3/1/22 21: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). >> >> Nitpick: the power going to the laptop has long been converted >> to DC when it goes into the laptop and when e.g. charging >> with a car-lighter-connection-to-type-c convertor it has never >> not been DC. >> >> IMHO external_power vs battery would be better names. >> >>> 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. >>> >>> 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. >>> >>> 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. >>> >>> Thanks in advance for any feedback. >>> >>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> >> If I understand things correctly, then there is no difference >> between e.g. performance on AC vs on battery, this is just >> automatic switching the profiles when connecting/disconnecting >> the charger, correct? >> >> If I got that correct, there is no reason why userspace could >> not do this itself and implementing this in userspace has the >> advantage that it will work everywhere including on non >> PSC ThinkPads > > I don't think that's right for the PSC Thinkpads. They have dedicated > different tunings for each of the slider positions on AC vs DC. > > So "balanced" on AC will not be the same as "balanced" on DC. I see, but it is not like balanced on AC is closer to performance on DC then it is to balanced on DC, right? IOW in the UI we should still call them both balanced ? If that is right then I think my point still stands, if PSC has 2 separate slots (one AC one DC) for the performance setting, then we can just set both when userspace selects a performance level and have the actual e.g. balanced -> performance change be done by userspace when userspace select the machine has been connected to a charger. Regards, Hans > >> >> The hardest part of implementing something like this would >> be the userspace UI design and any policy decisions surrounding >> this, if we spend time on implementing those then making userspace >> do the actual switching of the profiles is pretty trivial in >> comparison and as said would be a much more universal way >> to implement this. >> >> So IMHO even though some hardware may offer this functionality, >> this is best left to an universal userspace implementation. >> >> Regards, >> >> Hans >> >> >>> --- >>> drivers/acpi/platform_profile.c | 130 +++++++++++++++++++++++++++++-- >>> include/linux/platform_profile.h | 1 + >>> 2 files changed, 125 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >>> index d418462ab791..e4246e6632cf 100644 >>> --- a/drivers/acpi/platform_profile.c >>> +++ b/drivers/acpi/platform_profile.c >>> @@ -7,6 +7,7 @@ >>> #include <linux/init.h> >>> #include <linux/mutex.h> >>> #include <linux/platform_profile.h> >>> +#include <linux/power_supply.h> >>> #include <linux/sysfs.h> >>> >>> static struct platform_profile_handler *cur_profile; >>> @@ -22,6 +23,51 @@ static const char * const profile_names[] = { >>> }; >>> static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); >>> >>> +static struct notifier_block ac_nb; >>> +static int cur_profile_ac; >>> +static int cur_profile_dc; >>> + >>> +static int platform_profile_set(void) >>> +{ >>> + int profile, err; >>> + >>> + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) >>> + profile = cur_profile_ac; >>> + else { >>> + if (power_supply_is_system_supplied() > 0) >>> + profile = cur_profile_ac; >>> + else >>> + profile = cur_profile_dc; >>> + } >>> + >>> + err = mutex_lock_interruptible(&profile_lock); >>> + if (err) >>> + return err; >>> + >>> + err = cur_profile->profile_set(cur_profile, profile); >>> + if (err) >>> + return err; >>> + >>> + sysfs_notify(acpi_kobj, NULL, "platform_profile"); >>> + mutex_unlock(&profile_lock); >>> + return 0; >>> +} >>> + >>> +static int platform_profile_acpi_event(struct notifier_block *nb, >>> + unsigned long val, >>> + void *data) >>> +{ >>> + struct acpi_bus_event *entry = (struct acpi_bus_event *)data; >>> + >>> + WARN_ON(cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED); >>> + >>> + /* if power supply changed, then update profile */ >>> + if (strcmp(entry->device_class, "ac_adapter") == 0) >>> + return platform_profile_set(); >>> + >>> + return 0; >>> +} >>> + >>> static ssize_t platform_profile_choices_show(struct device *dev, >>> struct device_attribute *attr, >>> char *buf) >>> @@ -77,9 +123,34 @@ static ssize_t platform_profile_show(struct device >> *dev, >>> return sysfs_emit(buf, "%s\n", profile_names[profile]); >>> } >>> >>> -static ssize_t platform_profile_store(struct device *dev, >>> +static ssize_t configured_profile_show(struct device *dev, >>> struct device_attribute *attr, >>> - const char *buf, size_t count) >>> + char *buf, int profile) >>> +{ >>> + if (profile == PLATFORM_PROFILE_UNCONFIGURED) >>> + return sysfs_emit(buf, "Not-configured\n"); >>> + >>> + /* Check that profile is valid index */ >>> + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) >>> + return -EIO; >>> + return sysfs_emit(buf, "%s\n", profile_names[profile]); >>> +} >>> + >>> +static ssize_t platform_profile_ac_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + return configured_profile_show(dev, attr, buf, cur_profile_ac); >>> +} >>> + >>> +static ssize_t platform_profile_dc_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + return configured_profile_show(dev, attr, buf, cur_profile_dc); >>> +} >>> + >>> +static int profile_select(const char *buf) >>> { >>> int err, i; >>> >>> @@ -105,11 +176,50 @@ static ssize_t platform_profile_store(struct device >> *dev, >>> return -EOPNOTSUPP; >>> } >>> >>> - err = cur_profile->profile_set(cur_profile, i); >>> - if (!err) >>> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >>> - >>> mutex_unlock(&profile_lock); >>> + return i; >>> +} >>> + >>> +static ssize_t platform_profile_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + int profile, err; >>> + >>> + profile = profile_select(buf); >>> + if (profile < 0) >>> + return profile; >>> + >>> + cur_profile_ac = profile; >>> + err = platform_profile_set(); >>> + if (err) >>> + return err; >>> + return count; >>> +} >>> + >>> +static ssize_t platform_profile_ac_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + return platform_profile_store(dev, attr, buf, count); >>> +} >>> + >>> +static ssize_t platform_profile_dc_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + int profile, err; >>> + >>> + profile = profile_select(buf); >>> + if (profile < 0) >>> + return profile; >>> + >>> + /* We need to register for ACPI events now */ >>> + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) >>> + register_acpi_notifier(&ac_nb); >>> + >>> + cur_profile_dc = profile; >>> + err = platform_profile_set(); >>> if (err) >>> return err; >>> return count; >>> @@ -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); >>> >>> static struct attribute *platform_profile_attrs[] = { >>> &dev_attr_platform_profile_choices.attr, >>> &dev_attr_platform_profile.attr, >>> + &dev_attr_platform_profile_ac.attr, >>> + &dev_attr_platform_profile_dc.attr, >>> NULL >>> }; >>> >>> @@ -161,7 +275,9 @@ int platform_profile_register(struct >> platform_profile_handler *pprof) >>> } >>> >>> cur_profile = pprof; >>> + cur_profile_ac = cur_profile_dc = >> PLATFORM_PROFILE_UNCONFIGURED; >>> mutex_unlock(&profile_lock); >>> + ac_nb.notifier_call = platform_profile_acpi_event; >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(platform_profile_register); >>> @@ -169,6 +285,8 @@ EXPORT_SYMBOL_GPL(platform_profile_register); >>> int platform_profile_remove(void) >>> { >>> sysfs_remove_group(acpi_kobj, &platform_profile_group); >>> + if (cur_profile_dc != PLATFORM_PROFILE_UNCONFIGURED) >>> + unregister_acpi_notifier(&ac_nb); >>> >>> mutex_lock(&profile_lock); >>> cur_profile = NULL; >>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h >>> index e5cbb6841f3a..34566256bb60 100644 >>> --- a/include/linux/platform_profile.h >>> +++ b/include/linux/platform_profile.h >>> @@ -15,6 +15,7 @@ >>> * If more options are added please update profile_names array in >>> * platform_profile.c and sysfs-platform_profile documentation. >>> */ >>> +#define PLATFORM_PROFILE_UNCONFIGURED -1 >>> >>> enum platform_profile_option { >>> PLATFORM_PROFILE_LOW_POWER,
[AMD Official Use Only] > > I don't think that's right for the PSC Thinkpads. They have dedicated > > different tunings for each of the slider positions on AC vs DC. > > > > So "balanced" on AC will not be the same as "balanced" on DC. > > I see, but it is not like balanced on AC is closer to performance > on DC then it is to balanced on DC, right? IOW in the UI we should > still call them both balanced ? I feel that's a gross oversimplification to say balanced on AC is close to performance on DC. There are *so many* other (otherwise invisible) tuning knobs behind what PSC does that Lenovo has weighed out the benefits of using for different circumstances. You nerf all this by just having one user space facing knob and let userspace change to performance mode when you on charger. At least the way Windows does this is that it offers "one" UI slider but you have last selected values based on if you're plugged in or on battery. 1) So on battery I might have balanced selected to start out. 2) Then I plug in a charger, and balanced is still selected but this has different characteristics from balanced on battery. 3) Now I change to performance while on charger. 4) Then I unplug charger and it goes back to my selection for battery: "balanced". > > If that is right then I think my point still stands, if PSC > has 2 separate slots (one AC one DC) for the performance > setting, then we can just set both when userspace selects a > performance level and have the actual e.g. balanced -> performance > change be done by userspace when userspace select the machine > has been connected to a charger. But you *don't want to* actually select performance when you switch to a charger. If you have 3 value slots for AC and 3 value slots for DC you should only be swapping between what is in those 3 values slots. > > Regards, > > Hans > >
Hi, On 3/8/22 16:55, Limonciello, Mario wrote: > [AMD Official Use Only] > >>> I don't think that's right for the PSC Thinkpads. They have dedicated >>> different tunings for each of the slider positions on AC vs DC. >>> >>> So "balanced" on AC will not be the same as "balanced" on DC. >> >> I see, but it is not like balanced on AC is closer to performance >> on DC then it is to balanced on DC, right? IOW in the UI we should >> still call them both balanced ? > > I feel that's a gross oversimplification to say balanced on AC is close > to performance on DC. There are *so many* other (otherwise invisible) > tuning knobs behind what PSC does that Lenovo has weighed out the benefits > of using for different circumstances. > > You nerf all this by just having one user space facing knob and let userspace > change to performance mode when you on charger. The way I see this there are 2 ways this can work on the kernel to fw/ec boundary: 1. There are actually 6 values we can write to a single slot: ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc-performance 2. There are separate ac-setting + dc-setting slots to which we can write one of 3 values: low-power, balanced, performance; and the fw/ec automatically picks which slot to used based on ac vs battery status If 1 is the case for PSC then I agree that the kernel should indeed get involved and it should automatically write either the ac or dc variant of the last userspace requested value so that things behave as expected. If 2 however is the case then I think all that is necessary is for the driver to just write the last userspace selected value to both slots. Note that neither case requires a userspace API change when solved as I suggest. > At least the way Windows does this is that it offers "one" UI slider but you > have last selected values based on if you're plugged in or on battery. > > 1) So on battery I might have balanced selected to start out. > 2) Then I plug in a charger, and balanced is still selected but this has > different characteristics from balanced on battery. > 3) Now I change to performance while on charger. > 4) Then I unplug charger and it goes back to my selection for battery: "balanced". The above is more about policy then it is about mechanism, userspace can easily remember 2 separate settings for ac vs battery and restore the last set value for ac or battery when changing between the 2. Since this mostly about the policy which profile to set when this really belongs in userspace IMHO and solving this in userspace means that we will have a single universal solution for all the different platform_profile implementations, and we seem to have quite a lot of those (at least one per laptop vendor, Lenovo currently has 2) >> If that is right then I think my point still stands, if PSC >> has 2 separate slots (one AC one DC) for the performance >> setting, then we can just set both when userspace selects a >> performance level and have the actual e.g. balanced -> performance >> change be done by userspace when userspace select the machine >> has been connected to a charger. > > But you *don't want to* actually select performance when you switch to a > charger. If you have 3 value slots for AC and 3 value slots for DC you > should only be swapping between what is in those 3 values slots. That only works if all implementation have separate AC and DC profile slots, which most won't have. If we just sync the 2 slots for implementations which do have 2 slots and then always "fake" 2 slots in userspace we have a universal implementation which will work well everywhere, without any significant downside to the implementations which do have 2 slots. Regards, Hans
Thanks Mario & Hans On 2022-03-08 11:10, Hans de Goede wrote: > Hi, > > On 3/8/22 16:55, Limonciello, Mario wrote: >> [AMD Official Use Only] >> >>>> I don't think that's right for the PSC Thinkpads. They have dedicated >>>> different tunings for each of the slider positions on AC vs DC. >>>> >>>> So "balanced" on AC will not be the same as "balanced" on DC. >>> >>> I see, but it is not like balanced on AC is closer to performance >>> on DC then it is to balanced on DC, right? IOW in the UI we should >>> still call them both balanced ? >> >> I feel that's a gross oversimplification to say balanced on AC is close >> to performance on DC. There are *so many* other (otherwise invisible) >> tuning knobs behind what PSC does that Lenovo has weighed out the benefits >> of using for different circumstances. >> >> You nerf all this by just having one user space facing knob and let userspace >> change to performance mode when you on charger. > > The way I see this there are 2 ways this can work on the kernel to fw/ec > boundary: > > 1. There are actually 6 values we can write to a single slot: > ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc-performance > > 2. There are separate ac-setting + dc-setting slots to which we can > write one of 3 values: low-power, balanced, performance; and the fw/ec > automatically picks which slot to used based on ac vs battery status > > If 1 is the case for PSC then I agree that the kernel should indeed get involved > and it should automatically write either the ac or dc variant of the last > userspace requested value so that things behave as expected. > > If 2 however is the case then I think all that is necessary is for the > driver to just write the last userspace selected value to both slots. > > Note that neither case requires a userspace API change when solved > as I suggest. I cycled through a few different implementations but came down on what I proposed. I considered 6 values - but I don't think that makes sense and makes it overall more complicated than it needs to be and less flexible. The biggest use case I can think of is that a user wants performance when plugged in and power-save when unplugged; and they want that to happen automatically. This patch let's that happen for any platform - regardless of if it has separate support. If a vendor wants to handle plugged in vs battery to a more nuanced degree they can, but that's at the individual driver level. I originally thought that maybe this should be done in user space but: 1) It takes a lot longer for user space changes to get rolled out and make it into distros. 2) Not all users will get to use it - sure Gnome and KDE might get the feature but chances of other desktops picking it up are small. I could look at releasing a utility to do it but....urgh. Nobody gets a good experience that way. Linux packaging is a minefield. 3) The power events happen in the kernel which is perfect. Once I figured that out it seemed a no-brainer to me. I think user space should add the ability to have a nice GUI to toggle a unplugged profile setting. But the guts of it seem to me to belong better in the kernel. > >> At least the way Windows does this is that it offers "one" UI slider but you >> have last selected values based on if you're plugged in or on battery. >> >> 1) So on battery I might have balanced selected to start out. >> 2) Then I plug in a charger, and balanced is still selected but this has >> different characteristics from balanced on battery. >> 3) Now I change to performance while on charger. >> 4) Then I unplug charger and it goes back to my selection for battery: "balanced". > > The above is more about policy then it is about mechanism, userspace > can easily remember 2 separate settings for ac vs battery and restore > the last set value for ac or battery when changing between the 2. > > Since this mostly about the policy which profile to set when this > really belongs in userspace IMHO and solving this in userspace means that > we will have a single universal solution for all the different > platform_profile implementations, and we seem to have quite a lot of > those (at least one per laptop vendor, Lenovo currently has 2) I disagree here. This is more universal by design. I was surprised at how many vendors are using platform-profiles (I think it's awesome!) but now they can all get this too. The intention here is very strongly not supposed to be Lenovo specific. The follow on patch that I could do in thinkpad_acpi to use a different setting in unplugged/plugged mode - that will be Lenovo specific and taking advantage of the functionality the Lenovo FW is offering. That doesn't seem unreasonable to me. >>> If that is right then I think my point still stands, if PSC >>> has 2 separate slots (one AC one DC) for the performance >>> setting, then we can just set both when userspace selects a >>> performance level and have the actual e.g. balanced -> performance >>> change be done by userspace when userspace select the machine >>> has been connected to a charger. >> >> But you *don't want to* actually select performance when you switch to a >> charger. If you have 3 value slots for AC and 3 value slots for DC you >> should only be swapping between what is in those 3 values slots. > > That only works if all implementation have separate AC and DC profile > slots, which most won't have. If we just sync the 2 slots for implementations > which do have 2 slots and then always "fake" 2 slots in userspace we > have a universal implementation which will work well everywhere, without > any significant downside to the implementations which do have 2 slots. > I'm missing something in this bit. If a vendor is providing platform profiles all we're doing is letting a user choose the profile they want depending on their power situation. I don't think there are empty slots particularly. I've got a feeling I'm missing something obvious - but my experience of user space is it's really hard to get a consistent experience for all Linux users reliably - everybody is running something different. If nothing else I think that should be a big factor for adding this support to the kernel. Obviously if this feature isn't wanted I'll drop it - but I think it's something useful that users will appreciate on any HW. Mark
Hi Mark, On 3/8/22 18:44, Mark Pearson wrote: > > Thanks Mario & Hans > > On 2022-03-08 11:10, Hans de Goede wrote: >> Hi, >> >> On 3/8/22 16:55, Limonciello, Mario wrote: >>> [AMD Official Use Only] >>> >>>>> I don't think that's right for the PSC Thinkpads. They have dedicated >>>>> different tunings for each of the slider positions on AC vs DC. >>>>> >>>>> So "balanced" on AC will not be the same as "balanced" on DC. >>>> >>>> I see, but it is not like balanced on AC is closer to performance >>>> on DC then it is to balanced on DC, right? IOW in the UI we should >>>> still call them both balanced ? >>> >>> I feel that's a gross oversimplification to say balanced on AC is close >>> to performance on DC. There are *so many* other (otherwise invisible) >>> tuning knobs behind what PSC does that Lenovo has weighed out the benefits >>> of using for different circumstances. >>> >>> You nerf all this by just having one user space facing knob and let userspace >>> change to performance mode when you on charger. >> >> The way I see this there are 2 ways this can work on the kernel to fw/ec >> boundary: >> >> 1. There are actually 6 values we can write to a single slot: >> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc-performance >> >> 2. There are separate ac-setting + dc-setting slots to which we can >> write one of 3 values: low-power, balanced, performance; and the fw/ec >> automatically picks which slot to used based on ac vs battery status >> >> If 1 is the case for PSC then I agree that the kernel should indeed get involved >> and it should automatically write either the ac or dc variant of the last >> userspace requested value so that things behave as expected. >> >> If 2 however is the case then I think all that is necessary is for the >> driver to just write the last userspace selected value to both slots. >> >> Note that neither case requires a userspace API change when solved >> as I suggest. > > I cycled through a few different implementations but came down on what I > proposed. I considered 6 values - but I don't think that makes sense and > makes it overall more complicated than it needs to be and less flexible. Ah, so to be clear, my 2 scenarios above were theoretical scenarios, because I'm wondering how the firmware API here actually looks like, something which so far is not really clear to me. When you say that you considered using 6 values, then I guess that the firmware API actually offers 6 values which we can write to a single slot: ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc-performance ? But that is not what the RFC patch that started this thread shows at all, the API to the driver is totally unchanged and does not get passed any info on ac/dc selection ? So it seems to me that the ACPI API Linux uses for this writes only 1 of 3 values to a single slot and the EC automatically switches between say ac-balanced and dc-balanced internally. IOW there really being 2 differently tuned balance-profiles is not visible to the OS at all, this is handled internally inside the EC, correct ? Otherwise I would expect the kernel internal driver API to also change and to also see a matching thinkpad_acpi patch in the RFC series? > The biggest use case I can think of is that a user wants performance > when plugged in and power-save when unplugged; and they want that to > happen automatically. Right, so this what I have understood all along and I'm not disagreeing that this is a desirable feature, but it _does not belong in the kernel_! Also taking Mario's remark about the EC-firmware using differently tuned balanced profiles based on ac vs dc, here is how I envision this working: 1. Laptop is connected to charger 2. EC notices this and: 2.1 Internally switches from balanced-dc settings to balanced-ac settings 2.2 Sends out an event about the laptop now being on AC, which the kernel picks up and then sends to userspace (this already happens) 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is now an AC and in its settings sees that the user wants to switch to performance mode on AC and uses the platform_api in its current form to ask for a switch to performance mode 4. The EC gets a command telling it to switch to performance mode and switches to the ac-tuned version of performance mode since the laptop is on ac. So same end-result, but without introducing new userspace API. Note that even if we were to go with your RFC userspace would still need to be adjusted to use the new uapi, so AFAICT we really win nothing by doing this in the kernel. > This patch let's that happen for any platform - regardless of if it has > separate support. Doing this fully in userspace also allows this to happen for any platform. > If a vendor wants to handle plugged in vs battery to a > more nuanced degree they can, but that's at the individual driver level. > > I originally thought that maybe this should be done in user space but: > > 1) It takes a lot longer for user space changes to get rolled out and > make it into distros. Distros usually are quicker in updating userspace bits which are required for hw-enablement purposes; and for this to be really useful it needs a UI to control it, so it will need userspace support regardless of where we solve it. > 2) Not all users will get to use it - sure Gnome and KDE might get the > feature but chances of other desktops picking it up are small. I could > look at releasing a utility to do it but....urgh. Nobody gets a good > experience that way. Linux packaging is a minefield. Many many hw-enablement things, like thunderbolt authentication, proper automatic switching of audio streams, proper handling of various hotkeys like vol / brightness up/down, privacy screen on/off toggling, easy use of VPNs and wwan modems, are all not or very poorly supported in other desktop-environments. I often get bug reports from users using other DEs about things like this and there really is nothing which we can do about this. In some cases like i3 window-manager the norm is actually for users to have to write custom scripts for everything. Other DEs do try, but simply lack the manpower, this is an unfortunate situation, but not really a good argument for just shoving everything in the kernel. > 3) The power events happen in the kernel which is perfect. Once I > figured that out it seemed a no-brainer to me. That same event also gets forwarded to userspace over the power_supply class uapi. > I think user space should add the ability to have a nice GUI to toggle a > unplugged profile setting. But the guts of it seem to me to belong > better in the kernel. There is nothing which the kernel can do here, which userspace cannot also do; and generally speaking in cases like this userspace can do it better, because it can add a lot more fancy policy like (random example): switch to performance mode when on AC and the battery is fully charged. There will be many different usecases here and hardcoding a single simple but also dumb policy in the kernel really is not doing anyone any favors. >>> At least the way Windows does this is that it offers "one" UI slider but you >>> have last selected values based on if you're plugged in or on battery. >>> >>> 1) So on battery I might have balanced selected to start out. >>> 2) Then I plug in a charger, and balanced is still selected but this has >>> different characteristics from balanced on battery. >>> 3) Now I change to performance while on charger. >>> 4) Then I unplug charger and it goes back to my selection for battery: "balanced". >> >> The above is more about policy then it is about mechanism, userspace >> can easily remember 2 separate settings for ac vs battery and restore >> the last set value for ac or battery when changing between the 2. >> >> Since this mostly about the policy which profile to set when this >> really belongs in userspace IMHO and solving this in userspace means that >> we will have a single universal solution for all the different >> platform_profile implementations, and we seem to have quite a lot of >> those (at least one per laptop vendor, Lenovo currently has 2) > > I disagree here. This is more universal by design. I was surprised at > how many vendors are using platform-profiles (I think it's awesome!) but > now they can all get this too. The intention here is very strongly not > supposed to be Lenovo specific. A userspace solution, at least at the power-profile-daemon level will be just as universal. And as for unsupported DEs, whether users have to manually poke a sysfs file, or have to manually make a dbus call (which can also be done from the shell) really does not make much of a difference. > The follow on patch that I could do in thinkpad_acpi to use a different > setting in unplugged/plugged mode - that will be Lenovo specific and > taking advantage of the functionality the Lenovo FW is offering. That > doesn't seem unreasonable to me. > >>>> If that is right then I think my point still stands, if PSC >>>> has 2 separate slots (one AC one DC) for the performance >>>> setting, then we can just set both when userspace selects a >>>> performance level and have the actual e.g. balanced -> performance >>>> change be done by userspace when userspace select the machine >>>> has been connected to a charger. >>> >>> But you *don't want to* actually select performance when you switch to a >>> charger. If you have 3 value slots for AC and 3 value slots for DC you >>> should only be swapping between what is in those 3 values slots. >> >> That only works if all implementation have separate AC and DC profile >> slots, which most won't have. If we just sync the 2 slots for implementations >> which do have 2 slots and then always "fake" 2 slots in userspace we >> have a universal implementation which will work well everywhere, without >> any significant downside to the implementations which do have 2 slots. >> > I'm missing something in this bit. If a vendor is providing platform > profiles all we're doing is letting a user choose the profile they want > depending on their power situation. I don't think there are empty slots > particularly. > > I've got a feeling I'm missing something obvious - but my experience of > user space is it's really hard to get a consistent experience for all > Linux users reliably - everybody is running something different. > If nothing else I think that should be a big factor for adding this > support to the kernel. See above, userspace not doing its job properly is really not a good argument to put something in the kernel. The kernel generally is about resource management/sharing and hardware abstraction, in general where possible the kernel should provide mechanism with no, or a very minimal policy and leave the rest up to userspace. This feels like just shoving something into the kernel just because that is convenient, not because it really belongs in the kernel, but we want the kernel to be as small as possible (it really already is much too big) because: 1. Any kernel bugs are fatal if they get triggered, unlike hitting a bug in userdspace code, hitting a bug in kernelspace code will almost always take the entire system down. 2. Kernel memory is not swapable > Obviously if this feature isn't wanted I'll drop it - but I think it's > something useful that users will appreciate on any HW. Above you are talking about changes to the thinkpad_acpi driver to possibly use some sort of "hardware" support for ac/dc profile switching, that is something which might be sensible to support in the kernel, although given that that is Lenovo specific a generic userspace solution to offer the same functionality by having p-p-d switch profiles on ac/dc changes seems like it would work just as well, avoiding the need for anything lenovo specific . Regards, Hans
[Public] > > > > I cycled through a few different implementations but came down on what I > > proposed. I considered 6 values - but I don't think that makes sense and > > makes it overall more complicated than it needs to be and less flexible. > > Ah, so to be clear, my 2 scenarios above were theoretical scenarios, > because I'm wondering how the firmware API here actually looks like, > something which so far is not really clear to me. > > When you say that you considered using 6 values, then I guess that > the firmware API actually offers 6 values which we can write to a single slot: > ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- > performance > > ? > > But that is not what the RFC patch that started this thread shows at all, > the API to the driver is totally unchanged and does not get passed > any info on ac/dc selection ? So it seems to me that the ACPI API Linux > uses for this writes only 1 of 3 values to a single slot and the EC automatically > switches between say ac-balanced and dc-balanced internally. > > IOW there really being 2 differently tuned balance-profiles is not visible to > the OS at all, this is handled internally inside the EC, correct ? > No - on Lenovo's platform there are 6 different profiles that can be selected from the kernel driver. 3 are intended for use on battery, 3 are intended for use on AC. > Otherwise I would expect the kernel internal driver API to also change and > to also see a matching thinkpad_acpi patch in the RFC series? The idea I see from Mark's thread was to send out RFC change for the platform profile and based on the direction try to implement the thinkpad-acpi change after that. Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi modeled on top of this the way that you want. > > > The biggest use case I can think of is that a user wants performance > > when plugged in and power-save when unplugged; and they want that to > > happen automatically. > > Right, so this what I have understood all along and I'm not disagreeing > that this is a desirable feature, but it _does not belong in the kernel_! > > Also taking Mario's remark about the EC-firmware using differently > tuned balanced profiles based on ac vs dc, here is how I envision this > working: > > 1. Laptop is connected to charger > 2. EC notices this and: > 2.1 Internally switches from balanced-dc settings to balanced-ac settings > 2.2 Sends out an event about the laptop now being on AC, which the kernel > picks up and then sends to userspace (this already happens) > 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is > now an AC and in its settings sees that the user wants to switch to > performance mode on AC and uses the platform_api in its current form to > ask for a switch to performance mode > 4. The EC gets a command telling it to switch to performance mode and > switches to the ac-tuned version of performance mode since the laptop is > on ac. > None of this happens internally on the EC. Also there is nothing in this design that guarantees it needs to be EC driven profile changes. It could be other mailboxes, ASL code, SMM etc. The key point here is that thinkpad acpi has 3 AC and 3 DC profiles to choose from, so some level from thinkpad acpi above needs to pick among them. > So same end-result, but without introducing new userspace API. Note that > even if we were to go with your RFC userspace would still need to be > adjusted to use the new uapi, so AFAICT we really win nothing by doing this > in the kernel. > > > This patch let's that happen for any platform - regardless of if it has > > separate support. > > Doing this fully in userspace also allows this to happen for any platform. > > > If a vendor wants to handle plugged in vs battery to a > > more nuanced degree they can, but that's at the individual driver level. > > > > I originally thought that maybe this should be done in user space but: > > > > 1) It takes a lot longer for user space changes to get rolled out and > > make it into distros. > > Distros usually are quicker in updating userspace bits which are required > for hw-enablement purposes; and for this to be really useful it > needs a UI to control it, so it will need userspace support regardless > of where we solve it. > > > 2) Not all users will get to use it - sure Gnome and KDE might get the > > feature but chances of other desktops picking it up are small. I could > > look at releasing a utility to do it but....urgh. Nobody gets a good > > experience that way. Linux packaging is a minefield. > > Many many hw-enablement things, like thunderbolt authentication, proper > automatic switching of audio streams, proper handling of various hotkeys > like vol / brightness up/down, privacy screen on/off toggling, easy use > of VPNs and wwan modems, are all not or very poorly supported in other > desktop-environments. I often get bug reports from users using other > DEs about things like this and there really is nothing which we can > do about this. In some cases like i3 window-manager the norm > is actually for users to have to write custom scripts for everything. > > Other DEs do try, but simply lack the manpower, this is an unfortunate > situation, but not really a good argument for just shoving everything > in the kernel. > > > 3) The power events happen in the kernel which is perfect. Once I > > figured that out it seemed a no-brainer to me. > > That same event also gets forwarded to userspace over the power_supply > class uapi. > > I think user space should add the ability to have a nice GUI to toggle a > > unplugged profile setting. But the guts of it seem to me to belong > > better in the kernel. > > There is nothing which the kernel can do here, which userspace cannot > also do; and generally speaking in cases like this userspace can do it > better, because it can add a lot more fancy policy like (random example): > switch to performance mode when on AC and the battery is fully charged. > > There will be many different usecases here and hardcoding a single > simple but also dumb policy in the kernel really is not doing anyone > any favors. > > >>> At least the way Windows does this is that it offers "one" UI slider but > you > >>> have last selected values based on if you're plugged in or on battery. > >>> > >>> 1) So on battery I might have balanced selected to start out. > >>> 2) Then I plug in a charger, and balanced is still selected but this has > >>> different characteristics from balanced on battery. > >>> 3) Now I change to performance while on charger. > >>> 4) Then I unplug charger and it goes back to my selection for battery: > "balanced". > >> > >> The above is more about policy then it is about mechanism, userspace > >> can easily remember 2 separate settings for ac vs battery and restore > >> the last set value for ac or battery when changing between the 2. > >> > >> Since this mostly about the policy which profile to set when this > >> really belongs in userspace IMHO and solving this in userspace means that > >> we will have a single universal solution for all the different > >> platform_profile implementations, and we seem to have quite a lot of > >> those (at least one per laptop vendor, Lenovo currently has 2) > > > > I disagree here. This is more universal by design. I was surprised at > > how many vendors are using platform-profiles (I think it's awesome!) but > > now they can all get this too. The intention here is very strongly not > > supposed to be Lenovo specific. > > A userspace solution, at least at the power-profile-daemon level will > be just as universal. And as for unsupported DEs, whether users have > to manually poke a sysfs file, or have to manually make a dbus call > (which can also be done from the shell) really does not make much > of a difference. > > > The follow on patch that I could do in thinkpad_acpi to use a different > > setting in unplugged/plugged mode - that will be Lenovo specific and > > taking advantage of the functionality the Lenovo FW is offering. That > > doesn't seem unreasonable to me. > > > >>>> If that is right then I think my point still stands, if PSC > >>>> has 2 separate slots (one AC one DC) for the performance > >>>> setting, then we can just set both when userspace selects a > >>>> performance level and have the actual e.g. balanced -> performance > >>>> change be done by userspace when userspace select the machine > >>>> has been connected to a charger. > >>> > >>> But you *don't want to* actually select performance when you switch to > a > >>> charger. If you have 3 value slots for AC and 3 value slots for DC you > >>> should only be swapping between what is in those 3 values slots. > >> > >> That only works if all implementation have separate AC and DC profile > >> slots, which most won't have. If we just sync the 2 slots for > implementations > >> which do have 2 slots and then always "fake" 2 slots in userspace we > >> have a universal implementation which will work well everywhere, > without > >> any significant downside to the implementations which do have 2 slots. > >> > > I'm missing something in this bit. If a vendor is providing platform > > profiles all we're doing is letting a user choose the profile they want > > depending on their power situation. I don't think there are empty slots > > particularly. > > > > I've got a feeling I'm missing something obvious - but my experience of > > user space is it's really hard to get a consistent experience for all > > Linux users reliably - everybody is running something different. > > If nothing else I think that should be a big factor for adding this > > support to the kernel. > > See above, userspace not doing its job properly is really not a good > argument to put something in the kernel. The kernel generally is > about resource management/sharing and hardware abstraction, > in general where possible the kernel should provide mechanism with > no, or a very minimal policy and leave the rest up to userspace. > > This feels like just shoving something into the kernel just because > that is convenient, not because it really belongs in the kernel, > but we want the kernel to be as small as possible (it really already > is much too big) because: > > 1. Any kernel bugs are fatal if they get triggered, unlike hitting > a bug in userdspace code, hitting a bug in kernelspace code will > almost always take the entire system down. > 2. Kernel memory is not swapable > > > Obviously if this feature isn't wanted I'll drop it - but I think it's > > something useful that users will appreciate on any HW. > > Above you are talking about changes to the thinkpad_acpi driver > to possibly use some sort of "hardware" support for ac/dc profile > switching, that is something which might be sensible to support > in the kernel, although given that that is Lenovo specific a > generic userspace solution to offer the same functionality by > having p-p-d switch profiles on ac/dc changes seems like it > would work just as well, avoiding the need for anything lenovo > specific . > > Regards, > > Hans
Hi Mario, On 3/14/22 14:39, Limonciello, Mario wrote: > [Public] > >>> >>> I cycled through a few different implementations but came down on what I >>> proposed. I considered 6 values - but I don't think that makes sense and >>> makes it overall more complicated than it needs to be and less flexible. >> >> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >> because I'm wondering how the firmware API here actually looks like, >> something which so far is not really clear to me. >> >> When you say that you considered using 6 values, then I guess that >> the firmware API actually offers 6 values which we can write to a single slot: >> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >> performance >> >> ? >> >> But that is not what the RFC patch that started this thread shows at all, >> the API to the driver is totally unchanged and does not get passed >> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >> uses for this writes only 1 of 3 values to a single slot and the EC automatically >> switches between say ac-balanced and dc-balanced internally. >> >> IOW there really being 2 differently tuned balance-profiles is not visible to >> the OS at all, this is handled internally inside the EC, correct ? >> > > No - on Lenovo's platform there are 6 different profiles that can be selected > from the kernel driver. 3 are intended for use on battery, 3 are intended for > use on AC. Ah, I already got that feeling from the rest of the thread, so I reread Mark's RFC again before posting my reply today and the RFC looked like the same 3 profiles were being set and the only functionality added was auto profile switching when changing between AC/battery. Thank you for clarifying this. Having 6 different stories indeed is a very different story. >> Otherwise I would expect the kernel internal driver API to also change and >> to also see a matching thinkpad_acpi patch in the RFC series? > > The idea I see from Mark's thread was to send out RFC change for the platform profile > and based on the direction try to implement the thinkpad-acpi change after that. > > Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi > modeled on top of this the way that you want. I fully agree and since you introduce the concept of being on AC/battery to the drivers/acpi/platform_profile.c cpde, please change the profile_set and profile_get function prototypes in struct platform_profile_handler to also take a "bool on_battery" extra argument and use that in the thinkpad driver to select either the ac or the battery tuned low/balanced/performance profile. And please also include an update to Documentation/ABI/testing/sysfs-platform_profile in the next RFC. Also notice how I've tried to consistently use AC/battery in my last reply, DC really is not a good term for "on battery". AC also is sort of dubious for "connected to an external power-supply" but its use for that is sorta common and it is nice and short. Sorry if this seems like bikeshedding, but using DC for "on battery" just feels wrong to me. >>> The biggest use case I can think of is that a user wants performance >>> when plugged in and power-save when unplugged; and they want that to >>> happen automatically. >> >> Right, so this what I have understood all along and I'm not disagreeing >> that this is a desirable feature, but it _does not belong in the kernel_! >> >> Also taking Mario's remark about the EC-firmware using differently >> tuned balanced profiles based on ac vs dc, here is how I envision this >> working: >> >> 1. Laptop is connected to charger >> 2. EC notices this and: >> 2.1 Internally switches from balanced-dc settings to balanced-ac settings >> 2.2 Sends out an event about the laptop now being on AC, which the kernel >> picks up and then sends to userspace (this already happens) >> 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is >> now an AC and in its settings sees that the user wants to switch to >> performance mode on AC and uses the platform_api in its current form to >> ask for a switch to performance mode >> 4. The EC gets a command telling it to switch to performance mode and >> switches to the ac-tuned version of performance mode since the laptop is >> on ac. >> > > None of this happens internally on the EC. Ack, I understand now thank you for clarifying this. > Also there is nothing in this design > that guarantees it needs to be EC driven profile changes. It could be other > mailboxes, ASL code, SMM etc. > > The key point here is that thinkpad acpi has 3 AC and 3 DC profiles to choose from, > so some level from thinkpad acpi above needs to pick among them. Ack. Regards, Hans
Hi Hans & Mario Thanks for all the comments. On 2022-03-14 10:43, Hans de Goede wrote: > Hi Mario, > > On 3/14/22 14:39, Limonciello, Mario wrote: >> [Public] >> >>>> >>>> I cycled through a few different implementations but came down on what I >>>> proposed. I considered 6 values - but I don't think that makes sense and >>>> makes it overall more complicated than it needs to be and less flexible. >>> >>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>> because I'm wondering how the firmware API here actually looks like, >>> something which so far is not really clear to me. >>> >>> When you say that you considered using 6 values, then I guess that >>> the firmware API actually offers 6 values which we can write to a single slot: >>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>> performance >>> >>> ? >>> >>> But that is not what the RFC patch that started this thread shows at all, >>> the API to the driver is totally unchanged and does not get passed >>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>> switches between say ac-balanced and dc-balanced internally. >>> >>> IOW there really being 2 differently tuned balance-profiles is not visible to >>> the OS at all, this is handled internally inside the EC, correct ? >>> >> >> No - on Lenovo's platform there are 6 different profiles that can be selected >> from the kernel driver. 3 are intended for use on battery, 3 are intended for >> use on AC. > > Ah, I already got that feeling from the rest of the thread, so I reread > Mark's RFC again before posting my reply today and the RFC looked like > the same 3 profiles were being set and the only functionality added > was auto profile switching when changing between AC/battery. > > Thank you for clarifying this. Having 6 different stories > indeed is a very different story. Apologies if I wasn't clear. I was trying to come up with a design that took advantage of the AMD platforms have 6 settings, but was extensible generally to other situations. I will redo the patches and add the thinkpad_acpi on top - that will help it be clearer. > >>> Otherwise I would expect the kernel internal driver API to also change and >>> to also see a matching thinkpad_acpi patch in the RFC series? >> >> The idea I see from Mark's thread was to send out RFC change for the platform profile >> and based on the direction try to implement the thinkpad-acpi change after that. >> >> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >> modeled on top of this the way that you want. > > I fully agree and since you introduce the concept of being on AC/battery to the > drivers/acpi/platform_profile.c cpde, please change the > profile_set and profile_get function prototypes in struct platform_profile_handler > to also take a "bool on_battery" extra argument and use that in the thinkpad > driver to select either the ac or the battery tuned low/balanced/performance > profile. OK - I was thinking that, but I also figured the thinkpad driver could get the power status directly so it was largely redundant (and saves churn on all the other platform profile drivers - there are quite a few now) > > And please also include an update to Documentation/ABI/testing/sysfs-platform_profile > in the next RFC. Absolutely - that was intended. My aim with this RFC was to get feedback on if it was acceptable or not, and if the design had to change. Really appreciate all the good points. > > Also notice how I've tried to consistently use AC/battery in my last reply, > DC really is not a good term for "on battery". AC also is sort of dubious > for "connected to an external power-supply" but its use for that is sorta > common and it is nice and short. > > Sorry if this seems like bikeshedding, but using DC for "on battery" just > feels wrong to me. Ack - and I'm good with the suggestion. > > >>>> The biggest use case I can think of is that a user wants performance >>>> when plugged in and power-save when unplugged; and they want that to >>>> happen automatically. >>> >>> Right, so this what I have understood all along and I'm not disagreeing >>> that this is a desirable feature, but it _does not belong in the kernel_! >>> >>> Also taking Mario's remark about the EC-firmware using differently >>> tuned balanced profiles based on ac vs dc, here is how I envision this >>> working: >>> >>> 1. Laptop is connected to charger >>> 2. EC notices this and: >>> 2.1 Internally switches from balanced-dc settings to balanced-ac settings >>> 2.2 Sends out an event about the laptop now being on AC, which the kernel >>> picks up and then sends to userspace (this already happens) >>> 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is >>> now an AC and in its settings sees that the user wants to switch to >>> performance mode on AC and uses the platform_api in its current form to >>> ask for a switch to performance mode >>> 4. The EC gets a command telling it to switch to performance mode and >>> switches to the ac-tuned version of performance mode since the laptop is >>> on ac. >>> >> >> None of this happens internally on the EC. > > Ack, I understand now thank you for clarifying this. Sorry for not being clear here > >> Also there is nothing in this design >> that guarantees it needs to be EC driven profile changes. It could be other >> mailboxes, ASL code, SMM etc. >> >> The key point here is that thinkpad acpi has 3 AC and 3 DC profiles to choose from, >> so some level from thinkpad acpi above needs to pick among them. > > Ack. > I think this is what makes having the design in the kernel more important. I understand the keeping the kernel small, but the thinkpad_acpi driver needs to guarantee it knows it will get the notification. Without that I don't think I can implement the feature reliably An alternative to the implementation is for me to do this in just the thinkpad_acpi driver and just for PSC mode, and that's what I started with when I looked at this (it's quite a nice simple implementation FWIW). But I figured having something that was configurable has benefits, and something that is applicable to all platforms is a nice feature as well. If doing thinkpad_acpi only would be preferred and more acceptable let me know - but I think it's more limiting overall Thanks Mark
Hi, On 3/14/22 15:59, Mark Pearson wrote: > > Hi Hans & Mario > > Thanks for all the comments. > > On 2022-03-14 10:43, Hans de Goede wrote: >> Hi Mario, >> >> On 3/14/22 14:39, Limonciello, Mario wrote: >>> [Public] >>> >>>>> >>>>> I cycled through a few different implementations but came down on what I >>>>> proposed. I considered 6 values - but I don't think that makes sense and >>>>> makes it overall more complicated than it needs to be and less flexible. >>>> >>>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>>> because I'm wondering how the firmware API here actually looks like, >>>> something which so far is not really clear to me. >>>> >>>> When you say that you considered using 6 values, then I guess that >>>> the firmware API actually offers 6 values which we can write to a single slot: >>>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>>> performance >>>> >>>> ? >>>> >>>> But that is not what the RFC patch that started this thread shows at all, >>>> the API to the driver is totally unchanged and does not get passed >>>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>>> switches between say ac-balanced and dc-balanced internally. >>>> >>>> IOW there really being 2 differently tuned balance-profiles is not visible to >>>> the OS at all, this is handled internally inside the EC, correct ? >>>> >>> >>> No - on Lenovo's platform there are 6 different profiles that can be selected >>> from the kernel driver. 3 are intended for use on battery, 3 are intended for >>> use on AC. >> >> Ah, I already got that feeling from the rest of the thread, so I reread >> Mark's RFC again before posting my reply today and the RFC looked like >> the same 3 profiles were being set and the only functionality added >> was auto profile switching when changing between AC/battery. >> >> Thank you for clarifying this. Having 6 different stories >> indeed is a very different story. > > Apologies if I wasn't clear. I was trying to come up with a design that > took advantage of the AMD platforms have 6 settings, but was extensible > generally to other situations. > > I will redo the patches and add the thinkpad_acpi on top - that will > help it be clearer. >> >>>> Otherwise I would expect the kernel internal driver API to also change and >>>> to also see a matching thinkpad_acpi patch in the RFC series? >>> >>> The idea I see from Mark's thread was to send out RFC change for the platform profile >>> and based on the direction try to implement the thinkpad-acpi change after that. >>> >>> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >>> modeled on top of this the way that you want. >> >> I fully agree and since you introduce the concept of being on AC/battery to the >> drivers/acpi/platform_profile.c cpde, please change the >> profile_set and profile_get function prototypes in struct platform_profile_handler >> to also take a "bool on_battery" extra argument and use that in the thinkpad >> driver to select either the ac or the battery tuned low/balanced/performance >> profile. > > OK - I was thinking that, but I also figured the thinkpad driver could > get the power status directly so it was largely redundant (and saves churn > on all the other platform profile drivers - there are quite a few now) If we get the power-status in 2 places things could get out of sync, there could be unexpected race conditions, etc. Better to do just do it in one place and pass the result along. >> And please also include an update to Documentation/ABI/testing/sysfs-platform_profile >> in the next RFC. > Absolutely - that was intended. My aim with this RFC was to get feedback > on if it was acceptable or not, and if the design had to change. Really > appreciate all the good points. > >> >> Also notice how I've tried to consistently use AC/battery in my last reply, >> DC really is not a good term for "on battery". AC also is sort of dubious >> for "connected to an external power-supply" but its use for that is sorta >> common and it is nice and short. >> >> Sorry if this seems like bikeshedding, but using DC for "on battery" just >> feels wrong to me. > Ack - and I'm good with the suggestion. > >> >> >>>>> The biggest use case I can think of is that a user wants performance >>>>> when plugged in and power-save when unplugged; and they want that to >>>>> happen automatically. >>>> >>>> Right, so this what I have understood all along and I'm not disagreeing >>>> that this is a desirable feature, but it _does not belong in the kernel_! >>>> >>>> Also taking Mario's remark about the EC-firmware using differently >>>> tuned balanced profiles based on ac vs dc, here is how I envision this >>>> working: >>>> >>>> 1. Laptop is connected to charger >>>> 2. EC notices this and: >>>> 2.1 Internally switches from balanced-dc settings to balanced-ac settings >>>> 2.2 Sends out an event about the laptop now being on AC, which the kernel >>>> picks up and then sends to userspace (this already happens) >>>> 3. Userspace, e.g. power-profiles-daemon, gets the event that the laptop is >>>> now an AC and in its settings sees that the user wants to switch to >>>> performance mode on AC and uses the platform_api in its current form to >>>> ask for a switch to performance mode >>>> 4. The EC gets a command telling it to switch to performance mode and >>>> switches to the ac-tuned version of performance mode since the laptop is >>>> on ac. >>>> >>> >>> None of this happens internally on the EC. >> >> Ack, I understand now thank you for clarifying this. > Sorry for not being clear here > >> >>> Also there is nothing in this design >>> that guarantees it needs to be EC driven profile changes. It could be other >>> mailboxes, ASL code, SMM etc. >>> >>> The key point here is that thinkpad acpi has 3 AC and 3 DC profiles to choose from, >>> so some level from thinkpad acpi above needs to pick among them. >> >> Ack. >> > I think this is what makes having the design in the kernel more important. > > I understand the keeping the kernel small, but the thinkpad_acpi driver > needs to guarantee it knows it will get the notification. Without that I > don't think I can implement the feature reliably > > An alternative to the implementation is for me to do this in just the > thinkpad_acpi driver and just for PSC mode, and that's what I started > with when I looked at this (it's quite a nice simple implementation FWIW). > But I figured having something that was configurable has benefits, and > something that is applicable to all platforms is a nice feature as well. > > If doing thinkpad_acpi only would be preferred and more acceptable let > me know - but I think it's more limiting overall I believe that given the hardware interface it makes sense to handle this in the kernel; and for other platforms this will also give the option to have 2 separate profiles for ac/battery and have the kernel auto-switch, and they can just ignore the extra bool on_battery parameter to the getters/setters. Note we do still eventually need to get Rafael to weigh in and get his consent on this too. Regards, Hans
Hi, On 3/14/22 15:43, Hans de Goede wrote: > Hi Mario, > > On 3/14/22 14:39, Limonciello, Mario wrote: >> [Public] >> >>>> >>>> I cycled through a few different implementations but came down on what I >>>> proposed. I considered 6 values - but I don't think that makes sense and >>>> makes it overall more complicated than it needs to be and less flexible. >>> >>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>> because I'm wondering how the firmware API here actually looks like, >>> something which so far is not really clear to me. >>> >>> When you say that you considered using 6 values, then I guess that >>> the firmware API actually offers 6 values which we can write to a single slot: >>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>> performance >>> >>> ? >>> >>> But that is not what the RFC patch that started this thread shows at all, >>> the API to the driver is totally unchanged and does not get passed >>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>> switches between say ac-balanced and dc-balanced internally. >>> >>> IOW there really being 2 differently tuned balance-profiles is not visible to >>> the OS at all, this is handled internally inside the EC, correct ? >>> >> >> No - on Lenovo's platform there are 6 different profiles that can be selected >> from the kernel driver. 3 are intended for use on battery, 3 are intended for >> use on AC. > > Ah, I already got that feeling from the rest of the thread, so I reread > Mark's RFC again before posting my reply today and the RFC looked like > the same 3 profiles were being set and the only functionality added > was auto profile switching when changing between AC/battery. > > Thank you for clarifying this. Having 6 different stories > indeed is a very different story. > >>> Otherwise I would expect the kernel internal driver API to also change and >>> to also see a matching thinkpad_acpi patch in the RFC series? >> >> The idea I see from Mark's thread was to send out RFC change for the platform profile >> and based on the direction try to implement the thinkpad-acpi change after that. >> >> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >> modeled on top of this the way that you want. > > I fully agree and since you introduce the concept of being on AC/battery to the > drivers/acpi/platform_profile.c cpde, please change the > profile_set and profile_get function prototypes in struct platform_profile_handler > to also take a "bool on_battery" extra argument and use that in the thinkpad > driver to select either the ac or the battery tuned low/balanced/performance > profile. > > And please also include an update to Documentation/ABI/testing/sysfs-platform_profile > in the next RFC. > > Also notice how I've tried to consistently use AC/battery in my last reply, > DC really is not a good term for "on battery". AC also is sort of dubious > for "connected to an external power-supply" but its use for that is sorta > common and it is nice and short. One last request for the v2 RFC, please also Cc Bastien Nocera, so that he can take a look at the proposed uapi changes from the userspace side of things. Regards, Hans
On 2022-03-14 11:31, Hans de Goede wrote: > Hi, > > On 3/14/22 15:43, Hans de Goede wrote: >> Hi Mario, >> >> On 3/14/22 14:39, Limonciello, Mario wrote: >>> [Public] >>> >>>>> >>>>> I cycled through a few different implementations but came down on what I >>>>> proposed. I considered 6 values - but I don't think that makes sense and >>>>> makes it overall more complicated than it needs to be and less flexible. >>>> >>>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>>> because I'm wondering how the firmware API here actually looks like, >>>> something which so far is not really clear to me. >>>> >>>> When you say that you considered using 6 values, then I guess that >>>> the firmware API actually offers 6 values which we can write to a single slot: >>>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>>> performance >>>> >>>> ? >>>> >>>> But that is not what the RFC patch that started this thread shows at all, >>>> the API to the driver is totally unchanged and does not get passed >>>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>>> switches between say ac-balanced and dc-balanced internally. >>>> >>>> IOW there really being 2 differently tuned balance-profiles is not visible to >>>> the OS at all, this is handled internally inside the EC, correct ? >>>> >>> >>> No - on Lenovo's platform there are 6 different profiles that can be selected >>> from the kernel driver. 3 are intended for use on battery, 3 are intended for >>> use on AC. >> >> Ah, I already got that feeling from the rest of the thread, so I reread >> Mark's RFC again before posting my reply today and the RFC looked like >> the same 3 profiles were being set and the only functionality added >> was auto profile switching when changing between AC/battery. >> >> Thank you for clarifying this. Having 6 different stories >> indeed is a very different story. >> >>>> Otherwise I would expect the kernel internal driver API to also change and >>>> to also see a matching thinkpad_acpi patch in the RFC series? >>> >>> The idea I see from Mark's thread was to send out RFC change for the platform profile >>> and based on the direction try to implement the thinkpad-acpi change after that. >>> >>> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >>> modeled on top of this the way that you want. >> >> I fully agree and since you introduce the concept of being on AC/battery to the >> drivers/acpi/platform_profile.c cpde, please change the >> profile_set and profile_get function prototypes in struct platform_profile_handler >> to also take a "bool on_battery" extra argument and use that in the thinkpad >> driver to select either the ac or the battery tuned low/balanced/performance >> profile. >> >> And please also include an update to Documentation/ABI/testing/sysfs-platform_profile >> in the next RFC. >> >> Also notice how I've tried to consistently use AC/battery in my last reply, >> DC really is not a good term for "on battery". AC also is sort of dubious >> for "connected to an external power-supply" but its use for that is sorta >> common and it is nice and short. > > One last request for the v2 RFC, please also Cc Bastien Nocera, so that > he can take a look at the proposed uapi changes from the userspace side > of things. > Ack - will do. Thanks! Mark
Hi, On 3/14/22 16:32, Mark Pearson wrote: > > > On 2022-03-14 11:31, Hans de Goede wrote: >> Hi, >> >> On 3/14/22 15:43, Hans de Goede wrote: >>> Hi Mario, >>> >>> On 3/14/22 14:39, Limonciello, Mario wrote: >>>> [Public] >>>> >>>>>> >>>>>> I cycled through a few different implementations but came down on what I >>>>>> proposed. I considered 6 values - but I don't think that makes sense and >>>>>> makes it overall more complicated than it needs to be and less flexible. >>>>> >>>>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>>>> because I'm wondering how the firmware API here actually looks like, >>>>> something which so far is not really clear to me. >>>>> >>>>> When you say that you considered using 6 values, then I guess that >>>>> the firmware API actually offers 6 values which we can write to a single slot: >>>>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>>>> performance >>>>> >>>>> ? >>>>> >>>>> But that is not what the RFC patch that started this thread shows at all, >>>>> the API to the driver is totally unchanged and does not get passed >>>>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>>>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>>>> switches between say ac-balanced and dc-balanced internally. >>>>> >>>>> IOW there really being 2 differently tuned balance-profiles is not visible to >>>>> the OS at all, this is handled internally inside the EC, correct ? >>>>> >>>> >>>> No - on Lenovo's platform there are 6 different profiles that can be selected >>>> from the kernel driver. 3 are intended for use on battery, 3 are intended for >>>> use on AC. >>> >>> Ah, I already got that feeling from the rest of the thread, so I reread >>> Mark's RFC again before posting my reply today and the RFC looked like >>> the same 3 profiles were being set and the only functionality added >>> was auto profile switching when changing between AC/battery. >>> >>> Thank you for clarifying this. Having 6 different stories >>> indeed is a very different story. >>> >>>>> Otherwise I would expect the kernel internal driver API to also change and >>>>> to also see a matching thinkpad_acpi patch in the RFC series? >>>> >>>> The idea I see from Mark's thread was to send out RFC change for the platform profile >>>> and based on the direction try to implement the thinkpad-acpi change after that. >>>> >>>> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >>>> modeled on top of this the way that you want. >>> >>> I fully agree and since you introduce the concept of being on AC/battery to the >>> drivers/acpi/platform_profile.c cpde, please change the >>> profile_set and profile_get function prototypes in struct platform_profile_handler >>> to also take a "bool on_battery" extra argument and use that in the thinkpad >>> driver to select either the ac or the battery tuned low/balanced/performance >>> profile. >>> >>> And please also include an update to Documentation/ABI/testing/sysfs-platform_profile >>> in the next RFC. >>> >>> Also notice how I've tried to consistently use AC/battery in my last reply, >>> DC really is not a good term for "on battery". AC also is sort of dubious >>> for "connected to an external power-supply" but its use for that is sorta >>> common and it is nice and short. >> >> One last request for the v2 RFC, please also Cc Bastien Nocera, so that >> he can take a look at the proposed uapi changes from the userspace side >> of things. >> > Ack - will do. So I've been thinking a bit more about this while I was outside for some fresh air. First of all let me say that I do agree that the having in essence 6 different profiles thing needs a kernel solution. What I'm not entirely sure about is if this needs to be something generic, with a new userspace-API as you proposed in the v1 RFC, or if it would be better to just solve this in thinkpad_acpi.c . Now that I've a better grasp of the problem, I'll start a new email thread on this tomorrow with all the various take-holders in the Cc to try and answer that question. It probably is a good idea to wait with doing a v2 of the RFC until we've had that discussion... Regards, Hans
[Public] +Shyam > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Monday, March 14, 2022 11:56 > To: Mark Pearson <markpearson@lenovo.com>; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: rafael@kernel.org; linux-acpi@vger.kernel.org; platform-driver- > x86@vger.kernel.org > Subject: Re: [External] Re: [RFC] ACPI: platform-profile: support for AC vs DC > modes > > Hi, > > On 3/14/22 16:32, Mark Pearson wrote: > > > > > > On 2022-03-14 11:31, Hans de Goede wrote: > >> Hi, > >> > >> On 3/14/22 15:43, Hans de Goede wrote: > >>> Hi Mario, > >>> > >>> On 3/14/22 14:39, Limonciello, Mario wrote: > >>>> [Public] > >>>> > >>>>>> > >>>>>> I cycled through a few different implementations but came down on > what I > >>>>>> proposed. I considered 6 values - but I don't think that makes sense and > >>>>>> makes it overall more complicated than it needs to be and less flexible. > >>>>> > >>>>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, > >>>>> because I'm wondering how the firmware API here actually looks like, > >>>>> something which so far is not really clear to me. > >>>>> > >>>>> When you say that you considered using 6 values, then I guess that > >>>>> the firmware API actually offers 6 values which we can write to a single > slot: > >>>>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac- > performance,dc- > >>>>> performance > >>>>> > >>>>> ? > >>>>> > >>>>> But that is not what the RFC patch that started this thread shows at all, > >>>>> the API to the driver is totally unchanged and does not get passed > >>>>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux > >>>>> uses for this writes only 1 of 3 values to a single slot and the EC > automatically > >>>>> switches between say ac-balanced and dc-balanced internally. > >>>>> > >>>>> IOW there really being 2 differently tuned balance-profiles is not visible to > >>>>> the OS at all, this is handled internally inside the EC, correct ? > >>>>> > >>>> > >>>> No - on Lenovo's platform there are 6 different profiles that can be > selected > >>>> from the kernel driver. 3 are intended for use on battery, 3 are intended > for > >>>> use on AC. > >>> > >>> Ah, I already got that feeling from the rest of the thread, so I reread > >>> Mark's RFC again before posting my reply today and the RFC looked like > >>> the same 3 profiles were being set and the only functionality added > >>> was auto profile switching when changing between AC/battery. > >>> > >>> Thank you for clarifying this. Having 6 different stories > >>> indeed is a very different story. > >>> > >>>>> Otherwise I would expect the kernel internal driver API to also change and > >>>>> to also see a matching thinkpad_acpi patch in the RFC series? > >>>> > >>>> The idea I see from Mark's thread was to send out RFC change for the > platform profile > >>>> and based on the direction try to implement the thinkpad-acpi change after > that. > >>>> > >>>> Because of the confusion @Mark I think you should send out an RFC v2 > with thinkpad acpi > >>>> modeled on top of this the way that you want. > >>> > >>> I fully agree and since you introduce the concept of being on AC/battery to > the > >>> drivers/acpi/platform_profile.c cpde, please change the > >>> profile_set and profile_get function prototypes in struct > platform_profile_handler > >>> to also take a "bool on_battery" extra argument and use that in the > thinkpad > >>> driver to select either the ac or the battery tuned > low/balanced/performance > >>> profile. > >>> > >>> And please also include an update to Documentation/ABI/testing/sysfs- > platform_profile > >>> in the next RFC. > >>> > >>> Also notice how I've tried to consistently use AC/battery in my last reply, > >>> DC really is not a good term for "on battery". AC also is sort of dubious > >>> for "connected to an external power-supply" but its use for that is sorta > >>> common and it is nice and short. > >> > >> One last request for the v2 RFC, please also Cc Bastien Nocera, so that > >> he can take a look at the proposed uapi changes from the userspace side > >> of things. > >> > > Ack - will do. > > So I've been thinking a bit more about this while I was outside for some > fresh air. > > First of all let me say that I do agree that the having in essence 6 > different profiles thing needs a kernel solution. > > What I'm not entirely sure about is if this needs to be something > generic, with a new userspace-API as you proposed in the v1 RFC, > or if it would be better to just solve this in thinkpad_acpi.c . > > Now that I've a better grasp of the problem, I'll start a new email > thread on this tomorrow with all the various take-holders in the Cc > to try and answer that question. I've been viewing Lenovo's implementation as "the first one to do it this way". In the future AMD plans to submit a driver that provides platform profiles for some SOCs. It will provide similar capabilities to what thinkpad_acpi does. I haven't mentioned it thus far because it's still under development and of course this could change or be cancelled. I expect that the decision made around what to do with thinpad_acpi will also have ramifications for what that driver will do too when it's finished and submitted. Please include Shyam (now on CC) in the new discussion thread. His team owns that new solution I mention and this can affect their development direction too. I say this all to mean "thinkpad_acpi might not be the outlier". > > It probably is a good idea to wait with doing a v2 of the RFC until > we've had that discussion... > > Regards, > > Hans
On 2022-03-14 12:56, Hans de Goede wrote: > Hi, > > On 3/14/22 16:32, Mark Pearson wrote: >> >> >> On 2022-03-14 11:31, Hans de Goede wrote: >>> Hi, >>> >>> On 3/14/22 15:43, Hans de Goede wrote: >>>> Hi Mario, >>>> >>>> On 3/14/22 14:39, Limonciello, Mario wrote: >>>>> [Public] >>>>> >>>>>>> >>>>>>> I cycled through a few different implementations but came down on what I >>>>>>> proposed. I considered 6 values - but I don't think that makes sense and >>>>>>> makes it overall more complicated than it needs to be and less flexible. >>>>>> >>>>>> Ah, so to be clear, my 2 scenarios above were theoretical scenarios, >>>>>> because I'm wondering how the firmware API here actually looks like, >>>>>> something which so far is not really clear to me. >>>>>> >>>>>> When you say that you considered using 6 values, then I guess that >>>>>> the firmware API actually offers 6 values which we can write to a single slot: >>>>>> ac-low-power,dc-lowpower,ac-balanced,dc-balanced,ac-performance,dc- >>>>>> performance >>>>>> >>>>>> ? >>>>>> >>>>>> But that is not what the RFC patch that started this thread shows at all, >>>>>> the API to the driver is totally unchanged and does not get passed >>>>>> any info on ac/dc selection ? So it seems to me that the ACPI API Linux >>>>>> uses for this writes only 1 of 3 values to a single slot and the EC automatically >>>>>> switches between say ac-balanced and dc-balanced internally. >>>>>> >>>>>> IOW there really being 2 differently tuned balance-profiles is not visible to >>>>>> the OS at all, this is handled internally inside the EC, correct ? >>>>>> >>>>> >>>>> No - on Lenovo's platform there are 6 different profiles that can be selected >>>>> from the kernel driver. 3 are intended for use on battery, 3 are intended for >>>>> use on AC. >>>> >>>> Ah, I already got that feeling from the rest of the thread, so I reread >>>> Mark's RFC again before posting my reply today and the RFC looked like >>>> the same 3 profiles were being set and the only functionality added >>>> was auto profile switching when changing between AC/battery. >>>> >>>> Thank you for clarifying this. Having 6 different stories >>>> indeed is a very different story. >>>> >>>>>> Otherwise I would expect the kernel internal driver API to also change and >>>>>> to also see a matching thinkpad_acpi patch in the RFC series? >>>>> >>>>> The idea I see from Mark's thread was to send out RFC change for the platform profile >>>>> and based on the direction try to implement the thinkpad-acpi change after that. >>>>> >>>>> Because of the confusion @Mark I think you should send out an RFC v2 with thinkpad acpi >>>>> modeled on top of this the way that you want. >>>> >>>> I fully agree and since you introduce the concept of being on AC/battery to the >>>> drivers/acpi/platform_profile.c cpde, please change the >>>> profile_set and profile_get function prototypes in struct platform_profile_handler >>>> to also take a "bool on_battery" extra argument and use that in the thinkpad >>>> driver to select either the ac or the battery tuned low/balanced/performance >>>> profile. >>>> >>>> And please also include an update to Documentation/ABI/testing/sysfs-platform_profile >>>> in the next RFC. >>>> >>>> Also notice how I've tried to consistently use AC/battery in my last reply, >>>> DC really is not a good term for "on battery". AC also is sort of dubious >>>> for "connected to an external power-supply" but its use for that is sorta >>>> common and it is nice and short. >>> >>> One last request for the v2 RFC, please also Cc Bastien Nocera, so that >>> he can take a look at the proposed uapi changes from the userspace side >>> of things. >>> >> Ack - will do. > > So I've been thinking a bit more about this while I was outside for some > fresh air. > > First of all let me say that I do agree that the having in essence 6 > different profiles thing needs a kernel solution. > > What I'm not entirely sure about is if this needs to be something > generic, with a new userspace-API as you proposed in the v1 RFC, > or if it would be better to just solve this in thinkpad_acpi.c . > > Now that I've a better grasp of the problem, I'll start a new email > thread on this tomorrow with all the various take-holders in the Cc > to try and answer that question. > > It probably is a good idea to wait with doing a v2 of the RFC until > we've had that discussion... > No problem - and thanks! I'll hold off until we have a better idea where we are going. If having some example code is useful though just let me know Mark
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index d418462ab791..e4246e6632cf 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -7,6 +7,7 @@ #include <linux/init.h> #include <linux/mutex.h> #include <linux/platform_profile.h> +#include <linux/power_supply.h> #include <linux/sysfs.h> static struct platform_profile_handler *cur_profile; @@ -22,6 +23,51 @@ static const char * const profile_names[] = { }; static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); +static struct notifier_block ac_nb; +static int cur_profile_ac; +static int cur_profile_dc; + +static int platform_profile_set(void) +{ + int profile, err; + + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) + profile = cur_profile_ac; + else { + if (power_supply_is_system_supplied() > 0) + profile = cur_profile_ac; + else + profile = cur_profile_dc; + } + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + err = cur_profile->profile_set(cur_profile, profile); + if (err) + return err; + + sysfs_notify(acpi_kobj, NULL, "platform_profile"); + mutex_unlock(&profile_lock); + return 0; +} + +static int platform_profile_acpi_event(struct notifier_block *nb, + unsigned long val, + void *data) +{ + struct acpi_bus_event *entry = (struct acpi_bus_event *)data; + + WARN_ON(cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED); + + /* if power supply changed, then update profile */ + if (strcmp(entry->device_class, "ac_adapter") == 0) + return platform_profile_set(); + + return 0; +} + static ssize_t platform_profile_choices_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -77,9 +123,34 @@ static ssize_t platform_profile_show(struct device *dev, return sysfs_emit(buf, "%s\n", profile_names[profile]); } -static ssize_t platform_profile_store(struct device *dev, +static ssize_t configured_profile_show(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) + char *buf, int profile) +{ + if (profile == PLATFORM_PROFILE_UNCONFIGURED) + return sysfs_emit(buf, "Not-configured\n"); + + /* Check that profile is valid index */ + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) + return -EIO; + return sysfs_emit(buf, "%s\n", profile_names[profile]); +} + +static ssize_t platform_profile_ac_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return configured_profile_show(dev, attr, buf, cur_profile_ac); +} + +static ssize_t platform_profile_dc_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return configured_profile_show(dev, attr, buf, cur_profile_dc); +} + +static int profile_select(const char *buf) { int err, i; @@ -105,11 +176,50 @@ static ssize_t platform_profile_store(struct device *dev, return -EOPNOTSUPP; } - err = cur_profile->profile_set(cur_profile, i); - if (!err) - sysfs_notify(acpi_kobj, NULL, "platform_profile"); - mutex_unlock(&profile_lock); + return i; +} + +static ssize_t platform_profile_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int profile, err; + + profile = profile_select(buf); + if (profile < 0) + return profile; + + cur_profile_ac = profile; + err = platform_profile_set(); + if (err) + return err; + return count; +} + +static ssize_t platform_profile_ac_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + return platform_profile_store(dev, attr, buf, count); +} + +static ssize_t platform_profile_dc_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int profile, err; + + profile = profile_select(buf); + if (profile < 0) + return profile; + + /* We need to register for ACPI events now */ + if (cur_profile_dc == PLATFORM_PROFILE_UNCONFIGURED) + register_acpi_notifier(&ac_nb); + + cur_profile_dc = profile; + err = platform_profile_set(); if (err) return err; return count; @@ -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); static struct attribute *platform_profile_attrs[] = { &dev_attr_platform_profile_choices.attr, &dev_attr_platform_profile.attr, + &dev_attr_platform_profile_ac.attr, + &dev_attr_platform_profile_dc.attr, NULL }; @@ -161,7 +275,9 @@ int platform_profile_register(struct platform_profile_handler *pprof) } cur_profile = pprof; + cur_profile_ac = cur_profile_dc = PLATFORM_PROFILE_UNCONFIGURED; mutex_unlock(&profile_lock); + ac_nb.notifier_call = platform_profile_acpi_event; return 0; } EXPORT_SYMBOL_GPL(platform_profile_register); @@ -169,6 +285,8 @@ EXPORT_SYMBOL_GPL(platform_profile_register); int platform_profile_remove(void) { sysfs_remove_group(acpi_kobj, &platform_profile_group); + if (cur_profile_dc != PLATFORM_PROFILE_UNCONFIGURED) + unregister_acpi_notifier(&ac_nb); mutex_lock(&profile_lock); cur_profile = NULL; diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index e5cbb6841f3a..34566256bb60 100644 --- a/include/linux/platform_profile.h +++ b/include/linux/platform_profile.h @@ -15,6 +15,7 @@ * If more options are added please update profile_names array in * platform_profile.c and sysfs-platform_profile documentation. */ +#define PLATFORM_PROFILE_UNCONFIGURED -1 enum platform_profile_option { PLATFORM_PROFILE_LOW_POWER,
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. 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. 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. 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(-)