Re: [PATCH v6 4/5] alienware-wmi: added autodetect_thermal_profile for devices with quirk_unknown

[Armin Wolf <W_Armin@gmx.de>]

Am 20.10.24 um 22:40 schrieb Armin Wolf:

> Am 20.10.24 um 22:39 schrieb Armin Wolf:
>
>> Am 17.10.24 um 10:16 schrieb Kurt Borja:
>>
>>> Added autodetect_thermal_profile for devices with quirk_unknown.
>>> Autodetection is done through basic conditions most devices with WMAX's
>>> thermal interface meet. Function exits returning 0 in case of errors.
>>>
> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>
Oops, this should have been for patch 5. Please ignore this.

>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>>
>>> ---
>>> I apologize for the late inclusion. This feature can extend support to
>>> many devices without having to list them in alienware_quirks.
>>>
>>> The conditions for selecting the automatic thermal profile are based on
>>> observations on a lot of *issues* in AWCC open source alternatives.
>>>
>>> I observed only Dell's G-Series laptops have WMAX_THERMAL_BALANCED
>>> avaliable and when it's present none of the other profiles are
>>> avaliable, except for GMODE. When a model has USTT profiles avaliable
>>> usually they have all USTT profiles avaliable, except for cool on
>>> mostly
>>> Alienware devices.
>>>
>>> I made another implementation of this function, brute-forcing operation
>>> 0x03 of Thermal_Information, which is the operation that varies the
>>> most
>>> across models. I found the implementation too cumbersome to include in
>>> this series, but it could potentially extend support of this driver to
>>> all posible devices with this interface automatically.
>>
>> I like this patch, automatic configuration is always a nice feature.
>>
>> Please add support for operation 0x03, this way the driver can work
>> automatically
>> without users having to submit patches adding quirks for their machines.
>>
>> Maybe you can use an array for storing the supported thermal mode ids,
>> like this:
>>
>> enum thermal_modes = {
>>     THERMAL_MODE_QUIET,
>>     ...
>>     THERMAL_MODE_LOW_POWER,
>>     THERMAL_MODE_MAX
>> };
>>
>> const enumplatform_profile_option
>> thermal_mode_to_platform_profile[THERMAL_MODE_MAX] = {
>>     [THERMAL_MODE_QUIET] =PLATFORM_PROFILE_QUIET, ...
>> };
>>
>> const enumthermal_modes
>> platform_profile_to_thermal_mode[PLATFORM_PROFILE_LAST] = {
>>     [PLATFORM_PROFILE_LOW_POWER] = THERMAL_MODE_LOW_POWER, ...
>> };
>>
>>
>> u8 thermal_modes[THERMAL_MODE_MAX] = {};
>>
>> for (int i = 0; i < THERMAL_MODE_MAX; i++) {
>>     thermal_modes[i] = call_operation_3(0x06 + i);
>>     // TODO: Error handling
>>     if (thermal_modes[i] == 0xFFFFFFFF)
>>         continue;
>>
>>     set_bit(supported_profiles, thermal_mode_to_platform_profile[i]);
>> }
>>
>> then you can use platform_profile_to_thermal_mode[] when setting the
>> platform profile
>> and thermal_mode_to_platform_profile[] when getting the platform
>> profile.
>> I will leave it up to you on how to handle the existence of GMode.
>>
>> This of course is only a rough idea, you can change anything you want
>> in the above pseudo-code.
>>
>> Thanks,
>> Armin Wolf
>>
>>>
>>> Another possibility is just including every device I observed into
>>> alienware_quirks, which I can do but I want to know your opinion first.
>>> ---
>>>   drivers/platform/x86/dell/alienware-wmi.c | 42
>>> +++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/dell/alienware-wmi.c
>>> b/drivers/platform/x86/dell/alienware-wmi.c
>>> index 37a898273..a11ff4851 100644
>>> --- a/drivers/platform/x86/dell/alienware-wmi.c
>>> +++ b/drivers/platform/x86/dell/alienware-wmi.c
>>> @@ -30,8 +30,11 @@
>>>   #define WMAX_METHOD_DEEP_SLEEP_STATUS    0x0C
>>>   #define WMAX_METHOD_THERMAL_INFORMATION    0x14
>>>   #define WMAX_METHOD_THERMAL_CONTROL    0x15
>>> +#define WMAX_METHOD_GMODE_STATUS    0x25
>>>
>>> +#define WMAX_ARG_GET_DEFAULT_PROF    0x0A
>>>   #define WMAX_ARG_GET_CURRENT_PROF    0x0B
>>> +#define WMAX_ARG_GET_GMODE_STATUS    0x02
>>>
>>>   #define WMAX_FAILURE_CODE        0xFFFFFFFF
>>>
>>> @@ -968,6 +971,42 @@ static int thermal_profile_set_ustt(struct
>>> platform_profile_handler *pprof,
>>>       return 0;
>>>   }
>>>
>>> +static int autodetect_thermal_profile(void)
>>> +{
>>> +    acpi_status status;
>>> +    u32 in_args;
>>> +    u32 default_profile;
>>> +    u32 gmode;
>>> +
>>> +    in_args = WMAX_ARG_GET_DEFAULT_PROF;
>>> +    status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +                    WMAX_METHOD_THERMAL_INFORMATION,
>>> &default_profile);
>>> +
>>> +    if (ACPI_FAILURE(status))
>>> +        return 0;
>>> +
>>> +    in_args = WMAX_ARG_GET_GMODE_STATUS;
>>> +    status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +                    WMAX_METHOD_GMODE_STATUS, &gmode);
>>> +
>>> +    if (ACPI_FAILURE(status))
>>> +        return 0;
>>> +
>>> +    if (default_profile == WMAX_THERMAL_BALANCED && gmode == 1) {
>>> +        quirks->thermal = WMAX_THERMAL_TABLE_SIMPLE;
>>> +        quirks->gmode = 1;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (default_profile == WMAX_THERMAL_USTT_BALANCED)
>>> +        quirks->thermal = WMAX_THERMAL_TABLE_USTT;
>>> +
>>> +    if (gmode == 0 || gmode == 1)
>>> +        quirks->gmode = 1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int create_thermal_profile(void)
>>>   {
>>>       pp_handler.profile_get = thermal_profile_get;
>>> @@ -1050,6 +1089,9 @@ static int __init alienware_wmi_init(void)
>>>               goto fail_prep_deepsleep;
>>>       }
>>>
>>> +    if (interface == WMAX && quirks == &quirk_unknown)
>>> +        autodetect_thermal_profile();
>>> +
>>>       if (quirks->thermal > 0) {
>>>           ret = create_thermal_profile();
>>>           if (ret)
>>
>