From: Armin Wolf <W_Armin@gmx.de>
To: "Hridesh MG" <hridesh699@gmail.com>,
"Lee, Chun-Yi" <jlee@suse.com>,
"Hans de Goede" <hdegoede@redhat.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org,
Shuah Khan <skhan@linuxfoundation.org>,
onenowy@gmail.com
Subject: Re: [PATCH] platform/x86: acer-wmi: improve platform profile handling
Date: Wed, 1 Jan 2025 21:58:00 +0100 [thread overview]
Message-ID: <12ce2a4d-6a27-471e-b330-996753ff1bfb@gmx.de> (raw)
In-Reply-To: <20241231140442.10076-1-hridesh699@gmail.com>
Am 31.12.24 um 15:04 schrieb Hridesh MG:
> Two improvements to the platform profile handling for laptops using
> the Acer Predator interface -
>
> 1) Use WMI calls to fetch the current platform profile instead of
> directly accessing it from the EC since the EC address differs for
> certain laptops.
> 2) Use the supported profiles bitmap to dynamically set
> platform_profile_choices.
Hi,
can you please split the second part into a separate patch? This would make review easier.
> Link: https://lore.kernel.org/platform-driver-x86/d7be714c-3103-42ee-ad15-223a3fe67f80@gmx.de/
> Co-developed-by: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Hridesh MG <hridesh699@gmail.com>
> ---
> This patch has been tested on an acer nitro AN515-58 laptop, it would be
> good if someone with a predator laptop could also test it
> ---
> drivers/platform/x86/acer-wmi.c | 220 ++++++++++++++++++++++----------
> 1 file changed, 154 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index b3043d78a7b3..37f629b6e3d3 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -31,6 +31,7 @@
> #include <acpi/video.h>
> #include <linux/hwmon.h>
> #include <linux/units.h>
> +#include <linux/unaligned.h>
> #include <linux/bitfield.h>
>
> MODULE_AUTHOR("Carlos Corbacho");
> @@ -68,8 +69,11 @@ MODULE_LICENSE("GPL");
> #define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
> #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
> #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> +#define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>
> -#define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
> +#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)
>
> #define ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK GENMASK_ULL(7, 0)
> #define ACER_PREDATOR_V4_SENSOR_INDEX_BIT_MASK GENMASK_ULL(15, 8)
> @@ -115,6 +119,13 @@ enum acer_wmi_predator_v4_sensor_id {
> ACER_WMID_SENSOR_GPU_TEMPERATURE = 0x0A,
> };
>
> +enum acer_wmi_gaming_misc_setting {
> + ACER_WMID_MISC_SETTING_OC_1 = 0x0005,
> + ACER_WMID_MISC_SETTING_OC_2 = 0x0007,
> + ACER_WMID_MISC_SETTING_SUPPORTED_PROFILES = 0x000A,
> + ACER_WMID_MISC_SETTING_PLATFORM_PROFILE = 0x000B,
> +};
> +
> static const struct key_entry acer_wmi_keymap[] __initconst = {
> {KE_KEY, 0x01, {KEY_WLAN} }, /* WiFi */
> {KE_KEY, 0x03, {KEY_WLAN} }, /* WiFi */
> @@ -751,20 +762,12 @@ static bool platform_profile_support;
> */
> static int last_non_turbo_profile;
>
> -enum acer_predator_v4_thermal_profile_ec {
> - ACER_PREDATOR_V4_THERMAL_PROFILE_ECO = 0x04,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO = 0x03,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE = 0x02,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET = 0x01,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED = 0x00,
> -};
> -
> -enum acer_predator_v4_thermal_profile_wmi {
> - ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI = 0x060B,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI = 0x050B,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI = 0x040B,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI = 0x0B,
> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI = 0x010B,
> +enum acer_predator_v4_thermal_profile {
> + ACER_PREDATOR_V4_THERMAL_PROFILE_ECO = 0x06,
> + ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO = 0x05,
> + ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE = 0x04,
> + ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED = 0x01,
> + ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET = 0x00,
> };
>
> /* Find which quirks are needed for a particular vendor/ model pair */
> @@ -1477,6 +1480,45 @@ WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
> return status;
> }
>
> +static int WMI_gaming_execute_u32_u64(u32 method_id, u32 in, u64 *out)
> +{
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = {
> + .length = sizeof(in),
> + .pointer = &in,
> + };
> + union acpi_object *obj;
> + acpi_status status;
> + int ret = 0;
> +
> + status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + obj = result.pointer;
> + if (obj && out) {
> + switch (obj->type) {
> + case ACPI_TYPE_INTEGER:
> + *out = obj->integer.value;
> + break;
> + case ACPI_TYPE_BUFFER:
> + if (obj->buffer.length < sizeof(*out))
> + ret = -ENOMSG;
> + else
> + *out = get_unaligned_le64(obj->buffer.pointer);
> +
> + break;
> + default:
> + ret = -ENOMSG;
> + break;
> + }
> + }
> +
> + kfree(obj);
> +
> + return ret;
> +}
> +
> static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
> {
> u32 method_id = 0;
> @@ -1565,6 +1607,48 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
> WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
> }
>
> +static int WMID_gaming_set_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 value)
> +{
> + acpi_status status;
> + u64 input = 0;
> + u64 result;
> +
> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_INDEX_MASK, setting);
> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_VALUE_MASK, value);
> +
> + status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, input, &result);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + /* The return status must be zero for the operation to have succeeded */
> + if (FIELD_GET(ACER_GAMING_MISC_SETTING_STATUS_MASK, result))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int WMID_gaming_get_misc_setting(enum acer_wmi_gaming_misc_setting setting, u8 *value)
> +{
> + u64 input = 0;
> + u64 result;
> + int ret;
> +
> + input |= FIELD_PREP(ACER_GAMING_MISC_SETTING_INDEX_MASK, setting);
> +
> + ret = WMI_gaming_execute_u32_u64(ACER_WMID_GET_GAMING_MISC_SETTING_METHODID, input,
> + &result);
> + if (ret < 0)
> + return ret;
> +
> + /* The return status must be zero for the operation to have succeeded */
> + if (FIELD_GET(ACER_GAMING_MISC_SETTING_STATUS_MASK, result))
> + return -EIO;
> +
> + *value = FIELD_GET(ACER_GAMING_MISC_SETTING_VALUE_MASK, result);
> +
> + return 0;
> +}
> +
> /*
> * Generic Device (interface-independent)
> */
> @@ -1833,9 +1917,8 @@ acer_predator_v4_platform_profile_get(struct platform_profile_handler *pprof,
> u8 tp;
> int err;
>
> - err = ec_read(ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET, &tp);
> -
> - if (err < 0)
> + err = WMID_gaming_get_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, &tp);
> + if (err)
> return err;
>
> switch (tp) {
> @@ -1865,36 +1948,33 @@ static int
> acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
> enum platform_profile_option profile)
> {
> - int tp;
> - acpi_status status;
> + int tp, err;
>
> switch (profile) {
> case PLATFORM_PROFILE_PERFORMANCE:
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
> break;
> case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
> break;
> case PLATFORM_PROFILE_BALANCED:
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> break;
> case PLATFORM_PROFILE_QUIET:
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
> break;
> case PLATFORM_PROFILE_LOW_POWER:
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
> break;
> default:
> return -EOPNOTSUPP;
> }
>
> - status = WMI_gaming_execute_u64(
> - ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, tp, NULL);
> -
> - if (ACPI_FAILURE(status))
> - return -EIO;
> + err = WMID_gaming_set_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, tp);
> + if (err)
> + return err;
>
> - if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
> + if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
> last_non_turbo_profile = tp;
>
> return 0;
> @@ -1904,6 +1984,7 @@ static int acer_platform_profile_setup(struct platform_device *device)
> {
> if (quirks->predator_v4) {
> int err;
> + u8 supported_profiles;
Please order the variables in reverse xmas tree order, which means that the long declarations come first.
>
> platform_profile_handler.name = "acer-wmi";
> platform_profile_handler.dev = &device->dev;
> @@ -1912,16 +1993,27 @@ static int acer_platform_profile_setup(struct platform_device *device)
> platform_profile_handler.profile_set =
> acer_predator_v4_platform_profile_set;
>
> - set_bit(PLATFORM_PROFILE_PERFORMANCE,
> - platform_profile_handler.choices);
> - set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> - platform_profile_handler.choices);
> - set_bit(PLATFORM_PROFILE_BALANCED,
> - platform_profile_handler.choices);
> - set_bit(PLATFORM_PROFILE_QUIET,
> - platform_profile_handler.choices);
> - set_bit(PLATFORM_PROFILE_LOW_POWER,
> - platform_profile_handler.choices);
> + err = WMID_gaming_get_misc_setting(ACER_WMID_MISC_SETTING_SUPPORTED_PROFILES,
> + &supported_profiles);
> + if (err)
> + return err;
> +
> + if (supported_profiles & 1 << 0)
> + set_bit(PLATFORM_PROFILE_QUIET,
> + platform_profile_handler.choices);
> + if (supported_profiles & 1 << 1)
> + set_bit(PLATFORM_PROFILE_BALANCED,
> + platform_profile_handler.choices);
> + if (supported_profiles & 1 << 4)
> + set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> + platform_profile_handler.choices);
> + if (supported_profiles & 1 << 5)
> + set_bit(PLATFORM_PROFILE_PERFORMANCE,
> + platform_profile_handler.choices);
> + if (supported_profiles & 1 << 6)
> + set_bit(PLATFORM_PROFILE_LOW_POWER,
> + platform_profile_handler.choices);
Please use the appropriated values from "enum acer_predator_v4_thermal_profile" instead of those
magic numbers.
> +
>
> err = platform_profile_register(&platform_profile_handler);
> if (err)
> @@ -1931,7 +2023,7 @@ static int acer_platform_profile_setup(struct platform_device *device)
>
> /* Set default non-turbo profile */
> last_non_turbo_profile =
> - ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> }
> return 0;
> }
> @@ -1946,12 +2038,10 @@ static int acer_thermal_profile_change(void)
> u8 current_tp;
> int tp, err;
> u64 on_AC;
> - acpi_status status;
> -
> - err = ec_read(ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET,
> - ¤t_tp);
>
> - if (err < 0)
> + err = WMID_gaming_get_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE,
> + ¤t_tp);
> + if (err)
> return err;
>
> /* Check power source */
> @@ -1962,54 +2052,52 @@ static int acer_thermal_profile_change(void)
> switch (current_tp) {
> case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
> if (!on_AC)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> else if (cycle_gaming_thermal_profile)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
> else
> tp = last_non_turbo_profile;
> break;
> case ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE:
> if (!on_AC)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> else
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
> break;
> case ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED:
> if (!on_AC)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
> else if (cycle_gaming_thermal_profile)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
> else
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
> break;
> case ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET:
> if (!on_AC)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> else if (cycle_gaming_thermal_profile)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> else
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
> break;
> case ACER_PREDATOR_V4_THERMAL_PROFILE_ECO:
> if (!on_AC)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
> else if (cycle_gaming_thermal_profile)
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
> else
> - tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI;
> + tp = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
> break;
> default:
> return -EOPNOTSUPP;
> }
This needs a bit more work, since you might accidentally select unsupported platform profiles this way.
Using platform_profile_cycle() would make sense here, but we still need to handle "on_AC". I however wonder
if that is really necessary.
I CCed the original patch author hoping that he can give us some background information here.
Thanks,
Armin Wolf
>
> - status = WMI_gaming_execute_u64(
> - ACER_WMID_SET_GAMING_MISC_SETTING_METHODID, tp, NULL);
> -
> - if (ACPI_FAILURE(status))
> - return -EIO;
> + err = WMID_gaming_set_misc_setting(ACER_WMID_MISC_SETTING_PLATFORM_PROFILE, tp);
> + if (err)
> + return err;
>
> /* Store non-turbo profile for turbo mode toggle*/
> - if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO_WMI)
> + if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
> last_non_turbo_profile = tp;
>
> platform_profile_notify(&platform_profile_handler);
next prev parent reply other threads:[~2025-01-01 20:58 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-31 14:04 [PATCH] platform/x86: acer-wmi: improve platform profile handling Hridesh MG
2024-12-31 17:06 ` Kurt Borja
2025-01-01 5:20 ` Hridesh MG
2024-12-31 17:48 ` Kurt Borja
2025-01-01 20:58 ` Armin Wolf [this message]
2025-01-02 6:50 ` Hridesh MG
2025-01-03 19:01 ` Kurt Borja
2025-01-03 19:52 ` Hridesh MG
2025-01-03 21:07 ` Armin Wolf
2025-01-04 8:55 ` SungHwan Jung
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12ce2a4d-6a27-471e-b330-996753ff1bfb@gmx.de \
--to=w_armin@gmx.de \
--cc=hdegoede@redhat.com \
--cc=hridesh699@gmail.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jlee@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=onenowy@gmail.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox