diff mbox series

[RFC,v2,1/3] platform/x86: acer-wmi: Fix setting of fan behavior

Message ID 20250215174544.8790-2-W_Armin@gmx.de (mailing list archive)
State New
Headers show
Series platform/x86: acer-wmi: Add fan control support | expand

Commit Message

Armin Wolf Feb. 15, 2025, 5:45 p.m. UTC 
After studying the linuwu_sense driver
(https://github.com/0x7375646F/Linuwu-Sense) i was able to understand
the meaning of the SetGamingFanBehavior() WMI method:

- the first 16-bit are a bitmap of all fans affected by a fan behavior
  change request.

- the next 8 bits contain four fan mode fields (2-bit), each being
  associated with a bit inside the fan bitmap.

There are three fan modes: auto, turbo and custom.

Use this newfound knowledge to fix the turbo fan handling by setting
the correct bits before calling SetGamingFanBehavior(). Also check
the result of the WMI method call and return an error should the ACPI
firmware signal failure.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 75 +++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 23 deletions(-)

--
2.39.5

Comments

Kurt Borja Feb. 15, 2025, 6:51 p.m. UTC | #1 
On Sat Feb 15, 2025 at 12:45 PM -05, Armin Wolf wrote:
> After studying the linuwu_sense driver
> (https://github.com/0x7375646F/Linuwu-Sense) i was able to understand
> the meaning of the SetGamingFanBehavior() WMI method:
>
> - the first 16-bit are a bitmap of all fans affected by a fan behavior
>   change request.
>
> - the next 8 bits contain four fan mode fields (2-bit), each being
>   associated with a bit inside the fan bitmap.
>
> There are three fan modes: auto, turbo and custom.
>
> Use this newfound knowledge to fix the turbo fan handling by setting
> the correct bits before calling SetGamingFanBehavior(). Also check
> the result of the WMI method call and return an error should the ACPI
> firmware signal failure.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/acer-wmi.c | 75 +++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 69336bd778ee..f20a882e3650 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -68,10 +68,19 @@ MODULE_LICENSE("GPL");
>  #define ACER_WMID_SET_GAMING_LED_METHODID 2
>  #define ACER_WMID_GET_GAMING_LED_METHODID 4
>  #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
> -#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
> +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
>  #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>  #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>
> +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
> +#define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
> +
> +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
> +
> +#define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
> +#define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
> +
>  #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
> @@ -121,6 +130,12 @@ enum acer_wmi_predator_v4_sensor_id {
>  	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
>  };
>
> +enum acer_wmi_gaming_fan_mode {
> +	ACER_WMID_FAN_MODE_AUTO		= 0x01,
> +	ACER_WMID_FAN_MODE_TURBO	= 0x02,
> +	ACER_WMID_FAN_MODE_CUSTOM	= 0x03,
> +};
> +
>  enum acer_wmi_predator_v4_oc {
>  	ACER_WMID_OC_NORMAL			= 0x0000,
>  	ACER_WMID_OC_TURBO			= 0x0002,
> @@ -1565,9 +1580,6 @@ static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
>  	case ACER_CAP_TURBO_LED:
>  		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
>  		break;
> -	case ACER_CAP_TURBO_FAN:
> -		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
> -		break;
>  	default:
>  		return AE_BAD_PARAMETER;
>  	}
> @@ -1618,25 +1630,42 @@ static int WMID_gaming_get_sys_info(u32 command, u64 *out)
>  	return 0;
>  }
>
> +static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
> +{
> +	acpi_status status;
> +	u64 input = 0;
> +	u64 result;
> +
> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK, mode_bitmap);
> +
> +	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID, input,
> +					&result);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	/* TODO: Proper error handling */
> +	pr_notice("Fan behavior return status: %llu\n", result);

I guess this is missing some ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK
handling right? This shouldn't mess with testing tho.

