From: Luke Jones <luke@ljones.dev>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Hans de Goede <hdegoede@redhat.com>,
linux-kernel@vger.kernel.org, pobrn@protonmail.com,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves
Date: Mon, 30 Aug 2021 21:08:32 +1200 [thread overview]
Message-ID: <8QANYQ.I7VLKJ0RGQLF3@ljones.dev> (raw)
In-Reply-To: <XQGMYQ.9VWHCW8VQN7K1@ljones.dev>
On Mon, Aug 30 2021 at 10:20:57 +1200, Luke Jones <luke@ljones.dev>
wrote:
>
>
> On Mon, Aug 30 2021 at 08:55:17 +1200, Luke Jones <luke@ljones.dev>
> wrote:
>>
>>
>> On Sun, Aug 29 2021 at 08:18:01 -0700, Guenter Roeck
>> \x7f<linux@roeck-us.net> wrote:
>>> On 8/29/21 3:03 AM, Luke Jones wrote:
>>>>
>>>>
>>>> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede
>>>> \x7f\x7f\x7f\x7f\x7f<hdegoede@redhat.com> wrote:
>>>>> Hi Luke,
>>>>>
>>>>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>>>> Add support for custom fan curves found on some ASUS ROG
>>>>>> laptops.
>>>>>>
>>>>>> - V1
>>>>>> + Initial patch work
>>>>>> - V2
>>>>>> + Don't fail and remove wmi driver if error from
>>>>>> asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>>>> - V3
>>>>>> + Store the "default" fan curves
>>>>>> + Call throttle_thermal_policy_write() if a curve is erased
>>>>>> to \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fensure
>>>>>> that the factory default for a profile is applied again
>>>>>> - V4
>>>>>> + Do not apply default curves by default. Testers have found
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fthat the
>>>>>> default curves don't quite match actual no-curve behaviours
>>>>>> + Add method to enable/disable curves for each profile
>>>>>> - V5
>>>>>> + Remove an unrequired function left over from previous
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fiterations
>>>>>> + Ensure default curves are applied if user writes " " to a
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fcurve path
>>>>>> + Rename "active_fan_curve_profiles" to
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f"enabled_fan_curve_profiles" to
>>>>>> better reflect the behavious of this setting
>>>>>> + Move throttle_thermal_policy_write_*pu_curves() and rename
>>>>>> to
>>>>>> fan_curve_*pu_write()
>>>>>> + Merge fan_curve_check_valid() and fan_curve_write()
>>>>>> + Remove some leftover debug statements
>>>>>> - V6
>>>>>> + Refactor data structs to store array or u8 instead of
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fstrings.
>>>>>> This affects the entire patch except the enabled_fan_curves
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fblock
>>>>>> + Use sysfs_match_string in enabled_fan_curve block
>>>>>> + Add some extra comments to describe things
>>>>>> + Allow some variation in how fan curve input can be formatted
>>>>>> + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fper
>>>>>> fan+profile combo drastically
>>>>>
>>>>> Thank you for your continued work on this. I read in the
>>>>> discussin \x7f\x7f\x7f\x7f\x7f\x7f\x7fof v5
>>>>> that you discussed using the standard auto_point#_pwm,
>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7fauto_point#_temp
>>>>> pairs. I see here that you have decided to not go that route, may
>>>>> \x7f\x7f\x7f\x7fI \x7f\x7f\x7fask
>>>>> why ?
>>>>
>>>> Sure, primary reason is because the RPM for the fans is in
>>>> \x7f\x7f\x7f\x7f\x7fpercentage so it didn't really make sense to me to use that
>>>> \x7f\x7f\x7fformat.
>>>>
>>>> Also if the max for that is 255 then I'd need to introduce scaling
>>>> \x7f\x7f\x7f\x7f\x7fto make match what the ACPI method expects (max 100). But
>>>> yeah, \x7f\x7f\x7f\x7f\x7fauto_point#_pwm changes the meaning.
>>>>
>>>
>>> Bad argument. That is true for other controllers as well. You could
>>> just scale it up and down as needed.
>>>
>>> The whole point of an ABI is that it is standardized.
>>> If others would [be permitted to] follow your line of argument,
>>> we would not have a useful ABI because "my chip provides/needs
>>> data in some other format".
>>>
>>> Guenter
>>
>> Understood. But lets see if I understand fully:
>>
>> The key part is "pwmX_auto_pointN_temp and pwmX_auto_pointN_pwm",
>> \x7fwith X being a profile, and N the point in the curve graph. If I
>> use \x7fthis format I have:
>>
>> - 3 profiles
>> - each profile has two fans
>> - each fan has 8 points on it
>> - each point has 2 integers
>>
>> so that makes for a total of 96 individual sysfs paths. Is that
>> \x7freally okay? And where would the new paths god?
>> - Under /sys/devices/platform/asus-nb-wmi/ still, or
>> - /sys/devices/platform/asus-nb-wmi/hwmon/ ?
>>
>> I'm currently using SENSOR_DEVICE_ATTR_2_RW with index = profile, nr
>> \x7f= fan. If there weren't profiles involved then I could see it being
>> \x7feasily achieved with that.. Maybe I could use the index(profile)
>> with \x7fa mask to get the fan number.
>>
>> I've done all the groundwork for it at least, so it can certainly be
>> \x7fdone. My only worry is that because of the sheer number of sysfs
>> \x7fpaths being added (96) it could become unwieldy to use.
>>
>> Could I use the existing method + the above?
>
> I've had a bit of a think after morning coffee and I think there is
> another way to do this:
>
> - CPU Fan = pwm1_auto_pointN_pwm + pwm1_auto_pointN_temp
> - GPU Fan = pwm2_auto_pointN_pwm + pwm2_auto_pointN_temp
> for example. So we're not breaking the meaning of anything or making
> things obtuse and complex.
>
> Ending up with:
> /* CPU */
> // (name, function, fan, point)
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_temp, fan_curve, 1,
> 0);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_temp, fan_curve, 1,
> 1);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_temp, fan_curve, 1,
> 2);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_temp, fan_curve, 1,
> 3);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_temp, fan_curve, 1,
> 4);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_temp, fan_curve, 1,
> 5);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_temp, fan_curve, 1,
> 6);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_temp, fan_curve, 1,
> 7);
>
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_pwm, fan_curve, 1, 0);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_pwm, fan_curve, 1, 1);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_pwm, fan_curve, 1, 2);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_pwm, fan_curve, 1, 3);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_pwm, fan_curve, 1, 4);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_pwm, fan_curve, 1, 5);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_pwm, fan_curve, 1, 6);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_pwm, fan_curve, 1, 7);
> /* and the same for GPU fan */
>
> But because we still have 3 profiles to consider, I would propose
> that the settings be show/store dependant on the profile that the
> machine is in, e.g, internally show/store to correct profile via
> checking current profile number active.
>
> I do need some suggestions on what I see as an issue though:
> (1)
> Given that it now becomes difficult to write all the settings at
> once, at what point should I attempt to write the "block" to the
> device? Write out for every change?
> (2)
> Also given the above, how do I reasonably check the user isn't trying
> to create an invalid graph? E.g, lower fan speeds for higher
> temperature? Check that a point isn't higher/lower than neighbouring
> points and expect users to write the points in reverse?
>
> I could maybe also have pwm1_enable and pwm2_enable. Perhaps set this
> to false if a change is made, then only write out the full block if
> it is then set to enabled. Further to this, if the user changes
> profiles and the curves have been previously set and enabled, then
> that curve is written out per usual.
>
> Looking forward to some guidance on this. I'll try making a start on
> what I've proposed above for now.
>
>
>> Many thanks,
>> Luke.
>>
>
I have completed the above now. So I'll now complete testing and then
submit v7. What a journey, learning a lot.
Cheers!
prev parent reply other threads:[~2021-08-30 9:09 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-29 7:14 [PATCH v6 0/1] asus-wmi: Add support for custom fan curves Luke D. Jones
2021-08-29 7:14 ` [PATCH v6] " Luke D. Jones
2021-08-29 9:57 ` [PATCH v6 0/1] " Hans de Goede
2021-08-29 10:03 ` Luke Jones
2021-08-29 15:18 ` Guenter Roeck
2021-08-29 20:55 ` Luke Jones
2021-08-29 22:20 ` Luke Jones
2021-08-30 9:08 ` Luke Jones [this message]
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=8QANYQ.I7VLKJ0RGQLF3@ljones.dev \
--to=luke@ljones.dev \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/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;
as well as URLs for NNTP newsgroup(s).