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 13:56, 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.
> 

It has implementations in other drivers but I have yet to
see anyone using it. Without that it does nothing, unless
there are proprietary userspace pieces that make use of this.

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

The compositor could deal with it but this feels like the
compositor taking over things that should really be in the
hands of a display driver.

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

Maybe that's the right approach.

My initial idea was to tag along an existing property but
that turns out to be challenging when that existing property
doesn't even have a userspace implementation. IMO the existing
colorspace property shouldn't be a user-space controllable
property. I'll have to take a closer look at the
hdr_static_metadata to understand whether we might run into
similar issues with it.

The alternative is adding a new property to let userspace pick
the encoding and the min bpc value but that conflicts with the
expectation for a driver to always pick an encoding to satisfy
the bandwidth requirements for the mode on the wire [1].

In this scenario userspace would need to take full ownership of
the wire encoding and live with the consequences. If the bandwidth
is not sufficient a driver would then need to reject the commit
without having a mechanism to tell userspace the reason why.
The driver understands the bandwidth requirements but there is
currently no way for userspace to do so, in particular for
MST/USB-C/USB4/Thunderbold scenarios.

[1] https://lists.freedesktop.org/archives/amd-gfx/2022-December/087423.html

I don't think giving userspace full control of the encoding
buys us anything other than headaches.

Harry