> +
> +	return 0;
> +}
> +
>  static void WMID_gaming_set_fan_mode(u8 fan_mode)
>  {
> -	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
> -	u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
> -	int i;
> -
> -	if (quirks->cpu_fans > 0)
> -		gpu_fan_config2 |= 1;
> -	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> -		gpu_fan_config2 |= 1 << (i + 1);

I agree on with your explaination in the previous thread, so after the
TODO is addressed:

Reviewed-by: Kurt Borja <kuurtb@gmail.com>

I do wonder tho, isn't there a WMI operation to get the bitmap of
available fans? Like in the case of available thermal profiles and
sensors.
Armin Wolf Feb. 15, 2025, 7:35 p.m. UTC | #2 
Am 15.02.25 um 19:51 schrieb Kurt Borja:

> On Sat Feb 15, 2025 at 12:45 PM -05, Armin Wolf wrote:
>> After studying the linuwu_sense driver
>> (https://github.com/0x7375646F/Linuwu-Sense) i was able to understand
>> the meaning of the SetGamingFanBehavior() WMI method:
>>
>> - the first 16-bit are a bitmap of all fans affected by a fan behavior
>>    change request.
>>
>> - the next 8 bits contain four fan mode fields (2-bit), each being
>>    associated with a bit inside the fan bitmap.
>>
>> There are three fan modes: auto, turbo and custom.
>>
>> Use this newfound knowledge to fix the turbo fan handling by setting
>> the correct bits before calling SetGamingFanBehavior(). Also check
>> the result of the WMI method call and return an error should the ACPI
>> firmware signal failure.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/acer-wmi.c | 75 +++++++++++++++++++++++----------
>>   1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index 69336bd778ee..f20a882e3650 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -68,10 +68,19 @@ MODULE_LICENSE("GPL");
>>   #define ACER_WMID_SET_GAMING_LED_METHODID 2
>>   #define ACER_WMID_GET_GAMING_LED_METHODID 4
>>   #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>> -#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
>> +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
>>   #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>>   #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>>
>> +#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
>> +
>> +#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>> +
>> +#define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
>> +#define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
>> +
>>   #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>>   #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
>> @@ -121,6 +130,12 @@ enum acer_wmi_predator_v4_sensor_id {
>>   	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
>>   };
>>
>> +enum acer_wmi_gaming_fan_mode {
>> +	ACER_WMID_FAN_MODE_AUTO		= 0x01,
>> +	ACER_WMID_FAN_MODE_TURBO	= 0x02,
>> +	ACER_WMID_FAN_MODE_CUSTOM	= 0x03,
>> +};
>> +
>>   enum acer_wmi_predator_v4_oc {
>>   	ACER_WMID_OC_NORMAL			= 0x0000,
>>   	ACER_WMID_OC_TURBO			= 0x0002,
>> @@ -1565,9 +1580,6 @@ static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
>>   	case ACER_CAP_TURBO_LED:
>>   		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
>>   		break;
>> -	case ACER_CAP_TURBO_FAN:
>> -		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
>> -		break;
>>   	default:
>>   		return AE_BAD_PARAMETER;
>>   	}
>> @@ -1618,25 +1630,42 @@ static int WMID_gaming_get_sys_info(u32 command, u64 *out)
>>   	return 0;
>>   }
>>
>> +static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
>> +{
>> +	acpi_status status;
>> +	u64 input = 0;
>> +	u64 result;
>> +
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
>> +	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK, mode_bitmap);
>> +
>> +	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID, input,
>> +					&result);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	/* TODO: Proper error handling */
>> +	pr_notice("Fan behavior return status: %llu\n", result);
> I guess this is missing some ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK
> handling right? This shouldn't mess with testing tho.

Yes, i need to wait for the hardware testers to give feedback before i can implement this.

