Re: [PATCH v11] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Tue, Sep 28 2021 at 13:59:31 +0200, Hans de Goede 
<hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 9/28/21 1:56 PM, Luke Jones wrote:
> >  Hmm,
> >  A change via /sys/firmware/acpi/platform_profile does disable the 
> > fan-curve until a user re-enables it.
>
> Ah ok, so did get that part right :)
>
> So basically any write to /sys/firmware/acpi/platform_profile
> will reset the pwm1_enable to "2" if it was not "2" already.
>
> >  It doesn't wipe the actual curve setting though, I thought that 
> > would be bad UX,
>
> Ok that is fine.
>
> >  but yes the curve is definitely disabled on profile change and will 
> > remain disabled until turned on again. At which point another 
> > profile change will disable it again.
> >
> >  And as stated in previous reply use of 
> > /sys/devices/platform/asus-nb-wmi/hwmon/hwmon<N>/pwm<N>_enable to 
> > check the status is stabilised (1 = manual fan).
> >
> >  Looking at it with fresh eyes I just spotted some things I can 
> > clean up further. Very sorry, there'll be a v15 :(
>
> No worries, maybe wait a bit with posting v15 till Bastien has a 
> chance
> to way in on this discussion though.

No problem at all. Very little will change except for code clean up :)

> Regards,
>
> Hans
>
>
>
> >  On Tue, Sep 28 2021 at 13:44:32 +0200, Hans de Goede 
> > <hdegoede@xxxxxxxxxx> wrote:
> > >  Hi,
> > >
> > >  On 9/28/21 1:36 PM, Bastien Nocera wrote:
> > > >   On Wed, 2021-09-08 at 11:22 +1200, Luke D. Jones wrote:
> > > > >   Add support for custom fan curves found on some ASUS ROG 
> > > > > laptops.
> > > > >
> > > > >   These laptops have the ability to set a custom curve for the CPU
> > > > >   and GPU fans via two ACPI methods.
> > > > >
> > > > >   This patch adds two pwm<N> attributes to the hwmon sysfs,
> > > > >   pwm1 for CPU fan, pwm2 for GPU fan. Both are under the hwmon of 
> > > > > the
> > > > >   name `asus_custom_fan_curve`. There is no safety check of the 
> > > > > set
> > > > >   fan curves - this must be done in userspace.
> > > > >
> > > > >   The fans have settings [1,2,3] under pwm<N>_enable:
> > > > >   1. Enable and write settings out
> > > > >   2. Disable and use factory fan mode
> > > > >   3. Same as 2, additionally restoring default factory curve.
> > > > >
> > > > >   Use of 2 means that the curve the user has set is still stored 
> > > > > and
> > > > >   won't be erased, but the laptop will be using its default 
> > > > > auto-fan
> > > > >   mode. Re-enabling the manual mode then activates the curves 
> > > > > again.
> > > > >
> > > > >   Notes:
> > > > >   - pwm<N>_enable = 0 is an invalid setting.
> > > > >   - pwm is actually a percentage and is scaled on writing to 
> > > > > device.
> > > >
> > > >   I was trying to update:
> > > >
> > > >  
> > > > https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/merge_requests/80
> > > >   but I don't understand what files I need to check for what 
> > > > values to
> > > >   detect whether custom fan curves were used.
> > > >
> > > >   Can you help me out here?
> > >
> > >  How to deal with this is actually one of my remaining questions 
> > > too.
> > >
> > >  I've not looked at the new code closely yet, but if I understand
> > >  things correctly, the now code basically only allows to set 1
> > >  custom profile and setting that profile overrides the last
> > >  profile set through /sys/firmware/acpi/platform_profile.
> > >
> > >  And any write to /sys/firmware/acpi/platform_profile will
> > >  overwrite / replace the last custom set profile (if any) with
> > >  the one matching the requested platform-profile.
> > >
> > >  So basically users of custom fan profiles are expected to
> > >  disable power-profiles-daemon or at least to refrain from
> > >  making any platform_profile changes.
> > >
> > >  And if power-profile-daemon is actually active and
> > >  makes a change then any custom settings will be thrown away,
> > >  IOW p-p-d will always win. So I believe that it no longer needs
> > >  to check for custom profiles, since any time it requests a
> > >  standard profile that will overwrite any custom profile
> > >  which may be present.
> > >
> > >  Luke, do I have that right ?
> > >
> > > >   Also, was this patch accepted in the pdx86 tree?
> > >
> > >  No, I still need to find/make some time to review it and
> > >  I still have the same question as you :)
> > >
> > >  Regards,
> > >
> > >  Hans