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

From: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
To: Hans de Goede <hdegoede@redhat.com>, Bastien Nocera <bnocera@redhat.com>
Cc: markgross@kernel.org, platform-driver-x86@vger.kernel.org,
	Patil.Reddy@amd.com
Subject: Re: [PATCH 0/4] platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF
Date: Tue, 6 Sep 2022 15:23:45 +0530	[thread overview]
Message-ID: <d72d77c0-83bb-99b4-03ed-bb9a5b03f35e@amd.com> (raw)
In-Reply-To: <a528cbe2-847b-9b15-5123-536900e0d943@redhat.com>

Hi Hans,

Apologies for the delay in responding to this thread. Some responses below.

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. Can we not have a
documentation update saying it is advised to turn "off" CnQF when
battery % goes below a certain level? That way, the end user experiences
across Linux and Windows remains the same.

> 
> 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). This would need some amount of discussion with our
windows folks also to know what they think about it.

Please let me know your thoughts based on the above notes what I
provided(in the earlier section), so that we can make the code changes
accordingly.

Thanks,
Shyam

> 
> ###
> 
> 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.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> and for toggling ooff through sysfs
> 