All of lore.kernel.org
 help / color / mirror / Atom feed
* RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
@ 2020-09-17 11:22 Hans de Goede
  2020-09-17 11:50 ` Bastien Nocera
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 11:22 UTC (permalink / raw)
  To: Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Bastien Nocera, Jared Dominguez,
	platform-driver-x86, Andy Shevchenko

Hi Elia, Mark, et al.

Elia, Mark I'm mailing you both because both of you have pdx86 patches pending to add a vendor
specific sysfs-attribute for selecting performance-profiles for resp. HP and Lenovo Thinkpad laptops.

I think that this shows that we might need to start thinking
about a generic kernel API for this, otherwise we will
end up with slight different options per vendor ...

So it seems we may need something like:

/sys/class/system_performance_profile

Where we would then get e.g.:

/sys/class/system_performance_profile/thinkpad_acpi/performance_profile

And then we need to standardize on the names/values which
performance_profile can show / accept when written too.

The big question is what do we do if there are more then 3 profiles?

One option would be something like the following:

cat /sys/class/system_performance_profile/thinkpad_acpi/performance_profile

low-power [balanced] performance

cat /sys/class/system_performance_profile/thinkpad_acpi/extra_performance_profiles

extra-low-power balanced-performance-mix

So we add an optional extra_performance_profiles sysfs attribute and we ask all
drivers implemeting this class to implement at least the 3 standard profiles
(by mapping 3 of their options to these) and optional they can offer extra
profiles (with free form names) in the extra_performance_profiles
sysfs attribute under the class-device.

The idea behind putting the extra profiles in a separate sysfs-attribute
is that reading the main performance_profile attribute will always show
one selected, even if one of the extra profiles is actually in use,
then the driver should also show the closest standardized profile as
being active.

This will allow userspace code to always rely on the standard interface
both for getting a representation of the currently active profile as well
as for setting the active profile.

Elia, Mark, I assume that both of you want to get your patches for this
upstream sooner, rather then later. But I think we should put them on
hold until we have an agreement on a shared userspace API for this.

I would like to think that the above proposal is a good start,
if we can quickly (*) decide on an userspace API here

Regards,

Hans

p.s.

I guess we should also add an optional lap_mode sysfs attribute
to the class-device, to have all the info for the Thinkpads in
one place.


*) but not too quickly, it is important we get this right


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 11:22 RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles Hans de Goede
@ 2020-09-17 11:50 ` Bastien Nocera
  2020-09-17 12:51   ` Hans de Goede
  2020-09-17 13:36   ` Mark Pearson
  2020-09-17 12:22 ` Benjamin Berg
  2020-09-17 13:02 ` Barnabás Pőcze
  2 siblings, 2 replies; 30+ messages in thread
From: Bastien Nocera @ 2020-09-17 11:50 UTC (permalink / raw)
  To: Hans de Goede, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hey,

On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
> Hi Elia, Mark, et al.
> 
> Elia, Mark I'm mailing you both because both of you have pdx86
> patches pending to add a vendor
> specific sysfs-attribute for selecting performance-profiles for resp.
> HP and Lenovo Thinkpad laptops.
> 
> I think that this shows that we might need to start thinking
> about a generic kernel API for this, otherwise we will
> end up with slight different options per vendor ...

Some comments below based on possible use in power-profiles-daemon:
https://www.hadess.net/2020/09/power-profiles-daemon-new-project.html


> So it seems we may need something like:
> 
> /sys/class/system_performance_profile
> 
> Where we would then get e.g.:
> 
> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
> le
> 
> And then we need to standardize on the names/values which
> performance_profile can show / accept when written too.
> 
> The big question is what do we do if there are more then 3 profiles?

The Intel P-State driver in the kernel supports 4 separate ones (plus
default):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/intel_pstate.c#n591
which we crammed into 3 profiles:
https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/blob/master/src/ppd-driver-intel-pstate.c#L209-226

> One option would be something like the following:
> 
> cat
> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
> le
> 
> low-power [balanced] performance

Are the square brackets to show the currently selected profile? I'd
rather it was a separate sysfs attribute. I would expect to only ever
read the list of supported profiles once, and then monitor an "active
profile" attribute.

(a bit like the intel_pstate kernel driver does, but then all the
devices that support Intel P-State support all the profiles, so it's
not a good example ;)

> cat
> /sys/class/system_performance_profile/thinkpad_acpi/extra_performance
> _profiles
> 
> extra-low-power balanced-performance-mix
> 
> So we add an optional extra_performance_profiles sysfs attribute and
> we ask all
> drivers implemeting this class to implement at least the 3 standard
> profiles
> (by mapping 3 of their options to these) and optional they can offer
> extra
> profiles (with free form names) in the extra_performance_profiles
> sysfs attribute under the class-device.
> 
> The idea behind putting the extra profiles in a separate sysfs-
> attribute
> is that reading the main performance_profile attribute will always
> show
> one selected, even if one of the extra profiles is actually in use,
> then the driver should also show the closest standardized profile as
> being active.

I think it's fine having more than 3 profiles. Something like power-
profiles-daemon would likely trying to match them all to one of the 3
profiles it uses as an interface, or forcing the use of those 3
profiles, depending on what that profile behaves.

> This will allow userspace code to always rely on the standard
> interface
> both for getting a representation of the currently active profile as
> well
> as for setting the active profile.
> 
> Elia, Mark, I assume that both of you want to get your patches for
> this
> upstream sooner, rather then later. But I think we should put them on
> hold until we have an agreement on a shared userspace API for this.

Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
something to advertise the unavailability of a profile, and the reason
for that unavailability.

Cheers

> 
> I would like to think that the above proposal is a good start,
> if we can quickly (*) decide on an userspace API here
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> I guess we should also add an optional lap_mode sysfs attribute
> to the class-device, to have all the info for the Thinkpads in
> one place.
> 
> 
> *) but not too quickly, it is important we get this right
> 



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 11:22 RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles Hans de Goede
  2020-09-17 11:50 ` Bastien Nocera
@ 2020-09-17 12:22 ` Benjamin Berg
  2020-09-17 12:45   ` Hans de Goede
  2020-09-17 13:02 ` Barnabás Pőcze
  2 siblings, 1 reply; 30+ messages in thread
From: Benjamin Berg @ 2020-09-17 12:22 UTC (permalink / raw)
  To: Hans de Goede, Elia Devito, Mark Pearson
  Cc: Bastien Nocera, Jared Dominguez, platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Hi,

On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
> The big question is what do we do if there are more then 3 profiles?

The Intel p-state driver has the 4 modes:
 * performance
 * balance_performance
 * balance_power
 * power

This seems to also match what windows does with their power slider,
there the modes are mapped to integer values:
 * power: 25
 * balance_power: 50
 * balance_performance: 75
 * performance: 100

Which appears to be the same as what newer DPTF versions use. For older
DPTF versions this is done through OEM variables, which also appear to
have 4 separate states usually. The MS power slider seems to define the
four possible modes:
 * Battery Saver
 * Better Battery
 * Better Performance
 * Best Performance

https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/customize-power-slider#set-default-power-slider-mode

> One option would be something like the following:
> 
> cat /sys/class/system_performance_profile/thinkpad_acpi/performance_profile
> 
> low-power [balanced] performance

I guess userspace is responsible for setting all drivers to the correct
state when the user changes a global system setting?

> cat /sys/class/system_performance_profile/thinkpad_acpi/extra_performance_profiles
> 
> extra-low-power balanced-performance-mix
> 
> So we add an optional extra_performance_profiles sysfs attribute and we ask all
> drivers implemeting this class to implement at least the 3 standard profiles
> (by mapping 3 of their options to these) and optional they can offer extra
> profiles (with free form names) in the extra_performance_profiles
> sysfs attribute under the class-device.

I think it would be good if userspace can figure out where such these
extra profiles would be sorted in on the "power save -- performance"
scale. Assigning an integer in the range of 0-100 might be a solution
for that.

Benjamin

> The idea behind putting the extra profiles in a separate sysfs-attribute
> is that reading the main performance_profile attribute will always show
> one selected, even if one of the extra profiles is actually in use,
> then the driver should also show the closest standardized profile as
> being active.
> 
> This will allow userspace code to always rely on the standard interface
> both for getting a representation of the currently active profile as well
> as for setting the active profile.
> 
> Elia, Mark, I assume that both of you want to get your patches for this
> upstream sooner, rather then later. But I think we should put them on
> hold until we have an agreement on a shared userspace API for this.
> 
> I would like to think that the above proposal is a good start,
> if we can quickly (*) decide on an userspace API here
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> I guess we should also add an optional lap_mode sysfs attribute
> to the class-device, to have all the info for the Thinkpads in
> one place.
> 
> 
> *) but not too quickly, it is important we get this right
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 12:22 ` Benjamin Berg
@ 2020-09-17 12:45   ` Hans de Goede
  2020-09-17 13:07     ` Bastien Nocera
  0 siblings, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 12:45 UTC (permalink / raw)
  To: Benjamin Berg, Elia Devito, Mark Pearson
  Cc: Bastien Nocera, Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 2:22 PM, Benjamin Berg wrote:
> Hi,
> 
> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
>> The big question is what do we do if there are more then 3 profiles?
> 
> The Intel p-state driver has the 4 modes:
>   * performance
>   * balance_performance
>   * balance_power
>   * power
> 
> This seems to also match what windows does with their power slider,
> there the modes are mapped to integer values:
>   * power: 25
>   * balance_power: 50
>   * balance_performance: 75
>   * performance: 100
> 
> Which appears to be the same as what newer DPTF versions use. For older
> DPTF versions this is done through OEM variables, which also appear to
> have 4 separate states usually. The MS power slider seems to define the
> four possible modes:
>   * Battery Saver
>   * Better Battery
>   * Better Performance
>   * Best Performance
> 
> https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/customize-power-slider#set-default-power-slider-mode
> 
>> One option would be something like the following:
>>
>> cat /sys/class/system_performance_profile/thinkpad_acpi/performance_profile
>>
>> low-power [balanced] performance
> 
> I guess userspace is responsible for setting all drivers to the correct
> state when the user changes a global system setting?

Yes.

>> cat /sys/class/system_performance_profile/thinkpad_acpi/extra_performance_profiles
>>
>> extra-low-power balanced-performance-mix
>>
>> So we add an optional extra_performance_profiles sysfs attribute and we ask all
>> drivers implemeting this class to implement at least the 3 standard profiles
>> (by mapping 3 of their options to these) and optional they can offer extra
>> profiles (with free form names) in the extra_performance_profiles
>> sysfs attribute under the class-device.
> 
> I think it would be good if userspace can figure out where such these
> extra profiles would be sorted in on the "power save -- performance"
> scale. Assigning an integer in the range of 0-100 might be a solution
> for that.

Interesting, maybe the primary interface should even be an integer in
that range, so for each system_performance_profile class-device we
would then have the following attributes:

mappings (ro) - This attribute gives a list of valid performance-profile-values
              In the form of "<integer-value> <description-string>\n", e.g.:

              25 Low Power
              50 Balanced
              100 Performance

value (rw) - Integer in the range 0 (lowest performance setting) - 100
              (highest performance setting). Note most drivers will only
              support a number of specific discrete values, see the mappings
              attribute. Userspace may write an arbitrary value between 0
              and 100, this will be rounded to the closest supported discrete
              value.

value_string (ro) - String representation of the currently active value,
              this is a shortcut for looking up the string in the mappings
              attribute yourself.

lap_mode (ro) - <lap_mode text here>

Something like p-p-daemon would then just interact with the value and lap_mode
fields, ignoring the mappings. It would then also need to do some rounding of
its own when reading value to map things back to its own internal levels.
One issue for p-p-d here might be that it writes its internal integer value
corresponding to say "Low power", then reads back a value and when rounding
that to its own discrete steps ends up at a different level then "Low power".
This can be avoid by using the mappings file to get the supported discrete values
and then only generate mappings for the discrete values to its own internal
discrete steps once and always use those mappings, thus always writing a
supported discrete value, avoiding rounding issues.

I think that this will give us a nice and flexible interface. Note if
anyone disagrees, or has a better idea please speak up. Once we have
decided on what the interface is going to be, we are effectively stuck
with it.

Regards,

Hans








> 
> Benjamin
> 
>> The idea behind putting the extra profiles in a separate sysfs-attribute
>> is that reading the main performance_profile attribute will always show
>> one selected, even if one of the extra profiles is actually in use,
>> then the driver should also show the closest standardized profile as
>> being active.
>>
>> This will allow userspace code to always rely on the standard interface
>> both for getting a representation of the currently active profile as well
>> as for setting the active profile.
>>
>> Elia, Mark, I assume that both of you want to get your patches for this
>> upstream sooner, rather then later. But I think we should put them on
>> hold until we have an agreement on a shared userspace API for this.
>>
>> I would like to think that the above proposal is a good start,
>> if we can quickly (*) decide on an userspace API here
>>
>> Regards,
>>
>> Hans
>>
>> p.s.
>>
>> I guess we should also add an optional lap_mode sysfs attribute
>> to the class-device, to have all the info for the Thinkpads in
>> one place.
>>
>>
>> *) but not too quickly, it is important we get this right
>>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 11:50 ` Bastien Nocera
@ 2020-09-17 12:51   ` Hans de Goede
  2020-09-17 13:00     ` Bastien Nocera
  2020-09-17 13:50     ` Benjamin Berg
  2020-09-17 13:36   ` Mark Pearson
  1 sibling, 2 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 12:51 UTC (permalink / raw)
  To: Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 1:50 PM, Bastien Nocera wrote:
