On 3/8/23 03:59, Pekka Paalanen wrote:
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland <harry.wentland@xxxxxxx> wrote:
>
>> From: Joshua Ashton <joshua@xxxxxxxxx>
>>
>> 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 <joshua@xxxxxxxxx>
>> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
>> Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx>
>
> 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 <ppaalanen@xxxxxxxxx>
>> Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
>> Cc: Vitaly.Prosyak@xxxxxxx
>> Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Joshua Ashton <joshua@xxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> ---
>> 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
