[v4 0/3] Add Colorspace connector property interface

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Brian Starkey
>Sent: Wednesday, November 28, 2018 5:27 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; jonas at kwiboo.se; intel-
>gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
>hansverk at cisco.com; nd <nd at arm.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [v4 0/3] Add Colorspace connector property interface
>
>Hi,
>
>On Tue, Nov 27, 2018 at 10:10:40PM +0530, Uma Shankar wrote:
>> This patch series creates a new connector property to program
>> colorspace to sink devices. Modern sink devices support more than 1
>> type of colorspace like 601, 709, BT2020 etc. This helps to switch
>> based on content type which is to be displayed. The decision lies with
>> compositors as to in which scenarios, a particular colorspace will be
>> picked.
>>
>> This will be helpful mostly to switch to higher gamut colorspaces like
>> BT2020 when the media content is encoded as BT2020. Thereby giving a
>> good visual experience to users.
>>
>> The expectation from userspace is that it should parse the EDID and
>> get supported colorspaces. Use this property and switch to the one
>> supported. Kernel will not give the supported colorspaces since this
>> is panel dependent and our current property infrastructure is not
>> supporting it.
>
>So is the problem here that we've no way to change the supported enum values
>at runtime? Conceptually, do you think there's a problem with the kernel only
>exposing the colorspaces that the sink supports (if that were possible)? I'm
>wondering if changing the current property infrastructure is better than punting
>the job to userspace to decode the EDID.

Only problem which I see is that all the connector properties are created at connector
initialization including the connector->edid property. The respective sync devices may not be
attached also at that moment. There is an option to change the blob id's for case of edid when
new sink is plugged in, but the fundamental structure remains same. I mean the enum which
is used at creating the colorspace enum property can't be changed at runtime. It stays the same
throughout the drivers life (this is of course based on my understanding :), correct me I am wrong).
Hence changing it based on sink is not easy with the current design.

>>
>> Have tested this using xrandr by using below command:
>> xrandr --output HDMI2 --set "Colorspace" "BT2020_rgb"
>>
>
>It would also be really great to get some more comprehensive documentation
>about how this property is meant to be used. Is the expectation that userspace
>does everything? i.e.:
>
> - Userspace sets up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>   colorspace
> - Userspace sets this new property to let the sink know what it
>   converted the CRTC output to.
>
>Or in other words, I think this new property has zero impact on any pixel
>processing in the pipeline - it only sets the colorspace in the infoframe. That
>seems very valuable to write down explicitly.

Yes, this is what this property does. Userspace decides the blending and colorspace
target for blending output from pipe. This helps to pass that info to sink device which
was missing till now. I will update it and add documentation on the same to be clear
wrt property expectation and purpose.

>BTW, is there already a standard property for converting CRTC output to YCbCr?
>And does that interact with picking the YCC colorimetries with this property?

AFAIK, there is no dedicated property but the YUV modes are added as part of HDMI 2.0
development work from Shashank. User can set the mode and driver will do the conversion
internally if the hardware supports it, else it may choose to drop the mode.

Regards,
Uma Shankar

>Cheers,
>-Brian
>
>> v2: Addressed Ville and Maarten's review comments. Merged the 2nd and
>> 3rd patch into one common logical patch.
>>
>> v3: Removed Adobe references from enum definitions as per Ville, Hans
>> Verkuil and Jonas Karlman suggestions. Changed default to an unset
>> state where driver will assign the colorspace when not chosen by user,
>> suggested by Ville and Maarten. Addressed other misc review comments
>> from Maarten. Split the changes to have separate colorspace property
>> for DP and HDMI.
>>
>> v4: Addressed Chris and Ville's review comments, and created a common
>> colorspace property for DP and HDMI, filtered the list based on the
>> colorspaces supported by the respective protocol standard. Handled the
>> default case more efficiently.
>>
>> Uma Shankar (3):
>>   drm: Add HDMI colorspace property
>>   drm: Add DP colorspace property
>>   drm/i915: Attach colorspace property and enable modeset
>>
>>  drivers/gpu/drm/drm_atomic_uapi.c      |  4 ++
>>  drivers/gpu/drm/drm_connector.c        | 92
>++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_atomic.c    |  1 +
>>  drivers/gpu/drm/i915/intel_connector.c |  8 +++
>>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_hdmi.c      | 18 +++++++
>>  include/drm/drm_connector.h            | 14 ++++++
>>  include/uapi/drm/drm_mode.h            | 33 ++++++++++++
>>  8 files changed, 171 insertions(+)
>>
>> --
>> 1.9.1
>>
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel