[PATCH] drm/amd: Only allow one entity to control ABM

[Mario Limonciello]

On 2/16/2024 09:41, Christian König wrote:
> Am 16.02.24 um 16:12 schrieb Mario Limonciello:
>> On 2/16/2024 09:05, Harry Wentland wrote:
>>>
>>>
>>> On 2024-02-16 09:47, Christian König wrote:
>>>> Am 16.02.24 um 15:42 schrieb Mario Limonciello:
>>>>> On 2/16/2024 08:38, Christian König wrote:
>>>>>> Am 16.02.24 um 15:07 schrieb Mario Limonciello:
>>>>>>> By exporting ABM to sysfs it's possible that DRM master and software
>>>>>>> controlling the sysfs file fight over the value programmed for ABM.
>>>>>>>
>>>>>>> Adjust the module parameter behavior to control who control ABM:
>>>>>>> -2: DRM
>>>>>>> -1: sysfs (IE via software like power-profiles-daemon)
>>>>>>
>>>>>> Well that sounds extremely awkward. Why should a 
>>>>>> power-profiles-deamon has control over the panel power saving 
>>>>>> features?
>>>>>>
>>>>>> I mean we are talking about things like reducing backlight level 
>>>>>> when the is inactivity, don't we?
>>>>>
>>>>> We're talking about activating the ABM algorithm when the system is 
>>>>> in power saving mode; not from inactivity.  This allows the user to 
>>>>> squeeze out some extra power "just" in that situation.
>>>>>
>>>>> But given the comments on the other patch, I tend to agree with 
>>>>> Harry's proposal instead that we just drop the DRM property 
>>>>> entirely as there are no consumers of it.
>>>>
>>>> Yeah, but even then the design to let this be controlled by an 
>>>> userspace deamon is questionable. Stuff like that is handled inside 
>>>> the kernel and not exposed to userspace usually.
>>>>
>>
>> Regarding the "how" and "why" of PPD; besides this panel power savings 
>> sysfs file there are two other things that are nominally changed.
>>
>> ACPI platform profile: 
>> https://www.kernel.org/doc/html/latest/userspace-api/sysfs-platform_profile.html
>>
>> AMD-Pstate EPP value: 
>> https://www.kernel.org/doc/html//latest/admin-guide/pm/amd-pstate.html
>>
>> When a user goes into "power saving" mode both of those are tweaked. 
>> Before we introduced the EPP tweaking in PPD we did discuss a callback 
>> within the kernel so that userspace could change "just" the ACPI 
>> platform profile and everything else would react.  There was pushback 
>> on this, and so instead knobs are offered for things that should be 
>> tweaked and the userspace daemon can set up policy for what to do when 
>> a a user uses a userspace client (such as GNOME or KDE) to change the 
>> desired system profile.
> 
> Ok, well who came up with the idea of the userspace deamon? Cause I 
> think there will be even more push back on this approach.
> 
> Basically when we go from AC to battery (or whatever) the drivers 
> usually handle that all inside the kernel today. Involving userspace is 
> only done when there is a need for that, e.g. inactivity detection or 
> similar.
> 

It's more than AC vs battery.  It's user preference of how they want to 
use the system.

Here's some screenshots of how it all works:

https://linuxconfig.org/how-to-manage-power-profiles-over-d-bus-with-power-profiles-daemon-on-linux

>>>
>>> I think we'll need a bit in our kernel docs describing ABM. 
>>> Assumptions around what it is and does seem to lead to confusion.
>>>
>>> The thing is that ABM has a visual impact that not all users like. It 
>>> would make sense for users to be able to change the ABM level as 
>>> desired, and/or configure their power profiles to select a certain 
>>> ABM level.
>>>
>>> ABM will reduce the backlight and compensate by adjusting brightness 
>>> and contrast of the image. It has 5 levels: 0, 1, 2, 3, 4. 0 means 
>>> off. 4 means maximum backlight reduction. IMO, 1 and 2 look okay. 3 
>>> and 4 can be quite impactful, both to power and visual fidelity.
>>>
>>
>> Aside from this patch Hamza did make some improvements to the kdoc in 
>> the recent patches, can you read through again and see if you think it 
>> still needs improvements?
> 
> Well what exactly is the requirement? That we have different ABM 
> settings for AC and battery? If yes providing different DRM properties 
> would sound like the right approach to me.
> 

It's user wants system in "power-saving" state or they want it in 
"balanced" state or they want it in "performance" state.

User picks that state in a client and there is a designated ABM policy 
value that goes with it.  Daemon programs the ABM value.