[PATCH] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[PATCH] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Julien ROBIN]

The following patch adds support for HP Victus 16-s1000 laptop series,
by adding and fixing the following functionalities, which can be
accessed through hwmon and platform_profile sysfs:

  - Functional measured fan speed reading
  - Ability to enable and disable maximum fan speed
  - Platform profiles full setting ability for CPU and GPU

It sets appropriates CPU and GPU power settings both on AC and battery
power sources, for low-power, balanced and performance modes.

It has been thoroughly tested on a 16-s1034nf laptop based on a 8C9C DMI
board name, and behavior of the driver on previous boards is left
untouched thanks to the separated lists of DMI board names.

Signed-off-by: Julien ROBIN <julien.robin28@free.fr>
---
  /drivers/platform/x86/hp/hp-wmi.c | 304 ++++++++++++++++++++++++++++++++-
  1 file changed, 297 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c 
b/drivers/platform/x86/hp/hp-wmi.c
index 81ccc96..9ce2b80 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -83,11 +83,16 @@ static const char * const 
omen_timed_thermal_profile_boards[] = {
  	"8BAD", "8A42"
  };

-/* DMI Board names of Victus laptops */
+/* DMI Board names of Victus 16-d1xxx laptops */
  static const char * const victus_thermal_profile_boards[] = {
  	"8A25"
  };

+/* DMI Board names of Victus 16-s1000 laptops */
+static const char * const victus_s_thermal_profile_boards[] = {
+	"8C9C"
+};
+
  enum hp_wmi_radio {
  	HPWMI_WIFI	= 0x0,
  	HPWMI_BLUETOOTH	= 0x1,
@@ -153,6 +158,11 @@ enum hp_wmi_gm_commandtype {
  	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
  	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
  	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
+	HPWMI_FAN_COUNT_GET_QUERY = 0x10,
+	HPWMI_SET_GPU_THERMAL_MODES_QUERY = 0x22,
+	HPWMI_SET_POWER_LIMITS_QUERY = 0x29,
+	HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY = 0x2D,
+	HPWMI_FAN_SPEED_SET_QUERY = 0x2E,
  };

  enum hp_wmi_command {
@@ -211,6 +221,11 @@ enum hp_thermal_profile_victus {
  	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
  };

+enum hp_thermal_profile_victus_s {
+	HP_VICTUS_S_THERMAL_PROFILE_DEFAULT		= 0x00,
+	HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE		= 0x01,
+};
+
  enum hp_thermal_profile {
  	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
  	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
@@ -411,6 +426,26 @@ out_free:
  	return ret;
  }

+/*
+ * Calling this hp_wmi_get_fan_count_userdefine_trigger function also 
enables
+ * and/or maintains the laptop in user defined thermal and fan states, 
instead
+ * of using a fallback state. After a 120 seconds timeout however, the 
laptop
+ * goes back to its fallback state.
+ */
+static int hp_wmi_get_fan_count_userdefine_trigger(void)
+{
+	char fan_data[4] = { 0 };
+
+	int ret = hp_wmi_perform_query(HPWMI_FAN_COUNT_GET_QUERY, HPWMI_GM,
+				       &fan_data, sizeof(char),
+				       sizeof(fan_data));
+
+	if (ret != 0)
+		return -EINVAL;
+
+	return fan_data[0]; /* Others bytes aren't providing fan count */
+}
+
  static int hp_wmi_get_fan_speed(int fan)
  {
  	u8 fsh, fsl;
@@ -429,6 +464,23 @@ static int hp_wmi_get_fan_speed(int fan)
  	return (fsh << 8) | fsl;
  }

+static int hp_wmi_get_fan_speed_victus_s(int fan)
+{
+	char fan_data[128] = { 0 };
+
+	int ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY,
+				       HPWMI_GM, &fan_data, sizeof(char),
+				       sizeof(fan_data));
+
+	if (ret != 0)
+		return -EINVAL;
+
+	if (fan >= 0 && fan < sizeof(fan_data))
+		return fan_data[fan] * 100;
+	else
+		return -EINVAL;
+}
+
  static int hp_wmi_read_int(int query)
  {
  	int val = 0, ret;
@@ -557,6 +609,29 @@ static int hp_wmi_fan_speed_max_set(int enabled)
  	return enabled;
  }

+static int hp_wmi_fan_speed_reset(void)
+{
+	int ret;
+	char fan_speed[2] = { 0 }; /* Restores automatic speed */
+
+	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_SET_QUERY, HPWMI_GM,
+				   &fan_speed, sizeof(fan_speed), 0);
+
+	return ret;
+}
+
+static int hp_wmi_fan_speed_max_reset(void)
+{
+	int ret = hp_wmi_fan_speed_max_set(0);
+
+	if (ret)
+		return ret;
+
+	/* Disabling Max fan speed on Victus s1xxx laptops needs a 2nd step: */
+	ret = hp_wmi_fan_speed_reset();
+	return ret;
+}
+
  static int hp_wmi_fan_speed_max_get(void)
  {
  	int val = 0, ret;
@@ -1472,6 +1547,127 @@ static int platform_profile_victus_set_ec(enum 
platform_profile_option profile)
  	return 0;
  }

