On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Jani Nikula <[email protected]> wrote:
> > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
> >> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
> > wrote:
> > > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > [email protected]> 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. 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?
Regards
Yussuf
On Tue, 14 Apr 2020, Yussuf Khalil <[email protected]> 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 <[email protected]>
>> > wrote:
>> > > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
>> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
>> > > > [email protected]> 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.
> 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.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Yussuf Khalil <[email protected]> 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 <[email protected]>
> >> > wrote:
> >> > > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
> >> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> >> > > > [email protected]> 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
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 <[email protected]> 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 <[email protected]
> > > > > >
> > > > > wrote:
> > > > > > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
> > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > [email protected]> 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
On Thu, Apr 16, 2020 at 03:44:53PM +0200, Yussuf Khalil wrote:
> 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 <[email protected]> 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 <[email protected]
> > > > > > >
> > > > > > wrote:
> > > > > > > On Mon, 13 Apr 2020, Simon Ser <[email protected]> wrote:
> > > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > > [email protected]> 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.
Sounds best. I just generally refrain from volunteering people for work on
very old and abandoned drivers. But if you're willing to type the code,
I'm happy to take it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch