Messages in this thread | | | Subject | Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute | From | Mark Pearson <> | Date | Wed, 28 Oct 2020 20:55:02 -0400 |
| |
Thanks Hans and Bastien,
On 28/10/2020 13:23, Hans de Goede wrote: > Hi, > > On 10/28/20 2:45 PM, Bastien Nocera wrote: >> Hey Hans, Mark, >> >> On Tue, 2020-10-27 at 12:42 -0400, Mark Pearson wrote: >>> From: Hans de Goede <hdegoede@redhat.com> >>> >>> On modern systems the platform performance, temperature, fan and >>> other >>> hardware related characteristics are often dynamically configurable. >>> The >>> profile is often automatically adjusted to the load by somei >>> automatic-mechanism (which may very well live outside the kernel). >>> >>> These auto platform-adjustment mechanisms often can be configured >>> with >>> one of several 'platform-profiles', with either a bias towards low- >>> power >> >> Can you please make sure to quote 'platform-profile' and 'profile-name' >> this way all through the document? They're not existing words, and >> quoting them shows that they're attribute names, rather than English. I'm leaning towards changing these to become "platform profile" and "profile name" (no quotes in the actual text). Any objections?
>> >>> consumption or towards performance (and higher power consumption and >>> thermals). >> >> s/thermal/temperature/ >> >> "A thermal" is something else (it's seasonal underwear for me ;) I'm removing that sentence from an earlier review so it's moot, but enjoy your underwear! (which reminds me that I need a new set of thermals for the winter...)
>> >>> Introduce a new platform_profile sysfs API which offers a generic API >>> for >>> selecting the performance-profile of these automatic-mechanisms. >>> >>> Co-developed-by: Mark Pearson <markpearson@lenovo.com> >>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Changes in V1: >>> - Moved from RFC to proposed patch >>> - Added cool profile as requested >>> - removed extra-profiles as no longer relevant >>> >>> .../ABI/testing/sysfs-platform_profile | 66 >>> +++++++++++++++++++ >>> 1 file changed, 66 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-platform_profile >>> >>> diff --git a/Documentation/ABI/testing/sysfs-platform_profile >>> b/Documentation/ABI/testing/sysfs-platform_profile >>> new file mode 100644 >>> index 000000000000..240bd3d7532b >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-platform_profile >>> @@ -0,0 +1,66 @@ >>> +Platform-profile selection (e.g. >>> /sys/firmware/acpi/platform_profile) >>> + >>> +On modern systems the platform performance, temperature, fan and >>> other >>> +hardware related characteristics are often dynamically configurable. >>> The >>> +profile is often automatically adjusted to the load by some >>> +automatic-mechanism (which may very well live outside the kernel). >>> + >>> +These auto platform-adjustment mechanisms often can be configured >>> with >>> +one of several 'platform-profiles', with either a bias towards low- >>> power >>> +consumption or towards performance (and higher power consumption and >>> +thermals). >>> + >>> +The purpose of the platform_profile attribute is to offer a generic >>> sysfs >>> +API for selecting the platform-profile of these automatic- >>> mechanisms. >>> + >>> +Note that this API is only for selecting the platform-profile, it is >>> +NOT a goal of this API to allow monitoring the resulting performance >>> +characteristics. Monitoring performance is best done with >>> device/vendor >>> +specific tools such as e.g. turbostat. >>> + >>> +Specifically when selecting a high-performance profile the actual >>> achieved >>> +performance may be limited by various factors such as: the heat >>> generated >>> +by other components, room temperature, free air flow at the bottom >>> of a >>> +laptop, etc. It is explicitly NOT a goal of this API to let >>> userspace know >>> +about any sub-optimal conditions which are impeding reaching the >>> requested >>> +performance level. >>> + >>> +Since numbers are a rather meaningless way to describe platform- >>> profiles >> >> It's not meaningless, but rather ambiguous. For a range of 1 to 5, is 1 >> high performance, and 5 low power, or vice-versa? > > It is meaningless because the space we are trying to describe with the > profile-names is not 1 dimensional. E.g. as discussed before cool and > low-power are not necessarily the same thing. If you have a better way > to word this I'm definitely in favor of improving the text here.
I'm good with 'ambiguous' here as numbers are (interestingly) ambiguous. I've not thought of anything better Any objections?
> >> >>> +this API uses strings to describe the various profiles. To make sure >>> that >>> +userspace gets a consistent experience when using this API this API >> >> you can remove "when using this API". Ack >> >>> +document defines a fixed set of profile-names. Drivers *must* map >>> their >>> +internal profile representation/names onto this fixed set. >>> + >>> +If for some reason there is no good match when mapping then a new >>> profile-name >>> +may be added. >> >> "for some reason" can be removed. Ack >> >>> Drivers which wish to introduce new profile-names must: >>> +1. Have very good reasons to do so. >> >> "1. Explain why the existing 'profile-names' cannot be used" >> >>> +2. Add the new profile-name to this document, so that future drivers >>> which also >>> + have a similar problem can use the same name. >> >> "2. Add the new 'profile-name' to the documentation so that other >> drivers can use it, as well as user-space knowing clearly what >> behaviour the 'profile-name' corresponds to" How about just : "2. Add the new profile name, along with a clear description of the behaviour, to the documentation."
It should be clear for all 'consumers' - regardless of origin >> >>> + >>> +What: /sys/firmware/acpi/platform_profile_choices >>> +Date: October 2020 >>> +Contact: Hans de Goede <hdegoede@redhat.com> >>> +Description: >>> + Reading this file gives a space separated list of >>> profiles >>> + supported for this device. >> >> "This file contains a space-separated list of profiles..." Ack >> >>> + >>> + Drivers must use the following standard profile- >>> names: >>> + >>> + low-power: Emphasises low power >>> consumption >>> + cool: Emphasises cooler operation >>> + quiet: Emphasises quieter operation >>> + balanced: Balance between low power >>> consumption >>> + and performance >>> + performance: Emphasises performance (and >>> may lead to >>> + higher temperatures and fan >>> speeds) >> >> I'd replace "Emphasises" with either "Focus on" or the US English >> spelling of "Emphasizes". Darn - Google confirms that Emphasizes is more correct now. For some reason that's slightly disappointing :) Ack. >> >>> + Userspace may expect drivers to offer at least >>> several of these >>> + standard profile-names. >> >> Replce "at least several" with "more than one". Ack >> >>> + >>> +What: /sys/firmware/acpi/platform_profile >>> +Date: October 2020 >>> +Contact: Hans de Goede <hdegoede@redhat.com> >>> +Description: >>> + Reading this file gives the current selected profile >>> for this >>> + device. Writing this file with one of the strings >>> from >>> + available_profiles changes the profile to the new >>> value. >> >> Is there another file which explains whether those sysfs value will >> contain a trailing linefeed? > > sysfs APIs are typically created so that they can be used from the shell, > so on read a newline will be added. On write a newline at the end > typically is allowed, but ignored. There are even special helper functions > to deal with properly ignoring the newline on write. > > Regards, > > Hans > > OK - does that need to actually be specified here? Or is that just something I keep in mind for the implementation?
Mark
| |