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

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

On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
> > > 
> > > 
> > > On 2/3/23 11:00, Ville Syrjälä 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.
> > > > 
> > > > The automagic 4:2:0 fallback I think is rather fundementally
> > > > incompatible with fancy color management. How would we even
> > > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> > > > that stuff is just always BT.709 limited range, no questions
> > > > asked.
> > > > 
> > > 
> > > We use what we're telling the display, i.e., the value in the
> > > colorspace property. That way we know whether to use a BT.2020
> > > or BT.709 matrix.
> > 
> > And given how these things have gone in the past I think
> > that is likey to bite someone at in the future. Also not
> > what this property was meant to do nor does on any other
> > driver AFAIK.
> > 
> > > I don't see how it's fundamentally incompatible with fancy
> > > color management stuff.
> > > 
> > > If we start forbidding drivers from falling back to YCbCr
> > > (whether 4:4:4 or 4:2:0) we will break existing behavior on
> > > amdgpu and will see bug reports.
> > 
> > The compositors could deal with that if/when they start doing
> > the full color management stuff. The current stuff only really
> > works when the kernel is allowed to do whatever it wants.
> > 
> > > 
> > > > So I think if userspace wants real color management it's
> > > > going to have to set up the whole pipeline. And for that
> > > > we need at least one new property to control the RGB->YCbCr
> > > > conversion (or to explicitly avoid it).
> > > > 
> > > > And given that the proposed patch just swept all the
> > > > non-BT.2020 issues under the rug makes me think no
> > > > one has actually come up with any kind of consistent
> > > > plan for anything else really.
> > > > 
> > > 
> > > Does anyone actually use the non-BT.2020 colorspace stuff?
> > 
> > No idea if anyone is using any of it. It's a bit hard to do
> > right now outside the full passthrough case since we have no
> > properties to control how the hardware will convert stuff.
> > 
> > Anyways, sounds like what you're basically proposing is
> > getting rid of this property and starting from scratch.
> 
> Hmm. I guess one option would be to add that property to
> control the output encoding, but include a few extra
> "automagic" values to it which would retain the kernel's
> freedom to select whether to do the RGB->YCbCr conversion
> or not.
> 
> enum output_encoding {
> 	auto rgb=default/nodata,ycbcr=bt601
> 	auto rgb=default/nodata,ycbcr=bt709
> 	auto rgb=bt2020,ycbcr=bt2020
> 	passthrough,
> 	rgb->ycbcr bt601,
> 	rgb->ycbcr bt709,
> 	rgb->ycbcr bt2020,
> }

In fact there should perhaps be a lot more of the explicit
options to get all subsamlings and quantizations ranges
coverted. That might actually be really nice for an igt
to get more full test coverage.

-- 
Ville Syrjälä
Intel