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

["Kurt Borja" <kuurtb@gmail.com>]

On Sat Feb 15, 2025 at 12:40 PM -05, Armin Wolf wrote:
> Am 15.02.25 um 02:30 schrieb Kurt Borja:
>
>> Hi Armin,
>>
>> On Fri Feb 14, 2025 at 5:13 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(-)
>>>
>>> --
>>> 2.39.5
>>>
>>> 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);
>> This was not replicated bellow. Just to be sure, are there no fans at
>> BIT(1) and BIT(2)?
>
> AFAIK the Acer OEM software support the following fans:
>
> - CPU (BIT(0))
> - GPU 1 (BIT(3))
> - GPU 2 (BIT(4), but untested)
>
> The other bits seem to be unused.

Interesting.

I ask because quirks->cpu_fans + quirks->gpu_fans is currently always 2,
so the line two `-` lines I referenced make the following equivalent
operation:

fan_bitmap |= BIT(1);
mode_bitmap |= FIELD_PREP(GENMASK(3, 2), fan_mode);

fan_bitmap |= BIT(2);
mode_bitmap |= FIELD_PREP(GENMASK(5, 4), fan_mode);

So if any model has fans at BIT(1), BIT(2) this may cause regressions if
the behavior is not mimicked.

Am I missing something?

Anyway, your explaination for how this method works makes a lot of
sense, so it is weird that the original author of this is summing the
number of fans and setting this bits in the first place.

-- 
 ~ Kurt

>
> Thanks,
> Armin Wolf
>
>>> -	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);
>> Same as above.
>>