[PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling

[PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling

[Armin Wolf]

When support for Vivobook fan profiles was added, two mistakes where
made:

1. throttle_thermal_policy_set_default() was not called anymore during
probe.

2. The new thermal profiles where used inconsistently.

This patch series aims to fix both issues. Compile-tested only.

Armin Wolf (2):
  platform/x86: asus-wmi: Fix thermal profile initialization
  platform/x86: asus-wmi: Fix inconsistent use of thermal policies

 drivers/platform/x86/asus-wmi.c | 74 ++++++++++++++-------------------
 1 file changed, 31 insertions(+), 43 deletions(-)

--
2.39.5

[PATCH 1/2] platform/x86: asus-wmi: Fix thermal profile initialization

[Armin Wolf]

When support for vivobook fan profiles was added, the initial
call to throttle_thermal_policy_set_default() was removed, which
however is necessary for full initialization.

Fix this by calling throttle_thermal_policy_set_default() again
when setting up the platform profile.

Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
Reported-by: Michael Larabel <Michael@phoronix.com>
Closes: https://www.phoronix.com/review/lunar-lake-xe2/5
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/asus-wmi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2ccc23b259d3..ab9342a01a48 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3908,6 +3908,16 @@ static int platform_profile_setup(struct asus_wmi *asus)
 	if (!asus->throttle_thermal_policy_dev)
 		return 0;

+	/*
+	 * We need to set the default thermal profile during probe or otherwise
+	 * the system will often remain in silent mode, causing low performance.
+	 */
+	err = throttle_thermal_policy_set_default(asus);
+	if (err < 0) {
+		pr_warn("Failed to set default thermal profile\n");
+		return err;
+	}
+
 	dev_info(dev, "Using throttle_thermal_policy for platform_profile support\n");

 	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
--
2.39.5

[PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Armin Wolf]

When changing the thermal policy using the platform profile API,
a Vivobook thermal policy is stored in throttle_thermal_policy_mode.

However everywhere else a normal thermal policy is stored inside this
variable, potentially confusing the platform profile.

Fix this by always storing normal thermal policy values inside
throttle_thermal_policy_mode and only do the conversion when writing
the thermal policy to hardware.

Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ab9342a01a48..ce60835d0303 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
 /* Throttle thermal policy ****************************************************/
 static int throttle_thermal_policy_write(struct asus_wmi *asus)
 {
-	u8 value = asus->throttle_thermal_policy_mode;
 	u32 retval;
+	u8 value;
 	int err;

+	if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
+		switch (asus->throttle_thermal_policy_mode) {
+		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+			value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
+			break;
+		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+			value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
+			break;
+		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+			value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		value = asus->throttle_thermal_policy_mode;
+	}
+
 	err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
 				    value, &retval);

@@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 static DEVICE_ATTR_RW(throttle_thermal_policy);

 /* Platform profile ***********************************************************/
-static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
-{
-	bool vivo;
-
-	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
-	if (vivo) {
-		switch (mode) {
-		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
-			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
-		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
-			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
-		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
-			return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
-		}
-	}
-
-	return mode;
-}
-
-static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
-{
-	bool vivo;
-
-	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
-	if (vivo) {
-		switch (mode) {
-		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
-			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
-		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
-			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
-		case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
-			return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
-		}
-	}
-
-	return mode;
-}
-
 static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
 					enum platform_profile_option *profile)
 {
@@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
 	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
 	tp = asus->throttle_thermal_policy_mode;

-	switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
+	switch (tp) {
 	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
 		*profile = PLATFORM_PROFILE_BALANCED;
 		break;
@@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
 		return -EOPNOTSUPP;
 	}

-	asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
+	asus->throttle_thermal_policy_mode = tp;
 	return throttle_thermal_policy_write(asus);
 }

--
2.39.5

Re: [PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling

[srinivas pandruvada]

+Casey

On Fri, 2024-10-25 at 21:15 +0200, Armin Wolf wrote:
> When support for Vivobook fan profiles was added, two mistakes where
> made:
> 
> 1. throttle_thermal_policy_set_default() was not called anymore
> during
> probe.
> 
> 2. The new thermal profiles where used inconsistently.
> 
> This patch series aims to fix both issues. Compile-tested only.
> 
Thanks for these patches. The first one I already tested with the same
change, for the second one added Casey to check if he can give a quick
test for both on the new Asus Lunar Lake laptop.

Thanks,
Srinivas




> Armin Wolf (2):
>   platform/x86: asus-wmi: Fix thermal profile initialization
>   platform/x86: asus-wmi: Fix inconsistent use of thermal policies
> 
>  drivers/platform/x86/asus-wmi.c | 74 ++++++++++++++-----------------
> --
>  1 file changed, 31 insertions(+), 43 deletions(-)
> 
> --
> 2.39.5
> 

Re: [PATCH 1/2] platform/x86: asus-wmi: Fix thermal profile initialization

[srinivas pandruvada]

On Fri, 2024-10-25 at 21:15 +0200, Armin Wolf wrote:
> When support for vivobook fan profiles was added, the initial
> call to throttle_thermal_policy_set_default() was removed, which
> however is necessary for full initialization.
> 
> Fix this by calling throttle_thermal_policy_set_default() again
> when setting up the platform profile.
> 
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for
> vivobook fan profiles")
> Reported-by: Michael Larabel <Michael@phoronix.com>

For Michael to understand how this patch is related:

When Michael did test on 6.11 based kernel, there was no platform
profile support for the new Asus laptop. So the default boot Whisper
mode was active all the time.
My AIPT patch addressed that issue using FANL method.

But for 6.12 cycle, Mohamed added VIVO profile, which will also work
with the new laptop with AIPT even though the names of the profiles
don't match with the AIPT modes. But that patch removed the setting of
default policy in hardware to AIPT "standard" or 0 for VIVO default. So
mode was still whisper.

So this patch will address that.

Thanks,
Srinivas




> Closes: https://www.phoronix.com/review/lunar-lake-xe2/5
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/asus-wmi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c
> b/drivers/platform/x86/asus-wmi.c
> index 2ccc23b259d3..ab9342a01a48 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3908,6 +3908,16 @@ static int platform_profile_setup(struct
> asus_wmi *asus)
>         if (!asus->throttle_thermal_policy_dev)
>                 return 0;
> 
> +       /*
> +        * We need to set the default thermal profile during probe or
> otherwise
> +        * the system will often remain in silent mode, causing low
> performance.
> +        */
> +       err = throttle_thermal_policy_set_default(asus);
> +       if (err < 0) {
> +               pr_warn("Failed to set default thermal profile\n");
> +               return err;
> +       }
> +
>         dev_info(dev, "Using throttle_thermal_policy for
> platform_profile support\n");
> 
>         asus->platform_profile_handler.profile_get =
> asus_wmi_platform_profile_get;
> --
> 2.39.5
> 

Re: [PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling

[Hans de Goede]

Hi All,

On 25-Oct-24 9:15 PM, Armin Wolf wrote:
> When support for Vivobook fan profiles was added, two mistakes where
> made:
> 
> 1. throttle_thermal_policy_set_default() was not called anymore during
> probe.
> 
> 2. The new thermal profiles where used inconsistently.
> 
> This patch series aims to fix both issues. Compile-tested only.
> 
> Armin Wolf (2):
>   platform/x86: asus-wmi: Fix thermal profile initialization
>   platform/x86: asus-wmi: Fix inconsistent use of thermal policies

Taking another look at the vivobook stuff because of this series this
pre-existing code stands out to me:

static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
{
        struct fan_curve_data *curves;
        u8 buf[FAN_CURVE_BUF_LEN];
        int err, fan_idx;
        u8 mode = 0;

        if (asus->throttle_thermal_policy_dev)
                mode = asus->throttle_thermal_policy_mode;
        /* DEVID_<C/G>PU_FAN_CURVE is switched for OVERBOOST vs SILENT */
        if (mode == 2)
                mode = 1;
        else if (mode == 1)
                mode = 2;


Since the vivobook has silent and overboost swapped I wonder if we should
do this on vivobook to ?

Also note that patch 2/2 of this series impacts this code too. Until
now we were storing the swapped vivobook values in asus->throttle_thermal_policy_dev
and then here we are swapping them a second time, in essence using unswapped
non vivobook values here due to the double swapping.

Where as after Armin's changes from 2/2 we now store unswapped standard
asus laptop values in asus->throttle_thermal_policy_dev and swap them
here, using the same mode values as with normal asus laptops on vivobooks
now ( mode is swapped from non vivo throttle_thermal_policy_dev values).

Does anyone have any insight what we should do here ?

Regards,

Hans

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Hans de Goede]

Hi Armin,

On 25-Oct-24 9:15 PM, Armin Wolf wrote:
> When changing the thermal policy using the platform profile API,
> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
> 
> However everywhere else a normal thermal policy is stored inside this
> variable, potentially confusing the platform profile.

You say "potentially confusing the platform profile", but did you
spot any actual issues when reviewing the code ?

> Fix this by always storing normal thermal policy values inside
> throttle_thermal_policy_mode and only do the conversion when writing
> the thermal policy to hardware.
> 
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

The problem with this approach is that it changes the order in which
we step through the modes in throttle_thermal_policy_switch_next()
after this change we have:

Normal Asus: balanced -> performance -> silent -> balanced -> etc.
Vivobook:    balanced -> silent -> performance -> balanced -> etc.

where if we see "silent" as lower performance then the other 2,
the vivobook order is a bit weird.

I wonder if this is a big enough issue to really worry about it
though; and I do like the cleanup / simpler code.

Note that this also causes a behavior change in
fan_curve_get_factory_default() as I mentioned in my cover-letter.

I think that that behavior change might be a good thing to do actually,
but at a minimum it needs to be documented in the commit msg.

Regards,

Hans






> ---
>  drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ab9342a01a48..ce60835d0303 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
>  /* Throttle thermal policy ****************************************************/
>  static int throttle_thermal_policy_write(struct asus_wmi *asus)
>  {
> -	u8 value = asus->throttle_thermal_policy_mode;
>  	u32 retval;
> +	u8 value;
>  	int err;
> 
> +	if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
> +		switch (asus->throttle_thermal_policy_mode) {
> +		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> +			value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> +			break;
> +		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> +			value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> +			break;
> +		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> +			value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		value = asus->throttle_thermal_policy_mode;
> +	}
> +
>  	err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
>  				    value, &retval);
> 
> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>  static DEVICE_ATTR_RW(throttle_thermal_policy);
> 
>  /* Platform profile ***********************************************************/
> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
> -{
> -	bool vivo;
> -
> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> -	if (vivo) {
> -		switch (mode) {
> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> -		}
> -	}
> -
> -	return mode;
> -}
> -
> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
> -{
> -	bool vivo;
> -
> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> -	if (vivo) {
> -		switch (mode) {
> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> -		}
> -	}
> -
> -	return mode;
> -}
> -
>  static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>  					enum platform_profile_option *profile)
>  {
> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>  	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>  	tp = asus->throttle_thermal_policy_mode;
> 
> -	switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
> +	switch (tp) {
>  	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>  		*profile = PLATFORM_PROFILE_BALANCED;
>  		break;
> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
>  		return -EOPNOTSUPP;
>  	}
> 
> -	asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
> +	asus->throttle_thermal_policy_mode = tp;
>  	return throttle_thermal_policy_write(asus);
>  }
> 
> --
> 2.39.5
> 

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Mohamed Ghanmi]

On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
> When changing the thermal policy using the platform profile API,
> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
> 
> However everywhere else a normal thermal policy is stored inside this
> variable, potentially confusing the platform profile.
> 
> Fix this by always storing normal thermal policy values inside
> throttle_thermal_policy_mode and only do the conversion when writing
> the thermal policy to hardware.
> 
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)

the original patch that i submitted did actually have the remapping
of the different fan profiles in the throttle_thermal_policy_write() methods 
because it was the cleaner solution [1]. however after having a discussion with luke, 
he shared that he might be planning to remove the throttle_thermal_policy sysfs interface 
in favour of platform_profiles [2] because of a refactoring he had been working on.

currently to control fan profiles through this driver you could use
either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
(redundant and might get removed in the future) or through platform profiles which is the
better way of doing things.

for the reasons mentionned above, I decided to keep
throttle_therma_policy_write() unchanged and to move the remapping logic
to the asus_wmi_platform_profile_set(). this adopts the approach of
having a logical mapping stored in asus_wmi struct that has to be
converted to a physical mapping whenever needed [3].

so, if luke thinks that this won't cause any merge conflicts with his
work [4] then i see no problem with this approach even though it might cause an
order change when calling throttle_thermal_policy_switch_next()

Best Regards,
Mohamed G.

Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4] 

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Hans de Goede]

