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@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. >>> >>> 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