+static bool is_victus_s_thermal_profile(void)
+{
+	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
+
+	if (!board_name)
+		return false;
+
+	return match_string(victus_s_thermal_profile_boards,
+			    ARRAY_SIZE(victus_s_thermal_profile_boards),
+			    board_name) >= 0;
+}
+
+static int victus_s_gpu_thermal_profile_set(bool ctgp_enable,
+					    bool ppab_enable,
+					    char dstate)
+{
+	char gpu_settings[4];
+	int ret;
+
+	gpu_settings[0] = ctgp_enable ? 0x01 : 0x00;
+	gpu_settings[1] = ppab_enable ? 0x01 : 0x00;
+	gpu_settings[2] = dstate;
+	gpu_settings[3] = 0x57; /* it tells we're setting the 3 above values */
+
+	ret = hp_wmi_perform_query(HPWMI_SET_GPU_THERMAL_MODES_QUERY, HPWMI_GM,
+				   &gpu_settings, sizeof(gpu_settings), 0);
+
+	return ret;
+}
+
+static int victus_s_set_cpu_pl1_pl2(char pl1, char pl2)
+{
+	/* Placing 0xFF in the 2 last bytes tells we're setting PL1 and PL2 */
+	char buffer[4] = { pl1, pl2, 0xFF, 0xFF };
+
+	if (pl1 == 0xFF || pl2 == 0xFF)
+		return -EINVAL; /* the 0xFF value has a special meaning */
+
+	if (pl2 < pl1)
+		return -EINVAL; /* PL2 is not supposed to be lower than PL1 */
+
+	/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
+
+	int ret = hp_wmi_perform_query(HPWMI_SET_POWER_LIMITS_QUERY,
+				       HPWMI_GM,
+				       &buffer, sizeof(buffer), 0);
+
+	return ret;
+}
+
+static int platform_profile_victus_s_get(struct 
platform_profile_handler *pprof,
+					 enum platform_profile_option *profile)
+{
+	/* Same behaviour as platform_profile_omen_get */
+	return platform_profile_omen_get(pprof, profile);
+}
+
+static int platform_profile_victus_s_set_ec(enum 
platform_profile_option profile)
+{
+	int err, tp;
+	bool gpu_ctgp_enable, gpu_ppab_enable;
+	char gpu_dstate; /* Test shows 1 = 100%, 2 = 50%, 3 = 25%, 4 = 12.5% */
+
+	switch (profile) {
+	case PLATFORM_PROFILE_PERFORMANCE:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE;
+		gpu_ctgp_enable = true;
+		gpu_ppab_enable = true;
+		gpu_dstate = 1;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
+		gpu_ctgp_enable = false;
+		gpu_ppab_enable = true;
+		gpu_dstate = 1;
+		break;
+	case PLATFORM_PROFILE_LOW_POWER:
+		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
+		gpu_ctgp_enable = false;
+		gpu_ppab_enable = false;
+		gpu_dstate = 1;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	hp_wmi_get_fan_count_userdefine_trigger();
+
+	err = omen_thermal_profile_set(tp);
+	if (err < 0) {
+		pr_err("Failed to set platform profile %d: %d\n", profile, err);
+		return err;
+	}
+
+	err = victus_s_gpu_thermal_profile_set(gpu_ctgp_enable,
+					       gpu_ppab_enable,
+					       gpu_dstate);
+	if (err < 0) {
+		pr_err("Failed to set GPU profile %d: %d\n", profile, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int platform_profile_victus_s_set(struct 
platform_profile_handler *pprof,
+					 enum platform_profile_option profile)
+{
+	int err;
+
+	guard(mutex)(&active_platform_profile_lock);
+
+	err = platform_profile_victus_s_set_ec(profile);
+	if (err < 0)
+		return err;
+
+	active_platform_profile = profile;
+
+	return 0;
+}
+
  static int platform_profile_victus_set(struct platform_profile_handler 
*pprof,
  				       enum platform_profile_option profile)
  {
@@ -1545,6 +1741,38 @@ static int omen_powersource_event(struct 
notifier_block *nb,
  	return NOTIFY_OK;
  }

+static int victus_s_powersource_event(struct notifier_block *nb,
+				      unsigned long value,
+				      void *data)
+{
+	struct acpi_bus_event *event_entry = data;
+	int err;
+
+	if (strcmp(event_entry->device_class, ACPI_AC_CLASS) != 0)
+		return NOTIFY_DONE;
+
+	pr_debug("Received power source device event\n");
+
+	/*
+	 * Switching to battery power source while Performance mode is active
+	 * needs manual triggering of CPU power limits. Same goes when switching
+	 * to AC power source while Performance mode is active. Other modes
+	 * however are automatically behaving without any manual action.
+	 * Seen on HP 16-s1034nf (board 8C9C) with F.11 BIOS version.
+	 */
+
+	if (active_platform_profile == PLATFORM_PROFILE_PERFORMANCE) {
+		pr_debug("Triggering CPU PL1/PL2 actualization\n");
+		err = victus_s_set_cpu_pl1_pl2(0, 0);
+		if (err)
+			pr_warn("Failed to actualize power limits: %d\n", err);
+
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
  static int omen_register_powersource_event_handler(void)
  {
  	int err;
@@ -1560,11 +1788,31 @@ static int 
omen_register_powersource_event_handler(void)
  	return 0;
  }

+static int victus_s_register_powersource_event_handler(void)
+{
+	int err;
+
+	platform_power_source_nb.notifier_call = victus_s_powersource_event;
+	err = register_acpi_notifier(&platform_power_source_nb);
+
+	if (err < 0) {
+		pr_warn("Failed to install ACPI power source notify handler\n");
+		return err;
+	}
+
+	return 0;
+}
+
  static inline void omen_unregister_powersource_event_handler(void)
  {
  	unregister_acpi_notifier(&platform_power_source_nb);
  }

+static inline void victus_s_unregister_powersource_event_handler(void)
+{
+	unregister_acpi_notifier(&platform_power_source_nb);
+}
+
  static int thermal_profile_setup(void)
  {
  	int err, tp;
@@ -1603,6 +1851,22 @@ static int thermal_profile_setup(void)
  		platform_profile_handler.profile_set = platform_profile_victus_set;

  		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
+	} else if (is_victus_s_thermal_profile()) {
+		/*
+		 * Being unable to retrieve laptop's current thermal profile,
+		 * during this setup, we set it to Balanced by default.
+		 */
+		active_platform_profile = PLATFORM_PROFILE_BALANCED;
+
+		err = platform_profile_victus_s_set_ec(active_platform_profile);
+		if (err < 0)
+			return err;
+
+		platform_profile_handler.profile_get = platform_profile_victus_s_get;
+		platform_profile_handler.profile_set = platform_profile_victus_s_set;
+
+		/* Adding an equivalent to HP Omen software ECO mode: */
+		set_bit(PLATFORM_PROFILE_LOW_POWER, platform_profile_handler.choices);
  	} else {
  		tp = thermal_profile_get();

@@ -1628,9 +1892,14 @@ static int thermal_profile_setup(void)
  	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);

  	err = platform_profile_register(&platform_profile_handler);
-	if (err)
+	if (err == -EEXIST) {
+		pr_warn("A platform profile handler is already registered\n");
  		return err;
-
+	} else if (err) {
+		pr_err("Platform profile handler registration fail: %d\n", err);
+		return err;
+	}
+	pr_info("Registered as platform profile handler\n");
  	platform_profile_support = true;

  	return 0;
@@ -1759,8 +2028,13 @@ static umode_t hp_wmi_hwmon_is_visible(const void 
*data,
  	case hwmon_pwm:
  		return 0644;
  	case hwmon_fan:
-		if (hp_wmi_get_fan_speed(channel) >= 0)
-			return 0444;
+		if (is_victus_s_thermal_profile()) {
+			if (hp_wmi_get_fan_speed_victus_s(channel) >= 0)
+				return 0444;
+		} else {
+			if (hp_wmi_get_fan_speed(channel) >= 0)
+				return 0444;
+		}
  		break;
  	default:
  		return 0;
@@ -1776,7 +2050,10 @@ static int hp_wmi_hwmon_read(struct device *dev, 
enum hwmon_sensor_types type,

  	switch (type) {
  	case hwmon_fan:
-		ret = hp_wmi_get_fan_speed(channel);
+		if (is_victus_s_thermal_profile())
+			ret = hp_wmi_get_fan_speed_victus_s(channel);
+		else
+			ret = hp_wmi_get_fan_speed(channel);

  		if (ret < 0)
  			return ret;
@@ -1810,11 +2087,17 @@ static int hp_wmi_hwmon_write(struct device 
*dev, enum hwmon_sensor_types type,
  	case hwmon_pwm:
  		switch (val) {
  		case 0:
+			if (is_victus_s_thermal_profile())
+				hp_wmi_get_fan_count_userdefine_trigger();
  			/* 0 is no fan speed control (max), which is 1 for us */
  			return hp_wmi_fan_speed_max_set(1);
  		case 2:
  			/* 2 is automatic speed control, which is 0 for us */
-			return hp_wmi_fan_speed_max_set(0);
+			if (is_victus_s_thermal_profile()) {
+				hp_wmi_get_fan_count_userdefine_trigger();
+				return hp_wmi_fan_speed_max_reset();
+			} else
+				return hp_wmi_fan_speed_max_set(0);
  		default:
  			/* we don't support manual fan speed control */
  			return -EINVAL;
@@ -1893,6 +2176,10 @@ static int __init hp_wmi_init(void)
  		err = omen_register_powersource_event_handler();
  		if (err)
  			goto err_unregister_device;
+	} else if (is_victus_s_thermal_profile()) {
+		err = victus_s_register_powersource_event_handler();
+		if (err)
+			goto err_unregister_device;
  	}

  	return 0;
@@ -1912,6 +2199,9 @@ static void __exit hp_wmi_exit(void)
  	if (is_omen_thermal_profile() || is_victus_thermal_profile())
  		omen_unregister_powersource_event_handler();

+	if (is_victus_s_thermal_profile())
+		victus_s_unregister_powersource_event_handler();
+
  	if (wmi_has_guid(HPWMI_EVENT_GUID))
  		hp_wmi_input_destroy();

Re: [PATCH] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Ilpo Järvinen]

On Wed, 18 Dec 2024, Julien ROBIN wrote:

> The following patch adds support for HP Victus 16-s1000 laptop series,
> by adding and fixing the following functionalities, which can be
> accessed through hwmon and platform_profile sysfs:
> 
>  - Functional measured fan speed reading
>  - Ability to enable and disable maximum fan speed
>  - Platform profiles full setting ability for CPU and GPU
> 
> It sets appropriates CPU and GPU power settings both on AC and battery
> power sources, for low-power, balanced and performance modes.
> 
> It has been thoroughly tested on a 16-s1034nf laptop based on a 8C9C DMI
> board name, and behavior of the driver on previous boards is left
> untouched thanks to the separated lists of DMI board names.
> 
> Signed-off-by: Julien ROBIN <julien.robin28@free.fr>
> ---
>  /drivers/platform/x86/hp/hp-wmi.c | 304 ++++++++++++++++++++++++++++++++-
>  1 file changed, 297 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c
> b/drivers/platform/x86/hp/hp-wmi.c
> index 81ccc96..9ce2b80 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -83,11 +83,16 @@ static const char * const
> omen_timed_thermal_profile_boards[] = {
>  	"8BAD", "8A42"
>  };
> 
> -/* DMI Board names of Victus laptops */
> +/* DMI Board names of Victus 16-d1xxx laptops */
>  static const char * const victus_thermal_profile_boards[] = {
>  	"8A25"
>  };
> 
> +/* DMI Board names of Victus 16-s1000 laptops */
> +static const char * const victus_s_thermal_profile_boards[] = {
> +	"8C9C"
> +};
> +
>  enum hp_wmi_radio {
>  	HPWMI_WIFI	= 0x0,
>  	HPWMI_BLUETOOTH	= 0x1,
> @@ -153,6 +158,11 @@ enum hp_wmi_gm_commandtype {
>  	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>  	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>  	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
> +	HPWMI_FAN_COUNT_GET_QUERY = 0x10,
> +	HPWMI_SET_GPU_THERMAL_MODES_QUERY = 0x22,
> +	HPWMI_SET_POWER_LIMITS_QUERY = 0x29,
> +	HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY = 0x2D,
> +	HPWMI_FAN_SPEED_SET_QUERY = 0x2E,

Thank you for the patch.

Please align the values. I leave it up to you if you want to only align 
the values you're adding now or all of them.

>  };
> 
>  enum hp_wmi_command {
> @@ -211,6 +221,11 @@ enum hp_thermal_profile_victus {
>  	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
>  };
> 
> +enum hp_thermal_profile_victus_s {
> +	HP_VICTUS_S_THERMAL_PROFILE_DEFAULT		= 0x00,
> +	HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE		= 0x01,
> +};
> +
>  enum hp_thermal_profile {
>  	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
>  	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> @@ -411,6 +426,26 @@ out_free:
>  	return ret;
>  }
> 
> +/*
> + * Calling this hp_wmi_get_fan_count_userdefine_trigger function also enables
> + * and/or maintains the laptop in user defined thermal and fan states,
> instead
> + * of using a fallback state. After a 120 seconds timeout however, the laptop
> + * goes back to its fallback state.
> + */
> +static int hp_wmi_get_fan_count_userdefine_trigger(void)
> +{
> +	char fan_data[4] = { 0 };
> +
> +	int ret = hp_wmi_perform_query(HPWMI_FAN_COUNT_GET_QUERY, HPWMI_GM,
> +				       &fan_data, sizeof(char),
> +				       sizeof(fan_data));
> +
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	return fan_data[0]; /* Others bytes aren't providing fan count */
> +}
> +
>  static int hp_wmi_get_fan_speed(int fan)
>  {
>  	u8 fsh, fsl;
> @@ -429,6 +464,23 @@ static int hp_wmi_get_fan_speed(int fan)
>  	return (fsh << 8) | fsl;
>  }
> 
> +static int hp_wmi_get_fan_speed_victus_s(int fan)
> +{
> +	char fan_data[128] = { 0 };
> +
> +	int ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY,
> +				       HPWMI_GM, &fan_data, sizeof(char),
> +				       sizeof(fan_data));
> +
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	if (fan >= 0 && fan < sizeof(fan_data))
> +		return fan_data[fan] * 100;
> +	else
> +		return -EINVAL;
> +}
> +
>  static int hp_wmi_read_int(int query)
>  {
>  	int val = 0, ret;
> @@ -557,6 +609,29 @@ static int hp_wmi_fan_speed_max_set(int enabled)
>  	return enabled;
>  }
> 
> +static int hp_wmi_fan_speed_reset(void)
> +{
> +	int ret;
> +	char fan_speed[2] = { 0 }; /* Restores automatic speed */

Instead of a comment, add a named define for the value?

> +
> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_SET_QUERY, HPWMI_GM,
> +				   &fan_speed, sizeof(fan_speed), 0);
> +
> +	return ret;
> +}
> +
> +static int hp_wmi_fan_speed_max_reset(void)
> +{
> +	int ret = hp_wmi_fan_speed_max_set(0);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Disabling Max fan speed on Victus s1xxx laptops needs a 2nd step:

Why is Max capitalized?

> */
> +	ret = hp_wmi_fan_speed_reset();
> +	return ret;
> +}
> +
>  static int hp_wmi_fan_speed_max_get(void)
>  {
>  	int val = 0, ret;
> @@ -1472,6 +1547,127 @@ static int platform_profile_victus_set_ec(enum
> platform_profile_option profile)
>  	return 0;
>  }
> 
> +static bool is_victus_s_thermal_profile(void)
> +{
> +	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> +	if (!board_name)
> +		return false;
> +
> +	return match_string(victus_s_thermal_profile_boards,
> +			    ARRAY_SIZE(victus_s_thermal_profile_boards),
> +			    board_name) >= 0;
> +}
> +
> +static int victus_s_gpu_thermal_profile_set(bool ctgp_enable,
> +					    bool ppab_enable,
> +					    char dstate)
> +{
> +	char gpu_settings[4];
> +	int ret;
> +
> +	gpu_settings[0] = ctgp_enable ? 0x01 : 0x00;
> +	gpu_settings[1] = ppab_enable ? 0x01 : 0x00;
> +	gpu_settings[2] = dstate;
> +	gpu_settings[3] = 0x57; /* it tells we're setting the 3 above values
> */
> +
> +	ret = hp_wmi_perform_query(HPWMI_SET_GPU_THERMAL_MODES_QUERY,
> HPWMI_GM,
> +				   &gpu_settings, sizeof(gpu_settings), 0);
> +
> +	return ret;
> +}
> +
> +static int victus_s_set_cpu_pl1_pl2(char pl1, char pl2)

Is char correct type here or should you use like u8? Given you use 0xff 
below I highly thing it's not a real character at all but generic 8-bit 
data that should be u8.

> +{
> +	/* Placing 0xFF in the 2 last bytes tells we're setting PL1 and PL2 */
> +	char buffer[4] = { pl1, pl2, 0xFF, 0xFF };

correct type?

I see lots of other char use elsewhere too in this patch, please check 
those too that it's correct.

> +
> +	if (pl1 == 0xFF || pl2 == 0xFF)
> +		return -EINVAL; /* the 0xFF value has a special meaning */

Could that "special meaning" be labeled with a named define?
(I understand you might not know anything beyond it being "special" but 
if you know a more precise meaning, a define would be much better than a 
literal.)

> +	if (pl2 < pl1)
> +		return -EINVAL; /* PL2 is not supposed to be lower than PL1 */

Please move those two comments before the if () line.

> +
> +	/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
> +

That comments sounds like something that could be a function level comment 
instead.

> +	int ret = hp_wmi_perform_query(HPWMI_SET_POWER_LIMITS_QUERY,
> +				       HPWMI_GM,
> +				       &buffer, sizeof(buffer), 0);
> +
> +	return ret;
> +}
> +
> +static int platform_profile_victus_s_get(struct platform_profile_handler
> *pprof,
> +					 enum platform_profile_option
> *profile)
> +{
> +	/* Same behaviour as platform_profile_omen_get */
> +	return platform_profile_omen_get(pprof, profile);
> +}

Just use platform_profile_omen_get() directly then instead?

-- 
 i.

> +
> +static int platform_profile_victus_s_set_ec(enum platform_profile_option
> profile)
> +{
> +	int err, tp;
> +	bool gpu_ctgp_enable, gpu_ppab_enable;
> +	char gpu_dstate; /* Test shows 1 = 100%, 2 = 50%, 3 = 25%, 4 = 12.5%
> */
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_PERFORMANCE;
> +		gpu_ctgp_enable = true;
> +		gpu_ppab_enable = true;
> +		gpu_dstate = 1;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
> +		gpu_ctgp_enable = false;
> +		gpu_ppab_enable = true;
> +		gpu_dstate = 1;
> +		break;
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		tp = HP_VICTUS_S_THERMAL_PROFILE_DEFAULT;
> +		gpu_ctgp_enable = false;
> +		gpu_ppab_enable = false;
> +		gpu_dstate = 1;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	hp_wmi_get_fan_count_userdefine_trigger();
> +
> +	err = omen_thermal_profile_set(tp);
> +	if (err < 0) {
> +		pr_err("Failed to set platform profile %d: %d\n", profile,
> err);
> +		return err;
> +	}
> +
> +	err = victus_s_gpu_thermal_profile_set(gpu_ctgp_enable,
> +					       gpu_ppab_enable,
> +					       gpu_dstate);
> +	if (err < 0) {
> +		pr_err("Failed to set GPU profile %d: %d\n", profile, err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int platform_profile_victus_s_set(struct platform_profile_handler
> *pprof,
> +					 enum platform_profile_option profile)
> +{
> +	int err;
> +
> +	guard(mutex)(&active_platform_profile_lock);
> +
> +	err = platform_profile_victus_s_set_ec(profile);
> +	if (err < 0)
> +		return err;
> +
> +	active_platform_profile = profile;
> +
> +	return 0;
> +}
> +
>  static int platform_profile_victus_set(struct platform_profile_handler
> *pprof,
>  				       enum platform_profile_option profile)
>  {
> @@ -1545,6 +1741,38 @@ static int omen_powersource_event(struct notifier_block
> *nb,
>  	return NOTIFY_OK;
>  }
> 
> +static int victus_s_powersource_event(struct notifier_block *nb,
> +				      unsigned long value,
> +				      void *data)
> +{
> +	struct acpi_bus_event *event_entry = data;
> +	int err;
> +
> +	if (strcmp(event_entry->device_class, ACPI_AC_CLASS) != 0)
> +		return NOTIFY_DONE;
> +
> +	pr_debug("Received power source device event\n");
> +
> +	/*
> +	 * Switching to battery power source while Performance mode is active
> +	 * needs manual triggering of CPU power limits. Same goes when
> switching
> +	 * to AC power source while Performance mode is active. Other modes
> +	 * however are automatically behaving without any manual action.
> +	 * Seen on HP 16-s1034nf (board 8C9C) with F.11 BIOS version.
> +	 */
> +
> +	if (active_platform_profile == PLATFORM_PROFILE_PERFORMANCE) {
> +		pr_debug("Triggering CPU PL1/PL2 actualization\n");
> +		err = victus_s_set_cpu_pl1_pl2(0, 0);
> +		if (err)
> +			pr_warn("Failed to actualize power limits: %d\n",
> err);
> +
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int omen_register_powersource_event_handler(void)
>  {
>  	int err;
> @@ -1560,11 +1788,31 @@ static int
> omen_register_powersource_event_handler(void)
>  	return 0;
>  }
> 
> +static int victus_s_register_powersource_event_handler(void)
> +{
> +	int err;
> +
> +	platform_power_source_nb.notifier_call = victus_s_powersource_event;
> +	err = register_acpi_notifier(&platform_power_source_nb);
> +
> +	if (err < 0) {
> +		pr_warn("Failed to install ACPI power source notify
> handler\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void omen_unregister_powersource_event_handler(void)
>  {
>  	unregister_acpi_notifier(&platform_power_source_nb);
>  }
> 
> +static inline void victus_s_unregister_powersource_event_handler(void)
> +{
> +	unregister_acpi_notifier(&platform_power_source_nb);
> +}
> +
>  static int thermal_profile_setup(void)
>  {
>  	int err, tp;
> @@ -1603,6 +1851,22 @@ static int thermal_profile_setup(void)
>  		platform_profile_handler.profile_set =
> platform_profile_victus_set;
> 
>  		set_bit(PLATFORM_PROFILE_QUIET,
> platform_profile_handler.choices);
> +	} else if (is_victus_s_thermal_profile()) {
> +		/*
> +		 * Being unable to retrieve laptop's current thermal profile,
> +		 * during this setup, we set it to Balanced by default.
> +		 */
> +		active_platform_profile = PLATFORM_PROFILE_BALANCED;
> +
> +		err =
> platform_profile_victus_s_set_ec(active_platform_profile);
> +		if (err < 0)
> +			return err;
> +
> +		platform_profile_handler.profile_get =
> platform_profile_victus_s_get;
> +		platform_profile_handler.profile_set =
> platform_profile_victus_s_set;
> +
> +		/* Adding an equivalent to HP Omen software ECO mode: */
> +		set_bit(PLATFORM_PROFILE_LOW_POWER,
> platform_profile_handler.choices);
>  	} else {
>  		tp = thermal_profile_get();
> 
> @@ -1628,9 +1892,14 @@ static int thermal_profile_setup(void)
>  	set_bit(PLATFORM_PROFILE_PERFORMANCE,
> platform_profile_handler.choices);
> 
>  	err = platform_profile_register(&platform_profile_handler);
> -	if (err)
> +	if (err == -EEXIST) {
> +		pr_warn("A platform profile handler is already registered\n");
>  		return err;
> -
> +	} else if (err) {
> +		pr_err("Platform profile handler registration fail: %d\n",
> err);
> +		return err;
> +	}
> +	pr_info("Registered as platform profile handler\n");
>  	platform_profile_support = true;
> 
>  	return 0;
> @@ -1759,8 +2028,13 @@ static umode_t hp_wmi_hwmon_is_visible(const void
> *data,
>  	case hwmon_pwm:
>  		return 0644;
>  	case hwmon_fan:
> -		if (hp_wmi_get_fan_speed(channel) >= 0)
> -			return 0444;
> +		if (is_victus_s_thermal_profile()) {
> +			if (hp_wmi_get_fan_speed_victus_s(channel) >= 0)
> +				return 0444;
> +		} else {
> +			if (hp_wmi_get_fan_speed(channel) >= 0)
> +				return 0444;
> +		}
>  		break;
>  	default:
>  		return 0;
> @@ -1776,7 +2050,10 @@ static int hp_wmi_hwmon_read(struct device *dev, enum
> hwmon_sensor_types type,
> 
>  	switch (type) {
>  	case hwmon_fan:
> -		ret = hp_wmi_get_fan_speed(channel);
> +		if (is_victus_s_thermal_profile())
> +			ret = hp_wmi_get_fan_speed_victus_s(channel);
> +		else
> +			ret = hp_wmi_get_fan_speed(channel);
> 
>  		if (ret < 0)
>  			return ret;
> @@ -1810,11 +2087,17 @@ static int hp_wmi_hwmon_write(struct device *dev, enum
> hwmon_sensor_types type,
>  	case hwmon_pwm:
>  		switch (val) {
>  		case 0:
> +			if (is_victus_s_thermal_profile())
> +				hp_wmi_get_fan_count_userdefine_trigger();
>  			/* 0 is no fan speed control (max), which is 1 for us
> */
>  			return hp_wmi_fan_speed_max_set(1);
>  		case 2:
>  			/* 2 is automatic speed control, which is 0 for us */
> -			return hp_wmi_fan_speed_max_set(0);
> +			if (is_victus_s_thermal_profile()) {
> +				hp_wmi_get_fan_count_userdefine_trigger();
> +				return hp_wmi_fan_speed_max_reset();
> +			} else
> +				return hp_wmi_fan_speed_max_set(0);
>  		default:
>  			/* we don't support manual fan speed control */
>  			return -EINVAL;
> @@ -1893,6 +2176,10 @@ static int __init hp_wmi_init(void)
>  		err = omen_register_powersource_event_handler();
>  		if (err)
>  			goto err_unregister_device;
> +	} else if (is_victus_s_thermal_profile()) {
> +		err = victus_s_register_powersource_event_handler();
> +		if (err)
> +			goto err_unregister_device;
>  	}
> 
>  	return 0;
> @@ -1912,6 +2199,9 @@ static void __exit hp_wmi_exit(void)
>  	if (is_omen_thermal_profile() || is_victus_thermal_profile())
>  		omen_unregister_powersource_event_handler();
> 
> +	if (is_victus_s_thermal_profile())
> +		victus_s_unregister_powersource_event_handler();
> +
>  	if (wmi_has_guid(HPWMI_EVENT_GUID))
>  		hp_wmi_input_destroy();
> 

Re: [PATCH] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Julien ROBIN]

Hello and thank you very much for the feedback.

I reworked my changes accordingly, sorry I took some time on it.
I'm going to send a corrected [Patch v2] soon, so that the changes may be reviewed (as it's my first patch submission, your feedback is welcome).

I'm going to list the changes into the [Patch v2] description, but as remainder, you can find my answers to your feedback and suggestions below.

On 08/01/2025 11:08 AM, Ilpo Järvinen wrote:

>>  enum hp_wmi_radio {
>>  	HPWMI_WIFI	= 0x0,
>>  	HPWMI_BLUETOOTH	= 0x1,
>> @@ -153,6 +158,11 @@ enum hp_wmi_gm_commandtype {
>>  	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>>  	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>>  	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
>> +	HPWMI_FAN_COUNT_GET_QUERY = 0x10,
>> +	HPWMI_SET_GPU_THERMAL_MODES_QUERY = 0x22,
>> +	HPWMI_SET_POWER_LIMITS_QUERY = 0x29,
>> +	HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY = 0x2D,
>> +	HPWMI_FAN_SPEED_SET_QUERY = 0x2E,
> 
> Thank you for the patch.
> 
> Please align the values. I leave it up to you if you want to only align 
> the values you're adding now or all of them.

Ok, I did all of them, so that it looks nicer and more consistent.

>> +	/* Disabling Max fan speed on Victus s1xxx laptops needs a 2nd step:
> 
> Why is Max capitalized?

Sorry, was just a mistake. I changed it to "max" not capitalized.

>> +static int victus_s_set_cpu_pl1_pl2(char pl1, char pl2)
> 
> Is char correct type here or should you use like u8? Given you use 0xff 
> below I highly thing it's not a real character at all but generic 8-bit 
> data that should be u8.
> 
>> +{
>> +	/* Placing 0xFF in the 2 last bytes tells we're setting PL1 and PL2 */
>> +	char buffer[4] = { pl1, pl2, 0xFF, 0xFF };
> 
> correct type?
> 
> I see lots of other char use elsewhere too in this patch, please check 
> those too that it's correct.

You're right, usage of char variable types was just copy of what I saw into existing functions; but used as unsigned numeric 8-bit values, u8 variable type is indeed more relevant.
So I changed my "char" additions to "u8" as you suggested.

>> +static int hp_wmi_fan_speed_reset(void)
>> +{
>> +	int ret;
>> +	char fan_speed[2] = { 0 }; /* Restores automatic speed */
> 
> Instead of a comment, add a named define for the value?

>> +
>> +	if (pl1 == 0xFF || pl2 == 0xFF)
>> +		return -EINVAL; /* the 0xFF value has a special meaning */
> 
> Could that "special meaning" be labeled with a named define?
> (I understand you might not know anything beyond it being "special" but 
> if you know a more precise meaning, a define would be much better than a 
> literal.)

I dug a little more about the observed special behavior of 0 and 0xFF values for these HP WMI queries.

It turns out 0xFF is more exactly used as "ignore / don't change" parameter for HPWMI_SET_POWER_LIMITS_QUERY (0x29).
Also, 3rd and 4th parameters for HPWMI_SET_POWER_LIMITS_QUERY turned out to be PL4, and CPU + GPU concurrent TDP, so for these 4 bytes, I've changed the buffer[4] to a struct with more clearly expressed names. In order to be consistent, I did the same work for GPU power modes.

For the named define for those values, I was about to propose the following ones:

#define HP_VICTUS_ARG_VALUE_RESTORE_DEFAULT 0x00
#define HP_VICTUS_ARG_VALUE_KEEP_UNCHANGED 0xFF

However these 2 statements wouldn't always be true and could be deceptive for other settings, as it turns out using 0 or 0x00 as arguments for others WMI queries (others than fan speed and CPU power limits settings) tends to set 0 as actual value instead of restoring default one, and same goes for 0xFF. 

So for now I just moved the comment as function level comment, as you suggested below, but in case you had something else in mind, I'm of course open to another approach.

>> +	if (pl2 < pl1)
>> +		return -EINVAL; /* PL2 is not supposed to be lower than PL1 */
> 
> Please move those two comments before the if () line.
> 
>> +
>> +	/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
>> +
> 
> That comments sounds like something that could be a function level comment 
> instead.

That's right, I moved those comments as you suggested.
 
>> +static int platform_profile_victus_s_get(struct platform_profile_handler
>> *pprof,
>> +					 enum platform_profile_option
>> *profile)
>> +{
>> +	/* Same behaviour as platform_profile_omen_get */
>> +	return platform_profile_omen_get(pprof, profile);
>> +}
> 
> Just use platform_profile_omen_get() directly then instead?
> 
Yes, I was copying again another approach I saw; but I indeed asked myself the same question.
So I removed this one and made platform_profile_omen_get() called instead.


Finally, while investigating about 0x00 and 0xFF special effects, I also dug a little more about a 0x57 fixed magic 4th byte used when applying GPU power modes (0x57 is 87 in decimal). It turned out to be a GPU slowdown temperature threshold, sometimes called GPS, that didn't need to be hard coded: despite being unable to use 0x00 and 0xFF to deal with it (was applying 0°C and 255°C), I successfully added HPWMI_GET_GPU_THERMAL_MODES_QUERY and associated function, and now the current setting for this value can be retrieved prior to overwrite.
So into the [Patch v2] this is used to retrieve and reuse the current value (which is confirmed to be 87 in my case after a power cycle) instead of using an hard coded value, before overwriting GPU power modes. I believe it's better that way, in case others machines are using another value in the future.

Best regards,
Julien ROBIN

Re: [PATCH] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

[Ilpo Järvinen]

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

On Mon, 13 Jan 2025, Julien ROBIN wrote:

> Hello and thank you very much for the feedback.
> 
> I reworked my changes accordingly, sorry I took some time on it.
> I'm going to send a corrected [Patch v2] soon, so that the changes may 
> be reviewed (as it's my first patch submission, your feedback is 
> welcome).
>
> I'm going to list the changes into the [Patch v2] description, but as 
> remainder, you can find my answers to your feedback and suggestions 
> below.
> 
> On 08/01/2025 11:08 AM, Ilpo Järvinen wrote:
> 
> >> +static int victus_s_set_cpu_pl1_pl2(char pl1, char pl2)
> > 
> > Is char correct type here or should you use like u8? Given you use 0xff 
> > below I highly thing it's not a real character at all but generic 8-bit 
> > data that should be u8.
> > 
> >> +{
> >> +	/* Placing 0xFF in the 2 last bytes tells we're setting PL1 and PL2 */
> >> +	char buffer[4] = { pl1, pl2, 0xFF, 0xFF };
> > 
> > correct type?
> > 
> > I see lots of other char use elsewhere too in this patch, please check 
> > those too that it's correct.
> 
> You're right, usage of char variable types was just copy of what I saw 
> into existing functions; but used as unsigned numeric 8-bit values, u8 
> variable type is indeed more relevant.
> So I changed my "char" additions to "u8" as you suggested.

Unfortunately in the past the developers have not paid too much 
attention to this difference so we're moving only (very) slowly towards 
consistency.

> >> +static int hp_wmi_fan_speed_reset(void)
> >> +{
> >> +	int ret;
> >> +	char fan_speed[2] = { 0 }; /* Restores automatic speed */
> > 
> > Instead of a comment, add a named define for the value?
> 
> >> +
> >> +	if (pl1 == 0xFF || pl2 == 0xFF)
> >> +		return -EINVAL; /* the 0xFF value has a special meaning */
> > 
> > Could that "special meaning" be labeled with a named define?
> > (I understand you might not know anything beyond it being "special" but 
> > if you know a more precise meaning, a define would be much better than a 
> > literal.)
> 
> I dug a little more about the observed special behavior of 0 and 0xFF 
> values for these HP WMI queries.
> 
> It turns out 0xFF is more exactly used as "ignore / don't change" 
> parameter for HPWMI_SET_POWER_LIMITS_QUERY (0x29).
> Also, 3rd and 4th parameters for HPWMI_SET_POWER_LIMITS_QUERY turned out 
> to be PL4, and CPU + GPU concurrent TDP, so for these 4 bytes, I've 
> changed the buffer[4] to a struct with more clearly expressed names. In 
> order to be consistent, I did the same work for GPU power modes.
> 
> For the named define for those values, I was about to propose the following ones:
> 
> #define HP_VICTUS_ARG_VALUE_RESTORE_DEFAULT 0x00
> #define HP_VICTUS_ARG_VALUE_KEEP_UNCHANGED 0xFF
> 
> However these 2 statements wouldn't always be true and could be 
> deceptive for other settings, as it turns out using 0 or 0x00 as 
> arguments for others WMI queries (others than fan speed and CPU power 
> limits settings) tends to set 0 as actual value instead of restoring 
> default one, and same goes for 0xFF. 
>
> So for now I just moved the comment as function level comment, as you 
> suggested below, but in case you had something else in mind, I'm of 
> course open to another approach.

You could add comment before the defines to tell which queries are known 
to use those values. It would still be very much preferrable to have those 
literals named so the code will become self-explanatory and those comments 
wouldn't be neede anymore.

> >> +	if (pl2 < pl1)
> >> +		return -EINVAL; /* PL2 is not supposed to be lower than PL1 */
> > 
> > Please move those two comments before the if () line.
> > 
> >> +
> >> +	/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
> >> +
> > 
> > That comments sounds like something that could be a function level comment 
> > instead.
> 
> That's right, I moved those comments as you suggested.
>  
> >> +static int platform_profile_victus_s_get(struct platform_profile_handler
> >> *pprof,
> >> +					 enum platform_profile_option
> >> *profile)
> >> +{
> >> +	/* Same behaviour as platform_profile_omen_get */
> >> +	return platform_profile_omen_get(pprof, profile);
> >> +}
> > 
> > Just use platform_profile_omen_get() directly then instead?
> 
> Yes, I was copying again another approach I saw; but I indeed asked 
> myself the same question.
> So I removed this one and made platform_profile_omen_get() called instead.
> 
> 
> Finally, while investigating about 0x00 and 0xFF special effects, I also 
> dug a little more about a 0x57 fixed magic 4th byte used when applying 
> GPU power modes (0x57 is 87 in decimal). It turned out to be a GPU 
> slowdown temperature threshold, sometimes called GPS, that didn't need 
> to be hard coded: despite being unable to use 0x00 and 0xFF to deal with 
> it (was applying 0°C and 255°C), I successfully added 
> HPWMI_GET_GPU_THERMAL_MODES_QUERY and associated function, and now the 
> current setting for this value can be retrieved prior to overwrite.
> So into the [Patch v2] this is used to retrieve and reuse the current 
> value (which is confirmed to be 87 in my case after a power cycle) 
> instead of using an hard coded value, before overwriting GPU power 
> modes. I believe it's better that way, in case others machines are using 
> another value in the future.

Great, thanks for looking into it! :-)

-- 
 i.