> Hey,
> 
> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
>> Hi Elia, Mark, et al.
>>
>> Elia, Mark I'm mailing you both because both of you have pdx86
>> patches pending to add a vendor
>> specific sysfs-attribute for selecting performance-profiles for resp.
>> HP and Lenovo Thinkpad laptops.
>>
>> I think that this shows that we might need to start thinking
>> about a generic kernel API for this, otherwise we will
>> end up with slight different options per vendor ...
> 
> Some comments below based on possible use in power-profiles-daemon:
> https://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
> 
> 
>> So it seems we may need something like:
>>
>> /sys/class/system_performance_profile
>>
>> Where we would then get e.g.:
>>
>> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
>> le
>>
>> And then we need to standardize on the names/values which
>> performance_profile can show / accept when written too.
>>
>> The big question is what do we do if there are more then 3 profiles?
> 
> The Intel P-State driver in the kernel supports 4 separate ones (plus
> default):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/intel_pstate.c#n591
> which we crammed into 3 profiles:
> https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/blob/master/src/ppd-driver-intel-pstate.c#L209-226
> 
>> One option would be something like the following:
>>
>> cat
>> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
>> le
>>
>> low-power [balanced] performance
> 
> Are the square brackets to show the currently selected profile? I'd
> rather it was a separate sysfs attribute. I would expect to only ever
> read the list of supported profiles once, and then monitor an "active
> profile" attribute.

See my new proposal in reaction to Benjamin's email.

> 
> (a bit like the intel_pstate kernel driver does, but then all the
> devices that support Intel P-State support all the profiles, so it's
> not a good example ;)
> 
>> cat
>> /sys/class/system_performance_profile/thinkpad_acpi/extra_performance
>> _profiles
>>
>> extra-low-power balanced-performance-mix
>>
>> So we add an optional extra_performance_profiles sysfs attribute and
>> we ask all
>> drivers implemeting this class to implement at least the 3 standard
>> profiles
>> (by mapping 3 of their options to these) and optional they can offer
>> extra
>> profiles (with free form names) in the extra_performance_profiles
>> sysfs attribute under the class-device.
>>
>> The idea behind putting the extra profiles in a separate sysfs-
>> attribute
>> is that reading the main performance_profile attribute will always
>> show
>> one selected, even if one of the extra profiles is actually in use,
>> then the driver should also show the closest standardized profile as
>> being active.
> 
> I think it's fine having more than 3 profiles. Something like power-
> profiles-daemon would likely trying to match them all to one of the 3
> profiles it uses as an interface, or forcing the use of those 3
> profiles, depending on what that profile behaves.
> 
>> This will allow userspace code to always rely on the standard
>> interface
>> both for getting a representation of the currently active profile as
>> well
>> as for setting the active profile.
>>
>> Elia, Mark, I assume that both of you want to get your patches for
>> this
>> upstream sooner, rather then later. But I think we should put them on
>> hold until we have an agreement on a shared userspace API for this.
> 
> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
> something to advertise the unavailability of a profile, and the reason
> for that unavailability.

UGh, do we really need to export this though. We have the lap_mode thing
already; and that is something which we will need for other reasons in
the future too. Any UI for selecting performance modes can display a
warning when lap_mode is true saying that: "The laptop has detected that it
is sitting on someone's lap and that performance may be limited
because of this." (feel free to improve the text).

I guess we could split the "value" attribute from my reply to Benjamin's
email into "configured_value" (rw) and "actual_value" (rw) attributes.
If we have the info we might as well export it I guess,.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 12:51   ` Hans de Goede
@ 2020-09-17 13:00     ` Bastien Nocera
  2020-09-17 13:50     ` Benjamin Berg
  1 sibling, 0 replies; 30+ messages in thread
From: Bastien Nocera @ 2020-09-17 13:00 UTC (permalink / raw)
  To: Hans de Goede, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko

On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
<snip>

> UGh, do we really need to export this though. We have the lap_mode
> thing
> already; and that is something which we will need for other reasons
> in
> the future too. Any UI for selecting performance modes can display a
> warning when lap_mode is true saying that: "The laptop has detected
> that it
> is sitting on someone's lap and that performance may be limited
> because of this." (feel free to improve the text).

Given that there might be "100" profiles in that range, which ones
would be considered to be the "performance" ones that would be
inhibited if the laptop was on a person's lap?


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 11:22 RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles Hans de Goede
  2020-09-17 11:50 ` Bastien Nocera
  2020-09-17 12:22 ` Benjamin Berg
@ 2020-09-17 13:02 ` Barnabás Pőcze
  2020-09-17 13:24   ` Hans de Goede
  2 siblings, 1 reply; 30+ messages in thread
From: Barnabás Pőcze @ 2020-09-17 13:02 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Elia Devito, Mark Pearson, Benjamin Berg, Bastien Nocera,
	Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi


2020. szeptember 17., csütörtök 13:22 keltezéssel, Hans de Goede írta:

> [...]
> I guess we should also add an optional lap_mode sysfs attribute
> to the class-device, to have all the info for the Thinkpads in
> one place.
> [...]


Excuse my ignorance, but why does "lap_mode" need to be here?
I understand the implications of it regarding performance, but
I think it would be more sense to export it via the hwmon (or
something similar) subsystem? What am I missing?


Thanks,
Barnabás Pőcze

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 12:45   ` Hans de Goede
@ 2020-09-17 13:07     ` Bastien Nocera
  2020-09-17 13:46       ` Hans de Goede
  0 siblings, 1 reply; 30+ messages in thread
From: Bastien Nocera @ 2020-09-17 13:07 UTC (permalink / raw)
  To: Hans de Goede, Benjamin Berg, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

On Thu, 2020-09-17 at 14:45 +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/17/20 2:22 PM, Benjamin Berg wrote:
> > Hi,
> > 
> > On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
> > > The big question is what do we do if there are more then 3
> > > profiles?
> > 
> > The Intel p-state driver has the 4 modes:
> >   * performance
> >   * balance_performance
> >   * balance_power
> >   * power
> > 
> > This seems to also match what windows does with their power slider,
> > there the modes are mapped to integer values:
> >   * power: 25
> >   * balance_power: 50
> >   * balance_performance: 75
> >   * performance: 100
> > 
> > Which appears to be the same as what newer DPTF versions use. For
> > older
> > DPTF versions this is done through OEM variables, which also appear
> > to
> > have 4 separate states usually. The MS power slider seems to define
> > the
> > four possible modes:
> >   * Battery Saver
> >   * Better Battery
> >   * Better Performance
> >   * Best Performance
> > 
> > https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/customize-power-slider#set-default-power-slider-mode
> > 
> > > One option would be something like the following:
> > > 
> > > cat
> > > /sys/class/system_performance_profile/thinkpad_acpi/performance_p
> > > rofile
> > > 
> > > low-power [balanced] performance
> > 
> > I guess userspace is responsible for setting all drivers to the
> > correct
> > state when the user changes a global system setting?
> 
> Yes.
> 
> > > cat
> > > /sys/class/system_performance_profile/thinkpad_acpi/extra_perform
> > > ance_profiles
> > > 
> > > extra-low-power balanced-performance-mix
> > > 
> > > So we add an optional extra_performance_profiles sysfs attribute
> > > and we ask all
> > > drivers implemeting this class to implement at least the 3
> > > standard profiles
> > > (by mapping 3 of their options to these) and optional they can
> > > offer extra
> > > profiles (with free form names) in the extra_performance_profiles
> > > sysfs attribute under the class-device.
> > 
> > I think it would be good if userspace can figure out where such
> > these
> > extra profiles would be sorted in on the "power save --
> > performance"
> > scale. Assigning an integer in the range of 0-100 might be a
> > solution
> > for that.
> 
> Interesting, maybe the primary interface should even be an integer in
> that range, so for each system_performance_profile class-device we
> would then have the following attributes:
> 
> mappings (ro) - This attribute gives a list of valid performance-
> profile-values
>               In the form of "<integer-value> <description-
> string>\n", e.g.:
> 
>               25 Low Power
>               50 Balanced
>               100 Performance

The "description strings" need to come from a list. We're not going to
use those anywhere but in debug messages, so we need a way to figure
out what they would correspond to.

> value (rw) - Integer in the range 0 (lowest performance setting) -
> 100
>               (highest performance setting). Note most drivers will
> only
>               support a number of specific discrete values, see the
> mappings
>               attribute. Userspace may write an arbitrary value
> between 0
>               and 100, this will be rounded to the closest supported
> discrete
>               value.
> 
> value_string (ro) - String representation of the currently active
> value,
>               this is a shortcut for looking up the string in the
> mappings
>               attribute yourself.
> 
> lap_mode (ro) - <lap_mode text here>
> 
> Something like p-p-daemon would then just interact with the value and
> lap_mode
> fields, ignoring the mappings. It would then also need to do some
> rounding of
> its own when reading value to map things back to its own internal
> levels.
> One issue for p-p-d here might be that it writes its internal integer
> value
> corresponding to say "Low power", then reads back a value and when
> rounding
> that to its own discrete steps ends up at a different level then "Low
> power".
> This can be avoid by using the mappings file to get the supported
> discrete values
> and then only generate mappings for the discrete values to its own
> internal
> discrete steps once and always use those mappings, thus always
> writing a
> supported discrete value, avoiding rounding issues.
> 
> I think that this will give us a nice and flexible interface. Note if
> anyone disagrees, or has a better idea please speak up. Once we have
> decided on what the interface is going to be, we are effectively
> stuck
> with it.

That sounds slightly more complicated than I'd have expected it to be,
but I can work with that API.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:02 ` Barnabás Pőcze
@ 2020-09-17 13:24   ` Hans de Goede
  2020-09-17 13:28     ` Bastien Nocera
  2020-09-17 13:59     ` Benjamin Berg
  0 siblings, 2 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 13:24 UTC (permalink / raw)
  To: Barnabás Pőcze
  Cc: Elia Devito, Mark Pearson, Benjamin Berg, Bastien Nocera,
	Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 3:02 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. szeptember 17., csütörtök 13:22 keltezéssel, Hans de Goede írta:
> 
>> [...]
>> I guess we should also add an optional lap_mode sysfs attribute
>> to the class-device, to have all the info for the Thinkpads in
>> one place.
>> [...]
> 
> 
> Excuse my ignorance, but why does "lap_mode" need to be here?
> I understand the implications of it regarding performance, but
> I think it would be more sense to export it via the hwmon (or
> something similar) subsystem? What am I missing?

Well hwmon has very clearly defined sensor types, like voltage,
fan-speed and temperature. lap_mode does not match any of them.

Also registering another-type class device just for the lap_mode
boolean seems overkill, esp. since lap_mode is inherently coupled
to the performance-profile stuff.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:24   ` Hans de Goede
@ 2020-09-17 13:28     ` Bastien Nocera
  2020-09-17 13:51       ` Hans de Goede
  2020-09-17 13:59     ` Benjamin Berg
  1 sibling, 1 reply; 30+ messages in thread
From: Bastien Nocera @ 2020-09-17 13:28 UTC (permalink / raw)
  To: Hans de Goede, Barnabás Pőcze
  Cc: Elia Devito, Mark Pearson, Benjamin Berg, Jared Dominguez,
	platform-driver-x86, Andy Shevchenko

On Thu, 2020-09-17 at 15:24 +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/17/20 3:02 PM, Barnabás Pőcze wrote:
> > Hi
> > 
> > 
> > 2020. szeptember 17., csütörtök 13:22 keltezéssel, Hans de Goede
> > írta:
> > 
> > > [...]
> > > I guess we should also add an optional lap_mode sysfs attribute
> > > to the class-device, to have all the info for the Thinkpads in
> > > one place.
> > > [...]
> > 
> > 
> > Excuse my ignorance, but why does "lap_mode" need to be here?
> > I understand the implications of it regarding performance, but
> > I think it would be more sense to export it via the hwmon (or
> > something similar) subsystem? What am I missing?
> 
> Well hwmon has very clearly defined sensor types, like voltage,
> fan-speed and temperature. lap_mode does not match any of them.
> 
> Also registering another-type class device just for the lap_mode
> boolean seems overkill, esp. since lap_mode is inherently coupled
> to the performance-profile stuff.

There's proximity sensors in the IIO subsystem which this could use (I
guess that was what was planned to be used, as that's how I got
involved in power-profiles-daemon).


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 11:50 ` Bastien Nocera
  2020-09-17 12:51   ` Hans de Goede
@ 2020-09-17 13:36   ` Mark Pearson
  2020-09-17 14:04     ` Hans de Goede
  1 sibling, 1 reply; 30+ messages in thread
From: Mark Pearson @ 2020-09-17 13:36 UTC (permalink / raw)
  To: Bastien Nocera, Hans de Goede, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi all,