>> +
>> +	return 0;
>> +}
>> +
>>   static void WMID_gaming_set_fan_mode(u8 fan_mode)
>>   {
>> -	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
>> -	u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
>> -	int i;
>> -
>> -	if (quirks->cpu_fans > 0)
>> -		gpu_fan_config2 |= 1;
>> -	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
>> -		gpu_fan_config2 |= 1 << (i + 1);
> I agree on with your explaination in the previous thread, so after the
> TODO is addressed:
>
> Reviewed-by: Kurt Borja <kuurtb@gmail.com>
>
> I do wonder tho, isn't there a WMI operation to get the bitmap of
> available fans? Like in the case of available thermal profiles and
> sensors.

I do not know. There are a couple of other fan-related WMI methods available but they seem
to do something else.

For now i use the hwmon interface for detecting the presence of the CPU and GPU fan. I suspect
that the second GPU fan can also be detected using the hwmon interface.

Thanks,
Armin Wolf
diff mbox series

Patch

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 69336bd778ee..f20a882e3650 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -68,10 +68,19 @@  MODULE_LICENSE("GPL");
 #define ACER_WMID_SET_GAMING_LED_METHODID 2
 #define ACER_WMID_GET_GAMING_LED_METHODID 4
 #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
-#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
+#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID 14
 #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
 #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23

+#define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
+#define ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK GENMASK_ULL(23, 16)
+
+#define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
+#define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
+
+#define ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK GENMASK(1, 0)
+#define ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK GENMASK(7, 6)
+
 #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
 #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
 #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
@@ -121,6 +130,12 @@  enum acer_wmi_predator_v4_sensor_id {
 	ACER_WMID_SENSOR_GPU_TEMPERATURE	= 0x0A,
 };

+enum acer_wmi_gaming_fan_mode {
+	ACER_WMID_FAN_MODE_AUTO		= 0x01,
+	ACER_WMID_FAN_MODE_TURBO	= 0x02,
+	ACER_WMID_FAN_MODE_CUSTOM	= 0x03,
+};
+
 enum acer_wmi_predator_v4_oc {
 	ACER_WMID_OC_NORMAL			= 0x0000,
 	ACER_WMID_OC_TURBO			= 0x0002,
@@ -1565,9 +1580,6 @@  static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
 	case ACER_CAP_TURBO_LED:
 		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
 		break;
-	case ACER_CAP_TURBO_FAN:
-		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
-		break;
 	default:
 		return AE_BAD_PARAMETER;
 	}
@@ -1618,25 +1630,42 @@  static int WMID_gaming_get_sys_info(u32 command, u64 *out)
 	return 0;
 }

+static int WMID_gaming_set_fan_behavior(u16 fan_bitmap, u8 mode_bitmap)
+{
+	acpi_status status;
+	u64 input = 0;
+	u64 result;
+
+	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_ID_MASK, fan_bitmap);
+	input |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_SET_MODE_MASK, mode_bitmap);
+
+	status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_FAN_BEHAVIOR_METHODID, input,
+					&result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* TODO: Proper error handling */
+	pr_notice("Fan behavior return status: %llu\n", result);
+
+	return 0;
+}
+
 static void WMID_gaming_set_fan_mode(u8 fan_mode)
 {
-	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
-	u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
-	int i;
-
-	if (quirks->cpu_fans > 0)
-		gpu_fan_config2 |= 1;
-	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
-		gpu_fan_config2 |= 1 << (i + 1);
-	for (i = 0; i < quirks->gpu_fans; ++i)
-		gpu_fan_config2 |= 1 << (i + 3);
-	if (quirks->cpu_fans > 0)
-		gpu_fan_config1 |= fan_mode;
-	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
-		gpu_fan_config1 |= fan_mode << (2 * i + 2);
-	for (i = 0; i < quirks->gpu_fans; ++i)
-		gpu_fan_config1 |= fan_mode << (2 * i + 6);
-	WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
+	u16 mode_bitmap = 0;
+	u16 fan_bitmap = 0;
+
+	if (quirks->cpu_fans > 0) {
+		fan_bitmap |= ACER_GAMING_FAN_BEHAVIOR_CPU;
+		mode_bitmap |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_CPU_MODE_MASK, fan_mode);
+	}
+
+	if (quirks->gpu_fans > 0) {
+		fan_bitmap |= ACER_GAMING_FAN_BEHAVIOR_GPU;
+		mode_bitmap |= FIELD_PREP(ACER_GAMING_FAN_BEHAVIOR_GPU_MODE_MASK, fan_mode);
+	}
+
+	WMID_gaming_set_fan_behavior(fan_bitmap, mode_bitmap);
 }

 static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
@@ -1923,7 +1952,7 @@  static int acer_toggle_turbo(void)
 		WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);

 		/* Set FAN mode to auto */
-		WMID_gaming_set_fan_mode(0x1);
+		WMID_gaming_set_fan_mode(ACER_WMID_FAN_MODE_AUTO);

 		/* Set OC to normal */
 		if (has_cap(ACER_CAP_TURBO_OC)) {
@@ -1937,7 +1966,7 @@  static int acer_toggle_turbo(void)
 		WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);

 		/* Set FAN mode to turbo */
-		WMID_gaming_set_fan_mode(0x2);
+		WMID_gaming_set_fan_mode(ACER_WMID_FAN_MODE_TURBO);

 		/* Set OC to turbo mode */
 		if (has_cap(ACER_CAP_TURBO_OC)) {