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

[Harry Wentland <harry.wentland@amd.com>]

On 3/8/23 03:59, Pekka Paalanen wrote:
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> From: Joshua Ashton <joshua@froggi.es>
>>
>> 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@froggi.es>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> 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@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 | 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