More emails came in since I wrote this....but I'm going to send anyway 
and catch up with those after. I need to write faster :)

On 9/17/2020 7:50 AM, Bastien Nocera wrote:
> Hey,
> 
> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
>> Hi Elia, Mark, et al.
>>
>> Elia, Mark I'm mailing you both because both of you have pdx86
>> patches pending to add a vendor
>> specific sysfs-attribute for selecting performance-profiles for resp.
>> HP and Lenovo Thinkpad laptops.
>>
>> I think that this shows that we might need to start thinking
>> about a generic kernel API for this, otherwise we will
>> end up with slight different options per vendor ...
> 
> Some comments below based on possible use in power-profiles-daemon:
> https://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
> 
> 
>> So it seems we may need something like:
>>
>> /sys/class/system_performance_profile
>>
>> Where we would then get e.g.:
>>
>> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
>> le
>>
>> And then we need to standardize on the names/values which
>> performance_profile can show / accept when written too.
>>
>> The big question is what do we do if there are more then 3 profiles?
> 
> The Intel P-State driver in the kernel supports 4 separate ones (plus
> default):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/intel_pstate.c#n591
> which we crammed into 3 profiles:
> https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/blob/master/src/ppd-driver-intel-pstate.c#L209-226
> 
>> One option would be something like the following:
>>
>> cat
>> /sys/class/system_performance_profile/thinkpad_acpi/performance_profi
>> le
>>
>> low-power [balanced] performance
> 
> Are the square brackets to show the currently selected profile? I'd
> rather it was a separate sysfs attribute. I would expect to only ever
> read the list of supported profiles once, and then monitor an "active
> profile" attribute.
> 
> (a bit like the intel_pstate kernel driver does, but then all the
> devices that support Intel P-State support all the profiles, so it's
> not a good example ;)
> 
>> cat
>> /sys/class/system_performance_profile/thinkpad_acpi/extra_performance
>> _profiles
>>
>> extra-low-power balanced-performance-mix
>>
>> So we add an optional extra_performance_profiles sysfs attribute and
>> we ask all
>> drivers implemeting this class to implement at least the 3 standard
>> profiles
>> (by mapping 3 of their options to these) and optional they can offer
>> extra
>> profiles (with free form names) in the extra_performance_profiles
>> sysfs attribute under the class-device.
>>
>> The idea behind putting the extra profiles in a separate sysfs-
>> attribute
>> is that reading the main performance_profile attribute will always
>> show
>> one selected, even if one of the extra profiles is actually in use,
>> then the driver should also show the closest standardized profile as
>> being active.
> 
> I think it's fine having more than 3 profiles. Something like power-
> profiles-daemon would likely trying to match them all to one of the 3
> profiles it uses as an interface, or forcing the use of those 3
> profiles, depending on what that profile behaves.
> 
>> This will allow userspace code to always rely on the standard
>> interface
>> both for getting a representation of the currently active profile as
>> well
>> as for setting the active profile.
>>
>> Elia, Mark, I assume that both of you want to get your patches for
>> this
>> upstream sooner, rather then later. But I think we should put them on
>> hold until we have an agreement on a shared userspace API for this.
> 
> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
> something to advertise the unavailability of a profile, and the reason
> for that unavailability.
> 
In all honesty I was slightly dreading this email :) I know the similar 
issue killed our ePrivacy patch...but I fully appreciate that is part of 
open source contribution

So yes - I agree that having a common interface would be a good idea and 
making it common between the vendors makes sense. Let me know how to 
contribute and make that happen.

 From Lenovo's firmware point of view - our three settings should map on 
to this quite closely with the exception that we have one setting that 
covers balance_power and power (I never understood why the FW team did 
that - as they have the four states in Windows - I wasn't able to get a 
satisfactory answer to that question)

> Cheers
> 
>>
>> I would like to think that the above proposal is a good start,
>> if we can quickly (*) decide on an userspace API here

Yes and understood. Let me know what is the best place to make this 
happen - from my point of view the main aim is to get this to our users 
to make the whole performance mode implemetnation more usable and 
obvious. Without my proposed patch it's really hard to tell what mode 
you are in on our platforms (and leads to lots of support questions).

I'm particularly aware of the eprivacy patch where that got rejected for 
a generic solution that was under development - but the person working 
on the generic solution stopped part way through to work on other 
things. We didn't have the knowledge or experience of the graphics 
driver to be able to really go and contribute effectively so for now 
that feature is dead even though our initial patch was fairly simple. It 
is still disappointing that our users don't get useful functionality 
(and I also have to argue with marketing as to whether we can sell Linux 
systems with ePrivacy screens which is no fun - I spend way too much of 
my life doing Lenovo internal paperwork).

I figure on this item it's less complicated (not tied into the graphics 
drivers details) so I hope I can contribute more directly - let me know 
if I'm being naive.

One question - the main reason for a common interface is for user space 
to not deal with a mess of APIs. Is it worth me doing a simplified 
version of my patch (maybe using debugfs?) so I can expose the modes to 
users whilst we work on the common solution? I'm assuming there is no 
mileage in getting my patch (with the fix I owe Benjamin) in and then 
changing it in the future once the generic solution is available as that 
potentially messes up userspace too much? Something as a stopgap measure 
that won't annoy the kernel community but is good for Linux users as I'm 
guessing the generic solution is likely to be months away

Let me know what you recommend as the next steps.

>>
>> Regards,
>>
>> Hans
>>
>> p.s.
>>
>> I guess we should also add an optional lap_mode sysfs attribute
>> to the class-device, to have all the info for the Thinkpads in
>> one place.
>>
I'm good with this too - but the lapmode patch is accepted and there is 
the palm sensor patch too which I'm hoping is accepted soon. Whilst I'm 
happy to make them part of this implementation (if they fit) I'd 
appreciate if they didn't get removed or held up as they're needed for 
our WWAN implementation which is already overdue.
The main consumer there will be our WWAN enablement utility and we can 
change that to support different API if needs be :)
>>
>> *) but not too quickly, it is important we get this right
>>
> 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:07     ` Bastien Nocera
@ 2020-09-17 13:46       ` Hans de Goede
  0 siblings, 0 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 13:46 UTC (permalink / raw)
  To: Bastien Nocera, Benjamin Berg, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 3:07 PM, Bastien Nocera wrote:
> On Thu, 2020-09-17 at 14:45 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 9/17/20 2:22 PM, Benjamin Berg wrote:
>>> Hi,
>>>
>>> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
>>>> The big question is what do we do if there are more then 3
>>>> profiles?
>>>
>>> The Intel p-state driver has the 4 modes:
>>>    * performance
>>>    * balance_performance
>>>    * balance_power
>>>    * power
>>>
>>> This seems to also match what windows does with their power slider,
>>> there the modes are mapped to integer values:
>>>    * power: 25
>>>    * balance_power: 50
>>>    * balance_performance: 75
>>>    * performance: 100
>>>
>>> Which appears to be the same as what newer DPTF versions use. For
>>> older
>>> DPTF versions this is done through OEM variables, which also appear
>>> to
>>> have 4 separate states usually. The MS power slider seems to define
>>> the
>>> four possible modes:
>>>    * Battery Saver
>>>    * Better Battery
>>>    * Better Performance
>>>    * Best Performance
>>>
>>> https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/customize-power-slider#set-default-power-slider-mode
>>>
>>>> One option would be something like the following:
>>>>
>>>> cat
>>>> /sys/class/system_performance_profile/thinkpad_acpi/performance_p
>>>> rofile
>>>>
>>>> low-power [balanced] performance
>>>
>>> I guess userspace is responsible for setting all drivers to the
>>> correct
>>> state when the user changes a global system setting?
>>
>> Yes.
>>
>>>> cat
>>>> /sys/class/system_performance_profile/thinkpad_acpi/extra_perform
>>>> ance_profiles
>>>>
>>>> extra-low-power balanced-performance-mix
>>>>
>>>> So we add an optional extra_performance_profiles sysfs attribute
>>>> and we ask all
>>>> drivers implemeting this class to implement at least the 3
>>>> standard profiles
>>>> (by mapping 3 of their options to these) and optional they can
>>>> offer extra
>>>> profiles (with free form names) in the extra_performance_profiles
>>>> sysfs attribute under the class-device.
>>>
>>> I think it would be good if userspace can figure out where such
>>> these
>>> extra profiles would be sorted in on the "power save --
>>> performance"
>>> scale. Assigning an integer in the range of 0-100 might be a
>>> solution
>>> for that.
>>
>> Interesting, maybe the primary interface should even be an integer in
>> that range, so for each system_performance_profile class-device we
>> would then have the following attributes:
>>
>> mappings (ro) - This attribute gives a list of valid performance-
>> profile-values
>>                In the form of "<integer-value> <description-
>> string>\n", e.g.:
>>
>>                25 Low Power
>>                50 Balanced
>>                100 Performance
> 
> The "description strings" need to come from a list. We're not going to
> use those anywhere but in debug messages, so we need a way to figure
> out what they would correspond to.

The whole idea behind the 101 possible profiles setup is to make the
API flexible/extensible. We don't know how much other profiles other
implementations will offer, nor what the naming used by that vendor
will be. So offering a fixed list in advance is sort of impossible.

For p-p-d the descriptions are probably not interesting at all,
but I think it would still be good for the driver to list the
Windows names of the various profiles in the mappings file.


> 
>> value (rw) - Integer in the range 0 (lowest performance setting) -
>> 100
>>                (highest performance setting). Note most drivers will
>> only
>>                support a number of specific discrete values, see the
>> mappings
>>                attribute. Userspace may write an arbitrary value
>> between 0
>>                and 100, this will be rounded to the closest supported
>> discrete
>>                value.
>>
>> value_string (ro) - String representation of the currently active
>> value,
>>                this is a shortcut for looking up the string in the
>> mappings
>>                attribute yourself.
>>
>> lap_mode (ro) - <lap_mode text here>
>>
>> Something like p-p-daemon would then just interact with the value and
>> lap_mode
>> fields, ignoring the mappings. It would then also need to do some
>> rounding of
>> its own when reading value to map things back to its own internal
>> levels.
>> One issue for p-p-d here might be that it writes its internal integer
>> value
>> corresponding to say "Low power", then reads back a value and when
>> rounding
>> that to its own discrete steps ends up at a different level then "Low
>> power".
>> This can be avoid by using the mappings file to get the supported
>> discrete values
>> and then only generate mappings for the discrete values to its own
>> internal
>> discrete steps once and always use those mappings, thus always
>> writing a
>> supported discrete value, avoiding rounding issues.
>>
>> I think that this will give us a nice and flexible interface. Note if
>> anyone disagrees, or has a better idea please speak up. Once we have
>> decided on what the interface is going to be, we are effectively
>> stuck
>> with it.
> 
> That sounds slightly more complicated than I'd have expected it to be,
> but I can work with that API.

