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