platform/surface: Add platform profile driver

Commit Message

Maximilian Luz Feb. 8, 2021, 7:49 p.m. UTC

Add a driver to provide platform profile support on 5th- and later
generation Microsoft Surface devices with a Surface System Aggregator
Module. On those devices, the platform profile can be used to influence
cooling behavior and power consumption.

For example, the default 'quiet' profile limits fan noise and in turn
sacrifices performance of the discrete GPU found on Surface Books. Its
full performance can only be unlocked on the 'performance' profile.

Signed-off-by: Maximilian Luz <[e-mail]>
---

Note: This patch builds ontop of the

    platform/surface: Add Surface Aggregator device registry

series. While that series is not strictly required for building this
driver, it provides the device against which it loads. So (at the moment
at least) this patch is essentially useless without that series.

---
 MAINTAINERS                                   |   6 +
 drivers/platform/surface/Kconfig              |  27 +++
 drivers/platform/surface/Makefile             |   1 +
 .../surface/surface_platform_profile.c        | 190 ++++++++++++++++++
 4 files changed, 224 insertions(+)
 create mode 100644 drivers/platform/surface/surface_platform_profile.c

Comments

Hans de Goede Feb. 8, 2021, 8:27 p.m. UTC | #1

Hi,

On 2/8/21 8:49 PM, Maximilian Luz wrote:
> Add a driver to provide platform profile support on 5th- and later
> generation Microsoft Surface devices with a Surface System Aggregator
> Module. On those devices, the platform profile can be used to influence
> cooling behavior and power consumption.
> 
> For example, the default 'quiet' profile limits fan noise and in turn
> sacrifices performance of the discrete GPU found on Surface Books. Its
> full performance can only be unlocked on the 'performance' profile.
> 
> Signed-off-by: Maximilian Luz <[e-mail]>
> ---
> 
> Note: This patch builds ontop of the
> 
>     platform/surface: Add Surface Aggregator device registry
> 
> series. While that series is not strictly required for building this
> driver, it provides the device against which it loads. So (at the moment
> at least) this patch is essentially useless without that series.

Oh, another user of the new platform-profile stuff, great, that means
that the time to get this in place was probably well spend :)

A few review remarks inline.

> 
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/platform/surface/Kconfig              |  27 +++
>  drivers/platform/surface/Makefile             |   1 +
>  .../surface/surface_platform_profile.c        | 190 ++++++++++++++++++
>  4 files changed, 224 insertions(+)
>  create mode 100644 drivers/platform/surface/surface_platform_profile.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 000a82f59c76..a08d65f8f0df 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11811,6 +11811,12 @@ L:	[e-mail]
>  S:	Maintained
>  F:	drivers/platform/surface/surface_hotplug.c
>  
> +MICROSOFT SURFACE PLATFORM PROFILE DRIVER
> +M:	Maximilian Luz <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/platform/surface/surface_platform_profile.c
> +
>  MICROSOFT SURFACE PRO 3 BUTTON DRIVER
>  M:	Chen Yu <[e-mail]>
>  L:	[e-mail]
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 1cd37c041710..e12c65909bcc 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -131,6 +131,33 @@ config SURFACE_HOTPLUG
>  	  Select M or Y here, if you want to (fully) support hot-plugging of
>  	  dGPU devices on the Surface Book 2 and/or 3 during D3cold.
>  
> +config SURFACE_PLATFORM_PROFILE
> +	tristate "Surface Platform Profile Driver"
> +	depends on SURFACE_AGGREGATOR_BUS
> +	depends on ACPI_PLATFORM_PROFILE

Not really about this patch, but it seems to me that it would be better
to make ACPI_PLATFORM_PROFILE not user selectable and use select here
and in the other 2 Kconfig bits which have depends on ACPI_PLATFORM_PROFILE ATM.
I would certainly welcome a patch for this.

Note such a patch should probably sit on top of this one, as it will need
some coordination with Rafael to get that upstream.

Although we may need some other changes to drivers/acpi/platform_profile.c
too, see below.

> +	help
> +	  Provides support for the ACPI platform profile on 5th- and later
> +	  generation Microsoft Surface devices.
> +
> +	  More specifically, this driver provides ACPI platform profile support
> +	  on Microsoft Surface devices with a Surface System Aggregator Module
> +	  (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In
> +	  other words, this driver provides platform profile support on the
> +	  Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and
> +	  later. On those devices, the platform profile can significantly
> +	  influence cooling behavior, e.g. setting it to 'quiet' (default) or
> +	  'low-power' can significantly limit performance of the discrete GPU on
> +	  Surface Books, while in turn leading to lower power consumption and/or
> +	  less fan noise.
> +
> +	  Note that this driver currently relies on the Surface Aggregator
> +	  registry (CONFIG_SURFACE_AGGREGATOR_REGISTRY) to provide the device it
> +	  loads against. Thus, without that registry, this module is essentially
> +	  of no use.

I would prefer if you dropped this paragraph and just add a:

depends on SURFACE_AGGREGATOR_REGISTRY

Technically this could be builtin while SURFACE_AGGREGATOR_REGISTRY is a module,
while adding the depends on will disallow that, but I see no reason why someone
would want to make this builtin without also having SURFACE_AGGREGATOR_REGISTRY
builtin.

> +
> +	  Select M or Y here, if you want to include ACPI platform profile
> +	  support on the above mentioned devices.
> +
>  config SURFACE_PRO3_BUTTON
>  	tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
>  	depends on INPUT
> diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
> index 80035ee540bf..99372c427b73 100644
> --- a/drivers/platform/surface/Makefile
> +++ b/drivers/platform/surface/Makefile
> @@ -13,4 +13,5 @@ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV)	+= surface_aggregator_cdev.o
>  obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o
>  obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
>  obj-$(CONFIG_SURFACE_HOTPLUG)		+= surface_hotplug.o
> +obj-$(CONFIG_SURFACE_PLATFORM_PROFILE)	+= surface_platform_profile.o
>  obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> new file mode 100644
> index 000000000000..548ad8af9cf1
> --- /dev/null
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Surface Platform Profile / Performance Mode driver for Surface System
> + * Aggregator Module (thermal subsystem).
> + *
> + * Copyright (C) 2021 Maximilian Luz <[e-mail]>
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_profile.h>
> +#include <linux/types.h>
> +
> +#include <linux/surface_aggregator/device.h>
> +
> +enum ssam_tmp_profile {
> +	SSAM_TMP_PROFILE_NORMAL             = 1,
> +	SSAM_TMP_PROFILE_BATTERY_SAVER      = 2,
> +	SSAM_TMP_PROFILE_BETTER_PERFORMANCE = 3,
> +	SSAM_TMP_PROFILE_BEST_PERFORMANCE   = 4,
> +};
> +
> +struct ssam_tmp_profile_info {
> +	__le32 profile;
> +	__le16 unknown1;
> +	__le16 unknown2;
> +} __packed;
> +
> +struct ssam_tmp_profile_device {
> +	struct ssam_device *sdev;
> +	struct platform_profile_handler handler;
> +};
> +
> +static SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, {
> +	.target_category = SSAM_SSH_TC_TMP,
> +	.command_id      = 0x02,
> +});
> +
> +static SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, {
> +	.target_category = SSAM_SSH_TC_TMP,
> +	.command_id      = 0x03,
> +});
> +
> +static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile *p)
> +{
> +	struct ssam_tmp_profile_info info;
> +	int status;
> +
> +	status = ssam_retry(__ssam_tmp_profile_get, sdev, &info);
> +	if (status < 0)
> +		return status;
> +
> +	*p = le32_to_cpu(info.profile);
> +	return 0;
> +}
> +
> +static int ssam_tmp_profile_set(struct ssam_device *sdev, enum ssam_tmp_profile p)
> +{
> +	__le32 profile_le = cpu_to_le32(p);
> +
> +	return ssam_retry(__ssam_tmp_profile_set, sdev, &profile_le);
> +}
> +
> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
> +{
> +	switch (p) {
> +	case SSAM_TMP_PROFILE_NORMAL:
> +		return PLATFORM_PROFILE_QUIET;
> +
> +	case SSAM_TMP_PROFILE_BATTERY_SAVER:
> +		return PLATFORM_PROFILE_LOW_POWER;
> +
> +	case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
> +		return PLATFORM_PROFILE_BALANCED;
> +
> +	case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
> +		return PLATFORM_PROFILE_PERFORMANCE;
> +
> +	default:
> +		dev_err(&sdev->dev, "invalid performance profile: %d", p);
> +		return -EINVAL;
> +	}
> +}

I'm not sure about the mapping which you have chosen here. I know that at least for
gnome there are plans to make this stuff available in the UI:

https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html

Notice there are only 3 levels in the UI, which will primarily be mapped to:

PLATFORM_PROFILE_LOW_POWER
PLATFORM_PROFILE_BALANCED
PLATFORM_PROFILE_PERFORMANCE

(with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
mostly is something for userspace to worry about).

And the power-profile-daemon will likely restore the last used setting on boot,
meaning with your mapping that it will always switch the profile away from
SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?

So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.

I know the ABI docs say that drivers should try to use existing values, but
this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.

During the discussion the following 2 options were given because some devices
may have more then one balanced profile:

PLATFORM_PROFILE_BALANCED_LOW_POWER:

                balanced-low-power:     Balances between low power consumption
                                        and performance with a slight bias
                                        towards low power

PLATFORM_PROFILE_BALANCED_PERFORMANCE:

                balanced-performance:   Balances between performance and low
                                        power consumption with a slight bias
                                        towards performance

I think it would be better to add 1 or both of these, if we add both
we could e.g. do the following mappings:

SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE

or we could do:

SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE

I'm not sure which is best, I hope you have a better idea of that then me.

I might even be wrong here and NORMAL might really be more about being QUIET
then it really being BALANCED ? In which case the mapping is fine as is.

Regards,

Hans









> +
> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
> +{
> +	switch (p) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		return SSAM_TMP_PROFILE_BATTERY_SAVER;
> +
> +	case PLATFORM_PROFILE_QUIET:
> +		return SSAM_TMP_PROFILE_NORMAL;
> +
> +	case PLATFORM_PROFILE_BALANCED:
> +		return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
> +
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
> +
> +	default:
> +		/* This should have already been caught by platform_profile_store(). */
> +		WARN(true, "unsupported platform profile");
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
> +				     enum platform_profile_option *profile)
> +{
> +	struct ssam_tmp_profile_device *tpd;
> +	enum ssam_tmp_profile tp;
> +	int status;
> +
> +	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> +
> +	status = ssam_tmp_profile_get(tpd->sdev, &tp);
> +	if (status)
> +		return status;
> +
> +	status = convert_ssam_to_profile(tpd->sdev, tp);
> +	if (status < 0)
> +		return status;
> +
> +	*profile = status;
> +	return 0;
> +}
> +
> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
> +				     enum platform_profile_option profile)
> +{
> +	struct ssam_tmp_profile_device *tpd;
> +	int tp;
> +
> +	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> +
> +	tp = convert_profile_to_ssam(tpd->sdev, profile);
> +	if (tp < 0)
> +		return tp;
> +
> +	return ssam_tmp_profile_set(tpd->sdev, tp);
> +}
> +
> +static int surface_platform_profile_probe(struct ssam_device *sdev)
> +{
> +	struct ssam_tmp_profile_device *tpd;
> +
> +	tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
> +	if (!tpd)
> +		return -ENOMEM;
> +
> +	tpd->sdev = sdev;
> +
> +	tpd->handler.profile_get = ssam_platform_profile_get;
> +	tpd->handler.profile_set = ssam_platform_profile_set;
> +
> +	set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
> +	set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
> +	set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
> +
> +	platform_profile_register(&tpd->handler);
> +	return 0;
> +}
> +
> +static void surface_platform_profile_remove(struct ssam_device *sdev)
> +{
> +	platform_profile_remove();
> +}
> +
> +static const struct ssam_device_id ssam_platform_profile_match[] = {
> +	{ SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
> +
> +static struct ssam_device_driver surface_platform_profile = {
> +	.probe = surface_platform_profile_probe,
> +	.remove = surface_platform_profile_remove,
> +	.match_table = ssam_platform_profile_match,
> +	.driver = {
> +		.name = "surface_platform_profile",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +};
> +module_ssam_device_driver(surface_platform_profile);
> +
> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
> +MODULE_LICENSE("GPL");
>

Maximilian Luz Feb. 8, 2021, 9:38 p.m. UTC | #2

On 2/8/21 9:27 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/8/21 8:49 PM, Maximilian Luz wrote:
>> Add a driver to provide platform profile support on 5th- and later
>> generation Microsoft Surface devices with a Surface System Aggregator
>> Module. On those devices, the platform profile can be used to influence
>> cooling behavior and power consumption.
>>
>> For example, the default 'quiet' profile limits fan noise and in turn
>> sacrifices performance of the discrete GPU found on Surface Books. Its
>> full performance can only be unlocked on the 'performance' profile.
>>
>> Signed-off-by: Maximilian Luz <[e-mail]>
>> ---
>>
>> Note: This patch builds ontop of the
>>
>>      platform/surface: Add Surface Aggregator device registry
>>
>> series. While that series is not strictly required for building this
>> driver, it provides the device against which it loads. So (at the moment
>> at least) this patch is essentially useless without that series.
> 
> Oh, another user of the new platform-profile stuff, great, that means
> that the time to get this in place was probably well spend :)
> 
> A few review remarks inline.
> 
>>
>> ---
>>   MAINTAINERS                                   |   6 +
>>   drivers/platform/surface/Kconfig              |  27 +++
>>   drivers/platform/surface/Makefile             |   1 +
>>   .../surface/surface_platform_profile.c        | 190 ++++++++++++++++++
>>   4 files changed, 224 insertions(+)
>>   create mode 100644 drivers/platform/surface/surface_platform_profile.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 000a82f59c76..a08d65f8f0df 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11811,6 +11811,12 @@ L:	[e-mail]
>>   S:	Maintained
>>   F:	drivers/platform/surface/surface_hotplug.c
>>   
>> +MICROSOFT SURFACE PLATFORM PROFILE DRIVER
>> +M:	Maximilian Luz <[e-mail]>
>> +L:	[e-mail]
>> +S:	Maintained
>> +F:	drivers/platform/surface/surface_platform_profile.c
>> +
>>   MICROSOFT SURFACE PRO 3 BUTTON DRIVER
>>   M:	Chen Yu <[e-mail]>
>>   L:	[e-mail]
>> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
>> index 1cd37c041710..e12c65909bcc 100644
>> --- a/drivers/platform/surface/Kconfig
>> +++ b/drivers/platform/surface/Kconfig
>> @@ -131,6 +131,33 @@ config SURFACE_HOTPLUG
>>   	  Select M or Y here, if you want to (fully) support hot-plugging of
>>   	  dGPU devices on the Surface Book 2 and/or 3 during D3cold.
>>   
>> +config SURFACE_PLATFORM_PROFILE
>> +	tristate "Surface Platform Profile Driver"
>> +	depends on SURFACE_AGGREGATOR_BUS
>> +	depends on ACPI_PLATFORM_PROFILE
> 
> Not really about this patch, but it seems to me that it would be better
> to make ACPI_PLATFORM_PROFILE not user selectable and use select here
> and in the other 2 Kconfig bits which have depends on ACPI_PLATFORM_PROFILE ATM.
> I would certainly welcome a patch for this.

That sounds like a good idea.
  
> Note such a patch should probably sit on top of this one, as it will need
> some coordination with Rafael to get that upstream.
> 
> Although we may need some other changes to drivers/acpi/platform_profile.c
> too, see below.
> 
>> +	help
>> +	  Provides support for the ACPI platform profile on 5th- and later
>> +	  generation Microsoft Surface devices.
>> +
>> +	  More specifically, this driver provides ACPI platform profile support
>> +	  on Microsoft Surface devices with a Surface System Aggregator Module
>> +	  (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In
>> +	  other words, this driver provides platform profile support on the
>> +	  Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and
>> +	  later. On those devices, the platform profile can significantly
>> +	  influence cooling behavior, e.g. setting it to 'quiet' (default) or
>> +	  'low-power' can significantly limit performance of the discrete GPU on
>> +	  Surface Books, while in turn leading to lower power consumption and/or
>> +	  less fan noise.
>> +
>> +	  Note that this driver currently relies on the Surface Aggregator
>> +	  registry (CONFIG_SURFACE_AGGREGATOR_REGISTRY) to provide the device it
>> +	  loads against. Thus, without that registry, this module is essentially
>> +	  of no use.
> 
> I would prefer if you dropped this paragraph and just add a:
> 
> depends on SURFACE_AGGREGATOR_REGISTRY
> 
> Technically this could be builtin while SURFACE_AGGREGATOR_REGISTRY is a module,
> while adding the depends on will disallow that, but I see no reason why someone
> would want to make this builtin without also having SURFACE_AGGREGATOR_REGISTRY
> builtin.

That's fair, I'll change that. My hope was that we someday may find a
way to dynamically query devices, but then we'll have to update the
registry anyway.

> 
>> +
>> +	  Select M or Y here, if you want to include ACPI platform profile
>> +	  support on the above mentioned devices.
>> +
>>   config SURFACE_PRO3_BUTTON
>>   	tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
>>   	depends on INPUT
>> diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
>> index 80035ee540bf..99372c427b73 100644
>> --- a/drivers/platform/surface/Makefile
>> +++ b/drivers/platform/surface/Makefile
>> @@ -13,4 +13,5 @@ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV)	+= surface_aggregator_cdev.o
>>   obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o
>>   obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
>>   obj-$(CONFIG_SURFACE_HOTPLUG)		+= surface_hotplug.o
>> +obj-$(CONFIG_SURFACE_PLATFORM_PROFILE)	+= surface_platform_profile.o
>>   obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
>> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
>> new file mode 100644
>> index 000000000000..548ad8af9cf1
>> --- /dev/null
>> +++ b/drivers/platform/surface/surface_platform_profile.c
>> @@ -0,0 +1,190 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Surface Platform Profile / Performance Mode driver for Surface System
>> + * Aggregator Module (thermal subsystem).
>> + *
>> + * Copyright (C) 2021 Maximilian Luz <[e-mail]>
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/surface_aggregator/device.h>
>> +
>> +enum ssam_tmp_profile {
>> +	SSAM_TMP_PROFILE_NORMAL             = 1,
>> +	SSAM_TMP_PROFILE_BATTERY_SAVER      = 2,
>> +	SSAM_TMP_PROFILE_BETTER_PERFORMANCE = 3,
>> +	SSAM_TMP_PROFILE_BEST_PERFORMANCE   = 4,
>> +};
>> +
>> +struct ssam_tmp_profile_info {
>> +	__le32 profile;
>> +	__le16 unknown1;
>> +	__le16 unknown2;
>> +} __packed;
>> +
>> +struct ssam_tmp_profile_device {
>> +	struct ssam_device *sdev;
>> +	struct platform_profile_handler handler;
>> +};
>> +
>> +static SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, {
>> +	.target_category = SSAM_SSH_TC_TMP,
>> +	.command_id      = 0x02,
>> +});
>> +
>> +static SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, {
>> +	.target_category = SSAM_SSH_TC_TMP,
>> +	.command_id      = 0x03,
>> +});
>> +
>> +static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile *p)
>> +{
>> +	struct ssam_tmp_profile_info info;
>> +	int status;
>> +
>> +	status = ssam_retry(__ssam_tmp_profile_get, sdev, &info);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	*p = le32_to_cpu(info.profile);
>> +	return 0;
>> +}
>> +
>> +static int ssam_tmp_profile_set(struct ssam_device *sdev, enum ssam_tmp_profile p)
>> +{
>> +	__le32 profile_le = cpu_to_le32(p);
>> +
>> +	return ssam_retry(__ssam_tmp_profile_set, sdev, &profile_le);
>> +}
>> +
>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>> +{
>> +	switch (p) {
>> +	case SSAM_TMP_PROFILE_NORMAL:
>> +		return PLATFORM_PROFILE_QUIET;
>> +
>> +	case SSAM_TMP_PROFILE_BATTERY_SAVER:
>> +		return PLATFORM_PROFILE_LOW_POWER;
>> +
>> +	case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>> +		return PLATFORM_PROFILE_BALANCED;
>> +
>> +	case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>> +		return PLATFORM_PROFILE_PERFORMANCE;
>> +
>> +	default:
>> +		dev_err(&sdev->dev, "invalid performance profile: %d", p);
>> +		return -EINVAL;
>> +	}
>> +}
> 
> I'm not sure about the mapping which you have chosen here. I know that at least for
> gnome there are plans to make this stuff available in the UI:
> 
> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html