Well Benjamin made a very valid point that we need to be able to
sort the various profiles along the low-power <-> performance axis
and maybe even also show some relative distance if some options
are closer together then others.  Which lead to (Bejamin's) 0-100
idea. I think this makes a lot of sense as it should give us
enough flexibility to cover other x86 vendor's implementations
and hopefully also similar functionality on other architectures.

I agree that this is slightly more complicated then I initially
expected too, but that is the price of building in some much
needed flexibility.  If you have options to simplify the interface
though I'm all ears.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 12:51   ` Hans de Goede
  2020-09-17 13:00     ` Bastien Nocera
@ 2020-09-17 13:50     ` Benjamin Berg
  2020-09-17 13:54       ` Hans de Goede
  1 sibling, 1 reply; 30+ messages in thread
From: Benjamin Berg @ 2020-09-17 13:50 UTC (permalink / raw)
  To: Hans de Goede, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
> > Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
> > something to advertise the unavailability of a profile, and the reason
> > for that unavailability.
> 
> UGh, do we really need to export this though. We have the lap_mode thing
> already; and that is something which we will need for other reasons in
> the future too. Any UI for selecting performance modes can display a
> warning when lap_mode is true saying that: "The laptop has detected that it
> is sitting on someone's lap and that performance may be limited
> because of this." (feel free to improve the text).

Well, for dytc_perfmode there are actually always the three states
L/M/H. It just happens that the kernel will write "H*" (was "M*" until
yesterday) when the performance mode is degraded due to lap detection.

Think of dytc_perfmode as a profile that sets a number of things:
 * Thermal Limits
 * Fan Behaviour
 * possibly more

While dytc_lapmode will only enforce a change to the thermal limit.

So "performance" (H) is technically a valid mode even when the lap is
detected.

> I guess we could split the "value" attribute from my reply to Benjamin's
> email into "configured_value" (rw) and "actual_value" (rw) attributes.
> If we have the info we might as well export it I guess,.

I consider the "*" purely a curtsey to users that read the attribute
directly using e.g. cat to help with the interpretation. It probably is
not interesting to userspace applications/daemons.

Benjamin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:28     ` Bastien Nocera
@ 2020-09-17 13:51       ` Hans de Goede
  0 siblings, 0 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 13:51 UTC (permalink / raw)
  To: Bastien Nocera, Barnabás Pőcze
  Cc: Elia Devito, Mark Pearson, Benjamin Berg, Jared Dominguez,
	platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 3:28 PM, Bastien Nocera wrote:
> On Thu, 2020-09-17 at 15:24 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 9/17/20 3:02 PM, Barnabás Pőcze wrote:
>>> Hi
>>>
>>>
>>> 2020. szeptember 17., csütörtök 13:22 keltezéssel, Hans de Goede
>>> írta:
>>>
>>>> [...]
>>>> I guess we should also add an optional lap_mode sysfs attribute
>>>> to the class-device, to have all the info for the Thinkpads in
>>>> one place.
>>>> [...]
>>>
>>>
>>> Excuse my ignorance, but why does "lap_mode" need to be here?
>>> I understand the implications of it regarding performance, but
>>> I think it would be more sense to export it via the hwmon (or
>>> something similar) subsystem? What am I missing?
>>
>> Well hwmon has very clearly defined sensor types, like voltage,
>> fan-speed and temperature. lap_mode does not match any of them.
>>
>> Also registering another-type class device just for the lap_mode
>> boolean seems overkill, esp. since lap_mode is inherently coupled
>> to the performance-profile stuff.
> 
> There's proximity sensors in the IIO subsystem which this could use

But this is not a proximity sensor, it is a state determined by
the firmware based on several data sources, which may or may not
include a proximity sensor and which may or may not include an
accelerometer + who knows what else.

But with the earlier discussed splitting of the value
sysfs-attr into a configure_value (rw) and actual_value(ro)
attributes I don't think we need lap_mode inside the
system_performance_profile class at all. We already have
a thinkpad_acpi specific sysfs attribute exporting this, and
since this is AFAIK a thinkpad specific thing, just leaving
it there is probably the best solution.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:50     ` Benjamin Berg
@ 2020-09-17 13:54       ` Hans de Goede
  2020-09-17 14:10         ` Benjamin Berg
  0 siblings, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 13:54 UTC (permalink / raw)
  To: Benjamin Berg, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 3:50 PM, Benjamin Berg wrote:
> On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
>>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>>> something to advertise the unavailability of a profile, and the reason
>>> for that unavailability.
>>
>> UGh, do we really need to export this though. We have the lap_mode thing
>> already; and that is something which we will need for other reasons in
>> the future too. Any UI for selecting performance modes can display a
>> warning when lap_mode is true saying that: "The laptop has detected that it
>> is sitting on someone's lap and that performance may be limited
>> because of this." (feel free to improve the text).
> 
> Well, for dytc_perfmode there are actually always the three states
> L/M/H. It just happens that the kernel will write "H*" (was "M*" until
> yesterday) when the performance mode is degraded due to lap detection.
> 
> Think of dytc_perfmode as a profile that sets a number of things:
>   * Thermal Limits
>   * Fan Behaviour
>   * possibly more
> 
> While dytc_lapmode will only enforce a change to the thermal limit.
> 
> So "performance" (H) is technically a valid mode even when the lap is
> detected.
> 
>> I guess we could split the "value" attribute from my reply to Benjamin's
>> email into "configured_value" (rw) and "actual_value" (rw) attributes.
>> If we have the info we might as well export it I guess,.
> 
> I consider the "*" purely a curtsey to users that read the attribute
> directly using e.g. cat to help with the interpretation. It probably is
> not interesting to userspace applications/daemons.

So if there is a difference between M and H and H* then I think we should
just do the KISS thing and only have a single value attribute and in the
new interface handle the H* like H (p-p-d can still check the lap_mode
attribute to differentiate the 2 if it wants to).

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:24   ` Hans de Goede
  2020-09-17 13:28     ` Bastien Nocera
@ 2020-09-17 13:59     ` Benjamin Berg
  1 sibling, 0 replies; 30+ messages in thread
From: Benjamin Berg @ 2020-09-17 13:59 UTC (permalink / raw)
  To: Hans de Goede, Barnabás Pőcze
  Cc: Elia Devito, Mark Pearson, Bastien Nocera, Jared Dominguez,
	platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Thu, 2020-09-17 at 15:24 +0200, Hans de Goede wrote:
> > Excuse my ignorance, but why does "lap_mode" need to be here?
> > I understand the implications of it regarding performance, but
> > I think it would be more sense to export it via the hwmon (or
> > something similar) subsystem? What am I missing?
> 
> Well hwmon has very clearly defined sensor types, like voltage,
> fan-speed and temperature. lap_mode does not match any of them.

Not sure. I think you can consider it a simple sensor. In this case it
just affects thermal limits that are imposed on the system (and some
other stuff like RF for modems).

It does have a major impact on something like a "performance" mode
because of those imposed limits and causes these modes to be degraded.
But I think this interface can just show the configured state, even if
the component knows that the performance state is severely limited due
to other factors.

Benjamin

> Also registering another-type class device just for the lap_mode
> boolean seems overkill, esp. since lap_mode is inherently coupled
> to the performance-profile stuff.
> 
> Regards,
> 
> Hans
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:36   ` Mark Pearson
@ 2020-09-17 14:04     ` Hans de Goede
  2020-09-17 16:51       ` Mark Pearson
  0 siblings, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 14:04 UTC (permalink / raw)
  To: Mark Pearson, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 3:36 PM, Mark Pearson wrote:
> Hi all,
> 
> More emails came in since I wrote this....but I'm going to send anyway and catch up with those after. I need to write faster :)
> 
> On 9/17/2020 7:50 AM, Bastien Nocera wrote:
>> Hey,
>>
>> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:

<snip>

>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>> something to advertise the unavailability of a profile, and the reason
>> for that unavailability.
>>
> In all honesty I was slightly dreading this email :) I know the similar issue killed our ePrivacy patch...but I fully appreciate that is part of open source contribution

Getting offtopic here, but the e-privacy screen stuff is already available to userspace through /proc/acpi/ibm/lcdshadow.
What is on hold is adding userspace support for it, because we want to use the new generic API for that once it lands.

> So yes - I agree that having a common interface would be a good idea and making it common between the vendors makes sense. Let me know how to contribute and make that happen.
> 
>  From Lenovo's firmware point of view - our three settings should map on to this quite closely with the exception that we have one setting that covers balance_power and power (I never understood why the FW team did that - as they have the four states in Windows - I wasn't able to get a satisfactory answer to that question)
>
>>> I would like to think that the above proposal is a good start,
>>> if we can quickly (*) decide on an userspace API here
> 
> Yes and understood. Let me know what is the best place to make this happen - from my point of view the main aim is to get this to our users to make the whole performance mode implemetnation more usable and obvious. Without my proposed patch it's really hard to tell what mode you are in on our platforms (and leads to lots of support questions).
> 
> I'm particularly aware of the eprivacy patch where that got rejected for a generic solution that was under development - but the person working on the generic solution stopped part way through to work on other things. We didn't have the knowledge or experience of the graphics driver to be able to really go and contribute effectively so for now that feature is dead even though our initial patch was fairly simple. It is still disappointing that our users don't get useful functionality (and I also have to argue with marketing as to whether we can sell Linux systems with ePrivacy screens which is 
> no fun - I spend way too much of my life doing Lenovo internal paperwork).
> 
> I figure on this item it's less complicated (not tied into the graphics drivers details) so I hope I can contribute more directly - let me know if I'm being naive.
> 
> One question - the main reason for a common interface is for user space to not deal with a mess of APIs.

Correct.

> Is it worth me doing a simplified version of my patch (maybe using debugfs?) so I can expose the modes to users whilst we work on the common solution? I'm assuming there is no mileage in getting my patch (with the fix I owe Benjamin) in and then changing it in the future once the generic solution is available as that potentially messes up userspace too much?

Using debugfs as an intermediate solution is a good idea. debugfs interfaces have no ABI guarantees, so we can simply drop it when the generic stuff lands.

> Something as a stopgap measure that won't annoy the kernel community but is good for Linux users as I'm guessing the generic solution 
> is likely to be months away

The generic solution will definitely not make the 5.10 kernel, but 5.11 is not entirely out of the question, although to be honest 5.12 seems more realistic.

<snip>

>>> I guess we should also add an optional lap_mode sysfs attribute
>>> to the class-device, to have all the info for the Thinkpads in
>>> one place.
>>>
> I'm good with this too - but the lapmode patch is accepted and there is the palm sensor patch too which I'm hoping is accepted soon. Whilst I'm happy to make them part of this implementation (if they fit) I'd appreciate if they didn't get removed or held up as they're needed for our WWAN implementation which is already overdue.
> The main consumer there will be our WWAN enablement utility and we can change that to support different API if needs be :)

As the rest of the dicussion has shown the lap_mode thingie indeed is best left in place as a
thinkpad_acpi only interface for now.

I guess the same applies to the palm_sensor stuff. We may want to advertise that through
some standardize API later, but for now we can just use a thinkpad_acpi specific API and
then also export the info through the standardized API later, as we will do for the
lcdshadow stuff.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 13:54       ` Hans de Goede
@ 2020-09-17 14:10         ` Benjamin Berg
  2020-09-17 16:58           ` [External] " Mark Pearson
  0 siblings, 1 reply; 30+ messages in thread
From: Benjamin Berg @ 2020-09-17 14:10 UTC (permalink / raw)
  To: Hans de Goede, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

Hi,

On Thu, 2020-09-17 at 15:54 +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/17/20 3:50 PM, Benjamin Berg wrote:
> > On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
> > > > Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
> > > > something to advertise the unavailability of a profile, and the reason
> > > > for that unavailability.
> > > 
> > > UGh, do we really need to export this though. We have the lap_mode thing
> > > already; and that is something which we will need for other reasons in
> > > the future too. Any UI for selecting performance modes can display a
> > > warning when lap_mode is true saying that: "The laptop has detected that it
> > > is sitting on someone's lap and that performance may be limited
> > > because of this." (feel free to improve the text).
> > 
> > Well, for dytc_perfmode there are actually always the three states
> > L/M/H. It just happens that the kernel will write "H*" (was "M*" until
> > yesterday) when the performance mode is degraded due to lap detection.
> > 
> > Think of dytc_perfmode as a profile that sets a number of things:
> >   * Thermal Limits
> >   * Fan Behaviour
> >   * possibly more
> > 
> > While dytc_lapmode will only enforce a change to the thermal limit.
> > So "performance" (H) is technically a valid mode even when the lap is
> > detected.
> > 
> > > I guess we could split the "value" attribute from my reply to Benjamin's
> > > email into "configured_value" (rw) and "actual_value" (rw) attributes.
> > > If we have the info we might as well export it I guess,.
> > 
> > I consider the "*" purely a curtsey to users that read the attribute
> > directly using e.g. cat to help with the interpretation. It probably is
> > not interesting to userspace applications/daemons.
> 
> So if there is a difference between M and H and H* then I think we should
> just do the KISS thing and only have a single value attribute and in the
> new interface handle the H* like H (p-p-d can still check the lap_mode
> attribute to differentiate the 2 if it wants to).

I guess you are saying to drop "H*" and only have "L"/"M"/"H"? If so,
fine with me, but we probably need that input in reply to
  https://patchwork.kernel.org/patch/11730133/#23618881
then :)

In principle it could be useful for userspace to know that performance
is or would be dramatically impacted. i.e. when dytc_lapmode is 1, then
you might want to say something like:

  performance states >= 75 are impacted due to "lapmode"

But, not sure if a kernel interface for that is useful or whether we
should just put that kind of knowledge into userspace.

Benjamin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 14:04     ` Hans de Goede
@ 2020-09-17 16:51       ` Mark Pearson
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Pearson @ 2020-09-17 16:51 UTC (permalink / raw)
  To: Hans de Goede, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Benjamin Berg, Jared Dominguez, platform-driver-x86, Andy Shevchenko


On 9/17/2020 10:04 AM, Hans de Goede wrote:
> Hi,
> 
> On 9/17/20 3:36 PM, Mark Pearson wrote:
>> Hi all,
>>
<snip>
>>
>> One question - the main reason for a common interface is for user 
>> space to not deal with a mess of APIs.
> 
> Correct.
> 
>> Is it worth me doing a simplified version of my patch (maybe using 
>> debugfs?) so I can expose the modes to users whilst we work on the 
>> common solution? I'm assuming there is no mileage in getting my patch 
>> (with the fix I owe Benjamin) in and then changing it in the future 
>> once the generic solution is available as that potentially messes up 
>> userspace too much?
> 
> Using debugfs as an intermediate solution is a good idea. debugfs 
> interfaces have no ABI guarantees, so we can simply drop it when the 
> generic stuff lands.
> 
Great. I'll look at that as an alternative for the short term.

>> Something as a stopgap measure that won't annoy the kernel community 
>> but is good for Linux users as I'm guessing the generic solution is 
>> likely to be months away
> 
> The generic solution will definitely not make the 5.10 kernel, but 5.11 
> is not entirely out of the question, although to be honest 5.12 seems 
> more realistic.
> 
OK.
> <snip>
> 
>>>> I guess we should also add an optional lap_mode sysfs attribute
>>>> to the class-device, to have all the info for the Thinkpads in
>>>> one place.
>>>>
>> I'm good with this too - but the lapmode patch is accepted and there 
>> is the palm sensor patch too which I'm hoping is accepted soon. Whilst 
>> I'm happy to make them part of this implementation (if they fit) I'd 
>> appreciate if they didn't get removed or held up as they're needed for 
>> our WWAN implementation which is already overdue.
>> The main consumer there will be our WWAN enablement utility and we can 
>> change that to support different API if needs be :)
> 
> As the rest of the dicussion has shown the lap_mode thingie indeed is 
> best left in place as a
> thinkpad_acpi only interface for now.
> 
> I guess the same applies to the palm_sensor stuff. We may want to 
> advertise that through
> some standardize API later, but for now we can just use a thinkpad_acpi 
> specific API and
> then also export the info through the standardized API later, as we will 
> do for the
> lcdshadow stuff.
> 
Sounds good.
Happy to work on that too in the future

Mark

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 14:10         ` Benjamin Berg
@ 2020-09-17 16:58           ` Mark Pearson
  2020-09-17 17:03             ` Hans de Goede
  0 siblings, 1 reply; 30+ messages in thread