Hi,

On 26-Oct-24 12:45 PM, Mohamed Ghanmi wrote:
> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
>>
>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>  drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> the original patch that i submitted did actually have the remapping
> of the different fan profiles in the throttle_thermal_policy_write() methods 
> because it was the cleaner solution [1]. however after having a discussion with luke, 
> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface 
> in favour of platform_profiles [2] because of a refactoring he had been working on.
> 
> currently to control fan profiles through this driver you could use
> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
> (redundant and might get removed in the future) or through platform profiles which is the
> better way of doing things.
> 
> for the reasons mentionned above, I decided to keep
> throttle_therma_policy_write() unchanged and to move the remapping logic
> to the asus_wmi_platform_profile_set(). this adopts the approach of
> having a logical mapping stored in asus_wmi struct that has to be
> converted to a physical mapping whenever needed [3].
> 
> so, if luke thinks that this won't cause any merge conflicts with his
> work [4] then i see no problem with this approach even though it might cause an
> order change when calling throttle_thermal_policy_switch_next()

Talking about throttle_thermal_policy_switch_next() we also
have platform_profile_cycle() and since asus-wmi supports
platform-profiles now I'm wondering if it would not be better
to simply completely drop throttle_thermal_policy_switch_next()
and call platform_profile_cycle() instead?