Thanks for those links!
  
> Notice there are only 3 levels in the UI, which will primarily be mapped to:
> 
> PLATFORM_PROFILE_LOW_POWER
> PLATFORM_PROFILE_BALANCED
> PLATFORM_PROFILE_PERFORMANCE
> 
> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
> mostly is something for userspace to worry about).

Interesting, I wasn't aware of that. I was aware of Bastien's work
towards implementing user-space support for this but I hadn't yet looked
at it in detail (e.g. the "fallback to quiet" is new to me).

> And the power-profile-daemon will likely restore the last used setting on boot,
> meaning with your mapping that it will always switch the profile away from
> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?

Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
the EC does. Same difference though.
  
> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
> 
> I know the ABI docs say that drivers should try to use existing values, but
> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
> 
> During the discussion the following 2 options were given because some devices
> may have more then one balanced profile:
> 
> PLATFORM_PROFILE_BALANCED_LOW_POWER:
> 
>                  balanced-low-power:     Balances between low power consumption
>                                          and performance with a slight bias
>                                          towards low power
> 
> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> 
>                  balanced-performance:   Balances between performance and low
>                                          power consumption with a slight bias
>                                          towards performance
> 
> I think it would be better to add 1 or both of these, if we add both
> we could e.g. do the following mappings:
> 
> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
> 
> or we could do:
> 
> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
> 
> I'm not sure which is best, I hope you have a better idea of that then me.
> 
> I might even be wrong here and NORMAL might really be more about being QUIET
> then it really being BALANCED ? In which case the mapping is fine as is.

I can only really speak on the behavior of my Surface Book 2. On that
device, the CPU is passively cooled, but the discrete GPU is actively
cooled, so I can actually only really talk about active cooling behavior
for the dGPU.

