Message-ID: <c780277a-cf52-42a4-803f-c29674a31715@gmx.de>
Date: Wed, 30 Oct 2024 00:11:51 +0100
From: Armin Wolf <W_Armin@....de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Kurt Borja <kuurtb@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
 platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v10 3/5] alienware-wmi: added platform profile support

Am 29.10.24 um 18:06 schrieb Ilpo Järvinen:

> On Tue, 29 Oct 2024, Kurt Borja wrote:
>
>> On Tue, Oct 29, 2024 at 05:44:05PM +0200, Ilpo Järvinen wrote:
>>> On Tue, 29 Oct 2024, Kurt Borja wrote:
>>>
>>>> Implements platform profile support for Dell laptops with new WMAX thermal
>>>> interface, present on some Alienware X-Series, Alienware M-Series and
>>>> Dell's G-Series laptops. This interface is suspected to be used by
>>>> Alienware Command Center (AWCC), which is not available for linux
>>>> systems, to manage thermal profiles.
>>>>
>>>> This implementation makes use of three WMI methods, namely
>>>> THERMAL_CONTROL, THERMAL_INFORMATION and GAME_SHIFT_STATUS, which take
>>>> u32 as input and output arguments. Each method has a set of supported
>>>> operations specified in their respective enums.
>>>>
>>>> Not all models with WMAX WMI interface support these methods. Because of
>>>> this, models have to manually declare support through new quirks
>>>> `thermal` for THERMAL_CONTROL and THERMAL_INFORMATION and `gmode` for
>>>> GAME_SHIFT_STATUS.
>>>>
>>>> Wrappers written for these methods support multiple operations.
>>>>
>>>> THERMAL_CONTROL switches thermal modes through operation
>>>> ACTIVATE_PROFILE. Available thermal codes are auto-detected at runtime
>>>> and matched against a list of known thermal codes:
>>>>
>>>> Thermal Table "User Selectable Thermal Tables" (USTT):
>>>> 	BALANCED			0xA0
>>>> 	BALANCED_PERFORMANCE		0xA1
>>>> 	COOL				0xA2
>>>> 	QUIET				0xA3
>>>> 	PERFORMANCE			0xA4
>>>> 	LOW_POWER			0xA5
>>>>
>>>> Thermal Table Basic:
>>>> 	QUIET				0x96
>>>> 	BALANCED			0x97
>>>> 	BALANCED_PERFORMANCE		0x98
>>>> 	PERFORMANCE			0x99
>>>>
>>>> Devices are known to implement only one of these tables without mixing
>>>> their thermal codes.
>>>>
>>>> The fact that the least significant digit of every thermal code is
>>>> consecutive of one another is exploited to efficiently match codes
>>>> through arrays.
>>>>
>>>> Autodetection of available codes is done through operation LIST_IDS of
>>>> method THERMAL_INFORMATION. This operation lists fan IDs, CPU sensor ID,
>>>> GPU sensor ID and available thermal profile codes, *in that order*. As
>>>> number of fans and thermal codes is very model dependent, almost every
>>>> ID is scanned and matched based on conditions found on
>>>> is_wmax_thermal_code(). The known upper bound for the number of IDs is
>>>> 13, corresponding to a device that have 4 fans, 2 sensors and 7 thermal
>>>> codes.
>>>>
>>>> Additionally G-Series laptops have a key called G-key, which (with AWCC
>>>> proprietary driver) switches the thermal mode to an special mode named
>>>> GMODE with code 0xAB and changes Game Shift Status to 1. Game Shift is a
>>>> mode the manufacturer claims, increases gaming performance.
>>>>
>>>> GAME_SHIFT_STATUS method is used to mimic this behavior when selecting
>>>> PLATFORM_PROFILE_PERFORMANCE option.
>>>>
>>>> All of these profiles are known to only change fan speed profiles,
>>>> although there are untested claims that some of them also change power
>>>> profiles.
>>>>
>>>> Activating a thermal mode with method THERMAL_CONTROL may cause short
>>>> hangs. This is a known problem present on every platform.
>>>>
>>>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
>>>> ---
>>>> v10:
>>>>   - Corrected THERMAL_MODE_BASIC_BALANCED -> THERMAL_MODE_BASIC_QUIET in
>>>>     is_wmax_thermal_code() logic
>>>>   - `thermal` and `gmode` quirks now have to be manually selected,
>>>>     because not all Dell devices posses the new WMI thermal methods.
>>>>   - Because of the above reason, errors in create_thermal_profile are now
>>>>     propagated
>>>> v9:
>>>>   - Bool comparisons are now coherent with their type
>>>> v8:
>>>>   - Fixed alignment in wmax_mode_to_platform_profile[]
>>>>   - Quirk thermal and gmode changed from u8 -> bool
>>>>   - Autodetected quirk entries are not initialized
>>>>   - is_wmax_thermal_code refactored to increase readibility
>>>>   - is_wmax_thermal_code now covers all possibilities
>>>>   - Better commit message
>>>> v7:
>>>>   - Method operations are now clearly listed as separate enums
>>>>   - wmax_thermal_modes are now listed without codes in order to support
>>>>     autodetection, as well as getting and setting thermal profiles
>>>>     cleanly through arrays
>>>>   - Added wmax_mode_to_platform_profile[]
>>>>   - Added struct wmax_u32_args to replace bit mask approach of
>>>>     constructing arguments for wmax methods
>>>>   - create_thermal_profile now autodetects available thermal codes
>>>>     through operation 0x03 of THERMAL_INFORMATION method. These are
>>>>     codes are stored in supported_thermal_profiles[]
>>>>   - thermal_profile_get now uses wmax_mode_to_platform_profile[] instead of
>>>>     switch-case approach
>>>>   - thermal_profile_set now uses supported_thermal_profiles[] instead of
>>>>     switch-case approach
>>>>   - When gmode is autodetected, thermal_profile_set also sets Game Shift
>>>>     status accordingly
>>>> v6:
>>>>   - Fixed alignment on some function definitions
>>>>   - Fixed braces on if statment
>>>>   - Removed quirk thermal_ustt
>>>>   - Now quirk thermal can take values defined in enum WMAX_THERMAL_TABLE.
>>>>   - Proper removal of thermal_profile
>>>> ---
>>>>   drivers/platform/x86/dell/Kconfig         |   1 +
>>>>   drivers/platform/x86/dell/alienware-wmi.c | 306 ++++++++++++++++++++++
>>>>   2 files changed, 307 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>>> index 68a49788a..b06d634cd 100644
>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>> @@ -21,6 +21,7 @@ config ALIENWARE_WMI
>>>>   	depends on LEDS_CLASS
>>>>   	depends on NEW_LEDS
>>>>   	depends on ACPI_WMI
>>>> +	select ACPI_PLATFORM_PROFILE
>>>>   	help
>>>>   	 This is a driver for controlling Alienware BIOS driven
>>>>   	 features.  It exposes an interface for controlling the AlienFX
>>>> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
>>>> index b27f3b64c..1d62c2ce7 100644
>>>> --- a/drivers/platform/x86/dell/alienware-wmi.c
>>>> +++ b/drivers/platform/x86/dell/alienware-wmi.c
>>>> @@ -8,8 +8,11 @@
>>>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>>
>>>>   #include <linux/acpi.h>
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bits.h>
>>>>   #include <linux/module.h>
>>>>   #include <linux/platform_device.h>
>>>> +#include <linux/platform_profile.h>
>>>>   #include <linux/dmi.h>
>>>>   #include <linux/leds.h>
>>>>
>>>> @@ -25,6 +28,13 @@
>>>>   #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>>>>   #define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
>>>>   #define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
>>>> +#define WMAX_METHOD_THERMAL_INFORMATION	0x14
>>>> +#define WMAX_METHOD_THERMAL_CONTROL	0x15
>>>> +#define WMAX_METHOD_GAME_SHIFT_STATUS	0x25
>>>> +
>>>> +#define WMAX_THERMAL_MODE_GMODE		0xAB
>>>> +
>>>> +#define WMAX_FAILURE_CODE		0xFFFFFFFF
>>>>
>>>>   MODULE_AUTHOR("Mario Limonciello <mario.limonciello@...look.com>");
>>>>   MODULE_DESCRIPTION("Alienware special feature control");
>>>> @@ -49,11 +59,59 @@ enum WMAX_CONTROL_STATES {
>>>>   	WMAX_SUSPEND = 3,
>>>>   };
>>>>
>>>> +enum WMAX_THERMAL_INFORMATION_OPERATIONS {
>>>> +	WMAX_OPERATION_LIST_IDS			= 0x03,
>>>> +	WMAX_OPERATION_CURRENT_PROFILE		= 0x0B,
>>>> +};
>>>> +
>>>> +enum WMAX_THERMAL_CONTROL_OPERATIONS {
>>>> +	WMAX_OPERATION_ACTIVATE_PROFILE		= 0x01,
>>>> +};
>>>> +
>>>> +enum WMAX_GAME_SHIFT_STATUS_OPERATIONS {
>>>> +	WMAX_OPERATION_TOGGLE_GAME_SHIFT	= 0x01,
>>>> +	WMAX_OPERATION_GET_GAME_SHIFT_STATUS	= 0x02,
>>>> +};
>>>> +
>>>> +enum WMAX_THERMAL_TABLES {
>>>> +	WMAX_THERMAL_TABLE_BASIC		= 0x90,
>>>> +	WMAX_THERMAL_TABLE_USTT			= 0xA0,
>>>> +};
>>>> +
>>>> +enum wmax_thermal_mode {
>>>> +	THERMAL_MODE_USTT_BALANCED,
>>>> +	THERMAL_MODE_USTT_BALANCED_PERFORMANCE,
>>>> +	THERMAL_MODE_USTT_COOL,
>>>> +	THERMAL_MODE_USTT_QUIET,
>>>> +	THERMAL_MODE_USTT_PERFORMANCE,
>>>> +	THERMAL_MODE_USTT_LOW_POWER,
>>>> +	THERMAL_MODE_BASIC_QUIET,
>>>> +	THERMAL_MODE_BASIC_BALANCED,
>>>> +	THERMAL_MODE_BASIC_BALANCED_PERFORMANCE,
>>>> +	THERMAL_MODE_BASIC_PERFORMANCE,
>>>> +	THERMAL_MODE_LAST,
>>>> +};
>>>> +
>>>> +static const enum platform_profile_option wmax_mode_to_platform_profile[THERMAL_MODE_LAST] = {
>>>> +	[THERMAL_MODE_USTT_BALANCED]			= PLATFORM_PROFILE_BALANCED,
>>>> +	[THERMAL_MODE_USTT_BALANCED_PERFORMANCE]	= PLATFORM_PROFILE_BALANCED_PERFORMANCE,
>>>> +	[THERMAL_MODE_USTT_COOL]			= PLATFORM_PROFILE_COOL,
>>>> +	[THERMAL_MODE_USTT_QUIET]			= PLATFORM_PROFILE_QUIET,
>>>> +	[THERMAL_MODE_USTT_PERFORMANCE]			= PLATFORM_PROFILE_PERFORMANCE,
>>>> +	[THERMAL_MODE_USTT_LOW_POWER]			= PLATFORM_PROFILE_LOW_POWER,
>>>> +	[THERMAL_MODE_BASIC_QUIET]			= PLATFORM_PROFILE_QUIET,
>>>> +	[THERMAL_MODE_BASIC_BALANCED]			= PLATFORM_PROFILE_BALANCED,
>>>> +	[THERMAL_MODE_BASIC_BALANCED_PERFORMANCE]	= PLATFORM_PROFILE_BALANCED_PERFORMANCE,
>>>> +	[THERMAL_MODE_BASIC_PERFORMANCE]		= PLATFORM_PROFILE_PERFORMANCE,
>>>> +};
>>>> +
>>>>   struct quirk_entry {
>>>>   	u8 num_zones;
>>>>   	u8 hdmi_mux;
>>>>   	u8 amplifier;
>>>>   	u8 deepslp;
>>>> +	bool thermal;
>>>> +	bool gmode;
>>>>   };
>>>>
>>>>   static struct quirk_entry *quirks;
>>>> @@ -64,6 +122,8 @@ static struct quirk_entry quirk_inspiron5675 = {
>>>>   	.hdmi_mux = 0,
>>>>   	.amplifier = 0,
>>>>   	.deepslp = 0,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_unknown = {
>>>> @@ -71,6 +131,8 @@ static struct quirk_entry quirk_unknown = {
>>>>   	.hdmi_mux = 0,
>>>>   	.amplifier = 0,
>>>>   	.deepslp = 0,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_x51_r1_r2 = {
>>>> @@ -78,6 +140,8 @@ static struct quirk_entry quirk_x51_r1_r2 = {
>>>>   	.hdmi_mux = 0,
>>>>   	.amplifier = 0,
>>>>   	.deepslp = 0,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_x51_r3 = {
>>>> @@ -85,6 +149,8 @@ static struct quirk_entry quirk_x51_r3 = {
>>>>   	.hdmi_mux = 0,
>>>>   	.amplifier = 1,
>>>>   	.deepslp = 0,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_asm100 = {
>>>> @@ -92,6 +158,8 @@ static struct quirk_entry quirk_asm100 = {
>>>>   	.hdmi_mux = 1,
>>>>   	.amplifier = 0,
>>>>   	.deepslp = 0,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_asm200 = {
>>>> @@ -99,6 +167,8 @@ static struct quirk_entry quirk_asm200 = {
>>>>   	.hdmi_mux = 1,
>>>>   	.amplifier = 0,
>>>>   	.deepslp = 1,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>>   };
>>>>
>>>>   static struct quirk_entry quirk_asm201 = {
>>>> @@ -106,6 +176,17 @@ static struct quirk_entry quirk_asm201 = {
>>>>   	.hdmi_mux = 1,
>>>>   	.amplifier = 1,
>>>>   	.deepslp = 1,
>>>> +	.thermal = false,
>>>> +	.gmode = false,
>>>> +};
>>>> +
>>>> +static struct quirk_entry quirk_x_series = {
>>>> +	.num_zones = 2,
>>>> +	.hdmi_mux = 0,
>>>> +	.amplifier = 0,
>>>> +	.deepslp = 0,
>>>> +	.thermal = true,
>>>> +	.gmode = false,
>>>>   };
>>> So now gmode is always false unless the module parameter from patch 4 is
>>> given?
>> G-Series laptops can also register a quirk_entry with gmode set in the
>> future. I can do it ahead of time, but I don't have a G-Series laptop to
>> test it.
> Understood. I'm fine with this in the current form if it's fine for Armin
> too.

I am fine with this, we can add the necessary quirks later should someone have access
to the necessary hardware.

Reviewed-by: Armin Wolf <W_Armin@....de>

>

Powered by blists - more mailing lists