linux-kernel - Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200415111336.GZ3456981@phenom.ffwll.local>
Date:   Wed, 15 Apr 2020 13:13:36 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     Yussuf Khalil <dev@...345.net>, Daniel Vetter <daniel@...ll.ch>,
        Simon Ser <contact@...rsion.fr>,
        David Airlie <airlied@...ux.ie>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>
Subject: Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB
 quantization range" property

On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Yussuf Khalil <dev@...345.net> wrote:
> > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> >> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> >> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@...ux.intel.com>
> >> > wrote:
> >> > > On Mon, 13 Apr 2020, Simon Ser <contact@...rsion.fr> wrote:
> >> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> >> > > > dev@...345.net> wrote:
> >> > > > 
> >> > > > > DRM now has a globally available "RGB quantization range"
> >> > > > > connector
> >> > > > > property. i915's "Broadcast RGB" that fulfils the same
> >> > > > > purpose is now
> >> > > > > considered deprecated, so drop it in favor of the DRM
> >> > > > > property.
> >> > > > 
> >> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
> >> > > > space
> >> > > > might depend on this property, dropping it would break such
> >> > > > user-space.
> >> > > 
> >> > > Agreed.
> >> > > 
> >> > > > Can we make this property deprecated but still keep it for
> >> > > > backwards
> >> > > > compatibility?
> >> > > 
> >> > > Would be nice to make the i915 specific property an "alias" for
> >> > > the new
> >> > > property, however I'm not sure how you'd make that happen.
> >> > > Otherwise
> >> > > juggling between the two properties is going to be a nightmare.
> >> > 
> >> > Ah, the obvious easy choice is to use the property and enum names
> >> > already being used by i915 and gma500, and you have no problem.
> >> > Perhaps
> >> > they're not the names you'd like, but then looking at the total
> >> > lack of
> >> > consistency across property naming makes them fit right in. ;)
> >> 
> >> Yeah if we don't have contradictory usage across drivers when
> >> modernizing
> >> these properties, then let's just stick with the names already there.
> >> It's
> >> not pretty, but works better since more userspace/internet howtos
> >> know how
> >> to use this stuff.
> >> -Daniel
> >
> > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
> > "Automatic" option, whereas gma500 does not.
> 
> Adding "Automatic" option that just defaults to "Full" in gma500 does
> not break existing userspace, but allows for extending it to do more
> clever things later.

gma500 could keep it's own property, without the "Automatic" value. We've
done tricks like this for other properties too.

> > Also, radeon has a property called
> > "output_csc" that fulfills a similar purpose. Looking at the code, though, it
> > seems that radeon does not adhere to the standard correctly (or I am missing
> > something).
> >
> > An alternative would be to leave the existing driver-specific properties and
> > change them to be pseudo-aliases for the "RGB quantization range" property.
> > This can be done by letting the drivers read from and write to the new property
> > when user-space tries to read or modify the driver's property. This way we could
> > retain full backwards compatibility for all drivers equally.
> >
> > What do you think?
> 
> I'm obviously biased and predisposed to avoid adding extra complexity to
> i915 when it's not necessary. We'd have *two* connector properties for
> the same thing until the end of time, even if one is an alias for the
> other.

Yeah just pick one, and implement the others as aliases. Drivers can do
the aliases in their atomic_get/set_property functions pretty easily,
atomic properties aren't stored anywhere else than decoded (which was done
partially to make stuff like this work).

Coming up a new property name just so that everyone suffers equally feels
silly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch