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

[Armin Wolf]

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