[PATCH] platform/x86: hp-wmi: add support for thermal policy

[PATCH] platform/x86: hp-wmi: add support for thermal policy

[Elia Devito]

HP Spectre notebooks (and probabily other model as well)
support at least 3 thermal policy:
 - Default
 - Performance
 - Cool

the correct thermal policy configuration make the firmware to correctly
set OEM variables for Intel DPTF and optimize fan management to reach
the best performance.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
 drivers/platform/x86/hp-wmi.c | 80 +++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 1762f335bac9..14ee176f5588 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -81,6 +81,7 @@ enum hp_wmi_commandtype {
 	HPWMI_FEATURE2_QUERY		= 0x0d,
 	HPWMI_WIRELESS2_QUERY		= 0x1b,
 	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
+	HPWMI_THERMAL_POLICY_QUERY	= 0x4c
 };
 
 enum hp_wmi_command {
@@ -114,6 +115,12 @@ enum hp_wireless2_bits {
 	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
 };
 
+enum hp_thermal_policy {
+	HP_THERMAL_POLICY_PERFORMANCE	= 0x00,
+	HP_THERMAL_POLICY_DEFAULT		= 0x01,
+	HP_THERMAL_POLICY_COOL			= 0x02
+};
+
 #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
 #define IS_SWBLOCKED(x) !(x & HPWMI_POWER_SOFT)
 
@@ -458,6 +465,28 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "0x%x\n", value);
 }
 
+static ssize_t thermal_policy_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	int value;
+
+	/* Get the current thermal policy */
+	value = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
+	if (value < 0)
+		return value;
+
+	switch (value) {
+	case HP_THERMAL_POLICY_PERFORMANCE:
+		return sprintf(buf, "Performance (%x)\n", value);
+	case HP_THERMAL_POLICY_DEFAULT:
+		return sprintf(buf, "Default (%x)\n", value);
+	case HP_THERMAL_POLICY_COOL:
+		return sprintf(buf, "Cool (%x)\n", value);
+	default:
+		return sprintf(buf, "Unknown (%x)\n", value);
+	}
+}
+
 static ssize_t als_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
@@ -499,12 +528,35 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t thermal_policy_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	u32 tmp;
+	int ret;
+
+	ret = kstrtou32(buf, 10, &tmp);
+	if (ret)
+		return ret;
+
+	if (tmp < HP_THERMAL_POLICY_PERFORMANCE || tmp > HP_THERMAL_POLICY_COOL)
+		return -EINVAL;
+
+	/* Set thermal policy */
+	ret = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tmp,
+				       sizeof(tmp), sizeof(tmp));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	return count;
+}
+
 static DEVICE_ATTR_RO(display);
 static DEVICE_ATTR_RO(hddtemp);
 static DEVICE_ATTR_RW(als);
 static DEVICE_ATTR_RO(dock);
 static DEVICE_ATTR_RO(tablet);
 static DEVICE_ATTR_RW(postcode);
+static DEVICE_ATTR_RW(thermal_policy);
 
 static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_display.attr,
@@ -861,6 +913,30 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	return err;
 }
 
+static int thermal_policy_setup(struct platform_device *device)
+{
+	int err, tp;
+
+	tp = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
+	if (tp < 0)
+		return tp;
+
+	/*
+	 * set thermal policy to ensure that the firmware correctly
+	 * sets the OEM variables for the DPTF
+	 */
+	err = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tp,
+							   sizeof(tp), 0);
+	if (err)
+		return err;
+
+	err = device_create_file(&device->dev, &dev_attr_thermal_policy);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int __init hp_wmi_bios_setup(struct platform_device *device)
 {
 	/* clear detected rfkill devices */
@@ -872,6 +948,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 	if (hp_wmi_rfkill_setup(device))
 		hp_wmi_rfkill2_setup(device);
 
+	thermal_policy_setup(device);
+
 	return 0;
 }
 
@@ -879,6 +957,8 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
 {
 	int i;
 
+	device_remove_file(&device->dev, &dev_attr_thermal_policy);
+
 	for (i = 0; i < rfkill2_count; i++) {
 		rfkill_unregister(rfkill2[i].rfkill);
 		rfkill_destroy(rfkill2[i].rfkill);
-- 
2.26.2

[PATCH] platform/x86: hp-wmi: add support for thermal policy

[Elia Devito]

HP Spectre notebooks (and probabily other model as well)
support at least 3 thermal policy:
 - Default
 - Performance
 - Cool

the correct thermal policy configuration make the firmware to correctly
set OEM variables for Intel DPTF and optimize fan management to reach
the best performance.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
 drivers/platform/x86/hp-wmi.c | 80 +++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 1762f335bac9..14ee176f5588 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -81,6 +81,7 @@ enum hp_wmi_commandtype {
 	HPWMI_FEATURE2_QUERY		= 0x0d,
 	HPWMI_WIRELESS2_QUERY		= 0x1b,
 	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
+	HPWMI_THERMAL_POLICY_QUERY	= 0x4c
 };
 
 enum hp_wmi_command {
@@ -114,6 +115,12 @@ enum hp_wireless2_bits {
 	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
 };
 
+enum hp_thermal_policy {
+	HP_THERMAL_POLICY_PERFORMANCE	= 0x00,
+	HP_THERMAL_POLICY_DEFAULT		= 0x01,
+	HP_THERMAL_POLICY_COOL			= 0x02
+};
+
 #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
 #define IS_SWBLOCKED(x) !(x & HPWMI_POWER_SOFT)
 
@@ -458,6 +465,28 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "0x%x\n", value);
 }
 
+static ssize_t thermal_policy_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	int value;
+
+	/* Get the current thermal policy */
+	value = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
+	if (value < 0)
+		return value;
+
+	switch (value) {
+	case HP_THERMAL_POLICY_PERFORMANCE:
+		return sprintf(buf, "Performance (%x)\n", value);
+	case HP_THERMAL_POLICY_DEFAULT:
+		return sprintf(buf, "Default (%x)\n", value);
+	case HP_THERMAL_POLICY_COOL:
+		return sprintf(buf, "Cool (%x)\n", value);
+	default:
+		return sprintf(buf, "Unknown (%x)\n", value);
+	}
+}
+
 static ssize_t als_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
@@ -499,12 +528,35 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t thermal_policy_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	u32 tmp;
+	int ret;
+
+	ret = kstrtou32(buf, 10, &tmp);
+	if (ret)
+		return ret;
+
+	if (tmp < HP_THERMAL_POLICY_PERFORMANCE || tmp > HP_THERMAL_POLICY_COOL)
+		return -EINVAL;
+
+	/* Set thermal policy */
+	ret = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tmp,
+				       sizeof(tmp), sizeof(tmp));
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	return count;
+}
+
 static DEVICE_ATTR_RO(display);
 static DEVICE_ATTR_RO(hddtemp);
 static DEVICE_ATTR_RW(als);
 static DEVICE_ATTR_RO(dock);
 static DEVICE_ATTR_RO(tablet);
 static DEVICE_ATTR_RW(postcode);
+static DEVICE_ATTR_RW(thermal_policy);
 
 static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_display.attr,
@@ -861,6 +913,30 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	return err;
 }
 
+static int thermal_policy_setup(struct platform_device *device)
+{
+	int err, tp;
+
+	tp = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
+	if (tp < 0)
+		return tp;
+
+	/*
+	 * set thermal policy to ensure that the firmware correctly
+	 * sets the OEM variables for the DPTF
+	 */
+	err = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tp,
+							   sizeof(tp), 0);
+	if (err)
+		return err;
+
+	err = device_create_file(&device->dev, &dev_attr_thermal_policy);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int __init hp_wmi_bios_setup(struct platform_device *device)
 {
 	/* clear detected rfkill devices */
@@ -872,6 +948,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 	if (hp_wmi_rfkill_setup(device))
 		hp_wmi_rfkill2_setup(device);
 
+	thermal_policy_setup(device);
+
 	return 0;
 }
 