This will also keep the cycle order the same for "normal" vs
vivo even after Armin's patch.

Anyways I'll go and apply patch 1/2 to pdx86/fixes since that one is
obviously correct and fixes th Lunar Lake performance issues.

And we can keep discussing what to do wrt 2/2 and maybe also drop
throttle_thermal_policy_switch_next() if favor of
platform_profile_cycle().

Regards,

Hans






> 
> Best Regards,
> Mohamed G.
> 
> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4] 
> 

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Armin Wolf]

Am 26.10.24 um 11:56 schrieb Hans de Goede:

> Hi Armin,
>
> On 25-Oct-24 9:15 PM, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
> You say "potentially confusing the platform profile", but did you
> spot any actual issues when reviewing the code ?
>
Yes, for example:

1. User sets thermal policy to "1" (overboost) through the throttle_thermal_policy sysfs attr.

2. "1" gets stored inside throttle_thermal_policy_mode.

3. Platform profile will now think that ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO (1) is set.

4. Platform profile reports current mode as PLATFORM_PROFILE_QUIET.

=> error!

>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> The problem with this approach is that it changes the order in which
> we step through the modes in throttle_thermal_policy_switch_next()
> after this change we have:
>
> Normal Asus: balanced -> performance -> silent -> balanced -> etc.
> Vivobook:    balanced -> silent -> performance -> balanced -> etc.
>
> where if we see "silent" as lower performance then the other 2,
> the vivobook order is a bit weird.
>
> I wonder if this is a big enough issue to really worry about it
> though; and I do like the cleanup / simpler code.

