[PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum

Tue Feb 14 21:18:49 UTC 2023

On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:
> > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <harry.wentland at amd.com> wrote:
> > >
> > >
> > >
> > > On 2/14/23 10:49, Sebastian Wick wrote:
> > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> > > > <ville.syrjala at linux.intel.com> wrote:
> > > >>
> > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > > >>>
> > > >>>
> > > >>> On 2/3/23 10:19, Ville Syrjälä wrote:
> > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > > >>>>>
> > > >>>>>
> > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > >>>>>> <ville.syrjala at linux.intel.com> wrote:
> > > >>>>>>>
> > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > >>>>>>>> Userspace has no way of controlling or knowing the pixel encoding
> > > >>>>>>>> currently, so there is no way for it to ever get the right values here.
> > > >>>>>>>
> > > >>>>>>> That applies to a lot of the other values as well (they are
> > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > > >>>>>>> added to for use userspace to control the pixel encoding/colorspace
> > > >>>>>>> conversion(if desired, or userspace just makes sure to
> > > >>>>>>> directly feed in correct kind of data).
> > > >>>>>>
> > > >>>>>> I'm all for getting userspace control over pixel encoding but even
> > > >>>>>> then the kernel always knows which pixel encoding is selected and
> > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > > >>>>>> want to control the variant explicitly to the wrong value?
> > > >>>>>>
> > > >>>>>
> > > >>>>> I've asked this before but haven't seen an answer: Is there an existing
> > > >>>>> upstream userspace project that makes use of this property (other than
> > > >>>>> what Joshua is working on in gamescope right now)? That would help us
> > > >>>>> understand the intent better.
> > > >>>>
> > > >>>> The intent was to control the infoframe colorimetry bits,
> > > >>>> nothing more. No idea what real userspace there was, if any.
> > > >>>>
> > > >>>>>
> > > >>>>> I don't think giving userspace explicit control over the exact infoframe
> > > >>>>> values is the right thing to do.
> > > >>>>
> > > >>>> Only userspace knows what kind of data it's stuffing into
> > > >>>> the pixels (and/or how it configures the csc units/etc.) to
> > > >>>> generate them.
> > > >>>>
> > > >>>
> > > >>> Yes, but userspace doesn't control or know whether we drive
> > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver
> > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There
> > > >>> is currently no way for userspace to know that and I don't
> > > >>> think it makes sense.
> > > >>
> > > >> People want that control as well for whatever reason. We've
> > > >> been asked to allow YCbCr 4:4:4 output many times.
> > > >
> > > > I don't really think it's a question of if we want it but rather how
> > > > we get there. Harry is completely right that if we would make the
> > > > subsampling controllable by user space instead of the kernel handling
> > > > it magically, user space which does not adapt to the new control won't
> > > > be able to light up some modes which worked before.
> > > >
> > >
> > > Thanks for continuing this discussion and touching on the model of how
> > > we get to where we want to go.
> > >
> > > > This is obviously a problem and not one we can easily fix. We would
> > > > need a new cap for user space to signal "I know that I can control
> > > > bpc, subsampling and compression to lower the bandwidth and light up
> > > > modes which otherwise fail". That cap would also remove all the
> > > > properties which require kernel magic to work (that's also what I
> > > > proposed for my KMS color pipeline API).
> > > >
> > > > We all want to expose more of the scanout capability and give user
> > > > space more control but I don't think an incremental approach works
> > > > here and we would all do better if we accept that the current API
> > > > requires kernel magic to work and has a few implicit assumptions baked
> > > > in.
> > > >
> > > > With all that being said, I think the right decision here is to
> > > >
> > > > 1. Ignore subsampling for now
> > > > 2. Let the kernel select YCC or RGB on the cable
> > > > 3. Let the kernel figure out the conversion between RGB and YCC based
> > > > on the color space selected
> > > > 4. Let the kernel send the correct infoframe based on the selected
> > > > color space and cable encoding
> > > > 5. Only expose color spaces for which the kernel can do the conversion
> > > > and send the infoframe
> > >
> > > I agree. We don't want to break or change existing behavior (that is
> > > used by userspace) and this will get us far without breaking things.
> > >
> > > > 6. Work on the new API which is hidden behind a cap
> > > >
> > >
> > > I assume you mean something like
> > > https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> >
> > Something like that, yes. The main point being a cap which removes a
> > lot of properties and sets new expectations between user space and
> > kernel. The actual API is not that important.
> >
> > > Above you say that you don't think an incremental approach works
> > > here. Can you elaborate?
> >
> > Backwards compatibility is really hard. If we add another property to
> > control e.g. the color range infoframe which doesn't magically convert
> > colors, we now have to define how it interacts with the existing
> > property.
>
> I think that's easy. The old prop simply override the
> infoframe/etc. stuff. Pretty sure that's how we have it
> implemented in i915 since the start.
>
> And if you set it wrong it's you own fault.

We need the "old" property to control both the conversion and the
infoframe and we need the "new" property to control only the
infoframe. So if we don't want to old property to to any conversion we
now also need a new value for the old property which disables the
property. So why not just go for a cap and remove the property
altogether?

And again, as long as user space doesn't have complete control over
the entire pipeline we can't use the property which only sets the
infoframe anyway so you can't even have an incremental approach here.

Let's turn the question around: what do we gain by keeping all the
properties and implicit assumptions in the current API around? They
make everything more complex so what's the benefit?

I also want to be able to clearly document all the requirements of
user space and the guarantees from the kernel in the new API which we
can't do when we just incrementally change some properties here and
there. The documentation right now is basically a joke and almost
nothing has been defined properly and when things work, they work by
accident. Purely from a documentation and usability POV the
incremental approach is already going to be horrible IMO.

>
> > We also have to figure out how a user space which doesn't
> > know about the new property behaves when another client has set that
> > property. If any property which currently might change the pixel
> > values is used, we can't expose the entire color pipeline because the
> > kernel might have to use some element in it to achieve its magic
> > conversion. So essentially you already have this hard device between
> > "old" and "new" and you can't use the new stuff incrementally.
>
> That problem exists with any new property. Old userspace and new
> userspace may interact badly enought that nothing works right.
> In that sense I think these props might even be pretty mundane
> as the worst you might get from setting the infoframe wrong is
> perhaps wrong colors on your display.
>
> To solve that particular problem there has been talk (for years)
> about some kind of "reset all" knob to make sure everything is
> at a safe default value. I have a feeling there was even some
> kind of semi-real proposal in recent times, but maybe I imgained
> it?

I'm one of the people who argued for this but met only resistance. It
sure would be nice to have but also doesn't help with a lot of the
issues.

> Right now I guess what you do is either just reboot, or muck
> around with modetest to manually reset properties.
>
> I have occasionally abused this btw, to set some prop with
> modetest and then run some real userspace that doesn't even
> know about said property. Easy way to test stuff :P
>
> --
> Ville Syrjälä
> Intel
>