From: Mark Pearson @ 2020-09-17 16:58 UTC (permalink / raw)
  To: Benjamin Berg, Hans de Goede, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

On 9/17/2020 10:10 AM, Benjamin Berg wrote:
> Hi,
> 
> On Thu, 2020-09-17 at 15:54 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 9/17/20 3:50 PM, Benjamin Berg wrote:
>>> On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
>>>>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>>>>> something to advertise the unavailability of a profile, and the reason
>>>>> for that unavailability.
>>>>
>>>> UGh, do we really need to export this though. We have the lap_mode thing
>>>> already; and that is something which we will need for other reasons in
>>>> the future too. Any UI for selecting performance modes can display a
>>>> warning when lap_mode is true saying that: "The laptop has detected that it
>>>> is sitting on someone's lap and that performance may be limited
>>>> because of this." (feel free to improve the text).
>>>
>>> Well, for dytc_perfmode there are actually always the three states
>>> L/M/H. It just happens that the kernel will write "H*" (was "M*" until
>>> yesterday) when the performance mode is degraded due to lap detection.
>>>
>>> Think of dytc_perfmode as a profile that sets a number of things:
>>>    * Thermal Limits
>>>    * Fan Behaviour
>>>    * possibly more
>>>
>>> While dytc_lapmode will only enforce a change to the thermal limit.
>>> So "performance" (H) is technically a valid mode even when the lap is
>>> detected.
>>>
>>>> I guess we could split the "value" attribute from my reply to Benjamin's
>>>> email into "configured_value" (rw) and "actual_value" (rw) attributes.
>>>> If we have the info we might as well export it I guess,.
>>>
>>> I consider the "*" purely a curtsey to users that read the attribute
>>> directly using e.g. cat to help with the interpretation. It probably is
>>> not interesting to userspace applications/daemons.
>>
>> So if there is a difference between M and H and H* then I think we should
>> just do the KISS thing and only have a single value attribute and in the
>> new interface handle the H* like H (p-p-d can still check the lap_mode
>> attribute to differentiate the 2 if it wants to).
> 
> I guess you are saying to drop "H*" and only have "L"/"M"/"H"? If so,
> fine with me, but we probably need that input in reply to
>    https://patchwork.kernel.org/patch/11730133/#23618881
> then :)
> 
> In principle it could be useful for userspace to know that performance
> is or would be dramatically impacted. i.e. when dytc_lapmode is 1, then
> you might want to say something like:
> 
>    performance states >= 75 are impacted due to "lapmode"
> 
> But, not sure if a kernel interface for that is useful or whether we
> should just put that kind of knowledge into userspace.
> 
> Benjamin
> 
I don't have a strong opinion on this but the kernel driver is already 
knowledgeable about the quirks of what does and doesn't work on the 
system so it seems like a good place to have that logic.