I think users expect the normal Asus switching behavior, but you are right
that this change should be explained inside the commit message.

>
> Note that this also causes a behavior change in
> fan_curve_get_factory_default() as I mentioned in my cover-letter.

Since fan_curve_get_factory_default() was introduced before commit bcbfcebda2cb,
i think this patches actually fixes the behavior of this function.

>
> I think that that behavior change might be a good thing to do actually,
> but at a minimum it needs to be documented in the commit msg.
>
> Regards,
>
> Hans
>
I will wait till the maintainer of asus-wmi can clarify whether or not
fan_curve_get_factory_default() expects normal Asus thermal policy values
or not.

Once we resolved this i will send an updated series.

Thanks,
Armin Wolf

>> ---
>>   drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>   1 file changed, 21 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index ab9342a01a48..ce60835d0303 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
>>   /* Throttle thermal policy ****************************************************/
>>   static int throttle_thermal_policy_write(struct asus_wmi *asus)
>>   {
>> -	u8 value = asus->throttle_thermal_policy_mode;
>>   	u32 retval;
>> +	u8 value;
>>   	int err;
>>
>> +	if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
>> +		switch (asus->throttle_thermal_policy_mode) {
>> +		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> +			break;
>> +		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> +			break;
>> +		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		value = asus->throttle_thermal_policy_mode;
>> +	}
>> +
>>   	err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
>>   				    value, &retval);
>>
>> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>>   static DEVICE_ATTR_RW(throttle_thermal_policy);
>>
>>   /* Platform profile ***********************************************************/
>> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
>> -{
>> -	bool vivo;
>> -
>> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> -	if (vivo) {
>> -		switch (mode) {
>> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> -		}
>> -	}
>> -
>> -	return mode;
>> -}
>> -
>> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
>> -{
>> -	bool vivo;
>> -
>> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> -	if (vivo) {
>> -		switch (mode) {
>> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>> -		}
>> -	}
>> -
>> -	return mode;
>> -}
>> -
>>   static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>>   					enum platform_profile_option *profile)
>>   {
>> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>>   	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>>   	tp = asus->throttle_thermal_policy_mode;
>>
>> -	switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
>> +	switch (tp) {
>>   	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>   		*profile = PLATFORM_PROFILE_BALANCED;
>>   		break;
>> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
>>   		return -EOPNOTSUPP;
>>   	}
>>
>> -	asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
>> +	asus->throttle_thermal_policy_mode = tp;
>>   	return throttle_thermal_policy_write(asus);
>>   }
>>
>> --
>> 2.39.5
>>
>

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Armin Wolf]

