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

[Sebastian Wick]

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.

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.

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?

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
>