On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 9/1/22 14:24, Bastien Nocera wrote: > > On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> > >> Hi, > >> > >> On 8/23/22 12:29, Shyam Sundar S K wrote: > >>> In this series, support for following features has been added. > >>> - "Cool n Quiet Framework (CnQF)" is an extension to the static slider, > >>> where the system power can be boosted or throttled independent > >>> of the selected slider position. > >>> - On the fly, the CnQF can be turned on/off via a sysfs knob. > >> > >> Thank you. I think that before doing a more in detail review > >> we first need to agree on the userspace interactions here. > >> > >> I've added Bastien, the power-profiles-daemon maintainer > >> to the Cc for this. > >> > >> From a quick peek at the patches I see that currently they do > >> the following: > >> > >> Probe time: > >> ----------- > >> > >> 1. If static slider (classic /sys/firmware/acpi/platform_profile) > >> is available register as a platform_profile provider > >> > >> 2. Query if the BIOS tells us that CnQF should be enable by > >> default if yes then unregister the platform_profile provider > >> and enable CnQF > >> > >> > >> Run time: > >> --------- > >> > >> Allow turning CnQF on/off by writing a sysfs attribute for this. > >> > >> 1. When CnQF gets enabled unregister the platform_profile provider > >> > >> 2. When CnQF gets disabled restore the last set profile and > >> register the platform_profile provider > >> > >> > >> Questions/remarks: > >> > >> 1. If you look at 1. and 2. under "Probe time", you will see that > >> when the BIOS requests to have CnQF enabled by default that > >> userspace will then still shortly see a platform_profile > >> provider. This must be fixed IMHO by checking whether to do > >> CnQF by default or not before the initial register call. > >> > >> 2. What about low-power scenarios ? Currently power-profiles-daemon > >> will always advertise a low-power mode even when there is no > >> platform-profile support, since this is also a hint for other > >> parts of the system to try and conserve power. But when this > >> mode is enabled we really want the system to also behave as > >> if the old static slider mode is active and set to low-power. > >> > >> Some ideas: > >> a) maybe still have the amd-pmf code register a (different) > >> platform_profile provider whn in CnQF mode and have it only > >> advertise low-power > >> > >> b) teach power-profiles-daemon about CnQF and have it > >> disable CnQF when entering low-power mode? > >> > >> c) make the CnQF code in PMF take the charge level into > >> account and have it not go "full throttle" when the chare > >> is below say 25% ? > >> > >> 3. Bastien, can power-profiles-daemon deal with > >> /sys/firmware/acpi/platform_profile disappearing or > >> appearing while it is running? > > > > No, it doesn't. > > > > It expects the platform_profile file to be available on startup, at > > worse with the choices not yet filled in. It doesn't handle the > > platform_profile file going away, it doesn't handle the > > platform_profile_choices file changing after it's been initially > > filled in, and it doesn't support less than one power profile being > > made available, and only supports hiding the performance profile if > > the platform doesn't support it. > > Ok, so this means that if we go with these changes as currently > proposed that if a user uses the sysfs file to turn CnQF on/off > they will need to restart power-profile-daemon. > > I think that that is acceptable given that the user needs to manually > poke things anyway. We should probably document this in the documentation > for the sysfs attribute (as well as in newer versions of the p-p-d > docs/README). > > > Some of those things we could change/fix, some other things will not. > > If the platform_profile_choices file only contained a single item, > > then power-profiles-daemon would just export the "low-power" and > > "balanced" profiles to user-space, as it does on unsupported hardware. > > Right. > > > The profiles in power-profiles-daemon are supposed to show the user > > intent, which having a single setting would effectively nullify. > > Yes that was my understanding too. > > > It's unclear to me how CnQF takes user intent into account (it's also > > unclear to me how that's a low-power setting rather than a combination > > of the existing cool and quiet settings). > > AMD folks, please correct me if any of the below is wrong: > > AFAIK even though it is called CnQF it is more like auto-profile > selection and as such does not take user intent into account > at all. > > It looks at the workload over a somewhat longer time period (say > 5 minutes or so I guess?) and then if that consistently has been > quite high, it will select something similar to performance. > > Where as for a more mixed workload it will select balanced and for > a mostly idle machine it will select low-power. > > I guess this auto feature is best treated the same as unsupported hw. > > > (it's also > > unclear to me how that's a low-power setting rather than a combination > > of the existing cool and quiet settings). > > Even though it is called cool and quiet AFAIK it won't be all that > cool and quiet when running a heavy workload. Which is why I wonder > how to re-conciliate this with showing low-power in e.g. the > GNOME shell system men. Because in essence even if the battery > is low the system will still go full-throttle when confronted > with a heavy workload. > > So selecting low-power would result in the screen-dimming which > I think is part of that, but the CPU's max power-consumption won't > get limited as it would when platform-profiles are supported. > > So I guess this is indeed very much like how p-p-d behaves > on unsupported hw... > > ### > > As mentioned I guess one option would be for CnQF to > still register a platform_profile provider and then in > balanced mode do its CnQF thing and in low-power mode > disable CnQF and apply the static-slider low-power settings > I think that that would work best from things actual > working in a way I would expect the avarage end-user to > expect things to work. > > So p-p-d would then still see platform-profile support > in CnQF mode but with only low-power + balanced advertised. > > Bastien would that work for you? That's something I can make work, yes. > AMD folks would that also work for you ? > > ### > > I'm also wondering if we are going to still export > balanced + low-power modes to userspace in CnQF mode > and disable CnQF in low-power mode then if we > even need a sysfs knob to turn it on/off at all. > > I guess the sysfs knob would then still be useful > to turn it on on systems where it defaults to off > in the BIOS. Might be better to do this as > a kernel-cmdline (module-param) then though, then we > also avoid the problem of platform_profile support > all of a sudden changing underneath's p-p-d's feet. I would say that, you could probably have CnQF transparently replacing the more static "balanced" profile if it is available, and have a separate setting to force enable/disable it as a kernel command-line for debugging or if the BIOS menu doesn't offer it as an option. That way the balanced mode would work like a more refined automatic profile switcher, and make the default experience better, without the disappearing profiles, or the user-space API headaches.