Am 26.10.24 um 12:45 schrieb Mohamed Ghanmi:

> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
>>
>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>   1 file changed, 21 insertions(+), 43 deletions(-)
> the original patch that i submitted did actually have the remapping
> of the different fan profiles in the throttle_thermal_policy_write() methods
> because it was the cleaner solution [1]. however after having a discussion with luke,
> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
> in favour of platform_profiles [2] because of a refactoring he had been working on.
>
> currently to control fan profiles through this driver you could use
> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
> (redundant and might get removed in the future) or through platform profiles which is the
> better way of doing things.
>
> for the reasons mentionned above, I decided to keep
> throttle_therma_policy_write() unchanged and to move the remapping logic
> to the asus_wmi_platform_profile_set(). this adopts the approach of
> having a logical mapping stored in asus_wmi struct that has to be
> converted to a physical mapping whenever needed [3].
>
> so, if luke thinks that this won't cause any merge conflicts with his
> work [4] then i see no problem with this approach even though it might cause an
> order change when calling throttle_thermal_policy_switch_next()

Ok, i will wait for Luke to give feedback on this series. In my opinion the order change is
ok since users likely expect the "old" Asus switching order.

>
> Best Regards,
> Mohamed G.
>
> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
>

Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Armin Wolf]

Am 26.10.24 um 12:59 schrieb Hans de Goede:

> Hi,
>
> On 26-Oct-24 12:45 PM, Mohamed Ghanmi wrote:
>> On Fri, Oct 25, 2024 at 09:15:14PM +0200, Armin Wolf wrote:
>>> When changing the thermal policy using the platform profile API,
>>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>>
>>> However everywhere else a normal thermal policy is stored inside this
>>> variable, potentially confusing the platform profile.
>>>
>>> Fix this by always storing normal thermal policy values inside
>>> throttle_thermal_policy_mode and only do the conversion when writing
>>> the thermal policy to hardware.
>>>
>>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>>   1 file changed, 21 insertions(+), 43 deletions(-)
>> the original patch that i submitted did actually have the remapping
>> of the different fan profiles in the throttle_thermal_policy_write() methods
>> because it was the cleaner solution [1]. however after having a discussion with luke,
>> he shared that he might be planning to remove the throttle_thermal_policy sysfs interface
>> in favour of platform_profiles [2] because of a refactoring he had been working on.
>>
>> currently to control fan profiles through this driver you could use
>> either /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
>> (redundant and might get removed in the future) or through platform profiles which is the
>> better way of doing things.
>>
>> for the reasons mentionned above, I decided to keep
>> throttle_therma_policy_write() unchanged and to move the remapping logic
>> to the asus_wmi_platform_profile_set(). this adopts the approach of
>> having a logical mapping stored in asus_wmi struct that has to be
>> converted to a physical mapping whenever needed [3].
>>
>> so, if luke thinks that this won't cause any merge conflicts with his
>> work [4] then i see no problem with this approach even though it might cause an
>> order change when calling throttle_thermal_policy_switch_next()
> Talking about throttle_thermal_policy_switch_next() we also
> have platform_profile_cycle() and since asus-wmi supports
> platform-profiles now I'm wondering if it would not be better
> to simply completely drop throttle_thermal_policy_switch_next()
> and call platform_profile_cycle() instead?
>
> This will also keep the cycle order the same for "normal" vs
> vivo even after Armin's patch.
>
> Anyways I'll go and apply patch 1/2 to pdx86/fixes since that one is
> obviously correct and fixes th Lunar Lake performance issues.
>
> And we can keep discussing what to do wrt 2/2 and maybe also drop
> throttle_thermal_policy_switch_next() if favor of
> platform_profile_cycle().
>
> Regards,
>
> Hans

