Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/6/22 11:59, Shyam Sundar S K wrote:
> Hi Bastien, Hans
> 
> On 9/1/2022 7:04 PM, Bastien Nocera wrote:
>> On Thu, 1 Sept 2022 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 9/1/22 14:24, Bastien Nocera wrote:
>>>> On Thu, 1 Sept 2022 at 13:16, Hans de Goede <hdegoede@redhat.com> 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.
>>
> 
> module param would be fine to force load CnQF if the BIOS does not
> advertise it.
> 
> But I still think, having a sysfs node would still help to give an
> option to the user to "opt-out" of the scenarios where he thinks that
> battery can drain out if CnQF is turned on? Or in any case to turn
> on/off CnQF on the fly, so that he can still switch back to the
> traditional "static slider" based power optimizations.
> 
> Please let me know your thoughts on this?

Having a sysfs file to turn CnQF on/off seems best to me. Users who
want to override the BIOS default at boot can then always do so
with a little script or udev rule writing to the sysfs file.

Regards,

Hans