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

[Harry Wentland <harry.wentland@amd.com>]

On 2/3/23 14:25, 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,
> }
> 

Is there a good reason to decouple the YCbCr encoding format
from the colorspace?



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

How would you fill in the AVI infoframe? Userspace would still
need to set that to BT.2020 if the pixels are in BT.2020.

Harry

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