Re: [PATCH 04/16] drm/connector: Convert DRM_MODE_COLORIMETRY to enum

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5599 bytes --]

On Tue, 13 Dec 2022 11:41:08 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> On 12/13/22 05:39, Pekka Paalanen wrote:
> > On Mon, 12 Dec 2022 13:21:25 -0500
> > Harry Wentland <harry.wentland@amd.com> wrote:
> >   
> >> This allows us to use strongly typed arguments.
> >>
> >> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >> Cc: Vitaly.Prosyak@amd.com
> >> Cc: Uma Shankar <uma.shankar@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Joshua Ashton <joshua@froggi.es>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: amd-gfx@lists.freedesktop.org
> >> ---
> >>  include/drm/display/drm_dp.h |  2 +-
> >>  include/drm/drm_connector.h  | 47 ++++++++++++++++++------------------
> >>  2 files changed, 25 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> >> index 4d0abe4c7ea9..b98697459f9c 100644
> >> --- a/include/drm/display/drm_dp.h
> >> +++ b/include/drm/display/drm_dp.h
> >> @@ -1615,7 +1615,7 @@ enum dp_pixelformat {
> >>   *
> >>   * This enum is used to indicate DP VSC SDP Colorimetry formats.
> >>   * It is based on DP 1.4 spec [Table 2-117: VSC SDP Payload for DB16 through
> >> - * DB18] and a name of enum member follows DRM_MODE_COLORIMETRY definition.
> >> + * DB18] and a name of enum member follows &enum drm_colorimetry definition.
> >>   *
> >>   * @DP_COLORIMETRY_DEFAULT: sRGB (IEC 61966-2-1) or
> >>   *                          ITU-R BT.601 colorimetry format
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 62c814241828..edef65388c29 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -371,28 +371,29 @@ enum drm_privacy_screen_status {
> >>   * a colorspace property which will be created and exposed to
> >>   * userspace.
> >>   */
> >> -
> >> -/* For Default case, driver will set the colorspace */
> >> -#define DRM_MODE_COLORIMETRY_DEFAULT			0
> >> -/* CEA 861 Normal Colorimetry options */
> >> -#define DRM_MODE_COLORIMETRY_SMPTE_170M_YCC		1
> >> -#define DRM_MODE_COLORIMETRY_BT709_YCC			2
> >> -/* CEA 861 Extended Colorimetry Options */
> >> -#define DRM_MODE_COLORIMETRY_XVYCC_601			3
> >> -#define DRM_MODE_COLORIMETRY_XVYCC_709			4
> >> -#define DRM_MODE_COLORIMETRY_SYCC_601			5
> >> -#define DRM_MODE_COLORIMETRY_OPYCC_601			6
> >> -#define DRM_MODE_COLORIMETRY_OPRGB			7
> >> -#define DRM_MODE_COLORIMETRY_BT2020_CYCC		8
> >> -#define DRM_MODE_COLORIMETRY_BT2020_RGB			9
> >> -#define DRM_MODE_COLORIMETRY_BT2020_YCC			10
> >> -/* Additional Colorimetry extension added as part of CTA 861.G */
> >> -#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65		11
> >> -#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER		12
> >> -/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
> >> -#define DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED		13
> >> -#define DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT		14
> >> -#define DRM_MODE_COLORIMETRY_BT601_YCC			15
> >> +enum drm_colorspace {
> >> +	/* For Default case, driver will set the colorspace */
> >> +	DRM_MODE_COLORIMETRY_DEFAULT,
> >> +	/* CEA 861 Normal Colorimetry options */
> >> +	DRM_MODE_COLORIMETRY_SMPTE_170M_YCC,
> >> +	DRM_MODE_COLORIMETRY_BT709_YCC,
> >> +	/* CEA 861 Extended Colorimetry Options */
> >> +	DRM_MODE_COLORIMETRY_XVYCC_601,
> >> +	DRM_MODE_COLORIMETRY_XVYCC_709,
> >> +	DRM_MODE_COLORIMETRY_SYCC_601,
> >> +	DRM_MODE_COLORIMETRY_OPYCC_601,
> >> +	DRM_MODE_COLORIMETRY_OPRGB,
> >> +	DRM_MODE_COLORIMETRY_BT2020_CYCC,
> >> +	DRM_MODE_COLORIMETRY_BT2020_RGB,
> >> +	DRM_MODE_COLORIMETRY_BT2020_YCC,
> >> +	/* Additional Colorimetry extension added as part of CTA 861.G */
> >> +	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
> >> +	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
> >> +	/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
> >> +	DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
> >> +	DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
> >> +	DRM_MODE_COLORIMETRY_BT601_YCC,
> >> +};  
> > 
> > Hi,
> > 
> > what's the entry for "the traditional sRGB"?
> > 
> > It cannot be DRM_MODE_COLORIMETRY_DEFAULT, because the doc here says
> > that it maps to some driver-defined entry which may be something else.
> >   
> 
> If I understand this list correctly the only entry that currently covers
> sRGB is DEFAULT.

Then either the list or the doc needs fixing:

- If DEFAULT is driver-chosen, we really do need an entry for "the
  traditional (s)RGB", which corresponds to the bits Y0, Y1, Y2, C0, C1
  being all zeros (CTA-861-H).

- If DEFAULT means that the bits Y0, Y1, Y2, C0, C1
  are all zeros, then it's not driver-chosen, and the doc needs to be
  more clear about this.

But since there are really the UAPI (right?), this should be said in
the UAPI docs for "Colorspace" property, and the internal doc here
could just point to that.

I think this could be just misdocumentation, because in CTA-861-H, the
state Y0=0, Y1=0, Y2=0 is labeled as "RGB (default)" in table Table 16
- AVI InfoFrame RGB or YC B C R Field, Data Byte 1. It's not "driver
will set something", it's "whatever the sink expects when nothing is
explicitly said otherwise" which I assume is practically "the
traditional RGB".

C0=0, C1=0 means "no colorimetry data".

This is something we need to be able to explicitly choose in userspace,
when that is what we want to have.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]