[v3,02/17] drm/connector: Add enum documentation to drm_colorspace

Commit Message

Harry Wentland March 7, 2023, 3:10 p.m. UTC

From: Joshua Ashton <[e-mail]>

To match the other enums, and add more information about these values.

v2:
 - Specify where an enum entry comes from
 - Clarify DEFAULT and NO_DATA behavior
 - BT.2020 CYCC is "constant luminance"
 - correct type for BT.601

Signed-off-by: Joshua Ashton <[e-mail]>
Signed-off-by: Harry Wentland <[e-mail]>
Reviewed-by: Harry Wentland <[e-mail]>

Cc: Pekka Paalanen <[e-mail]>
Cc: Sebastian Wick <[e-mail]>
Cc: [e-mail]
Cc: Uma Shankar <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Joshua Ashton <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

Comments

Pekka Paalanen March 8, 2023, 8:59 a.m. UTC | #1

On Tue, 7 Mar 2023 10:10:52 -0500
Harry Wentland <[e-mail]> wrote:

> From: Joshua Ashton <[e-mail]>
> 
> To match the other enums, and add more information about these values.
> 
> v2:
>  - Specify where an enum entry comes from
>  - Clarify DEFAULT and NO_DATA behavior
>  - BT.2020 CYCC is "constant luminance"
>  - correct type for BT.601
> 
> Signed-off-by: Joshua Ashton <[e-mail]>
> Signed-off-by: Harry Wentland <[e-mail]>
> Reviewed-by: Harry Wentland <[e-mail]>

Hi,

this effort is really good, but of course I still find things to
nitpick about. If there is no answer to my questions, then I would
prefer the documentation to spell out the unknowns and ambiguities.

> Cc: Pekka Paalanen <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: [e-mail]
> Cc: Uma Shankar <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Joshua Ashton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6d6a53a6b010..bb078666dc34 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
>  	PRIVACY_SCREEN_ENABLED_LOCKED,
>  };
>  
> -/*
> - * This is a consolidated colorimetry list supported by HDMI and
> +/**
> + * enum drm_colorspace - color space
> + *
> + * 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
>   * userspace.
> + *
> + * DP definitions come from the DP v2.0 spec
> + * HDMI definitions come from the CTA-861-H spec
> + *
> + * @DRM_MODE_COLORIMETRY_DEFAULT:
> + *   Driver specific behavior.
> + *   For DP:
> + *   	RGB encoded: sRGB (IEC 61966-2-1)
> + *   	YCbCr encoded: ITU-R BT.601 colorimetry format

Does this mean that HDMI behavior is driver-specific while DP behavior
is as defined?

Is it intentional that YCbCr encoding also uses different RGB-primaries
than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)

Or do you need to be more explicit on which parts of each spec apply
(ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
CICP parlance)?

E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.

> + * @DRM_MODE_COLORIMETRY_NO_DATA:
> + *   Driver specific behavior.
> + *   For HDMI:
> + * 	Sets "No Data" in infoframe

Does DEFAULT mean that something else than "No Data" may be set in the
HDMI infoframe?

If so, since these two have the same value, where is the difference? Is
DEFAULT purely an UAPI token, and NO_DATA used internally? Or NO_DATA
used only when crafting actual infoframe packets?

Should NO_DATA be documented to be a strictly driver-internal value,
and not documented with UAPI?

I am unclear if userspace is using these enum values directly, or do
they use the string names only.

> + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> + *   (HDMI)
> + *   SMPTE ST 170M colorimetry format

Does "colorimetry format" mean that the spec is used in full, for all
of ColourPrimaries, TransferCharacteristics and MatrixCoefficients?

If yes, good. If not, the wording misleads me.

> + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> + *   (HDMI, DP)
> + *   ITU-R BT.709 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> + *   (HDMI, DP)
> + *   xvYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_709:
> + *   (HDMI, DP)
> + *   xvYCC709 colorimetry format

Btw. xvYCC are funny because they require limited quantization range
encoding, but use the foot- and headroom to encode out-of-nominal-range
values in order to expand the color gamut with negative and greater
than unity values.

Just for curiosity, is it in any way possible today to make use of that
extended color gamut through KMS? Has it ever been possible?

I mean, the KMS color pipeline assumes full-range RGB, so I don't see
any way to make use of xvYCC.

> + * @DRM_MODE_COLORIMETRY_SYCC_601:
> + *   (HDMI, DP)
> + *   sYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPYCC_601:
> + *   (HDMI, DP)
> + *   opYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPRGB:
> + *   (HDMI, DP)
> + *   opRGB colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
> + *   (HDMI, DP)
> + *   ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_RGB:
> + *   (HDMI, DP)
> + *   ITU-R BT.2020 R' G' B' colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_YCC:
> + *   (HDMI, DP)
> + *   ITU-R BT.2020 Y' C'b C'r colorimetry format
> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> + *   (HDMI)
> + *   SMPTE ST 2113 P3D65 colorimetry format
> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> + *   (HDMI)
> + *   SMPTE ST 2113 P3DCI colorimetry format
> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
> + *   (DP)
> + *   RGB wide gamut fixed point colorimetry format
> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
> + *   (DP)
> + *   RGB wide gamut floating point
> + *   (scRGB (IEC 61966-2-2)) colorimetry format

Again, there is no way to actually make use of WIDE since the KMS color
pipeline is limited to the unit range color values, right? Or is it
possible by setting all color pipeline KMS properties to pass-through
and using a floating-point FB?

I suppose the FIXED vs. FLOAT has the exact same problems as BT2020_YCC
vs. BT2020_RGB, but I would be surprised if anyone cared.

> + * @DRM_MODE_COLORIMETRY_BT601_YCC:
> + *   (DP)
> + *   ITU-R BT.601 colorimetry format
> + *   The DP spec does not say whether this is the 525 or the 625
> + *   line version.

Good to note that ambiguity here. :-)

Or maybe the DP spec writer was thinking about BT.709 ColourPrimaries
and BT.601 MatrixCoefficients...

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


Thanks,
pq

Sebastian Wick March 9, 2023, 12:56 a.m. UTC | #2

On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen <[e-mail]> wrote:
>
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland <[e-mail]> wrote:
>
> > From: Joshua Ashton <[e-mail]>
> >
> > To match the other enums, and add more information about these values.
> >
> > v2:
> >  - Specify where an enum entry comes from
> >  - Clarify DEFAULT and NO_DATA behavior
> >  - BT.2020 CYCC is "constant luminance"
> >  - correct type for BT.601
> >
> > Signed-off-by: Joshua Ashton <[e-mail]>
> > Signed-off-by: Harry Wentland <[e-mail]>
> > Reviewed-by: Harry Wentland <[e-mail]>
>
> Hi,
>
> this effort is really good, but of course I still find things to
> nitpick about. If there is no answer to my questions, then I would
> prefer the documentation to spell out the unknowns and ambiguities.
>
> > Cc: Pekka Paalanen <[e-mail]>
> > Cc: Sebastian Wick <[e-mail]>
> > Cc: [e-mail]
> > Cc: Uma Shankar <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Joshua Ashton <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 6d6a53a6b010..bb078666dc34 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> >       PRIVACY_SCREEN_ENABLED_LOCKED,
> >  };
> >
> > -/*
> > - * This is a consolidated colorimetry list supported by HDMI and
> > +/**
> > + * enum drm_colorspace - color space
> > + *
> > + * 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
> >   * userspace.
> > + *
> > + * DP definitions come from the DP v2.0 spec
> > + * HDMI definitions come from the CTA-861-H spec
> > + *
> > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > + *   Driver specific behavior.
> > + *   For DP:
> > + *           RGB encoded: sRGB (IEC 61966-2-1)
> > + *           YCbCr encoded: ITU-R BT.601 colorimetry format
>
> Does this mean that HDMI behavior is driver-specific while DP behavior
> is as defined?
>
> Is it intentional that YCbCr encoding also uses different RGB-primaries
> than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
>
> Or do you need to be more explicit on which parts of each spec apply
> (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> CICP parlance)?
>
> E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.

Yeah, just adding to this: The Default Colorspace is something well
defined. CTA-861 says:

"If bits C0 and C1 are zero, the colorimetry shall correspond to the
default colorimetry defined in Section 5.1"

and in Section 5.1

"In all cases described above, the RGB color space used should be the
RGB color space the Sink declares in the Basic Display Parameters and
Feature Block of its EDID."

If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
EDID reports to be in effect and not some driver specific nonsense.

> > + * @DRM_MODE_COLORIMETRY_NO_DATA:
> > + *   Driver specific behavior.
> > + *   For HDMI:
> > + *   Sets "No Data" in infoframe
>
> Does DEFAULT mean that something else than "No Data" may be set in the
> HDMI infoframe?
>
> If so, since these two have the same value, where is the difference? Is
> DEFAULT purely an UAPI token, and NO_DATA used internally? Or NO_DATA
> used only when crafting actual infoframe packets?
>
> Should NO_DATA be documented to be a strictly driver-internal value,
> and not documented with UAPI?
>
> I am unclear if userspace is using these enum values directly, or do
> they use the string names only.
>
> > + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> > + *   (HDMI)
> > + *   SMPTE ST 170M colorimetry format
>
> Does "colorimetry format" mean that the spec is used in full, for all
> of ColourPrimaries, TransferCharacteristics and MatrixCoefficients?
>
> If yes, good. If not, the wording misleads me.
>
> > + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> > + *   (HDMI, DP)
> > + *   ITU-R BT.709 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> > + *   (HDMI, DP)
> > + *   xvYCC601 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XVYCC_709:
> > + *   (HDMI, DP)
> > + *   xvYCC709 colorimetry format
>
> Btw. xvYCC are funny because they require limited quantization range
> encoding, but use the foot- and headroom to encode out-of-nominal-range
> values in order to expand the color gamut with negative and greater
> than unity values.
>
> Just for curiosity, is it in any way possible today to make use of that
> extended color gamut through KMS? Has it ever been possible?
>
> I mean, the KMS color pipeline assumes full-range RGB, so I don't see
> any way to make use of xvYCC.
>
> > + * @DRM_MODE_COLORIMETRY_SYCC_601:
> > + *   (HDMI, DP)
> > + *   sYCC601 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_OPYCC_601:
> > + *   (HDMI, DP)
> > + *   opYCC601 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_OPRGB:
> > + *   (HDMI, DP)
> > + *   opRGB colorimetry format
> > + * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
> > + *   (HDMI, DP)
> > + *   ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format
> > + * @DRM_MODE_COLORIMETRY_BT2020_RGB:
> > + *   (HDMI, DP)
> > + *   ITU-R BT.2020 R' G' B' colorimetry format
> > + * @DRM_MODE_COLORIMETRY_BT2020_YCC:
> > + *   (HDMI, DP)
> > + *   ITU-R BT.2020 Y' C'b C'r colorimetry format
> > + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> > + *   (HDMI)
> > + *   SMPTE ST 2113 P3D65 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> > + *   (HDMI)
> > + *   SMPTE ST 2113 P3DCI colorimetry format
> > + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
> > + *   (DP)
> > + *   RGB wide gamut fixed point colorimetry format
> > + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
> > + *   (DP)
> > + *   RGB wide gamut floating point
> > + *   (scRGB (IEC 61966-2-2)) colorimetry format
>
> Again, there is no way to actually make use of WIDE since the KMS color
> pipeline is limited to the unit range color values, right? Or is it
> possible by setting all color pipeline KMS properties to pass-through
> and using a floating-point FB?
>
> I suppose the FIXED vs. FLOAT has the exact same problems as BT2020_YCC
> vs. BT2020_RGB, but I would be surprised if anyone cared.
>
> > + * @DRM_MODE_COLORIMETRY_BT601_YCC:
> > + *   (DP)
> > + *   ITU-R BT.601 colorimetry format
> > + *   The DP spec does not say whether this is the 525 or the 625
> > + *   line version.
>
> Good to note that ambiguity here. :-)
>
> Or maybe the DP spec writer was thinking about BT.709 ColourPrimaries
> and BT.601 MatrixCoefficients...
>
> >   */
> >  enum drm_colorspace {
> >       /* For Default case, driver will set the colorspace */
>
>
> Thanks,
> pq

Pekka Paalanen March 9, 2023, 10:03 a.m. UTC | #3

On Thu, 9 Mar 2023 01:56:11 +0100
Sebastian Wick <[e-mail]> wrote:

> On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen <[e-mail]> wrote:
> >
> > On Tue, 7 Mar 2023 10:10:52 -0500
> > Harry Wentland <[e-mail]> wrote:
> >  
> > > From: Joshua Ashton <[e-mail]>
> > >
> > > To match the other enums, and add more information about these values.
> > >
> > > v2:
> > >  - Specify where an enum entry comes from
> > >  - Clarify DEFAULT and NO_DATA behavior
> > >  - BT.2020 CYCC is "constant luminance"
> > >  - correct type for BT.601
> > >
> > > Signed-off-by: Joshua Ashton <[e-mail]>
> > > Signed-off-by: Harry Wentland <[e-mail]>
> > > Reviewed-by: Harry Wentland <[e-mail]>  
> >
> > Hi,
> >
> > this effort is really good, but of course I still find things to
> > nitpick about. If there is no answer to my questions, then I would
> > prefer the documentation to spell out the unknowns and ambiguities.
> >  
> > > Cc: Pekka Paalanen <[e-mail]>
> > > Cc: Sebastian Wick <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Uma Shankar <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: Joshua Ashton <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 65 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 6d6a53a6b010..bb078666dc34 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > >       PRIVACY_SCREEN_ENABLED_LOCKED,
> > >  };
> > >
> > > -/*
> > > - * This is a consolidated colorimetry list supported by HDMI and
> > > +/**
> > > + * enum drm_colorspace - color space
> > > + *
> > > + * 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
> > >   * userspace.
> > > + *
> > > + * DP definitions come from the DP v2.0 spec
> > > + * HDMI definitions come from the CTA-861-H spec
> > > + *
> > > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > > + *   Driver specific behavior.
> > > + *   For DP:
> > > + *           RGB encoded: sRGB (IEC 61966-2-1)
> > > + *           YCbCr encoded: ITU-R BT.601 colorimetry format  
> >
> > Does this mean that HDMI behavior is driver-specific while DP behavior
> > is as defined?
> >
> > Is it intentional that YCbCr encoding also uses different RGB-primaries
> > than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
> >
> > Or do you need to be more explicit on which parts of each spec apply
> > (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> > CICP parlance)?
> >
> > E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.  
> 
> Yeah, just adding to this: The Default Colorspace is something well
> defined. CTA-861 says:
> 
> "If bits C0 and C1 are zero, the colorimetry shall correspond to the
> default colorimetry defined in Section 5.1"
> 
> and in Section 5.1
> 
> "In all cases described above, the RGB color space used should be the
> RGB color space the Sink declares in the Basic Display Parameters and
> Feature Block of its EDID."
> 
> If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
> EDID reports to be in effect and not some driver specific nonsense.

Does that also define the MatrixCoefficients for YCbCr signal with
DRM_MODE_COLORIMETRY_DEFAULT?

Not that userspace would even care, since RGB-to-YCbCr is all
driver-internal.

It is interesting you point that out. I guess it means that the basic
colorimetry from EDID is supposed to be really only the default
colorimetry and might not have anything to do with the actual panel
primaries.


Thanks,
pq

Sebastian Wick March 9, 2023, 8:23 p.m. UTC | #4

On Thu, Mar 9, 2023 at 11:03 AM Pekka Paalanen <[e-mail]> wrote:
>
> On Thu, 9 Mar 2023 01:56:11 +0100
> Sebastian Wick <[e-mail]> wrote:
>
> > On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen <[e-mail]> wrote:
> > >
> > > On Tue, 7 Mar 2023 10:10:52 -0500
> > > Harry Wentland <[e-mail]> wrote:
> > >
> > > > From: Joshua Ashton <[e-mail]>
> > > >
> > > > To match the other enums, and add more information about these values.
> > > >
> > > > v2:
> > > >  - Specify where an enum entry comes from
> > > >  - Clarify DEFAULT and NO_DATA behavior
> > > >  - BT.2020 CYCC is "constant luminance"
> > > >  - correct type for BT.601
> > > >
> > > > Signed-off-by: Joshua Ashton <[e-mail]>
> > > > Signed-off-by: Harry Wentland <[e-mail]>
> > > > Reviewed-by: Harry Wentland <[e-mail]>
> > >
> > > Hi,
> > >
> > > this effort is really good, but of course I still find things to
> > > nitpick about. If there is no answer to my questions, then I would
> > > prefer the documentation to spell out the unknowns and ambiguities.
> > >
> > > > Cc: Pekka Paalanen <[e-mail]>
> > > > Cc: Sebastian Wick <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: Uma Shankar <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: Joshua Ashton <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > ---
> > > >  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 65 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 6d6a53a6b010..bb078666dc34 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > > >       PRIVACY_SCREEN_ENABLED_LOCKED,
> > > >  };
> > > >
> > > > -/*
> > > > - * This is a consolidated colorimetry list supported by HDMI and
> > > > +/**
> > > > + * enum drm_colorspace - color space
> > > > + *
> > > > + * 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
> > > >   * userspace.
> > > > + *
> > > > + * DP definitions come from the DP v2.0 spec
> > > > + * HDMI definitions come from the CTA-861-H spec
> > > > + *
> > > > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > > > + *   Driver specific behavior.
> > > > + *   For DP:
> > > > + *           RGB encoded: sRGB (IEC 61966-2-1)
> > > > + *           YCbCr encoded: ITU-R BT.601 colorimetry format
> > >
> > > Does this mean that HDMI behavior is driver-specific while DP behavior
> > > is as defined?
> > >
> > > Is it intentional that YCbCr encoding also uses different RGB-primaries
> > > than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
> > >
> > > Or do you need to be more explicit on which parts of each spec apply
> > > (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> > > CICP parlance)?
> > >
> > > E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
> >
> > Yeah, just adding to this: The Default Colorspace is something well
> > defined. CTA-861 says:
> >
> > "If bits C0 and C1 are zero, the colorimetry shall correspond to the
> > default colorimetry defined in Section 5.1"
> >
> > and in Section 5.1
> >
> > "In all cases described above, the RGB color space used should be the
> > RGB color space the Sink declares in the Basic Display Parameters and
> > Feature Block of its EDID."
> >
> > If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
> > EDID reports to be in effect and not some driver specific nonsense.
>
> Does that also define the MatrixCoefficients for YCbCr signal with
> DRM_MODE_COLORIMETRY_DEFAULT?

Good question. It doesn't seem like it does, which would make
supporting YCC with the default color space impossible.

> Not that userspace would even care, since RGB-to-YCbCr is all
> driver-internal.
>
> It is interesting you point that out. I guess it means that the basic
> colorimetry from EDID is supposed to be really only the default
> colorimetry and might not have anything to do with the actual panel
> primaries.
>
>
> Thanks,
> pq

Harry Wentland May 24, 2023, 5 p.m. UTC | #5

On 3/8/23 03:59, Pekka Paalanen wrote:
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland <[e-mail]> wrote:
> 
>> From: Joshua Ashton <[e-mail]>
>>
>> To match the other enums, and add more information about these values.
>>
>> v2:
>>  - Specify where an enum entry comes from
>>  - Clarify DEFAULT and NO_DATA behavior
>>  - BT.2020 CYCC is "constant luminance"
>>  - correct type for BT.601
>>
>> Signed-off-by: Joshua Ashton <[e-mail]>
>> Signed-off-by: Harry Wentland <[e-mail]>
>> Reviewed-by: Harry Wentland <[e-mail]>
> 
> Hi,
> 
> this effort is really good, but of course I still find things to
> nitpick about. If there is no answer to my questions, then I would
> prefer the documentation to spell out the unknowns and ambiguities.
> 

Finally finding time to look at this again and want to make sure I
try to address your comments as best as I can. I'm terribly at tracking
emails, so if anything was clarified already I apologize.

>> Cc: Pekka Paalanen <[e-mail]>
>> Cc: Sebastian Wick <[e-mail]>
>> Cc: [e-mail]
>> Cc: Uma Shankar <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Joshua Ashton <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 6d6a53a6b010..bb078666dc34 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
>>  	PRIVACY_SCREEN_ENABLED_LOCKED,
>>  };
>>  
>> -/*
>> - * This is a consolidated colorimetry list supported by HDMI and
>> +/**
>> + * enum drm_colorspace - color space
>> + *
>> + * 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
>>   * userspace.
>> + *
>> + * DP definitions come from the DP v2.0 spec
>> + * HDMI definitions come from the CTA-861-H spec
>> + *
>> + * @DRM_MODE_COLORIMETRY_DEFAULT:
>> + *   Driver specific behavior.
>> + *   For DP:
>> + *   	RGB encoded: sRGB (IEC 61966-2-1)
>> + *   	YCbCr encoded: ITU-R BT.601 colorimetry format
> 
> Does this mean that HDMI behavior is driver-specific while DP behavior
> is as defined?
> 

I should drop the bits that specify what this means for DP. I really
just took the 0h value for the colorimetry of the VSC SDP packet
(SDP = DP infoframe).

> Is it intentional that YCbCr encoding also uses different RGB-primaries
> than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
> 
> Or do you need to be more explicit on which parts of each spec apply
> (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> CICP parlance)?
> 
> E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
> 
>> + * @DRM_MODE_COLORIMETRY_NO_DATA:
>> + *   Driver specific behavior.
>> + *   For HDMI:
>> + * 	Sets "No Data" in infoframe
> 
> Does DEFAULT mean that something else than "No Data" may be set in the
> HDMI infoframe?
> 
> If so, since these two have the same value, where is the difference? Is
> DEFAULT purely an UAPI token, and NO_DATA used internally? Or NO_DATA
> used only when crafting actual infoframe packets?
> 
> Should NO_DATA be documented to be a strictly driver-internal value,
> and not documented with UAPI?
> 

I don't think I have an answer for you. I will remove the "For HDMI"
bit to avoid confusion.

> I am unclear if userspace is using these enum values directly, or do
> they use the string names only.
> 

Userspace is using these enum values directly.

>> + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
>> + *   (HDMI)
>> + *   SMPTE ST 170M colorimetry format
> 
> Does "colorimetry format" mean that the spec is used in full, for all
> of ColourPrimaries, TransferCharacteristics and MatrixCoefficients?
> 
> If yes, good. If not, the wording misleads me.
> 
>> + * @DRM_MODE_COLORIMETRY_BT709_YCC:
>> + *   (HDMI, DP)
>> + *   ITU-R BT.709 colorimetry format
>> + * @DRM_MODE_COLORIMETRY_XVYCC_601:
>> + *   (HDMI, DP)
>> + *   xvYCC601 colorimetry format
>> + * @DRM_MODE_COLORIMETRY_XVYCC_709:
>> + *   (HDMI, DP)
>> + *   xvYCC709 colorimetry format
> 
> Btw. xvYCC are funny because they require limited quantization range
> encoding, but use the foot- and headroom to encode out-of-nominal-range
> values in order to expand the color gamut with negative and greater
> than unity values.
> 
> Just for curiosity, is it in any way possible today to make use of that
> extended color gamut through KMS? Has it ever been possible?
> 
> I mean, the KMS color pipeline assumes full-range RGB, so I don't see
> any way to make use of xvYCC.
> 

I don't know it's possible. I wasn't the one to write the original
API for this. I think this API defines things that have never been
well understood. But since it's there I'll leave it as-is. The comments
(as requested) are trying to clarify things a bit. I think there
will be gaps if someone wants to actually implement it, even with
drivers that currently advertise support for the whole set.

>> + * @DRM_MODE_COLORIMETRY_SYCC_601:
>> + *   (HDMI, DP)
>> + *   sYCC601 colorimetry format
>> + * @DRM_MODE_COLORIMETRY_OPYCC_601:
>> + *   (HDMI, DP)
>> + *   opYCC601 colorimetry format
>> + * @DRM_MODE_COLORIMETRY_OPRGB:
>> + *   (HDMI, DP)
>> + *   opRGB colorimetry format
>> + * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
>> + *   (HDMI, DP)
>> + *   ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format
>> + * @DRM_MODE_COLORIMETRY_BT2020_RGB:
>> + *   (HDMI, DP)
>> + *   ITU-R BT.2020 R' G' B' colorimetry format
>> + * @DRM_MODE_COLORIMETRY_BT2020_YCC:
>> + *   (HDMI, DP)
>> + *   ITU-R BT.2020 Y' C'b C'r colorimetry format
>> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>> + *   (HDMI)
>> + *   SMPTE ST 2113 P3D65 colorimetry format
>> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
>> + *   (HDMI)
>> + *   SMPTE ST 2113 P3DCI colorimetry format
>> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
>> + *   (DP)
>> + *   RGB wide gamut fixed point colorimetry format
>> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
>> + *   (DP)
>> + *   RGB wide gamut floating point
>> + *   (scRGB (IEC 61966-2-2)) colorimetry format
> 
> Again, there is no way to actually make use of WIDE since the KMS color
> pipeline is limited to the unit range color values, right? Or is it
> possible by setting all color pipeline KMS properties to pass-through
> and using a floating-point FB?
> 
> I suppose the FIXED vs. FLOAT has the exact same problems as BT2020_YCC
> vs. BT2020_RGB, but I would be surprised if anyone cared.
> 

Again, I think pulling the actual enum values from the HDMI infoframes and
DP SDP packets into API was a mistake.

Harry

>> + * @DRM_MODE_COLORIMETRY_BT601_YCC:
>> + *   (DP)
>> + *   ITU-R BT.601 colorimetry format
>> + *   The DP spec does not say whether this is the 525 or the 625
>> + *   line version.
> 
> Good to note that ambiguity here. :-)
> 
> Or maybe the DP spec writer was thinking about BT.709 ColourPrimaries
> and BT.601 MatrixCoefficients...
> 
>>   */
>>  enum drm_colorspace {
>>  	/* For Default case, driver will set the colorspace */
> 
> 
> Thanks,
> pq

Patch

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6d6a53a6b010..bb078666dc34 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -363,13 +363,76 @@  enum drm_privacy_screen_status {
 	PRIVACY_SCREEN_ENABLED_LOCKED,
 };
 
-/*
- * This is a consolidated colorimetry list supported by HDMI and
+/**
+ * enum drm_colorspace - color space
+ *
+ * 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
  * userspace.
+ *
+ * DP definitions come from the DP v2.0 spec
+ * HDMI definitions come from the CTA-861-H spec
+ *
+ * @DRM_MODE_COLORIMETRY_DEFAULT:
+ *   Driver specific behavior.
+ *   For DP:
+ *   	RGB encoded: sRGB (IEC 61966-2-1)
+ *   	YCbCr encoded: ITU-R BT.601 colorimetry format
+ * @DRM_MODE_COLORIMETRY_NO_DATA:
+ *   Driver specific behavior.
+ *   For HDMI:
+ * 	Sets "No Data" in infoframe
+ * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
+ *   (HDMI)
+ *   SMPTE ST 170M colorimetry format
+ * @DRM_MODE_COLORIMETRY_BT709_YCC:
+ *   (HDMI, DP)
+ *   ITU-R BT.709 colorimetry format
+ * @DRM_MODE_COLORIMETRY_XVYCC_601:
+ *   (HDMI, DP)
+ *   xvYCC601 colorimetry format
+ * @DRM_MODE_COLORIMETRY_XVYCC_709:
+ *   (HDMI, DP)
+ *   xvYCC709 colorimetry format
+ * @DRM_MODE_COLORIMETRY_SYCC_601:
+ *   (HDMI, DP)
+ *   sYCC601 colorimetry format
+ * @DRM_MODE_COLORIMETRY_OPYCC_601:
+ *   (HDMI, DP)
+ *   opYCC601 colorimetry format
+ * @DRM_MODE_COLORIMETRY_OPRGB:
+ *   (HDMI, DP)
+ *   opRGB colorimetry format
+ * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
+ *   (HDMI, DP)
+ *   ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format
+ * @DRM_MODE_COLORIMETRY_BT2020_RGB:
+ *   (HDMI, DP)
+ *   ITU-R BT.2020 R' G' B' colorimetry format
+ * @DRM_MODE_COLORIMETRY_BT2020_YCC:
+ *   (HDMI, DP)
+ *   ITU-R BT.2020 Y' C'b C'r colorimetry format
+ * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
+ *   (HDMI)
+ *   SMPTE ST 2113 P3D65 colorimetry format
+ * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
+ *   (HDMI)
+ *   SMPTE ST 2113 P3DCI colorimetry format
+ * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
+ *   (DP)
+ *   RGB wide gamut fixed point colorimetry format
+ * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
+ *   (DP)
+ *   RGB wide gamut floating point
+ *   (scRGB (IEC 61966-2-2)) colorimetry format
+ * @DRM_MODE_COLORIMETRY_BT601_YCC:
+ *   (DP)
+ *   ITU-R BT.601 colorimetry format
+ *   The DP spec does not say whether this is the 525 or the 625
+ *   line version.
  */
 enum drm_colorspace {
 	/* For Default case, driver will set the colorspace */