[PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Harry Wentland]

On 2024-08-02 09:28, Sebastian Wick wrote:
> I'm very unhappy about how this has played out.
> 
> We have a new sysfs property that controls a feature of the display
> path that has been set to a default(!) which changes the color
> behavior! This broke color management for everyone who is on a device
> which supports this feature.
> 

Has this been a problem that people have noticed or complained about?
Are there bug reports?

AFAIK the default is "off" and PPD will enable ABM if in power or
balanced mode and on battery.

> What should have been done is the following:
> 
> * add the KMS property and have it default to "sysfs cannot override this"
> * add the sysfs property after the KMS property has been introduced so
> it stays disabled by default
> * add support for the new property in compositors and let them enable
> this feature only when they allow colors to randomly get broken
> 
> Every other path results in broken colors at least temporarily and is
> breaking user space! The sysfs property *must* be reverted because it
> breaks user space. The KMS property *must* be reverted because it
> didn't meet the merging criteria.
> 

I agree we should revert the KMS property until we have a userspace
implementation that is reviewed and ready to be merged. It was merged
based on a misunderstanding and shouldn't have been merged.

I don't think we should revert the sysfs property. The power savings to
end-users can be significant. I would like to see compositors take
control if it via the KMS property.

> Another note: The only way to make sure that this isn't breaking user
> space is if user space tells the kernel that this is okay. This means
> that the sysfs property can only be used if the compositor grows
> support for the new KMS property and at that point, why do we have the
> sysfs property?
> 

We have a good handful of widely used compositors. We have one PPD
with a replacement for it in the works. A sysfs allows all users
to get the power benefits even if compositors don't explicitly
enable support for power saving features in KMS. The goal of PPD
and co. is power savings while that is not always a primary goal
for all compositors (even though compositors play a large role in
a system's power usage).

Harry

> On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl at gmail.com> wrote:
>>
>> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula
>> <jani.nikula at linux.intel.com>:
>>>
>>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl at gmail.com> wrote:
>>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl at gmail.com>:
>>>>> Merging can only happen once a real world userspace application has
>>>>> implemented support for it. I'll try to do that sometime next week in
>>>>> KWin
>>>>
>>>> Here's the promised implementation:
>>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028
>>>
>>> The requirement is that the userspace patches must be reviewed and ready
>>> for merging into a suitable and canonical upstream project.
>>>
>>> Are they?
>>
>> I've talked about the property with other KWin developers before, but
>> there's indeed no official review for the MR yet.
>> As some new discussions about alternative approaches have started as
>> well, maybe it should be reverted until we're more certain about how
>> to proceed?
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>
>>>> In testing with the patches on top of kernel 6.9.6, setting the
>>>> property to `Require color accuracy` makes the sysfs file correctly
>>>> report "Device or resource busy" when trying to change the power
>>>> saving level, but setting the property to zero doesn't really work.
>>>> Once KWin sets the property to zero, changing the power saving level
>>>> "works" but the screen blanks for a moment (might just be a single
>>>> frame) and reading from the file returns zero again, with the visuals
>>>> and backlight level unchanged as well.
>>>
>>> --
>>> Jani Nikula, Intel
>>
>