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

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

Hi,

On 9/8/22 12:08, Shyam Sundar S K wrote:
> 
> 
> On 9/7/2022 8:18 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/6/22 11:53, Shyam Sundar S K wrote:
>>> Hi Hans,
>>>
>>> Apologies for the delay in responding to this thread. Some responses below.
>>
>> No worries.
>>
>>> On 9/1/2022 6:14 PM, Hans de Goede 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.
>>>
>>> Yes, You are right. Below is a brief note on how CnQF was designed.
>>>
>>> 1)CnQF – Cool And Quiet Framework - It’s an extension of the static
>>> slider concept wherein PMF dynamically manages system power limits and
>>> fan policy based on system power trends.
>>>
>>> 2)OEM can opt into the feature by defining the CnQF BIOS ACPI method.
>>>
>>> 3)Static slider control and CnQF are mutually exclusive.
>>>
>>> 4)CnQF supports up to 4 modes of operation – Turbo, Performance,
>>> Balanced and Quiet.
>>>
>>> - It can be configured for AC and DC distinctly.
>>> - PMF driver calculates the moving average of system power and switches
>>> the mode of operation.
>>>     *If system power is limited to the threshold of the current mode,
>>> move to the next higher mode
>>>     *If system power is not limited to the threshold of the current
>>> mode, reduce the power budget by moving to the next lower mode.
>>>
>>> 5)CnQF feature control is through Radeon SW (a GUI based tool on Windows)
>>>
>>> To match the behavior on Windows, we kept a sysfs node to turn on/off
>>> the CnQF on the fly like the way it can be done the windows side with
>>> the Radeon SW tool. If you think that having as a module param makes
>>> more sense, I am open to make the change and send a v2.
>>>
>>> Like I mentioned above, on Windows the static slider is absoultely dummy
>>> when the user goes on turns on the CnQF from Radeon SW tool. Based on
>>> the review remarks on the earlier series, we tried to
>>> register/de-register to platform_profile, as per sysfs input (like
>>> setting up and tearing down to platform_profile).
>>>
>>> The Difference between Auto-mode (for thinkpads) and CnQF(for others) is
>>> that:
>>>
>>> - Automode gets turned on only when the slider position is set to
>>> "balanced" in the platform_profile and
>>> - a corresponding AMT ON event is triggered.
>>> - it has 3 internal modes quiet, balanced, performance
>>>
>>> But for CnQF,
>>>
>>> - it is independent of the slider position and there are no ACPI events
>>> for it to get kicked in.
>>> - There are two seperate ACPI methods for AC and DC to get the
>>> corresponding thermal values.
>>> - it has 4 internal modes quiet, balanced, performance, turbo
>>>
>>>
>>> There is already a WIP feature called "policy builder" where the OEMs
>>> can build custom policies, which includes looking at the battery
>>> percentages and making thermal optimizations accordingly. But this was
>>> not taken into consideration while designing the CnQF on windows too. If
>>> we bring in this change for Linux, there maybe differences in the way
>>> the same feature behaves "differently" across OSes.
>>>
>>> Like you mentioned the usecase, where just a compilation can end up in
>>> battery drain if not connected to AC power.
>>
>> Thanks for the explanation above.
>>
>>> Can we not have a
>>> documentation update saying it is advised to turn "off" CnQF when
>>> battery % goes below a certain level?
>>
>> So we would need to document that the user needs to poke
>> the sysfs file when the battery is low ?  That seems very user
>> unfriendly.
>>
>> And also don't want power-performance-daemon to need to know about
>> this AMD specific sysfs knob. That is why we have the standardized
>> platform_profile userspace API.
>>
>>> That way, the end user experiences
>>> across Linux and Windows remains the same.
>>
>> I can understand that you want to keep things the same. If I've
>> read the above correctly then currently the user experience under
>> Windows is that the slider in Windows has been turned into a
>> dummy slider which does not do anything.
>>
>> That IMHO is quite a poor user-experience esp. when users want
>> their battery to last longer because they are going to be e.g.
>> on the road the entire day.
>>
>>>> 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.
>>>
>>> Right. The switch time would be dependent on the "time constant" values
>>> set in the BIOS  which is configurable to the OEMs.
>>>
>>>>
>>>> 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?
>>>>
>>>> AMD folks would that also work for you ?
>>>
>>> If we go with the above proposal it would become very identical to what
>>> is being done with automode (expect the extra "turbo" mode and not
>>> having a AMT event).
>>
>> Yes I think that the AMT mode, where the more dynamic behavior os
>> only done in balanced mode and low-power is still very much a low
>> power-mode (and performance is always tuned for permance) makes
>> a lot more sense from an enduser pov. Then the slider still actually
>> works as expected and in the default balanced mode users will get
>> the benefits of the new CnQF behavior / feature.
>>
>>> This would need some amount of discussion with our
>>> windows folks also to know what they think about it.
>>
>> Ok.
>>
> 
> OK. I get it. Your preference is to have CnQF getting ON only when
> 
> 1. BIOS advertises CnQF is "supported" and/or
> 2. Sysfs knob is set to ON. and
> 3. the static-slider (platform_profile) is set to "balanced"
> 
> In rest of the cases, (low-power or performance) the control would still
> remain with the static-slider so that, user can make his own choices.

Yes that is correct.

> If that's the case, let me have a word with the windows team also on how
> we can have user experiences same across OSes and come back.

Great, thank you.

Regards,

Hans