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

and then if you leave the colorspae property to "default"
the kernel will pick the "right" value based on the
output_encoding prop.

That would leave all the weird stuff in the colorspace
property alone and thus would still allow someone to
do more than just the basic stuff explicitly.

-- 
Ville Syrjälä
Intel