@@ -879,6 +957,8 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
 {
 	int i;
 
+	device_remove_file(&device->dev, &dev_attr_thermal_policy);
+
 	for (i = 0; i < rfkill2_count; i++) {
 		rfkill_unregister(rfkill2[i].rfkill);
 		rfkill_destroy(rfkill2[i].rfkill);
-- 
2.26.2

Re: [PATCH] platform/x86: hp-wmi: add support for thermal policy

[Hans de Goede]

Hi,

On 9/14/20 7:02 PM, Elia Devito wrote:
> HP Spectre notebooks (and probabily other model as well)
> support at least 3 thermal policy:
>   - Default
>   - Performance
>   - Cool
> 
> the correct thermal policy configuration make the firmware to correctly
> set OEM variables for Intel DPTF and optimize fan management to reach
> the best performance.

You mention DPTF, have you tested this patch together with Matthew Garret's
modified thermald which supports dynamic DPTF profiles ? :

https://mjg59.dreamwidth.org/54923.html

And if you have, have you alsoe tested it without this ?


> 
> Signed-off-by: Elia Devito <eliadevito@gmail.com>
> ---
>   drivers/platform/x86/hp-wmi.c | 80 +++++++++++++++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 1762f335bac9..14ee176f5588 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -81,6 +81,7 @@ enum hp_wmi_commandtype {
>   	HPWMI_FEATURE2_QUERY		= 0x0d,
>   	HPWMI_WIRELESS2_QUERY		= 0x1b,
>   	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> +	HPWMI_THERMAL_POLICY_QUERY	= 0x4c
>   };
>   
>   enum hp_wmi_command {
> @@ -114,6 +115,12 @@ enum hp_wireless2_bits {
>   	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>   };
>   
> +enum hp_thermal_policy {
> +	HP_THERMAL_POLICY_PERFORMANCE	= 0x00,
> +	HP_THERMAL_POLICY_DEFAULT		= 0x01,
> +	HP_THERMAL_POLICY_COOL			= 0x02
> +};
> +
>   #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
>   #define IS_SWBLOCKED(x) !(x & HPWMI_POWER_SOFT)
>   
> @@ -458,6 +465,28 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>   	return sprintf(buf, "0x%x\n", value);
>   }
>   
> +static ssize_t thermal_policy_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	int value;
> +
> +	/* Get the current thermal policy */
> +	value = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
> +	if (value < 0)
> +		return value;
> +
> +	switch (value) {
> +	case HP_THERMAL_POLICY_PERFORMANCE:
> +		return sprintf(buf, "Performance (%x)\n", value);
> +	case HP_THERMAL_POLICY_DEFAULT:
> +		return sprintf(buf, "Default (%x)\n", value);
> +	case HP_THERMAL_POLICY_COOL:
> +		return sprintf(buf, "Cool (%x)\n", value);
> +	default:
> +		return sprintf(buf, "Unknown (%x)\n", value);
> +	}
> +}
> +

So your showing it as a string here.


>   static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>   			 const char *buf, size_t count)
>   {
> @@ -499,12 +528,35 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>   	return count;
>   }
>   
> +static ssize_t thermal_policy_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 tmp;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 10, &tmp);
> +	if (ret)
> +		return ret;


But then taking an integer value here. That is not really a good userspace interface IMHO.

What you can do is put the strings in an array of strings and then loop
over the array in show, adding [] around the selected option, so that
showing it will e.g. output:

Performance [Default] Cool

or:

[Performance] Default Cool

(and in the unknown case none of the 3 would have [] around it)

And then in the store callback also loop over the array,
comparing the user provided string with the 3 strings and
then selecting the value based on that; or return -EINVAL
if non of the strings match. Note I'm open to other
suggestions, but this is more or less how we usually deal with
exporting enums in sysfs now a days.

Note please use sysfs_match_string for the store function,
this will do things like ignoring the '\n' which echo
adds for you without needing to code all this out.



> +
> +	if (tmp < HP_THERMAL_POLICY_PERFORMANCE || tmp > HP_THERMAL_POLICY_COOL)
> +		return -EINVAL;
> +
> +	/* Set thermal policy */
> +	ret = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tmp,
> +				       sizeof(tmp), sizeof(tmp));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	return count;
> +}
> +
>   static DEVICE_ATTR_RO(display);
>   static DEVICE_ATTR_RO(hddtemp);
>   static DEVICE_ATTR_RW(als);
>   static DEVICE_ATTR_RO(dock);
>   static DEVICE_ATTR_RO(tablet);
>   static DEVICE_ATTR_RW(postcode);
> +static DEVICE_ATTR_RW(thermal_policy);
>   
>   static struct attribute *hp_wmi_attrs[] = {
>   	&dev_attr_display.attr,
> @@ -861,6 +913,30 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>   	return err;
>   }
>   
> +static int thermal_policy_setup(struct platform_device *device)
> +{
> +	int err, tp;
> +
> +	tp = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
> +	if (tp < 0)
> +		return tp;
> +
> +	/*
> +	 * set thermal policy to ensure that the firmware correctly
> +	 * sets the OEM variables for the DPTF
> +	 */
> +	err = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tp,
> +							   sizeof(tp), 0);
> +	if (err)
> +		return err;
> +
> +	err = device_create_file(&device->dev, &dev_attr_thermal_policy);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>   static int __init hp_wmi_bios_setup(struct platform_device *device)
>   {
>   	/* clear detected rfkill devices */
> @@ -872,6 +948,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>   	if (hp_wmi_rfkill_setup(device))
>   		hp_wmi_rfkill2_setup(device);
>   
> +	thermal_policy_setup(device);
> +
>   	return 0;
>   }
>   
> @@ -879,6 +957,8 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
>   {
>   	int i;
>   
> +	device_remove_file(&device->dev, &dev_attr_thermal_policy);
> +
>   	for (i = 0; i < rfkill2_count; i++) {
>   		rfkill_unregister(rfkill2[i].rfkill);
>   		rfkill_destroy(rfkill2[i].rfkill);
> 

Otherwise this looks good to me.

Regsrds,

Hans