On Fri, Aug 2, 2024 at 4:37 PM Harry Wentland <harry.wentland@xxxxxxx> 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@xxxxxxxxx> wrote: > >> > >> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula > >> <jani.nikula@xxxxxxxxxxxxxxx>: > >>> > >>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@xxxxxxxxx> wrote: > >>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@xxxxxxxxx>: > >>>>> 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 > >> > > >
