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

[Sebastian Wick]

On Fri, Aug 2, 2024 at 4:37 PM Harry Wentland <harry.wentland at amd.com> wrote:
>
> 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?

Yes. Even worse, it's not obvious to people that something is broken,
where it is broken and why it is broken.

https://gitlab.gnome.org/GNOME/mutter/-/issues/3589

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

If this was just a KMS property then I wouldn't have anything to
complain about. The problem here is the sysfs / PPD thing.

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

The problem is that if the compositor didn't explicitly opt-in, then
it can break something (and it actually did). I appreciate the intent
(enabling it as broadly as possible) but the only way I can see how
you can enable feature at all is by somehow doing it per-compositor.

Given all of that, there shouldn't be a sysfs property (you should
revert this!) but it should be yet another KMS property.

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