[PATCH 06/16] drm/connector: Allow drivers to pass list of supported colorspaces

[Harry Wentland]

On 12/14/22 03:55, Pekka Paalanen wrote:
> On Tue, 13 Dec 2022 11:32:01 -0500
> Harry Wentland <harry.wentland at amd.com> wrote:
> 
>> On 12/13/22 05:23, Pekka Paalanen wrote:
>>> On Mon, 12 Dec 2022 13:21:27 -0500
>>> Harry Wentland <harry.wentland at amd.com> wrote:
>>>   
>>>> Drivers might not support all colorspaces defined in
>>>> dp_colorspaces and hdmi_colorspaces. This results in
>>>> undefined behavior when userspace is setting an
>>>> unsupported colorspace.
>>>>
>>>> Allow drivers to pass the list of supported colorspaces
>>>> when creating the colorspace property.  
>>>
>>> Hi Harry,
>>>
>>> what is there for drivers to support? Isn't this just infoframe data
>>> that shall be sent down to the sink as-is with no other effect?
>>>   
>>
>> You have a good point.
>>
>> Right now the supported colorspaces de-facto depend on driver implementations
>> as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will
>> also program the MSA [3] for DP connections, and a bunch of other things which
>> are deeper in the driver.
>>
>> [1] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741
>> [2] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155
>> [3] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368
>>
>> I don't know why the DP VSC stuff needs to be in drivers. It should be
>> common. The MSA packing would likely have to be driver specific since the
>> mechanism of packing it is HW specific.
> 
> What's MSA?

This is from the DP spec. It stands for Main Stream Attribute.
It's a packet similar to an SDP that is encoded in the DP main
signal stream (as opposed to an AUX message).

> 
> I don't see it in
> https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html
> or anywhere under Documentation/gpu or in CTA-861-H.
> 
>> I'll have a closer look and see if we can eliminate the "driver supported"
>> bit. If we can't we'll probably need to describe the reasoning better.
> 
> That would be nice, thanks!
> 
>> Will it be a problem if the list of supported colorspaces differs between
>> drivers?
> 
> I do not think so. It's just normal KMS UAPI that one must always
> inspect an enumeration to see what values are possible. Userspace
> cannot use a header with pre-defined numerical values, they always need
> to be introspected first like everything else about KMS properties.
> 
> I know there were some opinions about hard-coding enum numerical values
> in headers, but I think in the end everyone agreed to the introspection
> even if it didn't seem useful at the time.
> 
> Besides, if a driver never supported a given value but misbehaved or
> refused, I don't think that counts as a kernel regression?
> 

True, but I would imagine that can be confusing for developers of DRM
clients when enabling support for a new feature. I would guess it's
much less confusing if drivers only exposed functionality that are
expected to work (and are hopefully tested regularly).

Harry

> 
> Thanks,
> pq
> 
>>
>> Harry
>>
>>> Is the driver confusing colorimetry with color-representation (the
>>> RGB-YCbCr conversion)? Or is this property defining both?
>>>
>>> I feel that the documentation of "Colorspace" KMS connector property
>>> needs clarification, and a list of potentially available values with
>>> explanations, more than just a reference to CTA-816-H which it does not
>>> even do yet.
>>>
>>> Perhaps a table, where for each enum drm_colorspace entry has a row explaining:
>>>
>>>
>>> Thanks,
>>> pq
>>>
>>>   
>>>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>>>> Cc: Pekka Paalanen <ppaalanen at gmail.com>
>>>> Cc: Sebastian Wick <sebastian.wick at redhat.com>
>>>> Cc: Vitaly.Prosyak at amd.com
>>>> Cc: Uma Shankar <uma.shankar at intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Cc: Joshua Ashton <joshua at froggi.es>
>>>> Cc: dri-devel at lists.freedesktop.org
>>>> Cc: amd-gfx at lists.freedesktop.org
>>>> ---
>>>>  drivers/gpu/drm/drm_connector.c               | 140 +++++++++---------
>>>>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>>>>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>>>>  include/drm/drm_connector.h                   |   8 +-
>>>>  4 files changed, 83 insertions(+), 71 deletions(-)
>>>>