Re: [PATCH] asus-wmi: Add support for platform_profile

From: Luke Jones
Date: Fri Aug 13 2021 - 03:13:25 EST



On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,

On 8/13/21 7:27 AM, Luke Jones wrote:
I'm unsure of how to update the existing code for fn+f5 (next thermal profile) used by laptops like the TUF series that have keyboard over i2c. I was thinking of the following:

static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
{
struct platform_profile_handler *handler = &asus->platform_profile_handler; // added
u8 new_mode = asus->throttle_thermal_policy_mode + 1;

if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;

// asus->throttle_thermal_policy_mode = new_mode;
// return throttle_thermal_policy_write(asus);
return handler->profile_set(&asus->platform_profile_handler, new_mode); // added
}

* two lines added, two commented

I was going to say it is best to just send a key-press event to userspace
(we can define a new EV_KEY_.... code for this) and then let userspace
handle things. But I see that this is currently already handled by the kernel,
so that is not really an option.

I'm not able to test this though, and there are very few active people in my discord with TUF/i2c laptops to ask for testing also.

My concern here is to get the platform_profile correctly updated. Simply updating asus->throttle_thermal_policy_mode isn't going to achieve what we'll want.

Right, there is no need to go through handler->profile_set() you have defined
profile_set yourself after all and it does not do anything different then the
old code you are replacing here.

The trick is to call platform_profile_notify() after throttle_thermal_policy_write()
this will let userspace, e.g. power-profiles-daemon know that the profile has
been changed and it will re-read it then, resulting in a call to
handler->profile_get()

So the new throttle_thermal_policy_switch_next() would look like this:

static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
{
u8 new_mode = asus->throttle_thermal_policy_mode + 1;
int err; // new

if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;

asus->throttle_thermal_policy_mode = new_mode;

err = throttle_thermal_policy_write(asus); // changed
if (err == 0) // new
platform_profile_notify(); // new

return err; // new
}

As you can see the only new thing here is the
platform_profile_notify() call on a successful write,
which is such a small change that I'm not overly worried about
not being able to test this.

I hope this helps.

Regards,

Hans


On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones <luke@xxxxxxxxxx> wrote:
Add initial support for platform_profile where the support is
based on availability of ASUS_THROTTLE_THERMAL_POLICY.

Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
---
drivers/platform/x86/asus-wmi.c | 112 ++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 90a6a0d00deb..62260043c15c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -26,6 +26,7 @@
#include <linux/rfkill.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
+#include <linux/platform_profile.h>
#include <linux/power_supply.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
@@ -219,6 +220,9 @@ struct asus_wmi {
bool throttle_thermal_policy_available;
u8 throttle_thermal_policy_mode;

+ struct platform_profile_handler platform_profile_handler;
+ bool platform_profile_support;
+
// The RSOC controls the maximum charging percentage.
bool battery_rsoc_available;

@@ -2144,6 +2148,106 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
static DEVICE_ATTR_RW(throttle_thermal_policy);

+/* Platform profile ***********************************************************/
+static int platform_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct asus_wmi *asus;
+ int tp;
+
+ asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
+
+ /* All possible toggles like throttle_thermal_policy here */
+ if (asus->throttle_thermal_policy_available) {
+ tp = asus->throttle_thermal_policy_mode;
+ } else {
+ return -1;
+ }
+
+ if (tp < 0)
+ return tp;
+
+ switch (tp) {
+ case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+ *profile = PLATFORM_PROFILE_QUIET;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int platform_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct asus_wmi *asus;
+ int tp;
+
+ asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
+
+ /* All possible toggles like throttle_thermal_policy here */
+ if (!asus->throttle_thermal_policy_available) {
+ return -1;
+ }
+
+ switch (profile) {
+ case PLATFORM_PROFILE_PERFORMANCE:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+ break;
+ case PLATFORM_PROFILE_QUIET:
+ tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ if (asus->throttle_thermal_policy_available) {
+ asus->throttle_thermal_policy_mode = tp;
+ return throttle_thermal_policy_write(asus);
+ }
+ return 0;
+}
+
+static int platform_profile_setup(struct asus_wmi *asus)
+{
+ int err;
+
+ if (asus->throttle_thermal_policy_available) {
+ pr_info("Using throttle_thermal_policy for platform_profile support\n");
+ } else {
+ /*
+ * Not an error if a component platform_profile relies on is unavailable
+ * so early return, skipping the setup of platform_profile.
+ */
+ return 0;
+ }
+ asus->platform_profile_handler.profile_get = platform_profile_get;
+ asus->platform_profile_handler.profile_set = platform_profile_set;
+
+ set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED,
+ asus->platform_profile_handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE,
+ asus->platform_profile_handler.choices);
+
+ err = platform_profile_register(&asus->platform_profile_handler);
+ if (err)
+ return err;
+
+ asus->platform_profile_support = true;
+ return 0;
+}
+
/* Backlight ******************************************************************/

static int read_backlight_power(struct asus_wmi *asus)
@@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct platform_device *pdev)
else
throttle_thermal_policy_set_default(asus);

+ err = platform_profile_setup(asus);
+ if (err)
+ goto fail_platform_profile_setup;
+
err = panel_od_check_present(asus);
if (err)
goto fail_panel_od;
@@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct platform_device *pdev)
asus_wmi_sysfs_exit(asus->platform_device);
fail_sysfs:
fail_throttle_thermal_policy:
+fail_platform_profile_setup:
fail_fan_boost_mode:
fail_egpu_enable:
fail_dgpu_disable:
@@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct platform_device *device)
asus_fan_set_auto(asus);
asus_wmi_battery_exit(asus);

+ if (asus->platform_profile_support)
+ platform_profile_remove();
+
kfree(asus);
return 0;
}
--
2.31.1





Hi Hans,

Very helpful, thanks. I'd completely failed to notice platform_profile_notify in the platform_profile.h :| I've now put that in throttle_thermal_policy_write() just after sysfs_notify().

I will submit new version after completing other feedback.

Many Thanks,
Luke.