What if we have an API for "configured" and "actual" - and if they 
differ userspace knows it should figure out why (likely lapmode, but if 
the HW vendor adds a new setting related to "position of sun in the sky" 
or "how much money is in your account and can you afford the electricity 
bill?" that could be added too....)

Mark

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 16:58           ` [External] " Mark Pearson
@ 2020-09-17 17:03             ` Hans de Goede
  2020-09-17 17:16               ` Mark Pearson
  0 siblings, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-17 17:03 UTC (permalink / raw)
  To: Mark Pearson, Benjamin Berg, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi Mark,

On 9/17/20 6:58 PM, Mark Pearson wrote:
> On 9/17/2020 10:10 AM, Benjamin Berg wrote:
>> Hi,
>>
>> On Thu, 2020-09-17 at 15:54 +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 9/17/20 3:50 PM, Benjamin Berg wrote:
>>>> On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
>>>>>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>>>>>> something to advertise the unavailability of a profile, and the reason
>>>>>> for that unavailability.
>>>>>
>>>>> UGh, do we really need to export this though. We have the lap_mode thing
>>>>> already; and that is something which we will need for other reasons in
>>>>> the future too. Any UI for selecting performance modes can display a
>>>>> warning when lap_mode is true saying that: "The laptop has detected that it
>>>>> is sitting on someone's lap and that performance may be limited
>>>>> because of this." (feel free to improve the text).
>>>>
>>>> Well, for dytc_perfmode there are actually always the three states
>>>> L/M/H. It just happens that the kernel will write "H*" (was "M*" until
>>>> yesterday) when the performance mode is degraded due to lap detection.
>>>>
>>>> Think of dytc_perfmode as a profile that sets a number of things:
>>>>    * Thermal Limits
>>>>    * Fan Behaviour
>>>>    * possibly more
>>>>
>>>> While dytc_lapmode will only enforce a change to the thermal limit.
>>>> So "performance" (H) is technically a valid mode even when the lap is
>>>> detected.
>>>>
>>>>> I guess we could split the "value" attribute from my reply to Benjamin's
>>>>> email into "configured_value" (rw) and "actual_value" (rw) attributes.
>>>>> If we have the info we might as well export it I guess,.
>>>>
>>>> I consider the "*" purely a curtsey to users that read the attribute
>>>> directly using e.g. cat to help with the interpretation. It probably is
>>>> not interesting to userspace applications/daemons.
>>>
>>> So if there is a difference between M and H and H* then I think we should
>>> just do the KISS thing and only have a single value attribute and in the
>>> new interface handle the H* like H (p-p-d can still check the lap_mode
>>> attribute to differentiate the 2 if it wants to).
>>
>> I guess you are saying to drop "H*" and only have "L"/"M"/"H"? If so,
>> fine with me, but we probably need that input in reply to
>>    https://patchwork.kernel.org/patch/11730133/#23618881
>> then :)
>>
>> In principle it could be useful for userspace to know that performance
>> is or would be dramatically impacted. i.e. when dytc_lapmode is 1, then
>> you might want to say something like:
>>
>>    performance states >= 75 are impacted due to "lapmode"
>>
>> But, not sure if a kernel interface for that is useful or whether we
>> should just put that kind of knowledge into userspace.
>>
>> Benjamin
>>
> I don't have a strong opinion on this but the kernel driver is already knowledgeable about the quirks of what does and doesn't work on the system so it seems like a good place to have that logic.
> 
> What if we have an API for "configured" and "actual" - and if they differ userspace knows it should figure out why (likely lapmode, but if the HW vendor adds a new setting related to "position of sun in the sky" or "how much money is in your account and can you afford the electricity bill?" that could be added too....)

As I understand the problem with the configured and actual value/performance_level ideas is that if I understand things correctly that H* is not the same as M,
it behaves close-ish to M because of the lower thermal-limits from lapmode, but if I understood Benjamin correctly is is not exactly the same, so if we were
to advertise "M" in the actual_performance_level sysfs-attribute then that would not really be correct ?

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 17:03             ` Hans de Goede
@ 2020-09-17 17:16               ` Mark Pearson
  2020-09-17 18:16                 ` Benjamin Berg
  2020-09-22 10:30                 ` Hans de Goede
  0 siblings, 2 replies; 30+ messages in thread
From: Mark Pearson @ 2020-09-17 17:16 UTC (permalink / raw)
  To: Hans de Goede, Benjamin Berg, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko


On 9/17/2020 1:03 PM, Hans de Goede wrote:
> Hi Mark,
> 
> On 9/17/20 6:58 PM, Mark Pearson wrote:
>> On 9/17/2020 10:10 AM, Benjamin Berg wrote:
>>> Hi,
>>>
>>> On Thu, 2020-09-17 at 15:54 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 9/17/20 3:50 PM, Benjamin Berg wrote:
>>>>> On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
>>>>>>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>>>>>>> something to advertise the unavailability of a profile, and the 
>>>>>>> reason
>>>>>>> for that unavailability.
>>>>>>
>>>>>> UGh, do we really need to export this though. We have the lap_mode 
>>>>>> thing
>>>>>> already; and that is something which we will need for other 
>>>>>> reasons in
>>>>>> the future too. Any UI for selecting performance modes can display a
>>>>>> warning when lap_mode is true saying that: "The laptop has 
>>>>>> detected that it
>>>>>> is sitting on someone's lap and that performance may be limited
>>>>>> because of this." (feel free to improve the text).
>>>>>
>>>>> Well, for dytc_perfmode there are actually always the three states
>>>>> L/M/H. It just happens that the kernel will write "H*" (was "M*" until
>>>>> yesterday) when the performance mode is degraded due to lap detection.
>>>>>
>>>>> Think of dytc_perfmode as a profile that sets a number of things:
>>>>>    * Thermal Limits
>>>>>    * Fan Behaviour
>>>>>    * possibly more
>>>>>
>>>>> While dytc_lapmode will only enforce a change to the thermal limit.
>>>>> So "performance" (H) is technically a valid mode even when the lap is
>>>>> detected.
>>>>>
>>>>>> I guess we could split the "value" attribute from my reply to 
>>>>>> Benjamin's
>>>>>> email into "configured_value" (rw) and "actual_value" (rw) 
>>>>>> attributes.
>>>>>> If we have the info we might as well export it I guess,.
>>>>>
>>>>> I consider the "*" purely a curtsey to users that read the attribute
>>>>> directly using e.g. cat to help with the interpretation. It 
>>>>> probably is
>>>>> not interesting to userspace applications/daemons.
>>>>
>>>> So if there is a difference between M and H and H* then I think we 
>>>> should
>>>> just do the KISS thing and only have a single value attribute and in 
>>>> the
>>>> new interface handle the H* like H (p-p-d can still check the lap_mode
>>>> attribute to differentiate the 2 if it wants to).
>>>
>>> I guess you are saying to drop "H*" and only have "L"/"M"/"H"? If so,
>>> fine with me, but we probably need that input in reply to
>>>    https://patchwork.kernel.org/patch/11730133/#23618881
>>> then :)
>>>
>>> In principle it could be useful for userspace to know that performance
>>> is or would be dramatically impacted. i.e. when dytc_lapmode is 1, then
>>> you might want to say something like:
>>>
>>>    performance states >= 75 are impacted due to "lapmode"
>>>
>>> But, not sure if a kernel interface for that is useful or whether we
>>> should just put that kind of knowledge into userspace.
>>>
>>> Benjamin
>>>
>> I don't have a strong opinion on this but the kernel driver is already 
>> knowledgeable about the quirks of what does and doesn't work on the 
>> system so it seems like a good place to have that logic.
>>
>> What if we have an API for "configured" and "actual" - and if they 
>> differ userspace knows it should figure out why (likely lapmode, but 
>> if the HW vendor adds a new setting related to "position of sun in the 
>> sky" or "how much money is in your account and can you afford the 
>> electricity bill?" that could be added too....)
> 
> As I understand the problem with the configured and actual 
> value/performance_level ideas is that if I understand things correctly 
> that H* is not the same as M,
> it behaves close-ish to M because of the lower thermal-limits from 
> lapmode, but if I understood Benjamin correctly is is not exactly the 
> same, so if we were
> to advertise "M" in the actual_performance_level sysfs-attribute then 
> that would not really be correct ?
> 

Just a small clarification - in our case High performance is only for 
deskmode. It drops to Medium in lapmode.
Medium mode is slightly different in power rating between lap and desk 
mode (e.g on X1Carbon 14.5W on lap, 15W on desk). I haven't really 
worried about this in my patch implementation - it's still "medium"

Does that make it better or more confusing?

Mark

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 17:16               ` Mark Pearson
@ 2020-09-17 18:16                 ` Benjamin Berg
  2020-09-21  9:03                   ` Elia Devito
  2020-09-22 10:30                 ` Hans de Goede
  1 sibling, 1 reply; 30+ messages in thread
From: Benjamin Berg @ 2020-09-17 18:16 UTC (permalink / raw)
  To: Mark Pearson, Hans de Goede, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Thu, 2020-09-17 at 13:16 -0400, Mark Pearson wrote:
> > As I understand the problem with the configured and actual 
> > value/performance_level ideas is that if I understand things correctly 
> > that H* is not the same as M,
> > it behaves close-ish to M because of the lower thermal-limits from 
> > lapmode, but if I understood Benjamin correctly is is not exactly the 
> > same, so if we were
> > to advertise "M" in the actual_performance_level sysfs-attribute then 
> > that would not really be correct ?
> 
> Just a small clarification - in our case High performance is only for 
> deskmode. It drops to Medium in lapmode.
> Medium mode is slightly different in power rating between lap and desk 
> mode (e.g on X1Carbon 14.5W on lap, 15W on desk). I haven't really 
> worried about this in my patch implementation - it's still "medium"

Hmm, what I saw on the X1 Carbon 7th Gen was that putting it into
performance mode (H) increases the fan speed. And I believe that this
was also the case while it was in lap mode.

Specifically, I believe I observed that changing dytc_perfmode from M
to H caused the fan to turn on (and vice versa). So while CPU power
limits are very similar, there still seems to be differences in the
active cooling behaviour.

Benjamin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 18:16                 ` Benjamin Berg
@ 2020-09-21  9:03                   ` Elia Devito
  2020-09-22 10:43                     ` Hans de Goede
  2020-09-24 11:48                     ` Benjamin Berg
  0 siblings, 2 replies; 30+ messages in thread
From: Elia Devito @ 2020-09-21  9:03 UTC (permalink / raw)
  To: Mark Pearson, Hans de Goede, Bastien Nocera, Mark Pearson, Benjamin Berg
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi all, sorry for response delay I'm very busy at work this period

A common interface is surely the best solution, especially because it allows 
to standardize the user-space tools and maybe to integrate its with desktop, 
like Bastien is doing with gnome-power-profiles-daemon or like the similar tool
plasma-pstate.

I think we should keep separate performance and thermal profiles thus leaving 
the possibility of setting a thermal profile independently of the performance
profile and vice versa.

Hp implements up to 4 thermal profiles (apparently the same ones that implement
dell), my patch implements the first 3 profiles which are the ones supported by
my hardware.

1. HP Recommended -> fan stay off and start at low~medium speed when necessary
2. Performance    -> fan stay off and start at medium~hight speed when 
necessary
3. Cool           -> fan stay off and start at medium~hight speed when 
necessary
4. Quiet          -> fan should stay off and start at very low speed if 
necessary

for each profile the firmware set also a OEM variable to select DPTF profile
with the adeguate power limit.

combining these profiles with the performance profiles it is possible to obtain
the desired performance according to the needs of the moment

e.g.

For gaming purpose when the CPU and GPU share the thermal budget, in this case 
the best solution is to set thermal profile to performance to maximize the heat 
dissipation and the p-state profile to powersave, in this way during loadings 
the cpu gain a performance boost that allow to reduce loading time, instead, 
during gameplay the cpu performance will be limited in favor of the GPU 
allowing the maximum framerate to be reached.
(feral had to handle it for its gamemode tool: 
https://github.com/FeralInteractive/gamemode/pull/179)

Another opposed particular case could be thermal profile set to quiet and
p-state set to performance, usefull for example to maximizze cpu performance 
in silent ambient room like a library, obviously for CPU-only intesive tasks 
the best solution is to set either thermal and performance profile to 
performance.

Basically there are infinite combinations that can be made to obtain the best 
configuration for each situation, to allow this a common interface should offer
a possibility to:

- Define the list of thermal profiles separately from the performance ones
- Eventually define a list of on/off attributes (useful for lenovo lap_mode?)
- Provide a description of them
- Switching between thermal profiles regardless of the performance profile

A possible solution could be a "slider like" interface for performance level
and a list of thermal profile.

On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
> Elia, Mark, I assume that both of you want to get your patches for this
> upstream sooner, rather then later. But I think we should put them on
> hold until we have an agreement on a shared userspace API for this.
> 

I could maybe update the patch to expose the interface via debugfs like Mark
wants to do with lenovo driver and make update later when a common interface
will be fully defined.

I would prefer the patch to be merged (at lest the init function) because it
fix the thermald behaviour whit default thermal profile on fresh boot.

In the next days I will update the patch and send it in other thread to 
discuss and evaluate a merge in two steps

Best Regards
Elia




^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-17 17:16               ` Mark Pearson
  2020-09-17 18:16                 ` Benjamin Berg
@ 2020-09-22 10:30                 ` Hans de Goede
  2020-09-24  1:44                   ` Mark Pearson
  1 sibling, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-22 10:30 UTC (permalink / raw)
  To: Mark Pearson, Benjamin Berg, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/17/20 7:16 PM, Mark Pearson wrote:
> 
> On 9/17/2020 1:03 PM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 9/17/20 6:58 PM, Mark Pearson wrote:
>>> On 9/17/2020 10:10 AM, Benjamin Berg wrote:
>>>> Hi,
>>>>
>>>> On Thu, 2020-09-17 at 15:54 +0200, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/17/20 3:50 PM, Benjamin Berg wrote:
>>>>>> On Thu, 2020-09-17 at 14:51 +0200, Hans de Goede wrote:
>>>>>>>> Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
>>>>>>>> something to advertise the unavailability of a profile, and the reason
>>>>>>>> for that unavailability.
>>>>>>>
>>>>>>> UGh, do we really need to export this though. We have the lap_mode thing
>>>>>>> already; and that is something which we will need for other reasons in
>>>>>>> the future too. Any UI for selecting performance modes can display a
>>>>>>> warning when lap_mode is true saying that: "The laptop has detected that it
>>>>>>> is sitting on someone's lap and that performance may be limited
>>>>>>> because of this." (feel free to improve the text).
>>>>>>
>>>>>> Well, for dytc_perfmode there are actually always the three states
>>>>>> L/M/H. It just happens that the kernel will write "H*" (was "M*" until
>>>>>> yesterday) when the performance mode is degraded due to lap detection.
>>>>>>
>>>>>> Think of dytc_perfmode as a profile that sets a number of things:
>>>>>>    * Thermal Limits
>>>>>>    * Fan Behaviour
>>>>>>    * possibly more
>>>>>>
>>>>>> While dytc_lapmode will only enforce a change to the thermal limit.
>>>>>> So "performance" (H) is technically a valid mode even when the lap is
>>>>>> detected.
>>>>>>
>>>>>>> I guess we could split the "value" attribute from my reply to Benjamin's
>>>>>>> email into "configured_value" (rw) and "actual_value" (rw) attributes.
>>>>>>> If we have the info we might as well export it I guess,.
>>>>>>
>>>>>> I consider the "*" purely a curtsey to users that read the attribute
>>>>>> directly using e.g. cat to help with the interpretation. It probably is
>>>>>> not interesting to userspace applications/daemons.
>>>>>
>>>>> So if there is a difference between M and H and H* then I think we should
>>>>> just do the KISS thing and only have a single value attribute and in the
>>>>> new interface handle the H* like H (p-p-d can still check the lap_mode
>>>>> attribute to differentiate the 2 if it wants to).
>>>>
>>>> I guess you are saying to drop "H*" and only have "L"/"M"/"H"? If so,
>>>> fine with me, but we probably need that input in reply to
>>>>    https://patchwork.kernel.org/patch/11730133/#23618881
>>>> then :)
>>>>
>>>> In principle it could be useful for userspace to know that performance
>>>> is or would be dramatically impacted. i.e. when dytc_lapmode is 1, then
>>>> you might want to say something like:
>>>>
>>>>    performance states >= 75 are impacted due to "lapmode"
>>>>
>>>> But, not sure if a kernel interface for that is useful or whether we
>>>> should just put that kind of knowledge into userspace.
>>>>
>>>> Benjamin
>>>>
>>> I don't have a strong opinion on this but the kernel driver is already knowledgeable about the quirks of what does and doesn't work on the system so it seems like a good place to have that logic.
>>>
>>> What if we have an API for "configured" and "actual" - and if they differ userspace knows it should figure out why (likely lapmode, but if the HW vendor adds a new setting related to "position of sun in the sky" or "how much money is in your account and can you afford the electricity bill?" that could be added too....)
>>
>> As I understand the problem with the configured and actual value/performance_level ideas is that if I understand things correctly that H* is not the same as M,
>> it behaves close-ish to M because of the lower thermal-limits from lapmode, but if I understood Benjamin correctly is is not exactly the same, so if we were
>> to advertise "M" in the actual_performance_level sysfs-attribute then that would not really be correct ?
>>
> 
> Just a small clarification - in our case High performance is only for deskmode. It drops to Medium in lapmode.
> Medium mode is slightly different in power rating between lap and desk mode (e.g on X1Carbon 14.5W on lap, 15W on desk). I haven't really worried about this in my patch implementation - it's still "medium"
> 
> Does that make it better or more confusing?

I think that calling both medium is fair in this case.

The big question is, do we want to expose that even though the user configured
a performance-profile of high, the user is only getting medium atm because of
reasons?

Note I say "because of reasons" specifically, because things become even more
complicated if we want to spell out the reasons in the sysfs interface too.

I mean high will give different results even when in desk mode, depending
on if there is a cloth on the desk (bad) or if the table is a metal picknick
table full of round holes to drain the rain (allowing more airflow to the
bottom of the laptop) not to mention that the ambient temperature in which
the laptop is used can probably vary from 15 to 35 degrees celcius.

IOW there can be many factors why high may not really lead to high turbo
clocks; or why it leads to higher turbo clocks then normally expected...

I still have the feeling that it would be best to drop the UI requirement
to show being in a degraded performance mode, because the performance
with modern laptops is just very variable and dependent on many factors.

If we drop that UI requirement; then there also is no need to advertise
configured vs actual performance profile in the sysfs interface.

Users who really want to know what is going on will get much more
detailed and useful information when using something like turbostat
(or a UI for that) anyways.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-21  9:03                   ` Elia Devito
@ 2020-09-22 10:43                     ` Hans de Goede
  2020-09-24  2:10                       ` Mark Pearson
  2020-09-24 11:48                     ` Benjamin Berg
  1 sibling, 1 reply; 30+ messages in thread
From: Hans de Goede @ 2020-09-22 10:43 UTC (permalink / raw)
  To: Elia Devito, Mark Pearson, Bastien Nocera, Mark Pearson, Benjamin Berg
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi,

On 9/21/20 11:03 AM, Elia Devito wrote:
> Hi all, sorry for response delay I'm very busy at work this period

No problem.

> A common interface is surely the best solution, especially because it allows
> to standardize the user-space tools and maybe to integrate its with desktop,
> like Bastien is doing with gnome-power-profiles-daemon or like the similar tool
> plasma-pstate.
> 
> I think we should keep separate performance and thermal profiles thus leaving
> the possibility of setting a thermal profile independently of the performance
> profile and vice versa.
> 
> Hp implements up to 4 thermal profiles (apparently the same ones that implement
> dell), my patch implements the first 3 profiles which are the ones supported by
> my hardware.
> 
> 1. HP Recommended -> fan stay off and start at low~medium speed when necessary
> 2. Performance    -> fan stay off and start at medium~hight speed when
> necessary
> 3. Cool           -> fan stay off and start at medium~hight speed when
> necessary
> 4. Quiet          -> fan should stay off and start at very low speed if
> necessary
> 
> for each profile the firmware set also a OEM variable to select DPTF profile
> with the adeguate power limit.
> 
> combining these profiles with the performance profiles it is possible to obtain
> the desired performance according to the needs of the moment
> 
> e.g.
> 
> For gaming purpose when the CPU and GPU share the thermal budget, in this case
> the best solution is to set thermal profile to performance to maximize the heat
> dissipation and the p-state profile to powersave, in this way during loadings
> the cpu gain a performance boost that allow to reduce loading time, instead,
> during gameplay the cpu performance will be limited in favor of the GPU
> allowing the maximum framerate to be reached.
> (feral had to handle it for its gamemode tool:
> https://github.com/FeralInteractive/gamemode/pull/179)
> 
> Another opposed particular case could be thermal profile set to quiet and
> p-state set to performance, usefull for example to maximizze cpu performance
> in silent ambient room like a library, obviously for CPU-only intesive tasks
> the best solution is to set either thermal and performance profile to
> performance.
> 
> Basically there are infinite combinations that can be made to obtain the best
> configuration for each situation, to allow this a common interface should offer
> a possibility to:
> 
> - Define the list of thermal profiles separately from the performance ones
> - Eventually define a list of on/off attributes (useful for lenovo lap_mode?)
> - Provide a description of them
> - Switching between thermal profiles regardless of the performance profile
> 
> A possible solution could be a "slider like" interface for performance level
> and a list of thermal profile.

So I have been thinking about this and performance level and thermal profile
are really inherently couple to each other. Telling the CPU it can use
25W TPD instead of the default 15W, without also ramping up the cooling is
just going to lead to a whole bunch of thermal throttling.

In a desktop machine with a discrete GPU it is sorta easy, in essence you have
a GPU performance profile, controlling GPU TPD/turbo behavior and the GPU
fans too match, and a CPU performance profile which likewise controls
the CPU fan profile too match the CPU performance profile.

With laptops with a discrete GPU things become harder because there is a
single shared cooling mechanism. But there you could simply say that
performance-profile = max(gpu-profile, cpu-profile).

I mean telling the GPU and CPU that they can burn a gazillion watts and
then telling the cooling setup to be as quiet as possible, is clearly
not going to end well.

This all assumes that we have some nice way to tell the hardware about
the 3 separate (gpu / cpu / cooling) profiles we want.

But that is not always the case; and often when using a CPU with
integrated GPU they are all tied together.

So my proposal is to have a :

/sys/class/performance-profile

Underneath we can have one or more entries (performance-profile providers)
each one with a performance_level file on the previously suggested 0-100 scale
and a performance_mappings file listing the supported discrete values on that
scale and some descriptions of those discrete values purely for informational
purposes.

Besides the performance_level and performance_mappings files I would also like
to add a "type" sysfs attribute, which can have 1 of 3 values:
"system", "cpu" and "gpu".

So something like the thinkpad_acpi performance levels will be "system", and
the intel_pstate driver could maybe also register itself here as a "cpu"
type performance-profile provider.

This will allow userspace (if / when it wants to) to do things like put the
CPU in medium/balanced mode while telling the GPU to go full-throttle for when
playing a game which is clearly GPU limited.

This game scenario of course assumes that we then actually will have a
performance-profile driver for both the CPU and the GPU.

Note I'm still just brainstorming here, but I think that having the
0-100 scale + the type thing should cover all the use-cases we want
to cover.

As always feedback or alternative API proposals are very much welcome.

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-22 10:30                 ` Hans de Goede
@ 2020-09-24  1:44                   ` Mark Pearson
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Pearson @ 2020-09-24  1:44 UTC (permalink / raw)
  To: Hans de Goede, Benjamin Berg, Bastien Nocera, Elia Devito, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko



On 2020-09-22 6:30 a.m., Hans de Goede wrote:
> Hi,
> 
<snip>
> 
> The big question is, do we want to expose that even though the user 
> configured
> a performance-profile of high, the user is only getting medium atm 
> because of
> reasons?
> 
> Note I say "because of reasons" specifically, because things become even 
> more
> complicated if we want to spell out the reasons in the sysfs interface too.
> 
> I mean high will give different results even when in desk mode, depending
> on if there is a cloth on the desk (bad) or if the table is a metal 
> picknick
> table full of round holes to drain the rain (allowing more airflow to the
> bottom of the laptop) not to mention that the ambient temperature in which
> the laptop is used can probably vary from 15 to 35 degrees celcius.
> 
> IOW there can be many factors why high may not really lead to high turbo
> clocks; or why it leads to higher turbo clocks then normally expected...
> 
> I still have the feeling that it would be best to drop the UI requirement
> to show being in a degraded performance mode, because the performance
> with modern laptops is just very variable and dependent on many factors.
> 
> If we drop that UI requirement; then there also is no need to advertise
> configured vs actual performance profile in the sysfs interface.
> 
> Users who really want to know what is going on will get much more
> detailed and useful information when using something like turbostat
> (or a UI for that) anyways.
> 
> Regards,
> 
> Hans
> 
Thought about this some and I'm in agreement. My vote is to keep the 
first version simple and see where we go from there based on user feedback.

I'm going to make the lapmode information available via debugfs for 
those users who do want to see it anyway - and this exercise is really 
about the user space controller so I can see how fitting in all these 
extra pieces just makes it awkward.

I had some other thoughts but I think they tag on better to Elia's email 
so I'll save those for there

Mark



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-22 10:43                     ` Hans de Goede
@ 2020-09-24  2:10                       ` Mark Pearson
  2020-09-24  8:21                         ` Hans de Goede
  0 siblings, 1 reply; 30+ messages in thread
From: Mark Pearson @ 2020-09-24  2:10 UTC (permalink / raw)
  To: Hans de Goede, Elia Devito, Bastien Nocera, Mark Pearson, Benjamin Berg
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko



On 2020-09-22 6:43 a.m., Hans de Goede wrote:
> Hi,
> 
> On 9/21/20 11:03 AM, Elia Devito wrote:
>> Hi all, sorry for response delay I'm very busy at work this period
> 
> No problem.
> 
>> A common interface is surely the best solution, especially because it 
>> allows
>> to standardize the user-space tools and maybe to integrate its with 
>> desktop,
>> like Bastien is doing with gnome-power-profiles-daemon or like the 
>> similar tool
>> plasma-pstate.
>>
>> I think we should keep separate performance and thermal profiles thus 
>> leaving
>> the possibility of setting a thermal profile independently of the 
>> performance
>> profile and vice versa.
>>
>> Hp implements up to 4 thermal profiles (apparently the same ones that 
>> implement
>> dell), my patch implements the first 3 profiles which are the ones 
>> supported by
>> my hardware.
>>
>> 1. HP Recommended -> fan stay off and start at low~medium speed when 
>> necessary
>> 2. Performance    -> fan stay off and start at medium~hight speed when
>> necessary
>> 3. Cool           -> fan stay off and start at medium~hight speed when
>> necessary
>> 4. Quiet          -> fan should stay off and start at very low speed if
>> necessary
>>
>> for each profile the firmware set also a OEM variable to select DPTF 
>> profile
>> with the adeguate power limit.
>>
>> combining these profiles with the performance profiles it is possible 
>> to obtain
>> the desired performance according to the needs of the moment
>>
>> e.g.
>>
>> For gaming purpose when the CPU and GPU share the thermal budget, in 
>> this case
>> the best solution is to set thermal profile to performance to maximize 
>> the heat
>> dissipation and the p-state profile to powersave, in this way during 
>> loadings
>> the cpu gain a performance boost that allow to reduce loading time, 
>> instead,
>> during gameplay the cpu performance will be limited in favor of the GPU
>> allowing the maximum framerate to be reached.
>> (feral had to handle it for its gamemode tool:
>> https://github.com/FeralInteractive/gamemode/pull/179)
>>
>> Another opposed particular case could be thermal profile set to quiet and
>> p-state set to performance, usefull for example to maximizze cpu 
>> performance
>> in silent ambient room like a library, obviously for CPU-only intesive 
>> tasks
>> the best solution is to set either thermal and performance profile to
>> performance.
>>
>> Basically there are infinite combinations that can be made to obtain 
>> the best
>> configuration for each situation, to allow this a common interface 
>> should offer
>> a possibility to:
>>
>> - Define the list of thermal profiles separately from the performance 
>> ones
>> - Eventually define a list of on/off attributes (useful for lenovo 
>> lap_mode?)
>> - Provide a description of them
>> - Switching between thermal profiles regardless of the performance 
>> profile
>>
>> A possible solution could be a "slider like" interface for performance 
>> level
>> and a list of thermal profile.
> 
> So I have been thinking about this and performance level and thermal 
> profile
> are really inherently couple to each other. Telling the CPU it can use
> 25W TPD instead of the default 15W, without also ramping up the cooling is
> just going to lead to a whole bunch of thermal throttling.
> 
> In a desktop machine with a discrete GPU it is sorta easy, in essence 
> you have
> a GPU performance profile, controlling GPU TPD/turbo behavior and the GPU
> fans too match, and a CPU performance profile which likewise controls
> the CPU fan profile too match the CPU performance profile.
> 
> With laptops with a discrete GPU things become harder because there is a
> single shared cooling mechanism. But there you could simply say that
> performance-profile = max(gpu-profile, cpu-profile).
> 
> I mean telling the GPU and CPU that they can burn a gazillion watts and
> then telling the cooling setup to be as quiet as possible, is clearly
> not going to end well.
> 
> This all assumes that we have some nice way to tell the hardware about
> the 3 separate (gpu / cpu / cooling) profiles we want.
> 
> But that is not always the case; and often when using a CPU with
> integrated GPU they are all tied together.
> 
> So my proposal is to have a :
> 
> /sys/class/performance-profile
> 
> Underneath we can have one or more entries (performance-profile providers)
> each one with a performance_level file on the previously suggested 0-100 
> scale
> and a performance_mappings file listing the supported discrete values on 
> that
> scale and some descriptions of those discrete values purely for 
> informational
> purposes.
> 
> Besides the performance_level and performance_mappings files I would 
> also like
> to add a "type" sysfs attribute, which can have 1 of 3 values:
> "system", "cpu" and "gpu".
> 
> So something like the thinkpad_acpi performance levels will be "system", 
> and
> the intel_pstate driver could maybe also register itself here as a "cpu"
> type performance-profile provider.
> 
> This will allow userspace (if / when it wants to) to do things like put the
> CPU in medium/balanced mode while telling the GPU to go full-throttle 
> for when
> playing a game which is clearly GPU limited.
> 
> This game scenario of course assumes that we then actually will have a
> performance-profile driver for both the CPU and the GPU.
> 
> Note I'm still just brainstorming here, but I think that having the
> 0-100 scale + the type thing should cover all the use-cases we want
> to cover.
> 
> As always feedback or alternative API proposals are very much welcome.
> 
I was mulling this over and I think the 1 to 100 slider seems awkward - 
too many levels and the fine graduation mapping onto somewhat coarse (3, 
4 or 5) options doesn't seem to fit well for me. I'm struggling to see 
how it fits the tweaking CPU vs GPU vs system - do we end up introducing 
multiple sliders - which gets complicated with all the choices? I'm just 
not convinced yet.

I'm also not sure about being able to tweak everything too - Linux users 
are smart but is it just getting irritating at that point? Power (hah!) 
users have plenty of tweaking tools at their disposal if they want to 
get into change p-states and tweak every little power option; but this 
exercise is aimed at those who want to make simple general changes - 
increase the power because they have a build to complete, drop into 
quiet mode when watching a movie, or a low power setting when they know 
they're not going to be able to charge for a long time. I suspect some 
smart person will end up automating all this but that's by the by.

So as a counter proposal: Have the slider choose between some key modes 
that we think cover the use cases that people would *want*.

I'd keep the mode list small to avoid it being confusing - more could be 
added later if really needed but I'm guessing (and happy to be 
corrected) that the majority of activities would fit into just a few 
boxes reasonably well.

For instance (I think I get the following from the above):
  - low power/Cool
  - Quiet
  - balance/default
  - CPU performance
  - GPU performance

The vendors would just choose their optimal firmware setting for that 
use case depending on what they have available.

Apart from the GPU performance option that maps pretty well for Lenovo 
and I think it maps very nicely for HP. I wanted to squish low power ad 
quiet together but that seemed unfair - they are very similar though :)
I'm struggling to think of other use cases that would really 
matter...thoughts? Is this *too* simple?

Mark






^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-24  2:10                       ` Mark Pearson
@ 2020-09-24  8:21                         ` Hans de Goede
  0 siblings, 0 replies; 30+ messages in thread
From: Hans de Goede @ 2020-09-24  8:21 UTC (permalink / raw)
  To: Mark Pearson, Elia Devito, Bastien Nocera, Mark Pearson, Benjamin Berg
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

Hi Mark,

Thank you very much for your feedback, much appreciated.

On 9/24/20 4:10 AM, Mark Pearson wrote:
> 
> 
> On 2020-09-22 6:43 a.m., Hans de Goede wrote:
>> Hi,
>>
>> On 9/21/20 11:03 AM, Elia Devito wrote:
>>> Hi all, sorry for response delay I'm very busy at work this period
>>
>> No problem.
>>
>>> A common interface is surely the best solution, especially because it allows
>>> to standardize the user-space tools and maybe to integrate its with desktop,
>>> like Bastien is doing with gnome-power-profiles-daemon or like the similar tool
>>> plasma-pstate.
>>>
>>> I think we should keep separate performance and thermal profiles thus leaving
>>> the possibility of setting a thermal profile independently of the performance
>>> profile and vice versa.
>>>
>>> Hp implements up to 4 thermal profiles (apparently the same ones that implement
>>> dell), my patch implements the first 3 profiles which are the ones supported by
>>> my hardware.
>>>
>>> 1. HP Recommended -> fan stay off and start at low~medium speed when necessary
>>> 2. Performance    -> fan stay off and start at medium~hight speed when
>>> necessary
>>> 3. Cool           -> fan stay off and start at medium~hight speed when
>>> necessary
>>> 4. Quiet          -> fan should stay off and start at very low speed if
>>> necessary
>>>
>>> for each profile the firmware set also a OEM variable to select DPTF profile
>>> with the adeguate power limit.
>>>
>>> combining these profiles with the performance profiles it is possible to obtain
>>> the desired performance according to the needs of the moment
>>>
>>> e.g.
>>>
>>> For gaming purpose when the CPU and GPU share the thermal budget, in this case
>>> the best solution is to set thermal profile to performance to maximize the heat
>>> dissipation and the p-state profile to powersave, in this way during loadings
>>> the cpu gain a performance boost that allow to reduce loading time, instead,
>>> during gameplay the cpu performance will be limited in favor of the GPU
>>> allowing the maximum framerate to be reached.
>>> (feral had to handle it for its gamemode tool:
>>> https://github.com/FeralInteractive/gamemode/pull/179)
>>>
>>> Another opposed particular case could be thermal profile set to quiet and
>>> p-state set to performance, usefull for example to maximizze cpu performance
>>> in silent ambient room like a library, obviously for CPU-only intesive tasks
>>> the best solution is to set either thermal and performance profile to
>>> performance.
>>>
>>> Basically there are infinite combinations that can be made to obtain the best
>>> configuration for each situation, to allow this a common interface should offer
>>> a possibility to:
>>>
>>> - Define the list of thermal profiles separately from the performance ones
>>> - Eventually define a list of on/off attributes (useful for lenovo lap_mode?)
>>> - Provide a description of them
>>> - Switching between thermal profiles regardless of the performance profile
>>>
>>> A possible solution could be a "slider like" interface for performance level
>>> and a list of thermal profile.
>>
>> So I have been thinking about this and performance level and thermal profile
>> are really inherently couple to each other. Telling the CPU it can use
>> 25W TPD instead of the default 15W, without also ramping up the cooling is
>> just going to lead to a whole bunch of thermal throttling.
>>
>> In a desktop machine with a discrete GPU it is sorta easy, in essence you have
>> a GPU performance profile, controlling GPU TPD/turbo behavior and the GPU
>> fans too match, and a CPU performance profile which likewise controls
>> the CPU fan profile too match the CPU performance profile.
>>
>> With laptops with a discrete GPU things become harder because there is a
>> single shared cooling mechanism. But there you could simply say that
>> performance-profile = max(gpu-profile, cpu-profile).
>>
>> I mean telling the GPU and CPU that they can burn a gazillion watts and
>> then telling the cooling setup to be as quiet as possible, is clearly
>> not going to end well.
>>
>> This all assumes that we have some nice way to tell the hardware about
>> the 3 separate (gpu / cpu / cooling) profiles we want.
>>
>> But that is not always the case; and often when using a CPU with
>> integrated GPU they are all tied together.
>>
>> So my proposal is to have a :
>>
>> /sys/class/performance-profile
>>
>> Underneath we can have one or more entries (performance-profile providers)
>> each one with a performance_level file on the previously suggested 0-100 scale
>> and a performance_mappings file listing the supported discrete values on that
>> scale and some descriptions of those discrete values purely for informational
>> purposes.
>>
>> Besides the performance_level and performance_mappings files I would also like
>> to add a "type" sysfs attribute, which can have 1 of 3 values:
>> "system", "cpu" and "gpu".
>>
>> So something like the thinkpad_acpi performance levels will be "system", and
>> the intel_pstate driver could maybe also register itself here as a "cpu"
>> type performance-profile provider.
>>
>> This will allow userspace (if / when it wants to) to do things like put the
>> CPU in medium/balanced mode while telling the GPU to go full-throttle for when
>> playing a game which is clearly GPU limited.
>>
>> This game scenario of course assumes that we then actually will have a
>> performance-profile driver for both the CPU and the GPU.
>>
>> Note I'm still just brainstorming here, but I think that having the
>> 0-100 scale + the type thing should cover all the use-cases we want
>> to cover.
>>
>> As always feedback or alternative API proposals are very much welcome.
>>
> I was mulling this over and I think the 1 to 100 slider seems awkward - too many levels and the fine graduation mapping onto somewhat coarse (3, 4 or 5) options doesn't seem to fit well for me. I'm struggling to see how it fits the tweaking CPU vs GPU vs system - do we end up introducing multiple sliders - which gets complicated with all the choices? I'm just not convinced yet.

So one thing to keep in mind here is that this is the kernel <-> userspace
API. It certainly is not the intention for a UI to show this 0-100 scale
to end-users. The most raw version of a UI should display the number
of actually supported levels given by the mappings file, preferably with
the labels from the mappings file visible.

So the idea behind the 0-100 scale, is that lets sat that
we have 2 laptops, which each 4 profiles:

1. Game laptop:    Low power, Balanced, Performance, Ultra Performance
2. Ultra-portable: Ultra Low Power, Low Power, Balanced, Performance

And we have a UI which offers a balanced option, then to configure
the balanced option on laptop 1. we would need to select profile 1 (of 0-3)
and on laptop 2. we would need to select profile 2 (of 0-3)

The idea behind the 0-100 scale is that in that case both would map
there balanced profile to 50 and userspace can just write 50.

Note I'm not 100% sold on this idea myself yet.

> I'm also not sure about being able to tweak everything too - Linux users are smart but is it just getting irritating at that point? Power (hah!) users have plenty of tweaking tools at their disposal if they want to get into change p-states and tweak every little power option; but this exercise is aimed at those who want to make simple general changes - increase the power because they have a build to complete, drop into quiet mode when watching a movie, or a low power setting when they know they're not going to be able to charge for a long time. I suspect some smart person will end up 
> automating all this but that's by the by.

Well for power-users to be able to tweak, the kernel API
needs to expose all the support profiles. Before the 0-100
idea, the idea for the user space API was to use strings
and just support "low-power", "balanced" and "performance" +
optional extra profiles where the extra profiles where
sort of a free-for-all wrt names, etc.

> So as a counter proposal: Have the slider choose between some key modes that we think cover the use cases that people would *want*.
> 
> I'd keep the mode list small to avoid it being confusing - more could be added later if really needed but I'm guessing (and happy to be corrected) that the majority of activities would fit into just a few boxes reasonably well.
> 
> For instance (I think I get the following from the above):
>   - low power/Cool
>   - Quiet
>   - balance/default
>   - CPU performance
>   - GPU performance
 >
 > The vendors would just choose their optimal firmware setting for that use case depending on what they have available.

 From a UI perspective this might very well work, but chances are
that for GPU performance we would need to:

a) Tell the EC to go for maximum cooling / aggressive ramp up the fans
b) Tell the CPU (e.g. pstate, TPD) to go for a balanced profile
c) Tell the GPU, (e.g. the nouveau or radeon drivers) to aggressively
turbo and maybe if it has a configurable TPD give it some more TPD

