| Message ID | 20200914170252.41125-1-eliadevito@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: hp-wmi: add support for thermal policy | expand |
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
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);
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(+)