Good idea, using platform_profile_cycle() would also solve a potential locking issue here.

Thanks,
Armin Wolf

>> Best Regards,
>> Mohamed G.
>>
>> Link: https://lore.kernel.org/platform-driver-x86/20240421194320.48258-2-mohamed.ghanmi@supcom.tn/ # [1]
>> Link: https://lore.kernel.org/platform-driver-x86/4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com/ # [2]
>> Link: https://lore.kernel.org/platform-driver-x86/ZnlEuiP4Dgqpf51C@laptop/ # [3]
>> Link: https://lore.kernel.org/platform-driver-x86/20240930000046.51388-1-luke@ljones.dev/ # [4]
>>
>

Re: [PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling

[Bowman, Casey G]

Ran a quick test with the two patches on top of drm-tip while monitoring the sensor's temperature as well as thermald logs.

While running "stress -c 8" for around two minutes for each mode, I can see the SEN1 temperature plateau at the expected areas for the various power modes:

- Power Saver
    - Max temp observed: 51 C
    - Trip temperature: 51 C

- Balanced
    - Max temp observed: 56 C
    - Trip temperature: 56 C

- Performance
    - Max temp observed: 61 C
    - Trip temperature: 61 C

Cycling from higher power modes to lower power modes works as expected, with the temperatures falling back to the peak values listed for the given mode.

From my observations, these patches are working as intended.

Regards,
Casey

________________________________________
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Sent: Friday, October 25, 2024 9:09 PM
To: Armin Wolf <W_Armin@gmx.de>; corentin.chary@gmail.com <corentin.chary@gmail.com>; luke@ljones.dev <luke@ljones.dev>; Ghanmi, Mohamed <mohamed.ghanmi@supcom.tn>; Bowman, Casey G <casey.g.bowman@intel.com>
Cc: hdegoede@redhat.com <hdegoede@redhat.com>; ilpo.jarvinen@linux.intel.com <ilpo.jarvinen@linux.intel.com>; Michael@phoronix.com <Michael@phoronix.com>; Bowman, Casey G <casey.g.bowman@intel.com>; platform-driver-x86@vger.kernel.org <platform-driver-x86@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/2] platform/x86: asus-wmi: Fix thermal profile handling
 
+Casey

On Fri, 2024-10-25 at 21:15 +0200, Armin Wolf wrote:
> When support for Vivobook fan profiles was added, two mistakes where
> made:
>
> 1. throttle_thermal_policy_set_default() was not called anymore
> during
> probe.
>
> 2. The new thermal profiles where used inconsistently.
>
> This patch series aims to fix both issues. Compile-tested only.
>
Thanks for these patches. The first one I already tested with the same
change, for the second one added Casey to check if he can give a quick
test for both on the new Asus Lunar Lake laptop.

Thanks,
Srinivas




> Armin Wolf (2):
>   platform/x86: asus-wmi: Fix thermal profile initialization
>   platform/x86: asus-wmi: Fix inconsistent use of thermal policies
>
>  drivers/platform/x86/asus-wmi.c | 74 ++++++++++++++-----------------
> --
>  1 file changed, 31 insertions(+), 43 deletions(-)
>
> --
> 2.39.5
>