And we very likely need to talk to multiple drivers for this,
at least some firmware driver and the GPU driver.

Which is where my type attribute idea comes from. I know that for this
discussion started with controlling firmware (DPTF oem vars / EC)
performance-profile settings, but at some point we probably also need
to e.g. bring in controlling GPU turbo performance profiles.

> Apart from the GPU performance option that maps pretty well for Lenovo and I think it maps very nicely for HP. I wanted to squish low power ad quiet together but that seemed unfair - they are very similar though :)
> I'm struggling to think of other use cases that would really matter...thoughts? Is this *too* simple?

Well this looks a lot like what we started with and I think it is also
closer to what Bastien would prefer as an userspace API for this.

So I guess that would bring us full circle to the initial proposal
where we have a bunch of strings describing profiles, but too avoid
a free-for-all we pre-define a bunch of standard profile names and
we say in the specification that where-ever possible drivers should
map there avaivable profiles to the pre-defined profile names ?

Bastien, have I understood you right that having a list of
pre-defined profile-names and then using strings for the sysfs
API would be your preferred API for this ?

I do still think we should probably add a "type" attribute, which
for thinkpad_acpi and for hp-wmi would probably just be hardcoded
to "system".

I think that having the notion of a separate performance profile for at
least the GPU makes sense, not because of that is the UI which we want
to show to end users, but because the GPU's turbo behavior will likely
be controlled by a completely different driver. AFAIK currently different
GPU drivers have different knobs for this, preparing the
performance-profile API to allow the drivers to offer a unified
(and probably dumbed down a bit) API for this is something which we
should at least keep in mind.

I guess that for GPU drivers we could also have a "custom"
performance-profile, so when users use advanced vendor/driver specific
tools to tweak the GPU performance, then reading back the currently
selected profile would report custom; and writing one of the default
profiles would override the users/tools tweaks and go back to the
settings belonging to that profile.

Anyways adding a "type" sysfs attribute will allow us to also use
the performance-profile API for e.g. GPUs later, we can also even
define only 1 valid value for it for now ("system"). This way we
can document that tools interacting with the API should at least
check the type and skip performance-profiles with a type they do
not know.

Bastien, any comments on the concept of having a type sysfs attribute?

Regards,

Hans


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [External] Re: RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles
  2020-09-21  9:03                   ` Elia Devito
  2020-09-22 10:43                     ` Hans de Goede
@ 2020-09-24 11:48                     ` Benjamin Berg
  1 sibling, 0 replies; 30+ messages in thread
From: Benjamin Berg @ 2020-09-24 11:48 UTC (permalink / raw)
  To: Elia Devito, Mark Pearson, Hans de Goede, Bastien Nocera, Mark Pearson
  Cc: Jared Dominguez, platform-driver-x86, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 3858 bytes --]

On Mon, 2020-09-21 at 11:03 +0200, Elia Devito wrote:
> [SNIP]
> 
> For gaming purpose when the CPU and GPU share the thermal budget, in this case 
> the best solution is to set thermal profile to performance to maximize the heat 
> dissipation and the p-state profile to powersave, in this way during loadings 
> the cpu gain a performance boost that allow to reduce loading time, instead, 
> during gameplay the cpu performance will be limited in favor of the GPU 
> allowing the maximum framerate to be reached.
> (feral had to handle it for its gamemode tool: 
> https://github.com/FeralInteractive/gamemode/pull/179)

So I think that trying to put the CPU into a power-efficient state does
make sense. But, it is also a more generic issue.

It reminds me of the LPC talk
  "Improving CPU energy efficiency during i/o bottlenecks"
  https://youtu.be/hJa3YMgEu3M?t=8852
The point is, that what we really want here is to run in the most power
efficient state that gets the job done reliably[1]. It doesn't matter
which exact state it is, all that matters is that applications are
happy with regard to the latencies they see.

In other words, I think the problem here really is power-optimizing
long running tasks that wake up regularly but don't need that much CPU
time overall.

> Another opposed particular case could be thermal profile set to quiet and
> p-state set to performance, usefull for example to maximizze cpu performance 
> in silent ambient room like a library, obviously for CPU-only intesive tasks 
> the best solution is to set either thermal and performance profile to 
> performance.

Yeah, I have been wondering about that. If you need to meet a certain
power budget (due to thermal limits), then you want to first increase
the power efficiency before doing idle injection (i.e. intel_pstate vs.
intel_powerclamp). But, it seems to me that RAPL already achieves the
best possible behaviour without any need for further interaction. So
setting "performance" in the library should result in a high peak
performance if you have a "bursty" load while also giving optimal
results for long running tasks.

Benjamin

[1] Of course, there are more complexities as to which job is actually
relevant. It would for example make sense to inhibit/slow down
background tasks to prevent them from cutting into the power budget of
the running game. It isn't an easy problem, but should be solvable
using cgroups.

> Basically there are infinite combinations that can be made to obtain the best 
> configuration for each situation, to allow this a common interface should offer
> a possibility to:
> 
> - Define the list of thermal profiles separately from the performance ones
> - Eventually define a list of on/off attributes (useful for lenovo lap_mode?)
> - Provide a description of them
> - Switching between thermal profiles regardless of the performance profile
> 
> A possible solution could be a "slider like" interface for performance level
> and a list of thermal profile.
> 
> On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
> > Elia, Mark, I assume that both of you want to get your patches for this
> > upstream sooner, rather then later. But I think we should put them on
> > hold until we have an agreement on a shared userspace API for this.
> > 
> 
> I could maybe update the patch to expose the interface via debugfs like Mark
> wants to do with lenovo driver and make update later when a common interface
> will be fully defined.
> 
> I would prefer the patch to be merged (at lest the init function) because it
> fix the thermald behaviour whit default thermal profile on fresh boot.
> 
> In the next days I will update the patch and send it in other thread to 
> discuss and evaluate a merge in two steps
> 
> Best Regards
> Elia
> 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-09-24 11:48 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 11:22 RFC: offering a standardized (/sys/class) userspace API for selecting system/laptop performance-profiles Hans de Goede
2020-09-17 11:50 ` Bastien Nocera
2020-09-17 12:51   ` Hans de Goede
2020-09-17 13:00     ` Bastien Nocera
2020-09-17 13:50     ` Benjamin Berg
2020-09-17 13:54       ` Hans de Goede
2020-09-17 14:10         ` Benjamin Berg
2020-09-17 16:58           ` [External] " Mark Pearson
2020-09-17 17:03             ` Hans de Goede
2020-09-17 17:16               ` Mark Pearson
2020-09-17 18:16                 ` Benjamin Berg
2020-09-21  9:03                   ` Elia Devito
2020-09-22 10:43                     ` Hans de Goede
2020-09-24  2:10                       ` Mark Pearson
2020-09-24  8:21                         ` Hans de Goede
2020-09-24 11:48                     ` Benjamin Berg
2020-09-22 10:30                 ` Hans de Goede
2020-09-24  1:44                   ` Mark Pearson
2020-09-17 13:36   ` Mark Pearson
2020-09-17 14:04     ` Hans de Goede
2020-09-17 16:51       ` Mark Pearson
2020-09-17 12:22 ` Benjamin Berg
2020-09-17 12:45   ` Hans de Goede
2020-09-17 13:07     ` Bastien Nocera
2020-09-17 13:46       ` Hans de Goede
2020-09-17 13:02 ` Barnabás Pőcze
2020-09-17 13:24   ` Hans de Goede
2020-09-17 13:28     ` Bastien Nocera
2020-09-17 13:51       ` Hans de Goede
2020-09-17 13:59     ` Benjamin Berg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.