From: Harry Wentland <harry.wentland@amd.com> To: Pekka Paalanen <ppaalanen@gmail.com>, Joshua Ashton <joshua@froggi.es> Cc: Sebastian Wick <sebastian.wick@redhat.com>, amd-gfx@lists.freedesktop.org, Uma Shankar <uma.shankar@intel.com>, dri-devel@lists.freedesktop.org, Vitaly.Prosyak@amd.com Subject: Re: [PATCH 2/3] drm/connector: Add enum documentation to drm_colorspace Date: Thu, 16 Feb 2023 16:22:44 -0500 [thread overview] Message-ID: <fcce9fd0-ab33-8417-1e10-f4faf5e6d6a6@amd.com> (raw) In-Reply-To: <20230208105621.392fb2cc@eldfell> On 2/8/23 03:56, Pekka Paalanen wrote: > On Fri, 3 Feb 2023 02:07:43 +0000 > Joshua Ashton <joshua@froggi.es> wrote: > >> To match the other enums, and add more information about these values. >> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> >> 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/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". > It's sRGB when RGB encoding is used and BT.601 for YCbCr. > 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? > Constant luminance as defined in BT.2020. Table 4 and 5 of the spec talk about it. > 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"? > Agreed. The CTA-681 spec actually treats these as the same value and for DP the R'G'G' (for RGB encoded signals) and the Y'C'bC'r version (for YCbCr encoded signals) don't share an actual value but you can't use the R'G'B' for YCbCr and vice versa. Rolling both into one seems to be the most reasonable thing to do and doesn't clash with the spec. We can even combine it later with a new "encoding" property where userspace can provide a suggested encoding (or use the default, driver-selected one). >> + * @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. P3 D65 vs P3 DCI > >> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED: >> + * RGB wide gamut fixed point colorimetry format > > Is this one scRGB too? The spec doesn't seem to mention. -_- In another place it annotates this with (XR8, XR10, XR12), but I'm not sure if those are defined anywhere. Harry > >> + * @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
WARNING: multiple messages have this Message-ID (diff)
From: Harry Wentland <harry.wentland@amd.com> To: Pekka Paalanen <ppaalanen@gmail.com>, Joshua Ashton <joshua@froggi.es> Cc: "Sebastian Wick" <sebastian.wick@redhat.com>, amd-gfx@lists.freedesktop.org, "Uma Shankar" <uma.shankar@intel.com>, dri-devel@lists.freedesktop.org, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, Vitaly.Prosyak@amd.com Subject: Re: [PATCH 2/3] drm/connector: Add enum documentation to drm_colorspace Date: Thu, 16 Feb 2023 16:22:44 -0500 [thread overview] Message-ID: <fcce9fd0-ab33-8417-1e10-f4faf5e6d6a6@amd.com> (raw) In-Reply-To: <20230208105621.392fb2cc@eldfell> On 2/8/23 03:56, Pekka Paalanen wrote: > On Fri, 3 Feb 2023 02:07:43 +0000 > Joshua Ashton <joshua@froggi.es> wrote: > >> To match the other enums, and add more information about these values. >> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> >> 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/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". > It's sRGB when RGB encoding is used and BT.601 for YCbCr. > 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? > Constant luminance as defined in BT.2020. Table 4 and 5 of the spec talk about it. > 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"? > Agreed. The CTA-681 spec actually treats these as the same value and for DP the R'G'G' (for RGB encoded signals) and the Y'C'bC'r version (for YCbCr encoded signals) don't share an actual value but you can't use the R'G'B' for YCbCr and vice versa. Rolling both into one seems to be the most reasonable thing to do and doesn't clash with the spec. We can even combine it later with a new "encoding" property where userspace can provide a suggested encoding (or use the default, driver-selected one). >> + * @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. P3 D65 vs P3 DCI > >> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED: >> + * RGB wide gamut fixed point colorimetry format > > Is this one scRGB too? The spec doesn't seem to mention. -_- In another place it annotates this with (XR8, XR10, XR12), but I'm not sure if those are defined anywhere. Harry > >> + * @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 prev parent reply other threads:[~2023-02-16 21:22 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-03 2:07 [PATCH 1/3] drm/connector: Convert DRM_MODE_COLORIMETRY to enum Joshua Ashton 2023-02-03 2:07 ` Joshua Ashton 2023-02-03 2:07 ` [PATCH 2/3] drm/connector: Add enum documentation to drm_colorspace Joshua Ashton 2023-02-03 2:07 ` Joshua Ashton 2023-02-08 8:56 ` Pekka Paalanen 2023-02-08 8:56 ` Pekka Paalanen 2023-02-16 21:22 ` Harry Wentland [this message] 2023-02-16 21:22 ` Harry Wentland 2023-02-03 2:07 ` [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum Joshua Ashton 2023-02-03 2:07 ` Joshua Ashton 2023-02-03 10:39 ` Ville Syrjälä 2023-02-03 12:59 ` Sebastian Wick 2023-02-03 13:35 ` Ville Syrjälä 2023-02-03 13:52 ` Sebastian Wick 2023-02-03 14:02 ` Ville Syrjälä 2023-02-08 9:18 ` Pekka Paalanen 2023-02-08 14:49 ` Ville Syrjälä 2023-02-09 10:05 ` Pekka Paalanen 2023-02-09 10:05 ` Pekka Paalanen 2023-02-03 14:39 ` Harry Wentland 2023-02-03 15:19 ` Ville Syrjälä 2023-02-03 15:24 ` Harry Wentland 2023-02-03 16:00 ` Ville Syrjälä 2023-02-03 18:28 ` Harry Wentland 2023-02-03 18:56 ` Ville Syrjälä 2023-02-03 19:16 ` Harry Wentland 2023-02-03 19:25 ` Ville Syrjälä 2023-02-03 19:33 ` Harry Wentland 2023-02-08 10:03 ` Pekka Paalanen 2023-02-08 10:03 ` Pekka Paalanen 2023-02-03 19:34 ` Ville Syrjälä 2023-02-03 19:43 ` Harry Wentland 2023-02-04 6:09 ` Joshua Ashton 2023-02-06 9:47 ` Ville Syrjälä 2023-02-06 9:47 ` Ville Syrjälä 2023-02-06 17:16 ` Harry Wentland 2023-02-08 10:32 ` Pekka Paalanen 2023-02-08 10:32 ` Pekka Paalanen 2023-02-08 9:30 ` Pekka Paalanen 2023-02-08 9:30 ` Pekka Paalanen 2023-02-08 9:25 ` Pekka Paalanen 2023-02-08 9:25 ` Pekka Paalanen 2023-02-14 15:49 ` Sebastian Wick 2023-02-14 15:49 ` Sebastian Wick 2023-02-14 16:56 ` Harry Wentland 2023-02-14 19:45 ` Sebastian Wick 2023-02-14 19:45 ` Sebastian Wick 2023-02-14 20:04 ` Harry Wentland 2023-02-14 20:04 ` Harry Wentland 2023-02-15 9:40 ` Pekka Paalanen 2023-02-15 9:40 ` Pekka Paalanen 2023-02-15 20:45 ` Harry Wentland 2023-02-15 20:45 ` Harry Wentland 2023-02-14 20:10 ` Ville Syrjälä 2023-02-14 20:10 ` Ville Syrjälä 2023-02-14 21:18 ` Sebastian Wick 2023-02-14 21:18 ` Sebastian Wick 2023-02-14 21:27 ` Ville Syrjälä 2023-02-14 21:27 ` Ville Syrjälä 2023-02-15 10:01 ` Pekka Paalanen 2023-02-15 10:01 ` Pekka Paalanen 2023-02-15 10:33 ` Ville Syrjälä 2023-02-15 10:33 ` Ville Syrjälä 2023-02-15 11:46 ` Daniel Stone 2023-02-15 11:46 ` Daniel Stone 2023-02-15 20:54 ` Harry Wentland 2023-02-15 20:54 ` Harry Wentland 2023-02-15 22:07 ` Daniel Stone 2023-02-15 22:07 ` Daniel Stone 2023-02-03 14:52 ` Harry Wentland 2023-02-04 16:06 ` kernel test robot 2023-02-04 16:06 ` kernel test robot 2023-02-08 9:30 ` Pekka Paalanen 2023-02-08 9:30 ` Pekka Paalanen 2023-02-09 16:38 ` Joshua Ashton 2023-02-09 16:38 ` Joshua Ashton 2023-02-09 17:03 ` Simon Ser 2023-02-09 18:08 ` Ville Syrjälä 2023-02-10 9:37 ` Pekka Paalanen 2023-02-10 9:44 ` Simon Ser 2023-02-04 9:06 ` [PATCH 1/3] drm/connector: Convert DRM_MODE_COLORIMETRY to enum kernel test robot 2023-02-04 9:06 ` kernel test robot 2023-02-04 10:28 ` kernel test robot 2023-02-04 10:28 ` kernel test robot 2023-02-08 8:29 ` Pekka Paalanen 2023-02-08 8:29 ` Pekka Paalanen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=fcce9fd0-ab33-8417-1e10-f4faf5e6d6a6@amd.com \ --to=harry.wentland@amd.com \ --cc=Vitaly.Prosyak@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=joshua@froggi.es \ --cc=ppaalanen@gmail.com \ --cc=sebastian.wick@redhat.com \ --cc=uma.shankar@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.