Re: [PATCH v3 03/17] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum

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

On 3/8/23 04:09, Pekka Paalanen wrote:
> On Tue, 7 Mar 2023 10:10:53 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> From: Joshua Ashton <joshua@froggi.es>
>>
>> Userspace has no way of controlling or knowing the pixel encoding
>> currently, so there is no way for it to ever get the right values here.
>>
>> When we do add pixel_encoding control from userspace,we can pick the
>> right value for the colorimetry packet based on the
>> pixel_encoding + the colorspace.
>>
>> Let's deprecate these values, and have one BT.2020 colorspace entry
>> that userspace can use.
>>
>> v2:
>>  - leave CYCC alone for now; it serves a purpose
>>  - leave BT2020_RGB the new default BT2020
>>
>> 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>
>>
>> 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
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_helper.c |  7 +++----
>>  drivers/gpu/drm/drm_connector.c           |  8 ++++----
>>  drivers/gpu/drm/i915/display/intel_dp.c   | 14 +++++++-------
>>  include/drm/drm_connector.h               | 15 +++++++++------
>>  4 files changed, 23 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> index faf5e9efa7d3..05a0d03ffcda 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> @@ -97,8 +97,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
>>  #define HDMI_COLORIMETRY_OPYCC_601		(C(3) | EC(3) | ACE(0))
>>  #define HDMI_COLORIMETRY_OPRGB			(C(3) | EC(4) | ACE(0))
>>  #define HDMI_COLORIMETRY_BT2020_CYCC		(C(3) | EC(5) | ACE(0))
>> -#define HDMI_COLORIMETRY_BT2020_RGB		(C(3) | EC(6) | ACE(0))
>> -#define HDMI_COLORIMETRY_BT2020_YCC		(C(3) | EC(6) | ACE(0))
>> +#define HDMI_COLORIMETRY_BT2020			(C(3) | EC(6) | ACE(0))
>>  #define HDMI_COLORIMETRY_DCI_P3_RGB_D65		(C(3) | EC(7) | ACE(0))
>>  #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER	(C(3) | EC(7) | ACE(1))
>>  
>> @@ -112,8 +111,8 @@ static const u32 hdmi_colorimetry_val[] = {
>>  	[DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
>>  	[DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
>>  	[DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
>> -	[DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
>> -	[DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
>> +	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = HDMI_COLORIMETRY_BT2020,
>> +	[DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
>>  };
>>  
>>  #undef C
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 61c29ce74b03..fe7eab15f727 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1031,9 +1031,9 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>>  	/* Colorimetry based on ITU-R BT.2020 */
>>  	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>>  	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>>  	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
>>  	/* Added as part of Additional Colorimetry Extension in 861.G */
>>  	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
>>  	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
>> @@ -1054,7 +1054,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>  	/* Colorimetry based on SMPTE RP 431-2 */
>>  	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
>>  	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>>  	{ DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
>>  	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
>>  	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> @@ -1068,7 +1068,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>  	/* Colorimetry based on ITU-R BT.2020 */
>>  	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>>  	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> 
> Let's hope no-one complains about missing the old string names in UABI. :-)
> 

As discussed at the hackfest, I'll drop this patch.

Harry

> Actually, you should write in the commit message why removing old names
> is fine.
> 
>>  };
>>  
>>  /**
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index c9be61d2348e..be100a193bf5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -1766,11 +1766,11 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>>  	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
>>  		vsc->colorimetry = DP_COLORIMETRY_BT2020_CYCC;
>>  		break;
>> -	case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> -		vsc->colorimetry = DP_COLORIMETRY_BT2020_RGB;
>> -		break;
>> -	case DRM_MODE_COLORIMETRY_BT2020_YCC:
>> -		vsc->colorimetry = DP_COLORIMETRY_BT2020_YCC;
>> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED:
>> +	case DRM_MODE_COLORIMETRY_BT2020:
>> +		vsc->colorimetry = vsc->pixelformat == DP_PIXELFORMAT_RGB
>> +			? DP_COLORIMETRY_BT2020_RGB
>> +			: DP_COLORIMETRY_BT2020_YCC;
>>  		break;
>>  	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>>  	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
>> @@ -3043,9 +3043,9 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
>>  	switch (conn_state->colorspace) {
>>  	case DRM_MODE_COLORIMETRY_SYCC_601:
>>  	case DRM_MODE_COLORIMETRY_OPYCC_601:
>> -	case DRM_MODE_COLORIMETRY_BT2020_YCC:
>> -	case DRM_MODE_COLORIMETRY_BT2020_RGB:
>>  	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
>> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED:
>> +	case DRM_MODE_COLORIMETRY_BT2020:
>>  		return true;
>>  	default:
>>  		break;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index bb078666dc34..3e2e1bc7aa04 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -409,12 +409,15 @@ enum drm_privacy_screen_status {
>>   * @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:
>> + * @DRM_MODE_COLORIMETRY_BT2020:
>>   *   (HDMI, DP)
>> - *   ITU-R BT.2020 R' G' B' colorimetry format
>> - * @DRM_MODE_COLORIMETRY_BT2020_YCC:
>> + *   ITU-R BT.2020 [R' G' B'] or
>> + * 	 ITU-R BT.2020 [Y' C'b C'r] or
>> + *   ITU-R BT.2020 [Y'c C'bc C'rc] (linear)
> 
> Drop the constant luminance variant from this value's doc.
> 
>> + *   colorimetry format
>> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED:
>>   *   (HDMI, DP)
>> - *   ITU-R BT.2020 Y' C'b C'r colorimetry format
>> + *   deprecated; same as DRM_MODE_COLORIMETRY_BT2020
>>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>>   *   (HDMI)
>>   *   SMPTE ST 2113 P3D65 colorimetry format
>> @@ -448,8 +451,8 @@ enum drm_colorspace {
>>  	DRM_MODE_COLORIMETRY_OPYCC_601		= 6,
>>  	DRM_MODE_COLORIMETRY_OPRGB		= 7,
>>  	DRM_MODE_COLORIMETRY_BT2020_CYCC	= 8,
>> -	DRM_MODE_COLORIMETRY_BT2020_RGB		= 9,
>> -	DRM_MODE_COLORIMETRY_BT2020_YCC		= 10,
>> +	DRM_MODE_COLORIMETRY_BT2020		= 9,
>> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED	= 10,
>>  	/* Additional Colorimetry extension added as part of CTA 861.G */
>>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65	= 11,
>>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER	= 12,
> 
> With the caveat noted and nitpick fixed:
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> 
> 
> Thanks,
> pq