On that, at least, the normal (Windows calls this 'recommended') profile
feels like it targets quiet operation. Using the dGPU with that profile
pretty much ensures that the dGPU will be limited in performance by a
thermal limiter (around 75°C to 80°C; at least it feels that way), while
the fan is somewhat audible but definitely not at maximum speed.
Changing the profile to any higher profile (Windows calls those 'better
performance' and 'best performance'), the fan becomes significantly more
audible. I'm not entirely sure if the performance increase can solely be
attributed to cooling though.

As far as I've heard, that behavior seems to be similar on other devices
with fans for CPU cooling, but I can try to get some more feedback on
that.

Based on all of this, I thought that this would most resemble a 'quiet'
profile. But I'd also be fine with your second suggestion. Calling the
last two options 'balanced performance' and 'performance' might be a bit
closer to the Windows naming scheme. It doesn't seem like the normal
profile does much power limiting in terms of actually capping the power
limit of the dGPU, so I think calling this 'balanced' would also make
sense to me, especially in light of Gnome's defaults.

Regards,
Max

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 
> 
> 
>> +
>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>> +{
>> +	switch (p) {
>> +	case PLATFORM_PROFILE_LOW_POWER:
>> +		return SSAM_TMP_PROFILE_BATTERY_SAVER;
>> +
>> +	case PLATFORM_PROFILE_QUIET:
>> +		return SSAM_TMP_PROFILE_NORMAL;
>> +
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>> +
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>> +
>> +	default:
>> +		/* This should have already been caught by platform_profile_store(). */
>> +		WARN(true, "unsupported platform profile");
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>> +				     enum platform_profile_option *profile)
>> +{
>> +	struct ssam_tmp_profile_device *tpd;
>> +	enum ssam_tmp_profile tp;
>> +	int status;
>> +
>> +	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>> +
>> +	status = ssam_tmp_profile_get(tpd->sdev, &tp);
>> +	if (status)
>> +		return status;
>> +
>> +	status = convert_ssam_to_profile(tpd->sdev, tp);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	*profile = status;
>> +	return 0;
>> +}
>> +
>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>> +				     enum platform_profile_option profile)
>> +{
>> +	struct ssam_tmp_profile_device *tpd;
>> +	int tp;
>> +
>> +	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>> +
>> +	tp = convert_profile_to_ssam(tpd->sdev, profile);
>> +	if (tp < 0)
>> +		return tp;
>> +
>> +	return ssam_tmp_profile_set(tpd->sdev, tp);
>> +}
>> +
>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>> +{
>> +	struct ssam_tmp_profile_device *tpd;
>> +
>> +	tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>> +	if (!tpd)
>> +		return -ENOMEM;
>> +
>> +	tpd->sdev = sdev;
>> +
>> +	tpd->handler.profile_get = ssam_platform_profile_get;
>> +	tpd->handler.profile_set = ssam_platform_profile_set;
>> +
>> +	set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>> +	set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>> +	set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>> +	set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>> +
>> +	platform_profile_register(&tpd->handler);
>> +	return 0;
>> +}
>> +
>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>> +{
>> +	platform_profile_remove();
>> +}
>> +
>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>> +	{ SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>> +
>> +static struct ssam_device_driver surface_platform_profile = {
>> +	.probe = surface_platform_profile_probe,
>> +	.remove = surface_platform_profile_remove,
>> +	.match_table = ssam_platform_profile_match,
>> +	.driver = {
>> +		.name = "surface_platform_profile",
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>> +};
>> +module_ssam_device_driver(surface_platform_profile);
>> +
>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>> +MODULE_LICENSE("GPL");
>>
>

Hans de Goede Feb. 11, 2021, 3:56 p.m. UTC | #3

Hi,

On 2/8/21 10:38 PM, Maximilian Luz wrote:
> 
> 
> On 2/8/21 9:27 PM, Hans de Goede wrote:

<snip>

>>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>>> +{
>>> +    switch (p) {
>>> +    case SSAM_TMP_PROFILE_NORMAL:
>>> +        return PLATFORM_PROFILE_QUIET;
>>> +
>>> +    case SSAM_TMP_PROFILE_BATTERY_SAVER:
>>> +        return PLATFORM_PROFILE_LOW_POWER;
>>> +
>>> +    case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>>> +        return PLATFORM_PROFILE_BALANCED;
>>> +
>>> +    case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>>> +        return PLATFORM_PROFILE_PERFORMANCE;
>>> +
>>> +    default:
>>> +        dev_err(&sdev->dev, "invalid performance profile: %d", p);
>>> +        return -EINVAL;
>>> +    }
>>> +}
>>
>> I'm not sure about the mapping which you have chosen here. I know that at least for
>> gnome there are plans to make this stuff available in the UI:
>>
>> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
>> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
> 
> Thanks for those links!
>  
>> Notice there are only 3 levels in the UI, which will primarily be mapped to:
>>
>> PLATFORM_PROFILE_LOW_POWER
>> PLATFORM_PROFILE_BALANCED
>> PLATFORM_PROFILE_PERFORMANCE
>>
>> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
>> mostly is something for userspace to worry about).
> 
> Interesting, I wasn't aware of that. I was aware of Bastien's work
> towards implementing user-space support for this but I hadn't yet looked
> at it in detail (e.g. the "fallback to quiet" is new to me).

Note that the fallback stuff would not apply here, since you do provide
all 3 of low-power, balanced and performance. But the current way gnome
will handle this means that it will be impossible to select "normal" from
the GNOME ui which feels wrong.

>> And the power-profile-daemon will likely restore the last used setting on boot,
>> meaning with your mapping that it will always switch the profile away from
>> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
> 
> Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
> the EC does. Same difference though.
>  
>> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
>> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
>>
>> I know the ABI docs say that drivers should try to use existing values, but
>> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
>>
>> During the discussion the following 2 options were given because some devices
>> may have more then one balanced profile:
>>
>> PLATFORM_PROFILE_BALANCED_LOW_POWER:
>>
>>                  balanced-low-power:     Balances between low power consumption
>>                                          and performance with a slight bias
>>                                          towards low power
>>
>> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>
>>                  balanced-performance:   Balances between performance and low
>>                                          power consumption with a slight bias
>>                                          towards performance
>>
>> I think it would be better to add 1 or both of these, if we add both
>> we could e.g. do the following mappings:
>>
>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>
>> or we could do:
>>
>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>
>> I'm not sure which is best, I hope you have a better idea of that then me.
>>
>> I might even be wrong here and NORMAL might really be more about being QUIET
>> then it really being BALANCED ? In which case the mapping is fine as is.
> 
> I can only really speak on the behavior of my Surface Book 2. On that
> device, the CPU is passively cooled, but the discrete GPU is actively
> cooled, so I can actually only really talk about active cooling behavior
> for the dGPU.
> 
> On that, at least, the normal (Windows calls this 'recommended') profile
> feels like it targets quiet operation. Using the dGPU with that profile
> pretty much ensures that the dGPU will be limited in performance by a
> thermal limiter (around 75°C to 80°C; at least it feels that way), while
> the fan is somewhat audible but definitely not at maximum speed.
> Changing the profile to any higher profile (Windows calls those 'better
> performance' and 'best performance'), the fan becomes significantly more
> audible. I'm not entirely sure if the performance increase can solely be
> attributed to cooling though.
> 
> As far as I've heard, that behavior seems to be similar on other devices
> with fans for CPU cooling, but I can try to get some more feedback on
> that.
> 
> Based on all of this, I thought that this would most resemble a 'quiet'
> profile. But I'd also be fine with your second suggestion. Calling the
> last two options 'balanced performance' and 'performance' might be a bit
> closer to the Windows naming scheme. It doesn't seem like the normal
> profile does much power limiting in terms of actually capping the power
> limit of the dGPU, so I think calling this 'balanced' would also make
> sense to me, especially in light of Gnome's defaults.

Ack.

So that means that this is going to need to have a preparation patch
adding the 2 balanced variants which I mention above. Can you take care
of that in the next version?

And since that prep. patch needs to go through Rafael's PM tree anyways,
maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
and use select on it in the thinkpad_acpi and ideapad_laptop drivers?

Regards,

Hans




>>> +
>>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>>> +{
>>> +    switch (p) {
>>> +    case PLATFORM_PROFILE_LOW_POWER:
>>> +        return SSAM_TMP_PROFILE_BATTERY_SAVER;
>>> +
>>> +    case PLATFORM_PROFILE_QUIET:
>>> +        return SSAM_TMP_PROFILE_NORMAL;
>>> +
>>> +    case PLATFORM_PROFILE_BALANCED:
>>> +        return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>>> +
>>> +    case PLATFORM_PROFILE_PERFORMANCE:
>>> +        return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>>> +
>>> +    default:
>>> +        /* This should have already been caught by platform_profile_store(). */
>>> +        WARN(true, "unsupported platform profile");
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +}
>>> +
>>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>>> +                     enum platform_profile_option *profile)
>>> +{
>>> +    struct ssam_tmp_profile_device *tpd;
>>> +    enum ssam_tmp_profile tp;
>>> +    int status;
>>> +
>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>> +
>>> +    status = ssam_tmp_profile_get(tpd->sdev, &tp);
>>> +    if (status)
>>> +        return status;
>>> +
>>> +    status = convert_ssam_to_profile(tpd->sdev, tp);
>>> +    if (status < 0)
>>> +        return status;
>>> +
>>> +    *profile = status;
>>> +    return 0;
>>> +}
>>> +
>>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>>> +                     enum platform_profile_option profile)
>>> +{
>>> +    struct ssam_tmp_profile_device *tpd;
>>> +    int tp;
>>> +
>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>> +
>>> +    tp = convert_profile_to_ssam(tpd->sdev, profile);
>>> +    if (tp < 0)
>>> +        return tp;
>>> +
>>> +    return ssam_tmp_profile_set(tpd->sdev, tp);
>>> +}
>>> +
>>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>>> +{
>>> +    struct ssam_tmp_profile_device *tpd;
>>> +
>>> +    tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>>> +    if (!tpd)
>>> +        return -ENOMEM;
>>> +
>>> +    tpd->sdev = sdev;
>>> +
>>> +    tpd->handler.profile_get = ssam_platform_profile_get;
>>> +    tpd->handler.profile_set = ssam_platform_profile_set;
>>> +
>>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>>> +    set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>>> +    set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>>> +
>>> +    platform_profile_register(&tpd->handler);
>>> +    return 0;
>>> +}
>>> +
>>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>>> +{
>>> +    platform_profile_remove();
>>> +}
>>> +
>>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>>> +    { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>>> +
>>> +static struct ssam_device_driver surface_platform_profile = {
>>> +    .probe = surface_platform_profile_probe,
>>> +    .remove = surface_platform_profile_remove,
>>> +    .match_table = ssam_platform_profile_match,
>>> +    .driver = {
>>> +        .name = "surface_platform_profile",
>>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>> +    },
>>> +};
>>> +module_ssam_device_driver(surface_platform_profile);
>>> +
>>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>

Maximilian Luz Feb. 11, 2021, 4:17 p.m. UTC | #4

On 2/11/21 4:56 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/8/21 10:38 PM, Maximilian Luz wrote:
>>
>>
>> On 2/8/21 9:27 PM, Hans de Goede wrote:
> 
> <snip>
> 
>>>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>>>> +{
>>>> +    switch (p) {
>>>> +    case SSAM_TMP_PROFILE_NORMAL:
>>>> +        return PLATFORM_PROFILE_QUIET;
>>>> +
>>>> +    case SSAM_TMP_PROFILE_BATTERY_SAVER:
>>>> +        return PLATFORM_PROFILE_LOW_POWER;
>>>> +
>>>> +    case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>>>> +        return PLATFORM_PROFILE_BALANCED;
>>>> +
>>>> +    case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>>>> +        return PLATFORM_PROFILE_PERFORMANCE;
>>>> +
>>>> +    default:
>>>> +        dev_err(&sdev->dev, "invalid performance profile: %d", p);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +}
>>>
>>> I'm not sure about the mapping which you have chosen here. I know that at least for
>>> gnome there are plans to make this stuff available in the UI:
>>>
>>> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
>>> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
>>
>> Thanks for those links!
>>   
>>> Notice there are only 3 levels in the UI, which will primarily be mapped to:
>>>
>>> PLATFORM_PROFILE_LOW_POWER
>>> PLATFORM_PROFILE_BALANCED
>>> PLATFORM_PROFILE_PERFORMANCE
>>>
>>> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
>>> mostly is something for userspace to worry about).
>>
>> Interesting, I wasn't aware of that. I was aware of Bastien's work
>> towards implementing user-space support for this but I hadn't yet looked
>> at it in detail (e.g. the "fallback to quiet" is new to me).
> 
> Note that the fallback stuff would not apply here, since you do provide
> all 3 of low-power, balanced and performance. But the current way gnome
> will handle this means that it will be impossible to select "normal" from
> the GNOME ui which feels wrong.
> 
>>> And the power-profile-daemon will likely restore the last used setting on boot,
>>> meaning with your mapping that it will always switch the profile away from
>>> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
>>
>> Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
>> the EC does. Same difference though.
>>   
>>> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
>>> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
>>>
>>> I know the ABI docs say that drivers should try to use existing values, but
>>> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
>>>
>>> During the discussion the following 2 options were given because some devices
>>> may have more then one balanced profile:
>>>
>>> PLATFORM_PROFILE_BALANCED_LOW_POWER:
>>>
>>>                   balanced-low-power:     Balances between low power consumption
>>>                                           and performance with a slight bias
>>>                                           towards low power
>>>
>>> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>>
>>>                   balanced-performance:   Balances between performance and low
>>>                                           power consumption with a slight bias
>>>                                           towards performance
>>>
>>> I think it would be better to add 1 or both of these, if we add both
>>> we could e.g. do the following mappings:
>>>
>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>
>>> or we could do:
>>>
>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>
>>> I'm not sure which is best, I hope you have a better idea of that then me.
>>>
>>> I might even be wrong here and NORMAL might really be more about being QUIET
>>> then it really being BALANCED ? In which case the mapping is fine as is.
>>
>> I can only really speak on the behavior of my Surface Book 2. On that
>> device, the CPU is passively cooled, but the discrete GPU is actively
>> cooled, so I can actually only really talk about active cooling behavior
>> for the dGPU.
>>
>> On that, at least, the normal (Windows calls this 'recommended') profile
>> feels like it targets quiet operation. Using the dGPU with that profile
>> pretty much ensures that the dGPU will be limited in performance by a
>> thermal limiter (around 75°C to 80°C; at least it feels that way), while
>> the fan is somewhat audible but definitely not at maximum speed.
>> Changing the profile to any higher profile (Windows calls those 'better
>> performance' and 'best performance'), the fan becomes significantly more
>> audible. I'm not entirely sure if the performance increase can solely be
>> attributed to cooling though.
>>
>> As far as I've heard, that behavior seems to be similar on other devices
>> with fans for CPU cooling, but I can try to get some more feedback on
>> that.
>>
>> Based on all of this, I thought that this would most resemble a 'quiet'
>> profile. But I'd also be fine with your second suggestion. Calling the
>> last two options 'balanced performance' and 'performance' might be a bit
>> closer to the Windows naming scheme. It doesn't seem like the normal
>> profile does much power limiting in terms of actually capping the power
>> limit of the dGPU, so I think calling this 'balanced' would also make
>> sense to me, especially in light of Gnome's defaults.
> 
> Ack.
> 
> So that means that this is going to need to have a preparation patch
> adding the 2 balanced variants which I mention above. Can you take care
> of that in the next version?

Sure. Already prepared a patch for the 'balanced-performance' one over at [1].
Just needs some squashing and I can send in an updated series. Do you also want
me to add the 'balanced-low-power' version? I'd have chosen 'balanced' and
'balanced-performance' in the new mapping, so there wouldn't be any driver
right now using that.
  
> And since that prep. patch needs to go through Rafael's PM tree anyways,
> maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
> and use select on it in the thinkpad_acpi and ideapad_laptop drivers?

There's also already one at [1] for that just waiting to be sent :)

[1]: https://github.com/linux-surface/kernel/commits/s/surface-platform-profile/next

Regards,
Max

> Regards,
> 
> Hans
> 
> 
> 
> 
>>>> +
>>>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>>>> +{
>>>> +    switch (p) {
>>>> +    case PLATFORM_PROFILE_LOW_POWER:
>>>> +        return SSAM_TMP_PROFILE_BATTERY_SAVER;
>>>> +
>>>> +    case PLATFORM_PROFILE_QUIET:
>>>> +        return SSAM_TMP_PROFILE_NORMAL;
>>>> +
>>>> +    case PLATFORM_PROFILE_BALANCED:
>>>> +        return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>>>> +
>>>> +    case PLATFORM_PROFILE_PERFORMANCE:
>>>> +        return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>>>> +
>>>> +    default:
>>>> +        /* This should have already been caught by platform_profile_store(). */
>>>> +        WARN(true, "unsupported platform profile");
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>> +}
>>>> +
>>>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>>>> +                     enum platform_profile_option *profile)
>>>> +{
>>>> +    struct ssam_tmp_profile_device *tpd;
>>>> +    enum ssam_tmp_profile tp;
>>>> +    int status;
>>>> +
>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>> +
>>>> +    status = ssam_tmp_profile_get(tpd->sdev, &tp);
>>>> +    if (status)
>>>> +        return status;
>>>> +
>>>> +    status = convert_ssam_to_profile(tpd->sdev, tp);
>>>> +    if (status < 0)
>>>> +        return status;
>>>> +
>>>> +    *profile = status;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>>>> +                     enum platform_profile_option profile)
>>>> +{
>>>> +    struct ssam_tmp_profile_device *tpd;
>>>> +    int tp;
>>>> +
>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>> +
>>>> +    tp = convert_profile_to_ssam(tpd->sdev, profile);
>>>> +    if (tp < 0)
>>>> +        return tp;
>>>> +
>>>> +    return ssam_tmp_profile_set(tpd->sdev, tp);
>>>> +}
>>>> +
>>>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>>>> +{
>>>> +    struct ssam_tmp_profile_device *tpd;
>>>> +
>>>> +    tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>>>> +    if (!tpd)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    tpd->sdev = sdev;
>>>> +
>>>> +    tpd->handler.profile_get = ssam_platform_profile_get;
>>>> +    tpd->handler.profile_set = ssam_platform_profile_set;
>>>> +
>>>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>>>> +    set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>>>> +    set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>>>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>>>> +
>>>> +    platform_profile_register(&tpd->handler);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>>>> +{
>>>> +    platform_profile_remove();
>>>> +}
>>>> +
>>>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>>>> +    { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>>>> +
>>>> +static struct ssam_device_driver surface_platform_profile = {
>>>> +    .probe = surface_platform_profile_probe,
>>>> +    .remove = surface_platform_profile_remove,
>>>> +    .match_table = ssam_platform_profile_match,
>>>> +    .driver = {
>>>> +        .name = "surface_platform_profile",
>>>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>> +    },
>>>> +};
>>>> +module_ssam_device_driver(surface_platform_profile);
>>>> +
>>>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>>> +MODULE_LICENSE("GPL");
>>>>
>>>
>>
>

Hans de Goede Feb. 11, 2021, 4:31 p.m. UTC | #5

Hi,

On 2/11/21 5:17 PM, Maximilian Luz wrote:
> 
> 
> On 2/11/21 4:56 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 2/8/21 10:38 PM, Maximilian Luz wrote:
>>>
>>>
>>> On 2/8/21 9:27 PM, Hans de Goede wrote:
>>
>> <snip>
>>
>>>>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>>>>> +{
>>>>> +    switch (p) {
>>>>> +    case SSAM_TMP_PROFILE_NORMAL:
>>>>> +        return PLATFORM_PROFILE_QUIET;
>>>>> +
>>>>> +    case SSAM_TMP_PROFILE_BATTERY_SAVER:
>>>>> +        return PLATFORM_PROFILE_LOW_POWER;
>>>>> +
>>>>> +    case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>>>>> +        return PLATFORM_PROFILE_BALANCED;
>>>>> +
>>>>> +    case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>>>>> +        return PLATFORM_PROFILE_PERFORMANCE;
>>>>> +
>>>>> +    default:
>>>>> +        dev_err(&sdev->dev, "invalid performance profile: %d", p);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +}
>>>>
>>>> I'm not sure about the mapping which you have chosen here. I know that at least for
>>>> gnome there are plans to make this stuff available in the UI:
>>>>
>>>> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
>>>> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
>>>
>>> Thanks for those links!
>>>  
>>>> Notice there are only 3 levels in the UI, which will primarily be mapped to:
>>>>
>>>> PLATFORM_PROFILE_LOW_POWER
>>>> PLATFORM_PROFILE_BALANCED
>>>> PLATFORM_PROFILE_PERFORMANCE
>>>>
>>>> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
>>>> mostly is something for userspace to worry about).
>>>
>>> Interesting, I wasn't aware of that. I was aware of Bastien's work
>>> towards implementing user-space support for this but I hadn't yet looked
>>> at it in detail (e.g. the "fallback to quiet" is new to me).
>>
>> Note that the fallback stuff would not apply here, since you do provide
>> all 3 of low-power, balanced and performance. But the current way gnome
>> will handle this means that it will be impossible to select "normal" from
>> the GNOME ui which feels wrong.
>>
>>>> And the power-profile-daemon will likely restore the last used setting on boot,
>>>> meaning with your mapping that it will always switch the profile away from
>>>> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
>>>
>>> Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
>>> the EC does. Same difference though.
>>>  
>>>> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
>>>> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
>>>>
>>>> I know the ABI docs say that drivers should try to use existing values, but
>>>> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
>>>>
>>>> During the discussion the following 2 options were given because some devices
>>>> may have more then one balanced profile:
>>>>
>>>> PLATFORM_PROFILE_BALANCED_LOW_POWER:
>>>>
>>>>                   balanced-low-power:     Balances between low power consumption
>>>>                                           and performance with a slight bias
>>>>                                           towards low power
>>>>
>>>> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>>>
>>>>                   balanced-performance:   Balances between performance and low
>>>>                                           power consumption with a slight bias
>>>>                                           towards performance
>>>>
>>>> I think it would be better to add 1 or both of these, if we add both
>>>> we could e.g. do the following mappings:
>>>>
>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>
>>>> or we could do:
>>>>
>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>
>>>> I'm not sure which is best, I hope you have a better idea of that then me.
>>>>
>>>> I might even be wrong here and NORMAL might really be more about being QUIET
>>>> then it really being BALANCED ? In which case the mapping is fine as is.
>>>
>>> I can only really speak on the behavior of my Surface Book 2. On that
>>> device, the CPU is passively cooled, but the discrete GPU is actively
>>> cooled, so I can actually only really talk about active cooling behavior
>>> for the dGPU.
>>>
>>> On that, at least, the normal (Windows calls this 'recommended') profile
>>> feels like it targets quiet operation. Using the dGPU with that profile
>>> pretty much ensures that the dGPU will be limited in performance by a
>>> thermal limiter (around 75°C to 80°C; at least it feels that way), while
>>> the fan is somewhat audible but definitely not at maximum speed.
>>> Changing the profile to any higher profile (Windows calls those 'better
>>> performance' and 'best performance'), the fan becomes significantly more
>>> audible. I'm not entirely sure if the performance increase can solely be
>>> attributed to cooling though.
>>>
>>> As far as I've heard, that behavior seems to be similar on other devices
>>> with fans for CPU cooling, but I can try to get some more feedback on
>>> that.
>>>
>>> Based on all of this, I thought that this would most resemble a 'quiet'
>>> profile. But I'd also be fine with your second suggestion. Calling the
>>> last two options 'balanced performance' and 'performance' might be a bit
>>> closer to the Windows naming scheme. It doesn't seem like the normal
>>> profile does much power limiting in terms of actually capping the power
>>> limit of the dGPU, so I think calling this 'balanced' would also make
>>> sense to me, especially in light of Gnome's defaults.
>>
>> Ack.
>>
>> So that means that this is going to need to have a preparation patch
>> adding the 2 balanced variants which I mention above. Can you take care
>> of that in the next version?
> 
> Sure. Already prepared a patch for the 'balanced-performance' one over at [1].
> Just needs some squashing and I can send in an updated series. Do you also want
> me to add the 'balanced-low-power' version? I'd have chosen 'balanced' and
> 'balanced-performance' in the new mapping, so there wouldn't be any driver
> right now using that.

I see at [1] that for now you've just added 'balanced-performance' that is probably
best, since as you say atm there are no users for 'balanced-low-power' .

>> And since that prep. patch needs to go through Rafael's PM tree anyways,
>> maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
>> and use select on it in the thinkpad_acpi and ideapad_laptop drivers?
> 
> There's also already one at [1] for that just waiting to be sent :)

Nice, thank you!

> [1]: https://github.com/linux-surface/kernel/commits/s/surface-platform-profile/next

The platform-profile bits which you have here all look good to me.

Regards,

Hans





>>>>> +
>>>>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>>>>> +{
>>>>> +    switch (p) {
>>>>> +    case PLATFORM_PROFILE_LOW_POWER:
>>>>> +        return SSAM_TMP_PROFILE_BATTERY_SAVER;
>>>>> +
>>>>> +    case PLATFORM_PROFILE_QUIET:
>>>>> +        return SSAM_TMP_PROFILE_NORMAL;
>>>>> +
>>>>> +    case PLATFORM_PROFILE_BALANCED:
>>>>> +        return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>>>>> +
>>>>> +    case PLATFORM_PROFILE_PERFORMANCE:
>>>>> +        return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>>>>> +
>>>>> +    default:
>>>>> +        /* This should have already been caught by platform_profile_store(). */
>>>>> +        WARN(true, "unsupported platform profile");
>>>>> +        return -EOPNOTSUPP;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>>>>> +                     enum platform_profile_option *profile)
>>>>> +{
>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>> +    enum ssam_tmp_profile tp;
>>>>> +    int status;
>>>>> +
>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>> +
>>>>> +    status = ssam_tmp_profile_get(tpd->sdev, &tp);
>>>>> +    if (status)
>>>>> +        return status;
>>>>> +
>>>>> +    status = convert_ssam_to_profile(tpd->sdev, tp);
>>>>> +    if (status < 0)
>>>>> +        return status;
>>>>> +
>>>>> +    *profile = status;
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>>>>> +                     enum platform_profile_option profile)
>>>>> +{
>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>> +    int tp;
>>>>> +
>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>> +
>>>>> +    tp = convert_profile_to_ssam(tpd->sdev, profile);
>>>>> +    if (tp < 0)
>>>>> +        return tp;
>>>>> +
>>>>> +    return ssam_tmp_profile_set(tpd->sdev, tp);
>>>>> +}
>>>>> +
>>>>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>>>>> +{
>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>> +
>>>>> +    tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>>>>> +    if (!tpd)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    tpd->sdev = sdev;
>>>>> +
>>>>> +    tpd->handler.profile_get = ssam_platform_profile_get;
>>>>> +    tpd->handler.profile_set = ssam_platform_profile_set;
>>>>> +
>>>>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>>>>> +    set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>>>>> +    set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>>>>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>>>>> +
>>>>> +    platform_profile_register(&tpd->handler);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>>>>> +{
>>>>> +    platform_profile_remove();
>>>>> +}
>>>>> +
>>>>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>>>>> +    { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>>>>> +    { },
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>>>>> +
>>>>> +static struct ssam_device_driver surface_platform_profile = {
>>>>> +    .probe = surface_platform_profile_probe,
>>>>> +    .remove = surface_platform_profile_remove,
>>>>> +    .match_table = ssam_platform_profile_match,
>>>>> +    .driver = {
>>>>> +        .name = "surface_platform_profile",
>>>>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>>> +    },
>>>>> +};
>>>>> +module_ssam_device_driver(surface_platform_profile);
>>>>> +
>>>>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>>>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>>>> +MODULE_LICENSE("GPL");
>>>>>
>>>>
>>>
>>
>

Maximilian Luz Feb. 11, 2021, 4:34 p.m. UTC | #6

On 2/11/21 5:31 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/11/21 5:17 PM, Maximilian Luz wrote:
>>
>>
>> On 2/11/21 4:56 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 2/8/21 10:38 PM, Maximilian Luz wrote:
>>>>
>>>>
>>>> On 2/8/21 9:27 PM, Hans de Goede wrote:
>>>
>>> <snip>
>>>
>>>>>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>>>>>> +{
>>>>>> +    switch (p) {
>>>>>> +    case SSAM_TMP_PROFILE_NORMAL:
>>>>>> +        return PLATFORM_PROFILE_QUIET;
>>>>>> +
>>>>>> +    case SSAM_TMP_PROFILE_BATTERY_SAVER:
>>>>>> +        return PLATFORM_PROFILE_LOW_POWER;
>>>>>> +
>>>>>> +    case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>>>>>> +        return PLATFORM_PROFILE_BALANCED;
>>>>>> +
>>>>>> +    case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>>>>>> +        return PLATFORM_PROFILE_PERFORMANCE;
>>>>>> +
>>>>>> +    default:
>>>>>> +        dev_err(&sdev->dev, "invalid performance profile: %d", p);
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +}
>>>>>
>>>>> I'm not sure about the mapping which you have chosen here. I know that at least for
>>>>> gnome there are plans to make this stuff available in the UI:
>>>>>
>>>>> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
>>>>> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
>>>>
>>>> Thanks for those links!
>>>>   
>>>>> Notice there are only 3 levels in the UI, which will primarily be mapped to:
>>>>>
>>>>> PLATFORM_PROFILE_LOW_POWER
>>>>> PLATFORM_PROFILE_BALANCED
>>>>> PLATFORM_PROFILE_PERFORMANCE
>>>>>
>>>>> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
>>>>> mostly is something for userspace to worry about).
>>>>
>>>> Interesting, I wasn't aware of that. I was aware of Bastien's work
>>>> towards implementing user-space support for this but I hadn't yet looked
>>>> at it in detail (e.g. the "fallback to quiet" is new to me).
>>>
>>> Note that the fallback stuff would not apply here, since you do provide
>>> all 3 of low-power, balanced and performance. But the current way gnome
>>> will handle this means that it will be impossible to select "normal" from
>>> the GNOME ui which feels wrong.
>>>
>>>>> And the power-profile-daemon will likely restore the last used setting on boot,
>>>>> meaning with your mapping that it will always switch the profile away from
>>>>> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
>>>>
>>>> Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
>>>> the EC does. Same difference though.
>>>>   
>>>>> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
>>>>> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
>>>>>
>>>>> I know the ABI docs say that drivers should try to use existing values, but
>>>>> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
>>>>>
>>>>> During the discussion the following 2 options were given because some devices
>>>>> may have more then one balanced profile:
>>>>>
>>>>> PLATFORM_PROFILE_BALANCED_LOW_POWER:
>>>>>
>>>>>                    balanced-low-power:     Balances between low power consumption
>>>>>                                            and performance with a slight bias
>>>>>                                            towards low power
>>>>>
>>>>> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>>>>
>>>>>                    balanced-performance:   Balances between performance and low
>>>>>                                            power consumption with a slight bias
>>>>>                                            towards performance
>>>>>
>>>>> I think it would be better to add 1 or both of these, if we add both
>>>>> we could e.g. do the following mappings:
>>>>>
>>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
>>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>>
>>>>> or we could do:
>>>>>
>>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
>>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>>
>>>>> I'm not sure which is best, I hope you have a better idea of that then me.
>>>>>
>>>>> I might even be wrong here and NORMAL might really be more about being QUIET
>>>>> then it really being BALANCED ? In which case the mapping is fine as is.
>>>>
>>>> I can only really speak on the behavior of my Surface Book 2. On that
>>>> device, the CPU is passively cooled, but the discrete GPU is actively
>>>> cooled, so I can actually only really talk about active cooling behavior
>>>> for the dGPU.
>>>>
>>>> On that, at least, the normal (Windows calls this 'recommended') profile
>>>> feels like it targets quiet operation. Using the dGPU with that profile
>>>> pretty much ensures that the dGPU will be limited in performance by a
>>>> thermal limiter (around 75°C to 80°C; at least it feels that way), while
>>>> the fan is somewhat audible but definitely not at maximum speed.
>>>> Changing the profile to any higher profile (Windows calls those 'better
>>>> performance' and 'best performance'), the fan becomes significantly more
>>>> audible. I'm not entirely sure if the performance increase can solely be
>>>> attributed to cooling though.
>>>>
>>>> As far as I've heard, that behavior seems to be similar on other devices
>>>> with fans for CPU cooling, but I can try to get some more feedback on
>>>> that.
>>>>
>>>> Based on all of this, I thought that this would most resemble a 'quiet'
>>>> profile. But I'd also be fine with your second suggestion. Calling the
>>>> last two options 'balanced performance' and 'performance' might be a bit
>>>> closer to the Windows naming scheme. It doesn't seem like the normal
>>>> profile does much power limiting in terms of actually capping the power
>>>> limit of the dGPU, so I think calling this 'balanced' would also make
>>>> sense to me, especially in light of Gnome's defaults.
>>>
>>> Ack.
>>>
>>> So that means that this is going to need to have a preparation patch
>>> adding the 2 balanced variants which I mention above. Can you take care
>>> of that in the next version?
>>
>> Sure. Already prepared a patch for the 'balanced-performance' one over at [1].
>> Just needs some squashing and I can send in an updated series. Do you also want
>> me to add the 'balanced-low-power' version? I'd have chosen 'balanced' and
>> 'balanced-performance' in the new mapping, so there wouldn't be any driver
>> right now using that.
> 
> I see at [1] that for now you've just added 'balanced-performance' that is probably
> best, since as you say atm there are no users for 'balanced-low-power' .

Perfect.

> 
>>> And since that prep. patch needs to go through Rafael's PM tree anyways,
>>> maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
>>> and use select on it in the thinkpad_acpi and ideapad_laptop drivers?
>>
>> There's also already one at [1] for that just waiting to be sent :)
> 
> Nice, thank you!
> 
>> [1]: https://github.com/linux-surface/kernel/commits/s/surface-platform-profile/next
> 
> The platform-profile bits which you have here all look good to me.

Thanks, I'll try to send that and an updated registry series in later today.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>>>>>> +
>>>>>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>>>>>> +{
>>>>>> +    switch (p) {
>>>>>> +    case PLATFORM_PROFILE_LOW_POWER:
>>>>>> +        return SSAM_TMP_PROFILE_BATTERY_SAVER;
>>>>>> +
>>>>>> +    case PLATFORM_PROFILE_QUIET:
>>>>>> +        return SSAM_TMP_PROFILE_NORMAL;
>>>>>> +
>>>>>> +    case PLATFORM_PROFILE_BALANCED:
>>>>>> +        return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>>>>>> +
>>>>>> +    case PLATFORM_PROFILE_PERFORMANCE:
>>>>>> +        return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>>>>>> +
>>>>>> +    default:
>>>>>> +        /* This should have already been caught by platform_profile_store(). */
>>>>>> +        WARN(true, "unsupported platform profile");
>>>>>> +        return -EOPNOTSUPP;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>>>>>> +                     enum platform_profile_option *profile)
>>>>>> +{
>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>> +    enum ssam_tmp_profile tp;
>>>>>> +    int status;
>>>>>> +
>>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>>> +
>>>>>> +    status = ssam_tmp_profile_get(tpd->sdev, &tp);
>>>>>> +    if (status)
>>>>>> +        return status;
>>>>>> +
>>>>>> +    status = convert_ssam_to_profile(tpd->sdev, tp);
>>>>>> +    if (status < 0)
>>>>>> +        return status;
>>>>>> +
>>>>>> +    *profile = status;
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>>>>>> +                     enum platform_profile_option profile)
>>>>>> +{
>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>> +    int tp;
>>>>>> +
>>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>>> +
>>>>>> +    tp = convert_profile_to_ssam(tpd->sdev, profile);
>>>>>> +    if (tp < 0)
>>>>>> +        return tp;
>>>>>> +
>>>>>> +    return ssam_tmp_profile_set(tpd->sdev, tp);
>>>>>> +}
>>>>>> +
>>>>>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>>>>>> +{
>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>> +
>>>>>> +    tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>>>>>> +    if (!tpd)
>>>>>> +        return -ENOMEM;
>>>>>> +
>>>>>> +    tpd->sdev = sdev;
>>>>>> +
>>>>>> +    tpd->handler.profile_get = ssam_platform_profile_get;
>>>>>> +    tpd->handler.profile_set = ssam_platform_profile_set;
>>>>>> +
>>>>>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>>>>>> +    set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>>>>>> +    set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>>>>>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>>>>>> +
>>>>>> +    platform_profile_register(&tpd->handler);
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>>>>>> +{
>>>>>> +    platform_profile_remove();
>>>>>> +}
>>>>>> +
>>>>>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>>>>>> +    { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>>>>>> +    { },
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>>>>>> +
>>>>>> +static struct ssam_device_driver surface_platform_profile = {
>>>>>> +    .probe = surface_platform_profile_probe,
>>>>>> +    .remove = surface_platform_profile_remove,
>>>>>> +    .match_table = ssam_platform_profile_match,
>>>>>> +    .driver = {
>>>>>> +        .name = "surface_platform_profile",
>>>>>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>>>> +    },
>>>>>> +};
>>>>>> +module_ssam_device_driver(surface_platform_profile);
>>>>>> +
>>>>>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>>>>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>>>>> +MODULE_LICENSE("GPL");
>>>>>>
>>>>>
>>>>
>>>
>>
>

Hans de Goede Feb. 11, 2021, 4:36 p.m. UTC | #7

Hi,

On 2/11/21 5:34 PM, Maximilian Luz wrote:
> 
> 
> On 2/11/21 5:31 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 2/11/21 5:17 PM, Maximilian Luz wrote:
>>>
>>>
>>> On 2/11/21 4:56 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 2/8/21 10:38 PM, Maximilian Luz wrote:
>>>>>
>>>>>
>>>>> On 2/8/21 9:27 PM, Hans de Goede wrote:
>>>>
>>>> <snip>
>>>>
>>>>>>> +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>>>>>>> +{
>>>>>>> +    switch (p) {
>>>>>>> +    case SSAM_TMP_PROFILE_NORMAL:
>>>>>>> +        return PLATFORM_PROFILE_QUIET;
>>>>>>> +
>>>>>>> +    case SSAM_TMP_PROFILE_BATTERY_SAVER:
>>>>>>> +        return PLATFORM_PROFILE_LOW_POWER;
>>>>>>> +
>>>>>>> +    case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
>>>>>>> +        return PLATFORM_PROFILE_BALANCED;
>>>>>>> +
>>>>>>> +    case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
>>>>>>> +        return PLATFORM_PROFILE_PERFORMANCE;
>>>>>>> +
>>>>>>> +    default:
>>>>>>> +        dev_err(&sdev->dev, "invalid performance profile: %d", p);
>>>>>>> +        return -EINVAL;
>>>>>>> +    }
>>>>>>> +}
>>>>>>
>>>>>> I'm not sure about the mapping which you have chosen here. I know that at least for
>>>>>> gnome there are plans to make this stuff available in the UI:
>>>>>>
>>>>>> https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
>>>>>> http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
>>>>>
>>>>> Thanks for those links!
>>>>>  
>>>>>> Notice there are only 3 levels in the UI, which will primarily be mapped to:
>>>>>>
>>>>>> PLATFORM_PROFILE_LOW_POWER
>>>>>> PLATFORM_PROFILE_BALANCED
>>>>>> PLATFORM_PROFILE_PERFORMANCE
>>>>>>
>>>>>> (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
>>>>>> mostly is something for userspace to worry about).
>>>>>
>>>>> Interesting, I wasn't aware of that. I was aware of Bastien's work
>>>>> towards implementing user-space support for this but I hadn't yet looked
>>>>> at it in detail (e.g. the "fallback to quiet" is new to me).
>>>>
>>>> Note that the fallback stuff would not apply here, since you do provide
>>>> all 3 of low-power, balanced and performance. But the current way gnome
>>>> will handle this means that it will be impossible to select "normal" from
>>>> the GNOME ui which feels wrong.
>>>>
>>>>>> And the power-profile-daemon will likely restore the last used setting on boot,
>>>>>> meaning with your mapping that it will always switch the profile away from
>>>>>> SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
>>>>>
>>>>> Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
>>>>> the EC does. Same difference though.
>>>>>  
>>>>>> So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
>>>>>> GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
>>>>>>
>>>>>> I know the ABI docs say that drivers should try to use existing values, but
>>>>>> this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
>>>>>>
>>>>>> During the discussion the following 2 options were given because some devices
>>>>>> may have more then one balanced profile:
>>>>>>
>>>>>> PLATFORM_PROFILE_BALANCED_LOW_POWER:
>>>>>>
>>>>>>                    balanced-low-power:     Balances between low power consumption
>>>>>>                                            and performance with a slight bias
>>>>>>                                            towards low power
>>>>>>
>>>>>> PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>>>>>
>>>>>>                    balanced-performance:   Balances between performance and low
>>>>>>                                            power consumption with a slight bias
>>>>>>                                            towards performance
>>>>>>
>>>>>> I think it would be better to add 1 or both of these, if we add both
>>>>>> we could e.g. do the following mappings:
>>>>>>
>>>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED_LOW_POWER
>>>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>>>
>>>>>> or we could do:
>>>>>>
>>>>>> SSAM_TMP_PROFILE_BATTERY_SAVER      ->  PLATFORM_PROFILE_LOW_POWER
>>>>>> SSAM_TMP_PROFILE_NORMAL             ->  PLATFORM_PROFILE_BALANCED
>>>>>> SSAM_TMP_PROFILE_BETTER_PERFORMANCE ->  PLATFORM_PROFILE_BALANCED_PERFORMANCE
>>>>>> SSAM_TMP_PROFILE_BEST_PERFORMANCE   ->  PLATFORM_PROFILE_PERFORMANCE
>>>>>>
>>>>>> I'm not sure which is best, I hope you have a better idea of that then me.
>>>>>>
>>>>>> I might even be wrong here and NORMAL might really be more about being QUIET
>>>>>> then it really being BALANCED ? In which case the mapping is fine as is.
>>>>>
>>>>> I can only really speak on the behavior of my Surface Book 2. On that
>>>>> device, the CPU is passively cooled, but the discrete GPU is actively
>>>>> cooled, so I can actually only really talk about active cooling behavior
>>>>> for the dGPU.
>>>>>
>>>>> On that, at least, the normal (Windows calls this 'recommended') profile
>>>>> feels like it targets quiet operation. Using the dGPU with that profile
>>>>> pretty much ensures that the dGPU will be limited in performance by a
>>>>> thermal limiter (around 75°C to 80°C; at least it feels that way), while
>>>>> the fan is somewhat audible but definitely not at maximum speed.
>>>>> Changing the profile to any higher profile (Windows calls those 'better
>>>>> performance' and 'best performance'), the fan becomes significantly more
>>>>> audible. I'm not entirely sure if the performance increase can solely be
>>>>> attributed to cooling though.
>>>>>
>>>>> As far as I've heard, that behavior seems to be similar on other devices
>>>>> with fans for CPU cooling, but I can try to get some more feedback on
>>>>> that.
>>>>>
>>>>> Based on all of this, I thought that this would most resemble a 'quiet'
>>>>> profile. But I'd also be fine with your second suggestion. Calling the
>>>>> last two options 'balanced performance' and 'performance' might be a bit
>>>>> closer to the Windows naming scheme. It doesn't seem like the normal
>>>>> profile does much power limiting in terms of actually capping the power
>>>>> limit of the dGPU, so I think calling this 'balanced' would also make
>>>>> sense to me, especially in light of Gnome's defaults.
>>>>
>>>> Ack.
>>>>
>>>> So that means that this is going to need to have a preparation patch
>>>> adding the 2 balanced variants which I mention above. Can you take care
>>>> of that in the next version?
>>>
>>> Sure. Already prepared a patch for the 'balanced-performance' one over at [1].
>>> Just needs some squashing and I can send in an updated series. Do you also want
>>> me to add the 'balanced-low-power' version? I'd have chosen 'balanced' and
>>> 'balanced-performance' in the new mapping, so there wouldn't be any driver
>>> right now using that.
>>
>> I see at [1] that for now you've just added 'balanced-performance' that is probably
>> best, since as you say atm there are no users for 'balanced-low-power' .
> 
> Perfect.
> 
>>
>>>> And since that prep. patch needs to go through Rafael's PM tree anyways,
>>>> maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
>>>> and use select on it in the thinkpad_acpi and ideapad_laptop drivers?
>>>
>>> There's also already one at [1] for that just waiting to be sent :)
>>
>> Nice, thank you!
>>
>>> [1]: https://github.com/linux-surface/kernel/commits/s/surface-platform-profile/next
>>
>> The platform-profile bits which you have here all look good to me.
> 
> Thanks, I'll try to send that and an updated registry series in later today.

Sounds good, note please add Rafael to the "To" list on the new platform-profile series
(I guess you would have anyways, but just making sure).

Regards,

Hans



>>>>>>> +
>>>>>>> +static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
>>>>>>> +{
>>>>>>> +    switch (p) {
>>>>>>> +    case PLATFORM_PROFILE_LOW_POWER:
>>>>>>> +        return SSAM_TMP_PROFILE_BATTERY_SAVER;
>>>>>>> +
>>>>>>> +    case PLATFORM_PROFILE_QUIET:
>>>>>>> +        return SSAM_TMP_PROFILE_NORMAL;
>>>>>>> +
>>>>>>> +    case PLATFORM_PROFILE_BALANCED:
>>>>>>> +        return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
>>>>>>> +
>>>>>>> +    case PLATFORM_PROFILE_PERFORMANCE:
>>>>>>> +        return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
>>>>>>> +
>>>>>>> +    default:
>>>>>>> +        /* This should have already been caught by platform_profile_store(). */
>>>>>>> +        WARN(true, "unsupported platform profile");
>>>>>>> +        return -EOPNOTSUPP;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>>>>>>> +                     enum platform_profile_option *profile)
>>>>>>> +{
>>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>>> +    enum ssam_tmp_profile tp;
>>>>>>> +    int status;
>>>>>>> +
>>>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>>>> +
>>>>>>> +    status = ssam_tmp_profile_get(tpd->sdev, &tp);
>>>>>>> +    if (status)
>>>>>>> +        return status;
>>>>>>> +
>>>>>>> +    status = convert_ssam_to_profile(tpd->sdev, tp);
>>>>>>> +    if (status < 0)
>>>>>>> +        return status;
>>>>>>> +
>>>>>>> +    *profile = status;
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>>>>>>> +                     enum platform_profile_option profile)
>>>>>>> +{
>>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>>> +    int tp;
>>>>>>> +
>>>>>>> +    tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
>>>>>>> +
>>>>>>> +    tp = convert_profile_to_ssam(tpd->sdev, profile);
>>>>>>> +    if (tp < 0)
>>>>>>> +        return tp;
>>>>>>> +
>>>>>>> +    return ssam_tmp_profile_set(tpd->sdev, tp);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int surface_platform_profile_probe(struct ssam_device *sdev)
>>>>>>> +{
>>>>>>> +    struct ssam_tmp_profile_device *tpd;
>>>>>>> +
>>>>>>> +    tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>>>>>>> +    if (!tpd)
>>>>>>> +        return -ENOMEM;
>>>>>>> +
>>>>>>> +    tpd->sdev = sdev;
>>>>>>> +
>>>>>>> +    tpd->handler.profile_get = ssam_platform_profile_get;
>>>>>>> +    tpd->handler.profile_set = ssam_platform_profile_set;
>>>>>>> +
>>>>>>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>>>>>>> +    set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
>>>>>>> +    set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>>>>>>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
>>>>>>> +
>>>>>>> +    platform_profile_register(&tpd->handler);
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void surface_platform_profile_remove(struct ssam_device *sdev)
>>>>>>> +{
>>>>>>> +    platform_profile_remove();
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct ssam_device_id ssam_platform_profile_match[] = {
>>>>>>> +    { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
>>>>>>> +    { },
>>>>>>> +};
>>>>>>> +MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
>>>>>>> +
>>>>>>> +static struct ssam_device_driver surface_platform_profile = {
>>>>>>> +    .probe = surface_platform_profile_probe,
>>>>>>> +    .remove = surface_platform_profile_remove,
>>>>>>> +    .match_table = ssam_platform_profile_match,
>>>>>>> +    .driver = {
>>>>>>> +        .name = "surface_platform_profile",
>>>>>>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>>>>> +    },
>>>>>>> +};
>>>>>>> +module_ssam_device_driver(surface_platform_profile);
>>>>>>> +
>>>>>>> +MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
>>>>>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>>>>>> +MODULE_LICENSE("GPL");
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 000a82f59c76..a08d65f8f0df 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11811,6 +11811,12 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/platform/surface/surface_hotplug.c
 
+MICROSOFT SURFACE PLATFORM PROFILE DRIVER
+M:	Maximilian Luz <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/platform/surface/surface_platform_profile.c
+
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
 M:	Chen Yu <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 1cd37c041710..e12c65909bcc 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -131,6 +131,33 @@  config SURFACE_HOTPLUG
 	  Select M or Y here, if you want to (fully) support hot-plugging of
 	  dGPU devices on the Surface Book 2 and/or 3 during D3cold.
 
+config SURFACE_PLATFORM_PROFILE
+	tristate "Surface Platform Profile Driver"
+	depends on SURFACE_AGGREGATOR_BUS
+	depends on ACPI_PLATFORM_PROFILE
+	help
+	  Provides support for the ACPI platform profile on 5th- and later
+	  generation Microsoft Surface devices.
+
+	  More specifically, this driver provides ACPI platform profile support
+	  on Microsoft Surface devices with a Surface System Aggregator Module
+	  (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In
+	  other words, this driver provides platform profile support on the
+	  Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and
+	  later. On those devices, the platform profile can significantly
+	  influence cooling behavior, e.g. setting it to 'quiet' (default) or
+	  'low-power' can significantly limit performance of the discrete GPU on
+	  Surface Books, while in turn leading to lower power consumption and/or
+	  less fan noise.
+
+	  Note that this driver currently relies on the Surface Aggregator
+	  registry (CONFIG_SURFACE_AGGREGATOR_REGISTRY) to provide the device it
+	  loads against. Thus, without that registry, this module is essentially
+	  of no use.
+
+	  Select M or Y here, if you want to include ACPI platform profile
+	  support on the above mentioned devices.
+
 config SURFACE_PRO3_BUTTON
 	tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
 	depends on INPUT
diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
index 80035ee540bf..99372c427b73 100644
--- a/drivers/platform/surface/Makefile
+++ b/drivers/platform/surface/Makefile
@@ -13,4 +13,5 @@  obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV)	+= surface_aggregator_cdev.o
 obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o
 obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
 obj-$(CONFIG_SURFACE_HOTPLUG)		+= surface_hotplug.o
+obj-$(CONFIG_SURFACE_PLATFORM_PROFILE)	+= surface_platform_profile.o
 obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
new file mode 100644
index 000000000000..548ad8af9cf1
--- /dev/null
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -0,0 +1,190 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Surface Platform Profile / Performance Mode driver for Surface System
+ * Aggregator Module (thermal subsystem).
+ *
+ * Copyright (C) 2021 Maximilian Luz <[e-mail]>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_profile.h>
+#include <linux/types.h>
+
+#include <linux/surface_aggregator/device.h>
+
+enum ssam_tmp_profile {
+	SSAM_TMP_PROFILE_NORMAL             = 1,
+	SSAM_TMP_PROFILE_BATTERY_SAVER      = 2,
+	SSAM_TMP_PROFILE_BETTER_PERFORMANCE = 3,
+	SSAM_TMP_PROFILE_BEST_PERFORMANCE   = 4,
+};
+
+struct ssam_tmp_profile_info {
+	__le32 profile;
+	__le16 unknown1;
+	__le16 unknown2;
+} __packed;
+
+struct ssam_tmp_profile_device {
+	struct ssam_device *sdev;
+	struct platform_profile_handler handler;
+};
+
+static SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, {
+	.target_category = SSAM_SSH_TC_TMP,
+	.command_id      = 0x02,
+});
+
+static SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, {
+	.target_category = SSAM_SSH_TC_TMP,
+	.command_id      = 0x03,
+});
+
+static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile *p)
+{
+	struct ssam_tmp_profile_info info;
+	int status;
+
+	status = ssam_retry(__ssam_tmp_profile_get, sdev, &info);
+	if (status < 0)
+		return status;
+
+	*p = le32_to_cpu(info.profile);
+	return 0;
+}
+
+static int ssam_tmp_profile_set(struct ssam_device *sdev, enum ssam_tmp_profile p)
+{
+	__le32 profile_le = cpu_to_le32(p);
+
+	return ssam_retry(__ssam_tmp_profile_set, sdev, &profile_le);
+}
+
+static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
+{
+	switch (p) {
+	case SSAM_TMP_PROFILE_NORMAL:
+		return PLATFORM_PROFILE_QUIET;
+
+	case SSAM_TMP_PROFILE_BATTERY_SAVER:
+		return PLATFORM_PROFILE_LOW_POWER;
+
+	case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
+		return PLATFORM_PROFILE_BALANCED;
+
+	case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
+		return PLATFORM_PROFILE_PERFORMANCE;
+
+	default:
+		dev_err(&sdev->dev, "invalid performance profile: %d", p);
+		return -EINVAL;
+	}
+}
+
+static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
+{
+	switch (p) {
+	case PLATFORM_PROFILE_LOW_POWER:
+		return SSAM_TMP_PROFILE_BATTERY_SAVER;
+
+	case PLATFORM_PROFILE_QUIET:
+		return SSAM_TMP_PROFILE_NORMAL;
+
+	case PLATFORM_PROFILE_BALANCED:
+		return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
+
+	case PLATFORM_PROFILE_PERFORMANCE:
+		return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
+
+	default:
+		/* This should have already been caught by platform_profile_store(). */
+		WARN(true, "unsupported platform profile");
+		return -EOPNOTSUPP;
+	}
+}
+
+static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
+				     enum platform_profile_option *profile)
+{
+	struct ssam_tmp_profile_device *tpd;
+	enum ssam_tmp_profile tp;
+	int status;
+
+	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
+
+	status = ssam_tmp_profile_get(tpd->sdev, &tp);
+	if (status)
+		return status;
+
+	status = convert_ssam_to_profile(tpd->sdev, tp);
+	if (status < 0)
+		return status;
+
+	*profile = status;
+	return 0;
+}
+
+static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
+				     enum platform_profile_option profile)
+{
+	struct ssam_tmp_profile_device *tpd;
+	int tp;
+
+	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
+
+	tp = convert_profile_to_ssam(tpd->sdev, profile);
+	if (tp < 0)
+		return tp;
+
+	return ssam_tmp_profile_set(tpd->sdev, tp);
+}
+
+static int surface_platform_profile_probe(struct ssam_device *sdev)
+{
+	struct ssam_tmp_profile_device *tpd;
+
+	tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
+	if (!tpd)
+		return -ENOMEM;
+
+	tpd->sdev = sdev;
+
+	tpd->handler.profile_get = ssam_platform_profile_get;
+	tpd->handler.profile_set = ssam_platform_profile_set;
+
+	set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
+	set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
+
+	platform_profile_register(&tpd->handler);
+	return 0;
+}
+
+static void surface_platform_profile_remove(struct ssam_device *sdev)
+{
+	platform_profile_remove();
+}
+
+static const struct ssam_device_id ssam_platform_profile_match[] = {
+	{ SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
+	{ },
+};
+MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
+
+static struct ssam_device_driver surface_platform_profile = {
+	.probe = surface_platform_profile_probe,
+	.remove = surface_platform_profile_remove,
+	.match_table = ssam_platform_profile_match,
+	.driver = {
+		.name = "surface_platform_profile",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+module_ssam_device_driver(surface_platform_profile);
+
+MODULE_AUTHOR("Maximilian Luz <[e-mail]>");
+MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
+MODULE_LICENSE("GPL");