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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 14, 2023 at 10:18:49PM +0100, Sebastian Wick wrote:
> On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä
> <ville.syrjala@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@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@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@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.

Wait what? The old property only controls the infoframe.
The new property (last I looked) was supposed to do more.
Or did that plan change already when I wasn't paying attention.

> 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
> >

-- 
Ville Syrjälä
Intel