[PATCH 2/3] drm/connector: Add enum documentation to drm_colorspace

[Pekka Paalanen]

On Fri,  3 Feb 2023 02:07:43 +0000
Joshua Ashton <joshua at froggi.es> wrote:

> To match the other enums, and add more information about these values.
> 
> Signed-off-by: Joshua Ashton <joshua at froggi.es>
> 
> 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
> ---
>  include/drm/drm_connector.h | 41 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)

Hi Joshua,

sorry for pushing you into a rabbit hole a bit. :-)

> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index edef65388c29..eb4cc9076e16 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -363,13 +363,50 @@ enum drm_privacy_screen_status {
>  	PRIVACY_SCREEN_ENABLED_LOCKED,
>  };
>  
> -/*
> - * This is a consolidated colorimetry list supported by HDMI and
> +/**
> + * enum drm_colorspace - color space

Documenting this enum is really nice. What would be even better if
there was similar documentation in the UAPI doc of "Colorspace" under
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#standard-connector-properties
listing the strings that userspace must use/expect and what they refer
to.


> + *
> + * This enum is a consolidated colorimetry list supported by HDMI and
>   * DP protocol standard. The respective connectors will register
>   * a property with the subset of this list (supported by that
>   * respective protocol). Userspace will set the colorspace through
>   * a colorspace property which will be created and exposed to

Could this refer to "Colorspace" property explicitly instead of some
unmentioned property?

>   * userspace.
> + *
> + * @DRM_MODE_COLORIMETRY_DEFAULT:
> + *   sRGB (IEC 61966-2-1) or
> + *   ITU-R BT.601 colorimetry format

Is this what the "driver will set the colorspace" comment actually
means? If so, I think the comment "driver will set the colorspace"
could be better or replaced with "not from any standard" or "undefined".

sRGB and BT.601 have different primaries. There are actually two
different cases of BT.601 primaries: the 525 line and 625 line. How
does that work? Are the drivers really choosing anything, or will they
just send "undefined" to the sink, and then the sink does whatever it
does?

Or is this *only* about the RGB-to-YCbCr conversion matrix and not
about colorimetry at all?

If it's only about the conversion matrix (MatrixCoefficients in CICP
(H.273) terms), then which ones of the below also define only the
MatrixCoefficients but no colorimetry?

> + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> + *   SMPTE ST 170M colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> + *   ITU-R BT.709 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> + *   xvYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_709:
> + *   xvYCC709 colorimetry format
> + * @DRM_MODE_COLORIMETRY_SYCC_601:
> + *   sYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPYCC_601:
> + *   opYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPRGB:
> + *   opRGB colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
> + *   ITU-R BT.2020 Y'c C'bc C'rc (linear) colorimetry format

Is this one known as the constant luminance variant which requires
KMS/driver/hardware knowing also the transfer characteristic function?

Is there perhaps an assumed TF here, since there is no KMS property to
set a TF? Oh, maybe all of these imply the respective TF from the spec?

I suspect the "linear" should read as "constant luminance".

> + * @DRM_MODE_COLORIMETRY_BT2020_RGB:
> + *   ITU-R BT.2020 R' G' B' colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_YCC:
> + *   ITU-R BT.2020 Y' C'b C'r colorimetry format

...compared to this one known as the non-constant luminance variant,
i.e. "the simple RGB-to-YCbCr conversion"?

> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> + *   DCI-P3 (SMPTE RP 431-2) colorimetry format
> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> + *   DCI-P3 (SMPTE RP 431-2) colorimetry format

These two can't both be the same, right? That is, the description is
missing something.

> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
> + *   RGB wide gamut fixed point colorimetry format

Is this one scRGB too?

> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
> + *   RGB wide gamut floating point
> + *   (scRGB (IEC 61966-2-2)) colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT601_YCC:
> + *   ITU-R BT.609 colorimetry format

Typo: BT.609

Which one of the two BT.601?

>   */
>  enum drm_colorspace {
>  	/* For Default case, driver will set the colorspace */

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230208/75fcf873/attachment-0001.sig>