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

[Yussuf Khalil]

On Wed, 2020-04-15 at 13:13 +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> > On Tue, 14 Apr 2020, Yussuf Khalil <dev@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > > > > >
> > > > > wrote:
> > > > > > On Mon, 13 Apr 2020, Simon Ser <contact@xxxxxxxxxxx> wrote:
> > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > dev@xxxxxxxxxx> 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

Okay, I understand your point. Leaving gma500 without a proper implementation of
the "Automatic" value isn't an option though as that would break the whole
purpose of this patchset: having a property that works precisely the same way
across all drivers. I'll build a patch that implements standards-compliant
behavior for gma500 then, so that it can use the same property as the others.

Regards
Yussuf