[3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum

Commit Message

Joshua Ashton Feb. 3, 2023, 2:07 a.m. UTC

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.

Note: _CYCC was effectively 'removed' by this change, but that was not
possible to be taken advantage of anyway, as there is currently no
pixel_encoding control so it would not be possible to output
linear YCbCr.

Signed-off-by: Joshua Ashton <[e-mail]>

Cc: Pekka Paalanen <[e-mail]>
Cc: Sebastian Wick <[e-mail]>
Cc: [e-mail]
Cc: Uma Shankar <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Joshua Ashton <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
 drivers/gpu/drm/drm_connector.c           | 12 ++++++------
 drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
 include/drm/drm_connector.h               | 19 ++++++++++---------
 4 files changed, 29 insertions(+), 31 deletions(-)

Comments

Ville Syrjala Feb. 3, 2023, 10:39 a.m. UTC | #1

On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> 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.

That applies to a lot of the other values as well (they are
explicitly RGB or YCC). The idea was that this property sets the
infoframe/MSA/SDP value exactly, and other properties should be
added to for use userspace to control the pixel encoding/colorspace
conversion(if desired, or userspace just makes sure to
directly feed in correct kind of data).

> 
> 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.
> 
> Note: _CYCC was effectively 'removed' by this change, but that was not
> possible to be taken advantage of anyway, as there is currently no
> pixel_encoding control so it would not be possible to output
> linear YCbCr.
> 
> Signed-off-by: Joshua Ashton <[e-mail]>
> 
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: [e-mail]
> Cc: Uma Shankar <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Joshua Ashton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
>  drivers/gpu/drm/drm_connector.c           | 12 ++++++------
>  drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
>  include/drm/drm_connector.h               | 19 ++++++++++---------
>  4 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 0264abe55278..c85860600395 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -99,8 +99,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))
>  
> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
>  	[DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
>  	[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_1] = HDMI_COLORIMETRY_BT2020,
> +	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 */
>  	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>  	/* 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 */
> @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 [33] */
>  	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c9be61d2348e..1aa5dedeec7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1763,14 +1763,12 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>  	case DRM_MODE_COLORIMETRY_OPYCC_601:
>  		vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
>  		break;
> -	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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	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 +3041,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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	case DRM_MODE_COLORIMETRY_BT2020:
>  		return true;
>  	default:
>  		break;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index eb4cc9076e16..42a3cf43168c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -390,12 +390,13 @@ enum drm_privacy_screen_status {
>   *   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
> - * @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
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> + * @DRM_MODE_COLORIMETRY_BT2020:
> + *   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)
> + *   colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>   *   DCI-P3 (SMPTE RP 431-2) colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> @@ -420,9 +421,9 @@ enum drm_colorspace {
>  	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,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
> +	DRM_MODE_COLORIMETRY_BT2020,
>  	/* Additional Colorimetry extension added as part of CTA 861.G */
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
> -- 
> 2.39.1

Sebastian Wick Feb. 3, 2023, 12:59 p.m. UTC | #2

On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
<[e-mail]> wrote:
>
> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > 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.
>
> That applies to a lot of the other values as well (they are
> explicitly RGB or YCC). The idea was that this property sets the
> infoframe/MSA/SDP value exactly, and other properties should be
> added to for use userspace to control the pixel encoding/colorspace
> conversion(if desired, or userspace just makes sure to
> directly feed in correct kind of data).

I'm all for getting userspace control over pixel encoding but even
then the kernel always knows which pixel encoding is selected and
which InfoFrame has to be sent. Is there a reason why userspace would
want to control the variant explicitly to the wrong value?

>
> >
> > 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.
> >
> > Note: _CYCC was effectively 'removed' by this change, but that was not
> > possible to be taken advantage of anyway, as there is currently no
> > pixel_encoding control so it would not be possible to output
> > linear YCbCr.
> >
> > Signed-off-by: Joshua Ashton <[e-mail]>
> >
> > Cc: Pekka Paalanen <[e-mail]>
> > Cc: Sebastian Wick <[e-mail]>
> > Cc: [e-mail]
> > Cc: Uma Shankar <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Joshua Ashton <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
> >  drivers/gpu/drm/drm_connector.c           | 12 ++++++------
> >  drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
> >  include/drm/drm_connector.h               | 19 ++++++++++---------
> >  4 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index 0264abe55278..c85860600395 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -99,8 +99,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))
> >
> > @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
> >       [DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
> >       [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_1] = HDMI_COLORIMETRY_BT2020,
> > +     [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
> >       /* Colorimetry based on IEC 61966-2-5 */
> >       { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> >       /* Colorimetry based on ITU-R BT.2020 */
> > -     { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
> >       /* Colorimetry based on ITU-R BT.2020 */
> > -     { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
> >       /* Colorimetry based on ITU-R BT.2020 */
> > -     { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > +     { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> >       /* 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 */
> > @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >       /* Colorimetry based on IEC 61966-2-5 [33] */
> >       { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> >       /* Colorimetry based on ITU-R BT.2020 */
> > -     { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
> >       /* Colorimetry based on ITU-R BT.2020 */
> > -     { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
> >  };
> >
> >  /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index c9be61d2348e..1aa5dedeec7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1763,14 +1763,12 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> >       case DRM_MODE_COLORIMETRY_OPYCC_601:
> >               vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
> >               break;
> > -     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_1:
> > +     case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> > +     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 +3041,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_1:
> > +     case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> > +     case DRM_MODE_COLORIMETRY_BT2020:
> >               return true;
> >       default:
> >               break;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index eb4cc9076e16..42a3cf43168c 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -390,12 +390,13 @@ enum drm_privacy_screen_status {
> >   *   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
> > - * @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
> > + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
> > + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> > + * @DRM_MODE_COLORIMETRY_BT2020:
> > + *   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)
> > + *   colorimetry format
> >   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> >   *   DCI-P3 (SMPTE RP 431-2) colorimetry format
> >   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> > @@ -420,9 +421,9 @@ enum drm_colorspace {
> >       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,
> > +     DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
> > +     DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
> > +     DRM_MODE_COLORIMETRY_BT2020,
> >       /* Additional Colorimetry extension added as part of CTA 861.G */
> >       DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
> >       DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
> > --
> > 2.39.1
>
> --
> Ville Syrjälä
> Intel
>

Ville Syrjala Feb. 3, 2023, 1:35 p.m. UTC | #3

On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:
> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > 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.
> >
> > That applies to a lot of the other values as well (they are
> > explicitly RGB or YCC). The idea was that this property sets the
> > infoframe/MSA/SDP value exactly, and other properties should be
> > added to for use userspace to control the pixel encoding/colorspace
> > conversion(if desired, or userspace just makes sure to
> > directly feed in correct kind of data).
> 
> I'm all for getting userspace control over pixel encoding but even
> then the kernel always knows which pixel encoding is selected and
> which InfoFrame has to be sent. Is there a reason why userspace would
> want to control the variant explicitly to the wrong value?

What do you mean wrong value? Userspace sets it based on what
kind of data it has generated (or asked the display hardware
to generate if/when we get explicit control over that part).

Sebastian Wick Feb. 3, 2023, 1:52 p.m. UTC | #4

On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:
> > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > > 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.
> > >
> > > That applies to a lot of the other values as well (they are
> > > explicitly RGB or YCC). The idea was that this property sets the
> > > infoframe/MSA/SDP value exactly, and other properties should be
> > > added to for use userspace to control the pixel encoding/colorspace
> > > conversion(if desired, or userspace just makes sure to
> > > directly feed in correct kind of data).
> >
> > I'm all for getting userspace control over pixel encoding but even
> > then the kernel always knows which pixel encoding is selected and
> > which InfoFrame has to be sent. Is there a reason why userspace would
> > want to control the variant explicitly to the wrong value?
>
> What do you mean wrong value? Userspace sets it based on what
> kind of data it has generated (or asked the display hardware
> to generate if/when we get explicit control over that part).

Wrong in the sense of sending the YCC variant when the pixel encoding
is RGB for example.

Maybe I'm missing something here but my assumption is that the kernel
always has to know the pixel encoding anyway. The color pipeline also
assumes that the pixel values are RGB. User space might be able to
generate YCC content but for subsampling etc the pixel encoding still
has to be explicitly set.

So with the kernel always knowing exactly what pixel encoding is sent,
why do we need those variants? I just don't see why this is necessary.

>
> --
> Ville Syrjälä
> Intel
>

Ville Syrjala Feb. 3, 2023, 2:02 p.m. UTC | #5

On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote:
> On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:
> > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > > > 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.
> > > >
> > > > That applies to a lot of the other values as well (they are
> > > > explicitly RGB or YCC). The idea was that this property sets the
> > > > infoframe/MSA/SDP value exactly, and other properties should be
> > > > added to for use userspace to control the pixel encoding/colorspace
> > > > conversion(if desired, or userspace just makes sure to
> > > > directly feed in correct kind of data).
> > >
> > > I'm all for getting userspace control over pixel encoding but even
> > > then the kernel always knows which pixel encoding is selected and
> > > which InfoFrame has to be sent. Is there a reason why userspace would
> > > want to control the variant explicitly to the wrong value?
> >
> > What do you mean wrong value? Userspace sets it based on what
> > kind of data it has generated (or asked the display hardware
> > to generate if/when we get explicit control over that part).
> 
> Wrong in the sense of sending the YCC variant when the pixel encoding
> is RGB for example.
> 
> Maybe I'm missing something here but my assumption is that the kernel
> always has to know the pixel encoding anyway. The color pipeline also
> assumes that the pixel values are RGB. User space might be able to
> generate YCC content but for subsampling etc the pixel encoding still
> has to be explicitly set.

The kernel doesn't really know much atm. In theory you can just
configure the thing to do a straight passthough and put anything you
want into your pixels.

> 
> So with the kernel always knowing exactly what pixel encoding is sent,
> why do we need those variants? I just don't see why this is necessary.
> 
> >
> > --
> > Ville Syrjälä
> > Intel
> >

Harry Wentland Feb. 3, 2023, 2:39 p.m. UTC | #6

On 2/3/23 07:59, Sebastian Wick wrote:
> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> <[e-mail]> wrote:
>>
>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>> 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.
>>
>> That applies to a lot of the other values as well (they are
>> explicitly RGB or YCC). The idea was that this property sets the
>> infoframe/MSA/SDP value exactly, and other properties should be
>> added to for use userspace to control the pixel encoding/colorspace
>> conversion(if desired, or userspace just makes sure to
>> directly feed in correct kind of data).
> 
> I'm all for getting userspace control over pixel encoding but even
> then the kernel always knows which pixel encoding is selected and
> which InfoFrame has to be sent. Is there a reason why userspace would
> want to control the variant explicitly to the wrong value?
> 

I've asked this before but haven't seen an answer: Is there an existing
upstream userspace project that makes use of this property (other than
what Joshua is working on in gamescope right now)? That would help us
understand the intent better.

I don't think giving userspace explicit control over the exact infoframe
values is the right thing to do.

Harry

>>
>>>
>>> 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.
>>>
>>> Note: _CYCC was effectively 'removed' by this change, but that was not
>>> possible to be taken advantage of anyway, as there is currently no
>>> pixel_encoding control so it would not be possible to output
>>> linear YCbCr.
>>>
>>> Signed-off-by: Joshua Ashton <[e-mail]>
>>>
>>> Cc: Pekka Paalanen <[e-mail]>
>>> Cc: Sebastian Wick <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: Uma Shankar <[e-mail]>
>>> Cc: Ville Syrjälä <[e-mail]>
>>> Cc: Joshua Ashton <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>  drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
>>>  drivers/gpu/drm/drm_connector.c           | 12 ++++++------
>>>  drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
>>>  include/drm/drm_connector.h               | 19 ++++++++++---------
>>>  4 files changed, 29 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> index 0264abe55278..c85860600395 100644
>>> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> @@ -99,8 +99,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))
>>>
>>> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
>>>       [DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
>>>       [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_1] = HDMI_COLORIMETRY_BT2020,
>>> +     [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>>>       /* Colorimetry based on IEC 61966-2-5 */
>>>       { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>>>       /* Colorimetry based on ITU-R BT.2020 */
>>> -     { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>>> +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>>>       /* Colorimetry based on ITU-R BT.2020 */
>>> -     { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>>> +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>>>       /* Colorimetry based on ITU-R BT.2020 */
>>> -     { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>>> +     { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>>>       /* 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 */
>>> @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>>       /* Colorimetry based on IEC 61966-2-5 [33] */
>>>       { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>>>       /* Colorimetry based on ITU-R BT.2020 */
>>> -     { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>>> +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>>>       /* Colorimetry based on ITU-R BT.2020 */
>>> -     { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>>> +     { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>>>  };
>>>
>>>  /**
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index c9be61d2348e..1aa5dedeec7b 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -1763,14 +1763,12 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>>>       case DRM_MODE_COLORIMETRY_OPYCC_601:
>>>               vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
>>>               break;
>>> -     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_1:
>>> +     case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
>>> +     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 +3041,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_1:
>>> +     case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
>>> +     case DRM_MODE_COLORIMETRY_BT2020:
>>>               return true;
>>>       default:
>>>               break;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index eb4cc9076e16..42a3cf43168c 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -390,12 +390,13 @@ enum drm_privacy_screen_status {
>>>   *   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
>>> - * @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
>>> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
>>> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
>>> + * @DRM_MODE_COLORIMETRY_BT2020:
>>> + *   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)
>>> + *   colorimetry format
>>>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>>>   *   DCI-P3 (SMPTE RP 431-2) colorimetry format
>>>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
>>> @@ -420,9 +421,9 @@ enum drm_colorspace {
>>>       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,
>>> +     DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
>>> +     DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
>>> +     DRM_MODE_COLORIMETRY_BT2020,
>>>       /* Additional Colorimetry extension added as part of CTA 861.G */
>>>       DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
>>>       DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
>>> --
>>> 2.39.1
>>
>> --
>> Ville Syrjälä
>> Intel
>>
>

Harry Wentland Feb. 3, 2023, 2:52 p.m. UTC | #7

On 2/2/23 21:07, Joshua Ashton wrote:
> 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.
> 

Would be good to do the same for the other entries as well but those
are a bit more ambiguous since there are no clear _RGB variants and
the intention seems to be to use _DEFAULT for RGB.

> Note: _CYCC was effectively 'removed' by this change, but that was not
> possible to be taken advantage of anyway, as there is currently no
> pixel_encoding control so it would not be possible to output
> linear YCbCr.
> 
> Signed-off-by: Joshua Ashton <[e-mail]>
> 
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: [e-mail]
> Cc: Uma Shankar <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Joshua Ashton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
>  drivers/gpu/drm/drm_connector.c           | 12 ++++++------
>  drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
>  include/drm/drm_connector.h               | 19 ++++++++++---------
>  4 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 0264abe55278..c85860600395 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -99,8 +99,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))

These definitions might still be useful to a driver that is populating
the infoframe. But since they're currently unused I have no strong
objection to removing them.

If we're dropping them should we also drop the _CYCC variant?

>  #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))
>  
> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
>  	[DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
>  	[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_1] = HDMI_COLORIMETRY_BT2020,
> +	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 */
>  	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>  	/* 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" },

Above we've made the old _YCC value the new, non-deprecated BT2020
entry, but here you're using the _RGB one for that. Would it make
sense to make the old _YCC value deprecated and use the _RGB value
for the new, non-deprecated BT2020 variant?

I guess I'd like to avoid userspace having to pass in a different
value for DP or HDMI.

Harry

>  	{ DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
>  	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
>  	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 [33] */
>  	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c9be61d2348e..1aa5dedeec7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1763,14 +1763,12 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>  	case DRM_MODE_COLORIMETRY_OPYCC_601:
>  		vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
>  		break;
> -	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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	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 +3041,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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	case DRM_MODE_COLORIMETRY_BT2020:
>  		return true;
>  	default:
>  		break;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index eb4cc9076e16..42a3cf43168c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -390,12 +390,13 @@ enum drm_privacy_screen_status {
>   *   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
> - * @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
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> + * @DRM_MODE_COLORIMETRY_BT2020:
> + *   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)
> + *   colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>   *   DCI-P3 (SMPTE RP 431-2) colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> @@ -420,9 +421,9 @@ enum drm_colorspace {
>  	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,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
> +	DRM_MODE_COLORIMETRY_BT2020,
>  	/* Additional Colorimetry extension added as part of CTA 861.G */
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,

Ville Syrjala Feb. 3, 2023, 3:19 p.m. UTC | #8

On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> 
> 
> On 2/3/23 07:59, Sebastian Wick wrote:
> > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > <[e-mail]> wrote:
> >>
> >> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> >>> 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.
> >>
> >> That applies to a lot of the other values as well (they are
> >> explicitly RGB or YCC). The idea was that this property sets the
> >> infoframe/MSA/SDP value exactly, and other properties should be
> >> added to for use userspace to control the pixel encoding/colorspace
> >> conversion(if desired, or userspace just makes sure to
> >> directly feed in correct kind of data).
> > 
> > I'm all for getting userspace control over pixel encoding but even
> > then the kernel always knows which pixel encoding is selected and
> > which InfoFrame has to be sent. Is there a reason why userspace would
> > want to control the variant explicitly to the wrong value?
> > 
> 
> I've asked this before but haven't seen an answer: Is there an existing
> upstream userspace project that makes use of this property (other than
> what Joshua is working on in gamescope right now)? That would help us
> understand the intent better.

The intent was to control the infoframe colorimetry bits,
nothing more. No idea what real userspace there was, if any.

> 
> I don't think giving userspace explicit control over the exact infoframe
> values is the right thing to do.

Only userspace knows what kind of data it's stuffing into
the pixels (and/or how it configures the csc units/etc.) to
generate them.

I really don't want a repeat of the disaster of the
'Broadcast RGB' which has coupled together the infoframe 
and automagic conversion stuff. And I think this one would
be about 100x worse given this property has something
to do with actual colorspaces as well.

Harry Wentland Feb. 3, 2023, 3:24 p.m. UTC | #9

On 2/3/23 10:19, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>
>>
>> On 2/3/23 07:59, Sebastian Wick wrote:
>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>> <[e-mail]> wrote:
>>>>
>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>> 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.
>>>>
>>>> That applies to a lot of the other values as well (they are
>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>> added to for use userspace to control the pixel encoding/colorspace
>>>> conversion(if desired, or userspace just makes sure to
>>>> directly feed in correct kind of data).
>>>
>>> I'm all for getting userspace control over pixel encoding but even
>>> then the kernel always knows which pixel encoding is selected and
>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>> want to control the variant explicitly to the wrong value?
>>>
>>
>> I've asked this before but haven't seen an answer: Is there an existing
>> upstream userspace project that makes use of this property (other than
>> what Joshua is working on in gamescope right now)? That would help us
>> understand the intent better.
> 
> The intent was to control the infoframe colorimetry bits,
> nothing more. No idea what real userspace there was, if any.
> 
>>
>> I don't think giving userspace explicit control over the exact infoframe
>> values is the right thing to do.
> 
> Only userspace knows what kind of data it's stuffing into
> the pixels (and/or how it configures the csc units/etc.) to
> generate them.
> 

Yes, but userspace doesn't control or know whether we drive
RGB or YCbCr on the wire. In fact, in some cases our driver
needs to fallback to YCbCr420 for bandwidth reasons. There
is currently no way for userspace to know that and I don't
think it makes sense.

Userspace needs full control of framebuffer pixel formats,
as well as control over DEGAMMA, GAMMA, CTM color operations.
It also needs to be able to select whether to drive the panel
as sRGB or BT.2020/PQ but it doesn't make sense for it to
control the pixel encoding on the wire (RGB vs YCbCr).

> I really don't want a repeat of the disaster of the
> 'Broadcast RGB' which has coupled together the infoframe 
> and automagic conversion stuff. And I think this one would
> be about 100x worse given this property has something
> to do with actual colorspaces as well.
>  

I'm unaware of this disaster. Could you elaborate?

Harry

Ville Syrjala Feb. 3, 2023, 4 p.m. UTC | #10

On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> 
> 
> On 2/3/23 10:19, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2/3/23 07:59, Sebastian Wick wrote:
> >>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> >>>>> 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.
> >>>>
> >>>> That applies to a lot of the other values as well (they are
> >>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>> added to for use userspace to control the pixel encoding/colorspace
> >>>> conversion(if desired, or userspace just makes sure to
> >>>> directly feed in correct kind of data).
> >>>
> >>> I'm all for getting userspace control over pixel encoding but even
> >>> then the kernel always knows which pixel encoding is selected and
> >>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>> want to control the variant explicitly to the wrong value?
> >>>
> >>
> >> I've asked this before but haven't seen an answer: Is there an existing
> >> upstream userspace project that makes use of this property (other than
> >> what Joshua is working on in gamescope right now)? That would help us
> >> understand the intent better.
> > 
> > The intent was to control the infoframe colorimetry bits,
> > nothing more. No idea what real userspace there was, if any.
> > 
> >>
> >> I don't think giving userspace explicit control over the exact infoframe
> >> values is the right thing to do.
> > 
> > Only userspace knows what kind of data it's stuffing into
> > the pixels (and/or how it configures the csc units/etc.) to
> > generate them.
> > 
> 
> Yes, but userspace doesn't control or know whether we drive
> RGB or YCbCr on the wire. In fact, in some cases our driver
> needs to fallback to YCbCr420 for bandwidth reasons. There
> is currently no way for userspace to know that and I don't
> think it makes sense.

People want that control as well for whatever reason. We've
been asked to allow YCbCr 4:4:4 output many times.

The automagic 4:2:0 fallback I think is rather fundementally
incompatible with fancy color management. How would we even
know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
that stuff is just always BT.709 limited range, no questions
asked.

So I think if userspace wants real color management it's
going to have to set up the whole pipeline. And for that
we need at least one new property to control the RGB->YCbCr
conversion (or to explicitly avoid it).

And given that the proposed patch just swept all the
non-BT.2020 issues under the rug makes me think no
one has actually come up with any kind of consistent
plan for anything else really.

> 
> Userspace needs full control of framebuffer pixel formats,
> as well as control over DEGAMMA, GAMMA, CTM color operations.
> It also needs to be able to select whether to drive the panel
> as sRGB or BT.2020/PQ but it doesn't make sense for it to
> control the pixel encoding on the wire (RGB vs YCbCr).
> 
> > I really don't want a repeat of the disaster of the
> > 'Broadcast RGB' which has coupled together the infoframe 
> > and automagic conversion stuff. And I think this one would
> > be about 100x worse given this property has something
> > to do with actual colorspaces as well.
> >  
> 
> I'm unaware of this disaster. Could you elaborate?

The property now controls both the infoframe stuff (and
whatever super vague stuff DP has for it in MSA) and 
full->limited range compression in the display pipeline. 
And as a result  there is no way to eg. allow already 
limited range input, which is what some people wanted.

And naturally it's all made a lot more terrible by all
the displays that fail to implement the spec correctly,
but that's another topic.

Harry Wentland Feb. 3, 2023, 6:28 p.m. UTC | #11

On 2/3/23 11:00, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>
>>
>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>
>>>>
>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>> 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.
>>>>>>
>>>>>> That applies to a lot of the other values as well (they are
>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>> directly feed in correct kind of data).
>>>>>
>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>> then the kernel always knows which pixel encoding is selected and
>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>> want to control the variant explicitly to the wrong value?
>>>>>
>>>>
>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>> upstream userspace project that makes use of this property (other than
>>>> what Joshua is working on in gamescope right now)? That would help us
>>>> understand the intent better.
>>>
>>> The intent was to control the infoframe colorimetry bits,
>>> nothing more. No idea what real userspace there was, if any.
>>>
>>>>
>>>> I don't think giving userspace explicit control over the exact infoframe
>>>> values is the right thing to do.
>>>
>>> Only userspace knows what kind of data it's stuffing into
>>> the pixels (and/or how it configures the csc units/etc.) to
>>> generate them.
>>>
>>
>> Yes, but userspace doesn't control or know whether we drive
>> RGB or YCbCr on the wire. In fact, in some cases our driver
>> needs to fallback to YCbCr420 for bandwidth reasons. There
>> is currently no way for userspace to know that and I don't
>> think it makes sense.
> 
> People want that control as well for whatever reason. We've
> been asked to allow YCbCr 4:4:4 output many times.
> 
> The automagic 4:2:0 fallback I think is rather fundementally
> incompatible with fancy color management. How would we even
> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> that stuff is just always BT.709 limited range, no questions
> asked.
> 

We use what we're telling the display, i.e., the value in the
colorspace property. That way we know whether to use a BT.2020
or BT.709 matrix.

I don't see how it's fundamentally incompatible with fancy
color management stuff.

If we start forbidding drivers from falling back to YCbCr
(whether 4:4:4 or 4:2:0) we will break existing behavior on
amdgpu and will see bug reports.

> So I think if userspace wants real color management it's
> going to have to set up the whole pipeline. And for that
> we need at least one new property to control the RGB->YCbCr
> conversion (or to explicitly avoid it).
> 
> And given that the proposed patch just swept all the
> non-BT.2020 issues under the rug makes me think no
> one has actually come up with any kind of consistent
> plan for anything else really.
> 

Does anyone actually use the non-BT.2020 colorspace stuff?

Harry

>>
>> Userspace needs full control of framebuffer pixel formats,
>> as well as control over DEGAMMA, GAMMA, CTM color operations.
>> It also needs to be able to select whether to drive the panel
>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
>> control the pixel encoding on the wire (RGB vs YCbCr).
>>
>>> I really don't want a repeat of the disaster of the
>>> 'Broadcast RGB' which has coupled together the infoframe 
>>> and automagic conversion stuff. And I think this one would
>>> be about 100x worse given this property has something
>>> to do with actual colorspaces as well.
>>>  
>>
>> I'm unaware of this disaster. Could you elaborate?
> 
> The property now controls both the infoframe stuff (and
> whatever super vague stuff DP has for it in MSA) and 
> full->limited range compression in the display pipeline. 
> And as a result  there is no way to eg. allow already 
> limited range input, which is what some people wanted.
> 
> And naturally it's all made a lot more terrible by all
> the displays that fail to implement the spec correctly,
> but that's another topic.
>

Ville Syrjala Feb. 3, 2023, 6:56 p.m. UTC | #12

On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
> 
> 
> On 2/3/23 11:00, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2/3/23 10:19, Ville Syrjälä wrote:
> >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> >>>>
> >>>>
> >>>> On 2/3/23 07:59, Sebastian Wick wrote:
> >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> >>>>>>> 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.
> >>>>>>
> >>>>>> That applies to a lot of the other values as well (they are
> >>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>> directly feed in correct kind of data).
> >>>>>
> >>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>> then the kernel always knows which pixel encoding is selected and
> >>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>> want to control the variant explicitly to the wrong value?
> >>>>>
> >>>>
> >>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>> upstream userspace project that makes use of this property (other than
> >>>> what Joshua is working on in gamescope right now)? That would help us
> >>>> understand the intent better.
> >>>
> >>> The intent was to control the infoframe colorimetry bits,
> >>> nothing more. No idea what real userspace there was, if any.
> >>>
> >>>>
> >>>> I don't think giving userspace explicit control over the exact infoframe
> >>>> values is the right thing to do.
> >>>
> >>> Only userspace knows what kind of data it's stuffing into
> >>> the pixels (and/or how it configures the csc units/etc.) to
> >>> generate them.
> >>>
> >>
> >> Yes, but userspace doesn't control or know whether we drive
> >> RGB or YCbCr on the wire. In fact, in some cases our driver
> >> needs to fallback to YCbCr420 for bandwidth reasons. There
> >> is currently no way for userspace to know that and I don't
> >> think it makes sense.
> > 
> > People want that control as well for whatever reason. We've
> > been asked to allow YCbCr 4:4:4 output many times.
> > 
> > The automagic 4:2:0 fallback I think is rather fundementally
> > incompatible with fancy color management. How would we even
> > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> > that stuff is just always BT.709 limited range, no questions
> > asked.
> > 
> 
> We use what we're telling the display, i.e., the value in the
> colorspace property. That way we know whether to use a BT.2020
> or BT.709 matrix.

And given how these things have gone in the past I think
that is likey to bite someone at in the future. Also not
what this property was meant to do nor does on any other
driver AFAIK.

> I don't see how it's fundamentally incompatible with fancy
> color management stuff.
> 
> If we start forbidding drivers from falling back to YCbCr
> (whether 4:4:4 or 4:2:0) we will break existing behavior on
> amdgpu and will see bug reports.

The compositors could deal with that if/when they start doing
the full color management stuff. The current stuff only really
works when the kernel is allowed to do whatever it wants.

> 
> > So I think if userspace wants real color management it's
> > going to have to set up the whole pipeline. And for that
> > we need at least one new property to control the RGB->YCbCr
> > conversion (or to explicitly avoid it).
> > 
> > And given that the proposed patch just swept all the
> > non-BT.2020 issues under the rug makes me think no
> > one has actually come up with any kind of consistent
> > plan for anything else really.
> > 
> 
> Does anyone actually use the non-BT.2020 colorspace stuff?

No idea if anyone is using any of it. It's a bit hard to do
right now outside the full passthrough case since we have no
properties to control how the hardware will convert stuff.

Anyways, sounds like what you're basically proposing is
getting rid of this property and starting from scratch.

Harry Wentland Feb. 3, 2023, 7:16 p.m. UTC | #13

On 2/3/23 13:56, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
>>
>>
>> On 2/3/23 11:00, Ville Syrjälä wrote:
>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>
>>>>
>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>
>>>>>>
>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>> directly feed in correct kind of data).
>>>>>>>
>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>
>>>>>>
>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>> upstream userspace project that makes use of this property (other than
>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>> understand the intent better.
>>>>>
>>>>> The intent was to control the infoframe colorimetry bits,
>>>>> nothing more. No idea what real userspace there was, if any.
>>>>>
>>>>>>
>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>> values is the right thing to do.
>>>>>
>>>>> Only userspace knows what kind of data it's stuffing into
>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>> generate them.
>>>>>
>>>>
>>>> Yes, but userspace doesn't control or know whether we drive
>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>> is currently no way for userspace to know that and I don't
>>>> think it makes sense.
>>>
>>> People want that control as well for whatever reason. We've
>>> been asked to allow YCbCr 4:4:4 output many times.
>>>
>>> The automagic 4:2:0 fallback I think is rather fundementally
>>> incompatible with fancy color management. How would we even
>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>> that stuff is just always BT.709 limited range, no questions
>>> asked.
>>>
>>
>> We use what we're telling the display, i.e., the value in the
>> colorspace property. That way we know whether to use a BT.2020
>> or BT.709 matrix.
> 
> And given how these things have gone in the past I think
> that is likey to bite someone at in the future. Also not
> what this property was meant to do nor does on any other
> driver AFAIK.
> 

It has implementations in other drivers but I have yet to
see anyone using it. Without that it does nothing, unless
there are proprietary userspace pieces that make use of this.

>> I don't see how it's fundamentally incompatible with fancy
>> color management stuff.
>>
>> If we start forbidding drivers from falling back to YCbCr
>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
>> amdgpu and will see bug reports.
> 
> The compositors could deal with that if/when they start doing
> the full color management stuff. The current stuff only really
> works when the kernel is allowed to do whatever it wants.
> 

The compositor could deal with it but this feels like the
compositor taking over things that should really be in the
hands of a display driver.

>>
>>> So I think if userspace wants real color management it's
>>> going to have to set up the whole pipeline. And for that
>>> we need at least one new property to control the RGB->YCbCr
>>> conversion (or to explicitly avoid it).
>>>
>>> And given that the proposed patch just swept all the
>>> non-BT.2020 issues under the rug makes me think no
>>> one has actually come up with any kind of consistent
>>> plan for anything else really.
>>>
>>
>> Does anyone actually use the non-BT.2020 colorspace stuff?
> 
> No idea if anyone is using any of it. It's a bit hard to do
> right now outside the full passthrough case since we have no
> properties to control how the hardware will convert stuff.
> 
> Anyways, sounds like what you're basically proposing is
> getting rid of this property and starting from scratch.
> 

Maybe that's the right approach.

My initial idea was to tag along an existing property but
that turns out to be challenging when that existing property
doesn't even have a userspace implementation. IMO the existing
colorspace property shouldn't be a user-space controllable
property. I'll have to take a closer look at the
hdr_static_metadata to understand whether we might run into
similar issues with it.

The alternative is adding a new property to let userspace pick
the encoding and the min bpc value but that conflicts with the
expectation for a driver to always pick an encoding to satisfy
the bandwidth requirements for the mode on the wire [1].

In this scenario userspace would need to take full ownership of
the wire encoding and live with the consequences. If the bandwidth
is not sufficient a driver would then need to reject the commit
without having a mechanism to tell userspace the reason why.
The driver understands the bandwidth requirements but there is
currently no way for userspace to do so, in particular for
MST/USB-C/USB4/Thunderbold scenarios.

[1] https://lists.freedesktop.org/archives/amd-gfx/2022-December/087423.html

I don't think giving userspace full control of the encoding
buys us anything other than headaches.

Harry

Ville Syrjala Feb. 3, 2023, 7:25 p.m. UTC | #14

On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
> > 
> > 
> > On 2/3/23 11:00, Ville Syrjälä wrote:
> > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > >>
> > >>
> > >> On 2/3/23 10:19, Ville Syrjälä wrote:
> > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > >>>>
> > >>>>
> > >>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > >>>>> <[e-mail]> wrote:
> > >>>>>>
> > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > >>>>>>> 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.
> > >>>>>>
> > >>>>>> That applies to a lot of the other values as well (they are
> > >>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > >>>>>> added to for use userspace to control the pixel encoding/colorspace
> > >>>>>> conversion(if desired, or userspace just makes sure to
> > >>>>>> directly feed in correct kind of data).
> > >>>>>
> > >>>>> I'm all for getting userspace control over pixel encoding but even
> > >>>>> then the kernel always knows which pixel encoding is selected and
> > >>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > >>>>> want to control the variant explicitly to the wrong value?
> > >>>>>
> > >>>>
> > >>>> I've asked this before but haven't seen an answer: Is there an existing
> > >>>> upstream userspace project that makes use of this property (other than
> > >>>> what Joshua is working on in gamescope right now)? That would help us
> > >>>> understand the intent better.
> > >>>
> > >>> The intent was to control the infoframe colorimetry bits,
> > >>> nothing more. No idea what real userspace there was, if any.
> > >>>
> > >>>>
> > >>>> I don't think giving userspace explicit control over the exact infoframe
> > >>>> values is the right thing to do.
> > >>>
> > >>> Only userspace knows what kind of data it's stuffing into
> > >>> the pixels (and/or how it configures the csc units/etc.) to
> > >>> generate them.
> > >>>
> > >>
> > >> Yes, but userspace doesn't control or know whether we drive
> > >> RGB or YCbCr on the wire. In fact, in some cases our driver
> > >> needs to fallback to YCbCr420 for bandwidth reasons. There
> > >> is currently no way for userspace to know that and I don't
> > >> think it makes sense.
> > > 
> > > People want that control as well for whatever reason. We've
> > > been asked to allow YCbCr 4:4:4 output many times.
> > > 
> > > The automagic 4:2:0 fallback I think is rather fundementally
> > > incompatible with fancy color management. How would we even
> > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> > > that stuff is just always BT.709 limited range, no questions
> > > asked.
> > > 
> > 
> > We use what we're telling the display, i.e., the value in the
> > colorspace property. That way we know whether to use a BT.2020
> > or BT.709 matrix.
> 
> And given how these things have gone in the past I think
> that is likey to bite someone at in the future. Also not
> what this property was meant to do nor does on any other
> driver AFAIK.
> 
> > I don't see how it's fundamentally incompatible with fancy
> > color management stuff.
> > 
> > If we start forbidding drivers from falling back to YCbCr
> > (whether 4:4:4 or 4:2:0) we will break existing behavior on
> > amdgpu and will see bug reports.
> 
> The compositors could deal with that if/when they start doing
> the full color management stuff. The current stuff only really
> works when the kernel is allowed to do whatever it wants.
> 
> > 
> > > So I think if userspace wants real color management it's
> > > going to have to set up the whole pipeline. And for that
> > > we need at least one new property to control the RGB->YCbCr
> > > conversion (or to explicitly avoid it).
> > > 
> > > And given that the proposed patch just swept all the
> > > non-BT.2020 issues under the rug makes me think no
> > > one has actually come up with any kind of consistent
> > > plan for anything else really.
> > > 
> > 
> > Does anyone actually use the non-BT.2020 colorspace stuff?
> 
> No idea if anyone is using any of it. It's a bit hard to do
> right now outside the full passthrough case since we have no
> properties to control how the hardware will convert stuff.
> 
> Anyways, sounds like what you're basically proposing is
> getting rid of this property and starting from scratch.

Hmm. I guess one option would be to add that property to
control the output encoding, but include a few extra
"automagic" values to it which would retain the kernel's
freedom to select whether to do the RGB->YCbCr conversion
or not.

enum output_encoding {
	auto rgb=default/nodata,ycbcr=bt601
	auto rgb=default/nodata,ycbcr=bt709
	auto rgb=bt2020,ycbcr=bt2020
	passthrough
	rgb->ycbcr bt601,
	rgb->ycbcr bt709,
	rgb->ycbcr bt2020,
}

and then if you leave the colorspae property to "default"
the kernel will pick the "right" value based on the
output_encoding prop.

That would leave all the weird stuff in the colorspace
property alone and thus would still allow someone to
do more than just the basic stuff explicitly.

Harry Wentland Feb. 3, 2023, 7:33 p.m. UTC | #15

On 2/3/23 14:25, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
>>>
>>>
>>> On 2/3/23 11:00, Ville Syrjälä wrote:
>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>>
>>>>>
>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>> directly feed in correct kind of data).
>>>>>>>>
>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>
>>>>>>>
>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>> understand the intent better.
>>>>>>
>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>> nothing more. No idea what real userspace there was, if any.
>>>>>>
>>>>>>>
>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>> values is the right thing to do.
>>>>>>
>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>> generate them.
>>>>>>
>>>>>
>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>> is currently no way for userspace to know that and I don't
>>>>> think it makes sense.
>>>>
>>>> People want that control as well for whatever reason. We've
>>>> been asked to allow YCbCr 4:4:4 output many times.
>>>>
>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>> incompatible with fancy color management. How would we even
>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>> that stuff is just always BT.709 limited range, no questions
>>>> asked.
>>>>
>>>
>>> We use what we're telling the display, i.e., the value in the
>>> colorspace property. That way we know whether to use a BT.2020
>>> or BT.709 matrix.
>>
>> And given how these things have gone in the past I think
>> that is likey to bite someone at in the future. Also not
>> what this property was meant to do nor does on any other
>> driver AFAIK.
>>
>>> I don't see how it's fundamentally incompatible with fancy
>>> color management stuff.
>>>
>>> If we start forbidding drivers from falling back to YCbCr
>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
>>> amdgpu and will see bug reports.
>>
>> The compositors could deal with that if/when they start doing
>> the full color management stuff. The current stuff only really
>> works when the kernel is allowed to do whatever it wants.
>>
>>>
>>>> So I think if userspace wants real color management it's
>>>> going to have to set up the whole pipeline. And for that
>>>> we need at least one new property to control the RGB->YCbCr
>>>> conversion (or to explicitly avoid it).
>>>>
>>>> And given that the proposed patch just swept all the
>>>> non-BT.2020 issues under the rug makes me think no
>>>> one has actually come up with any kind of consistent
>>>> plan for anything else really.
>>>>
>>>
>>> Does anyone actually use the non-BT.2020 colorspace stuff?
>>
>> No idea if anyone is using any of it. It's a bit hard to do
>> right now outside the full passthrough case since we have no
>> properties to control how the hardware will convert stuff.
>>
>> Anyways, sounds like what you're basically proposing is
>> getting rid of this property and starting from scratch.
> 
> Hmm. I guess one option would be to add that property to
> control the output encoding, but include a few extra
> "automagic" values to it which would retain the kernel's
> freedom to select whether to do the RGB->YCbCr conversion
> or not.
> 
> enum output_encoding {
> 	auto rgb=default/nodata,ycbcr=bt601
> 	auto rgb=default/nodata,ycbcr=bt709
> 	auto rgb=bt2020,ycbcr=bt2020
> 	passthrough
> 	rgb->ycbcr bt601,
> 	rgb->ycbcr bt709,
> 	rgb->ycbcr bt2020,
> }
> 

Is there a good reason to decouple the YCbCr encoding format
from the colorspace?



> and then if you leave the colorspae property to "default"
> the kernel will pick the "right" value based on the
> output_encoding prop.
> 

How would you fill in the AVI infoframe? Userspace would still
need to set that to BT.2020 if the pixels are in BT.2020.

Harry

> That would leave all the weird stuff in the colorspace
> property alone and thus would still allow someone to
> do more than just the basic stuff explicitly.
>

Ville Syrjala Feb. 3, 2023, 7:34 p.m. UTC | #16

On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
> > > 
> > > 
> > > On 2/3/23 11:00, Ville Syrjälä wrote:
> > > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > > >>
> > > >>
> > > >> On 2/3/23 10:19, Ville Syrjälä wrote:
> > > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > > >>>>
> > > >>>>
> > > >>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > >>>>> <[e-mail]> wrote:
> > > >>>>>>
> > > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > >>>>>>> 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.
> > > >>>>>>
> > > >>>>>> That applies to a lot of the other values as well (they are
> > > >>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > > >>>>>> added to for use userspace to control the pixel encoding/colorspace
> > > >>>>>> conversion(if desired, or userspace just makes sure to
> > > >>>>>> directly feed in correct kind of data).
> > > >>>>>
> > > >>>>> I'm all for getting userspace control over pixel encoding but even
> > > >>>>> then the kernel always knows which pixel encoding is selected and
> > > >>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > > >>>>> want to control the variant explicitly to the wrong value?
> > > >>>>>
> > > >>>>
> > > >>>> I've asked this before but haven't seen an answer: Is there an existing
> > > >>>> upstream userspace project that makes use of this property (other than
> > > >>>> what Joshua is working on in gamescope right now)? That would help us
> > > >>>> understand the intent better.
> > > >>>
> > > >>> The intent was to control the infoframe colorimetry bits,
> > > >>> nothing more. No idea what real userspace there was, if any.
> > > >>>
> > > >>>>
> > > >>>> I don't think giving userspace explicit control over the exact infoframe
> > > >>>> values is the right thing to do.
> > > >>>
> > > >>> Only userspace knows what kind of data it's stuffing into
> > > >>> the pixels (and/or how it configures the csc units/etc.) to
> > > >>> generate them.
> > > >>>
> > > >>
> > > >> Yes, but userspace doesn't control or know whether we drive
> > > >> RGB or YCbCr on the wire. In fact, in some cases our driver
> > > >> needs to fallback to YCbCr420 for bandwidth reasons. There
> > > >> is currently no way for userspace to know that and I don't
> > > >> think it makes sense.
> > > > 
> > > > People want that control as well for whatever reason. We've
> > > > been asked to allow YCbCr 4:4:4 output many times.
> > > > 
> > > > The automagic 4:2:0 fallback I think is rather fundementally
> > > > incompatible with fancy color management. How would we even
> > > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> > > > that stuff is just always BT.709 limited range, no questions
> > > > asked.
> > > > 
> > > 
> > > We use what we're telling the display, i.e., the value in the
> > > colorspace property. That way we know whether to use a BT.2020
> > > or BT.709 matrix.
> > 
> > And given how these things have gone in the past I think
> > that is likey to bite someone at in the future. Also not
> > what this property was meant to do nor does on any other
> > driver AFAIK.
> > 
> > > I don't see how it's fundamentally incompatible with fancy
> > > color management stuff.
> > > 
> > > If we start forbidding drivers from falling back to YCbCr
> > > (whether 4:4:4 or 4:2:0) we will break existing behavior on
> > > amdgpu and will see bug reports.
> > 
> > The compositors could deal with that if/when they start doing
> > the full color management stuff. The current stuff only really
> > works when the kernel is allowed to do whatever it wants.
> > 
> > > 
> > > > So I think if userspace wants real color management it's
> > > > going to have to set up the whole pipeline. And for that
> > > > we need at least one new property to control the RGB->YCbCr
> > > > conversion (or to explicitly avoid it).
> > > > 
> > > > And given that the proposed patch just swept all the
> > > > non-BT.2020 issues under the rug makes me think no
> > > > one has actually come up with any kind of consistent
> > > > plan for anything else really.
> > > > 
> > > 
> > > Does anyone actually use the non-BT.2020 colorspace stuff?
> > 
> > No idea if anyone is using any of it. It's a bit hard to do
> > right now outside the full passthrough case since we have no
> > properties to control how the hardware will convert stuff.
> > 
> > Anyways, sounds like what you're basically proposing is
> > getting rid of this property and starting from scratch.
> 
> Hmm. I guess one option would be to add that property to
> control the output encoding, but include a few extra
> "automagic" values to it which would retain the kernel's
> freedom to select whether to do the RGB->YCbCr conversion
> or not.
> 
> enum output_encoding {
> 	auto rgb=default/nodata,ycbcr=bt601
> 	auto rgb=default/nodata,ycbcr=bt709
> 	auto rgb=bt2020,ycbcr=bt2020
> 	passthrough,
> 	rgb->ycbcr bt601,
> 	rgb->ycbcr bt709,
> 	rgb->ycbcr bt2020,
> }

In fact there should perhaps be a lot more of the explicit
options to get all subsamlings and quantizations ranges
coverted. That might actually be really nice for an igt
to get more full test coverage.

Harry Wentland Feb. 3, 2023, 7:43 p.m. UTC | #17

On 2/3/23 14:34, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
>>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
>>>>
>>>>
>>>> On 2/3/23 11:00, Ville Syrjälä wrote:
>>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>>>
>>>>>>
>>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>>> directly feed in correct kind of data).
>>>>>>>>>
>>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>>> understand the intent better.
>>>>>>>
>>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>>> nothing more. No idea what real userspace there was, if any.
>>>>>>>
>>>>>>>>
>>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>>> values is the right thing to do.
>>>>>>>
>>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>>> generate them.
>>>>>>>
>>>>>>
>>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>>> is currently no way for userspace to know that and I don't
>>>>>> think it makes sense.
>>>>>
>>>>> People want that control as well for whatever reason. We've
>>>>> been asked to allow YCbCr 4:4:4 output many times.
>>>>>
>>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>>> incompatible with fancy color management. How would we even
>>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>>> that stuff is just always BT.709 limited range, no questions
>>>>> asked.
>>>>>
>>>>
>>>> We use what we're telling the display, i.e., the value in the
>>>> colorspace property. That way we know whether to use a BT.2020
>>>> or BT.709 matrix.
>>>
>>> And given how these things have gone in the past I think
>>> that is likey to bite someone at in the future. Also not
>>> what this property was meant to do nor does on any other
>>> driver AFAIK.
>>>
>>>> I don't see how it's fundamentally incompatible with fancy
>>>> color management stuff.
>>>>
>>>> If we start forbidding drivers from falling back to YCbCr
>>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
>>>> amdgpu and will see bug reports.
>>>
>>> The compositors could deal with that if/when they start doing
>>> the full color management stuff. The current stuff only really
>>> works when the kernel is allowed to do whatever it wants.
>>>
>>>>
>>>>> So I think if userspace wants real color management it's
>>>>> going to have to set up the whole pipeline. And for that
>>>>> we need at least one new property to control the RGB->YCbCr
>>>>> conversion (or to explicitly avoid it).
>>>>>
>>>>> And given that the proposed patch just swept all the
>>>>> non-BT.2020 issues under the rug makes me think no
>>>>> one has actually come up with any kind of consistent
>>>>> plan for anything else really.
>>>>>
>>>>
>>>> Does anyone actually use the non-BT.2020 colorspace stuff?
>>>
>>> No idea if anyone is using any of it. It's a bit hard to do
>>> right now outside the full passthrough case since we have no
>>> properties to control how the hardware will convert stuff.
>>>
>>> Anyways, sounds like what you're basically proposing is
>>> getting rid of this property and starting from scratch.
>>
>> Hmm. I guess one option would be to add that property to
>> control the output encoding, but include a few extra
>> "automagic" values to it which would retain the kernel's
>> freedom to select whether to do the RGB->YCbCr conversion
>> or not.
>>
>> enum output_encoding {
>> 	auto rgb=default/nodata,ycbcr=bt601
>> 	auto rgb=default/nodata,ycbcr=bt709
>> 	auto rgb=bt2020,ycbcr=bt2020
>> 	passthrough,
>> 	rgb->ycbcr bt601,
>> 	rgb->ycbcr bt709,
>> 	rgb->ycbcr bt2020,
>> }
> 
> In fact there should perhaps be a lot more of the explicit
> options to get all subsamlings and quantizations ranges
> coverted. That might actually be really nice for an igt
> to get more full test coverage.
> 

I'd really like for IGT to get full test coverage but I'm still
conflicted by [1], i.e., the "chicken and egg problem" of the
DRM/KMS uAPI:

"The short summary is that any addition of DRM uAPI requires
corresponding open-sourced userspace patches, and those patches
must be reviewed and ready for merging into a suitable and
canonical upstream project."

[1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

We want low-level function in our DRM drivers and expose that up
to userspace. But when we do so the low level functionality might
be quite broad, e.g., all HDMI-spec defined colorspace formats in
the AVI infoframe, while a userspace implementation (if it exists)
is usually only interested in one new use case at a time. We then
go and define a couple handfuls of colorspaces but only use one,
in one scenario where the encoding is possible RGB anyways, and
the rest of the behavior is left unclear.

In short, what value does a new, well-designed and thought-out
property in DRM have if it isn't well covered by implementations
in upstream projects in production (not just testing, or
prototyping)?

Harry

Joshua Ashton Feb. 4, 2023, 6:09 a.m. UTC | #18

On 2/3/23 19:34, Ville Syrjälä wrote:
> On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
>>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
>>>>
>>>>
>>>> On 2/3/23 11:00, Ville Syrjälä wrote:
>>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>>>
>>>>>>
>>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>>> directly feed in correct kind of data).
>>>>>>>>>
>>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>>> understand the intent better.
>>>>>>>
>>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>>> nothing more. No idea what real userspace there was, if any.

Controlling the infoframe alone isn't useful at all unless you can 
guarantee the wire encoding, which we cannot do.

>>>>>>>
>>>>>>>>
>>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>>> values is the right thing to do.

+1

>>>>>>>
>>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>>> generate them.
>>>>>>>
>>>>>>
>>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>>> is currently no way for userspace to know that and I don't
>>>>>> think it makes sense.
>>>>>
>>>>> People want that control as well for whatever reason. We've
>>>>> been asked to allow YCbCr 4:4:4 output many times.
>>>>>
>>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>>> incompatible with fancy color management. How would we even
>>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>>> that stuff is just always BT.709 limited range, no questions
>>>>> asked.

That's what the Colorspace property *should* be determining here.
That's what we have it set up to do in SteamOS/my tree right now.

>>>>>
>>>>
>>>> We use what we're telling the display, i.e., the value in the
>>>> colorspace property. That way we know whether to use a BT.2020
>>>> or BT.709 matrix.
>>>
>>> And given how these things have gone in the past I think
>>> that is likey to bite someone at in the future. Also not
>>> what this property was meant to do nor does on any other
>>> driver AFAIK.
>>>
>>>> I don't see how it's fundamentally incompatible with fancy
>>>> color management stuff.
>>>>
>>>> If we start forbidding drivers from falling back to YCbCr
>>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
>>>> amdgpu and will see bug reports.
>>>
>>> The compositors could deal with that if/when they start doing
>>> the full color management stuff. The current stuff only really
>>> works when the kernel is allowed to do whatever it wants.
>>>
>>>>
>>>>> So I think if userspace wants real color management it's
>>>>> going to have to set up the whole pipeline. And for that
>>>>> we need at least one new property to control the RGB->YCbCr
>>>>> conversion (or to explicitly avoid it).

I mentioned this in my commit description, we absolutely should offer 
fine control here eventually.

I don't think we need to solve that problem here though.

>>>>>
>>>>> And given that the proposed patch just swept all the
>>>>> non-BT.2020 issues under the rug makes me think no
>>>>> one has actually come up with any kind of consistent
>>>>> plan for anything else really.
>>>>>
>>>>
>>>> Does anyone actually use the non-BT.2020 colorspace stuff?
>>>
>>> No idea if anyone is using any of it. It's a bit hard to do
>>> right now outside the full passthrough case since we have no
>>> properties to control how the hardware will convert stuff.

No, every userspace knows that encoding of the output buffer before 
going to the wire format is RGB.

It's the only way you can have planes alpha-blend, or mix and match RGB 
and NV12, etc.

>>>
>>> Anyways, sounds like what you're basically proposing is
>>> getting rid of this property and starting from scratch.
>>
>> Hmm. I guess one option would be to add that property to
>> control the output encoding, but include a few extra
>> "automagic" values to it which would retain the kernel's
>> freedom to select whether to do the RGB->YCbCr conversion
>> or not.
>>
>> enum output_encoding {
>> 	auto rgb=default/nodata,ycbcr=bt601
>> 	auto rgb=default/nodata,ycbcr=bt709
>> 	auto rgb=bt2020,ycbcr=bt2020
>> 	passthrough,
>> 	rgb->ycbcr bt601,
>> 	rgb->ycbcr bt709,
>> 	rgb->ycbcr bt2020,
>> }
> 
> In fact there should perhaps be a lot more of the explicit
> options to get all subsamlings and quantizations ranges
> coverted. That might actually be really nice for an igt
> to get more full test coverage.
> 
The choice of encoding of the pixel on the wire should be unrelated to 
the overall output colorspace from the userspace side -- but how the 
display engine converts the output to that wire format *is* dependent on 
the colorspace.
eg. picking a rec.709 ctc vs a rec.2020 ctc matrix.

I see you are proposing a "passthrough" but that wouldn't work at all as 
you still need to at know if you are RGB or YCbCr for the infoframe and 
to perform chroma subsampling in the display engine.

I perused the initial patches that added this property, and it seems 
there were no IGT tests or userspace implementation, so I am not 
entirely sure why it was committed in the first place.

Nobody can safely use Colorspace because of this problem right now.

If nobody is using this property, perhaps we could just get a fresh 
start, and either re-purpose it with new enum values, or obsolete it and 
make a new property.
If we do this, let's start with the absolute bare minimum, such as:
"Default/Rec.709 (sRGB), BT.2020"
and then grow as we need, making sure we have the full circle from 
userspace->output complete and working for each new value we add.

Please don't take this as me saying we shouldn't add all these other 
options like opRGB, etc, I just want us to progress to a solid base for 
expanding further here, which we really don't have right now.

- Joshie 

kernel test robot Feb. 4, 2023, 4:06 p.m. UTC | #19

Hi Joshua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes linus/master v6.2-rc6 next-20230203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Joshua-Ashton/drm-connector-Add-enum-documentation-to-drm_colorspace/20230203-100927
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230203020744.30745-3-joshua%40froggi.es
patch subject: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
reproduce:
        # https://github.com/intel-lab-lkp/linux/commit/14174503e23d2174ba6089fb4090778513cd202b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Joshua-Ashton/drm-connector-Add-enum-documentation-to-drm_colorspace/20230203-100927
        git checkout 14174503e23d2174ba6089fb4090778513cd202b
        make menuconfig
        # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
        make htmldocs

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> Documentation/gpu/drm-kms:417: ./include/drm/drm_connector.h:479: WARNING: Definition list ends without a blank line; unexpected unindent.

Ville Syrjala Feb. 6, 2023, 9:47 a.m. UTC | #20

On Sat, Feb 04, 2023 at 06:09:45AM +0000, Joshua Ashton wrote:
> 
> 
> On 2/3/23 19:34, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
> >> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
> >>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
> >>>>
> >>>>
> >>>> On 2/3/23 11:00, Ville Syrjälä wrote:
> >>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
> >>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> >>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>>>>>> <[e-mail]> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> >>>>>>>>>>> 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.
> >>>>>>>>>>
> >>>>>>>>>> That applies to a lot of the other values as well (they are
> >>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>>>>>> directly feed in correct kind of data).
> >>>>>>>>>
> >>>>>>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>>>>>> then the kernel always knows which pixel encoding is selected and
> >>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>>>>>> want to control the variant explicitly to the wrong value?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>>>>>> upstream userspace project that makes use of this property (other than
> >>>>>>>> what Joshua is working on in gamescope right now)? That would help us
> >>>>>>>> understand the intent better.
> >>>>>>>
> >>>>>>> The intent was to control the infoframe colorimetry bits,
> >>>>>>> nothing more. No idea what real userspace there was, if any.
> 
> Controlling the infoframe alone isn't useful at all unless you can 
> guarantee the wire encoding, which we cannot do.
> 
> >>>>>>>
> >>>>>>>>
> >>>>>>>> I don't think giving userspace explicit control over the exact infoframe
> >>>>>>>> values is the right thing to do.
> 
> +1
> 
> >>>>>>>
> >>>>>>> Only userspace knows what kind of data it's stuffing into
> >>>>>>> the pixels (and/or how it configures the csc units/etc.) to
> >>>>>>> generate them.
> >>>>>>>
> >>>>>>
> >>>>>> Yes, but userspace doesn't control or know whether we drive
> >>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
> >>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
> >>>>>> is currently no way for userspace to know that and I don't
> >>>>>> think it makes sense.
> >>>>>
> >>>>> People want that control as well for whatever reason. We've
> >>>>> been asked to allow YCbCr 4:4:4 output many times.
> >>>>>
> >>>>> The automagic 4:2:0 fallback I think is rather fundementally
> >>>>> incompatible with fancy color management. How would we even
> >>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> >>>>> that stuff is just always BT.709 limited range, no questions
> >>>>> asked.
> 
> That's what the Colorspace property *should* be determining here.
> That's what we have it set up to do in SteamOS/my tree right now.
> 
> >>>>>
> >>>>
> >>>> We use what we're telling the display, i.e., the value in the
> >>>> colorspace property. That way we know whether to use a BT.2020
> >>>> or BT.709 matrix.
> >>>
> >>> And given how these things have gone in the past I think
> >>> that is likey to bite someone at in the future. Also not
> >>> what this property was meant to do nor does on any other
> >>> driver AFAIK.
> >>>
> >>>> I don't see how it's fundamentally incompatible with fancy
> >>>> color management stuff.
> >>>>
> >>>> If we start forbidding drivers from falling back to YCbCr
> >>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
> >>>> amdgpu and will see bug reports.
> >>>
> >>> The compositors could deal with that if/when they start doing
> >>> the full color management stuff. The current stuff only really
> >>> works when the kernel is allowed to do whatever it wants.
> >>>
> >>>>
> >>>>> So I think if userspace wants real color management it's
> >>>>> going to have to set up the whole pipeline. And for that
> >>>>> we need at least one new property to control the RGB->YCbCr
> >>>>> conversion (or to explicitly avoid it).
> 
> I mentioned this in my commit description, we absolutely should offer 
> fine control here eventually.
> 
> I don't think we need to solve that problem here though.
> 
> >>>>>
> >>>>> And given that the proposed patch just swept all the
> >>>>> non-BT.2020 issues under the rug makes me think no
> >>>>> one has actually come up with any kind of consistent
> >>>>> plan for anything else really.
> >>>>>
> >>>>
> >>>> Does anyone actually use the non-BT.2020 colorspace stuff?
> >>>
> >>> No idea if anyone is using any of it. It's a bit hard to do
> >>> right now outside the full passthrough case since we have no
> >>> properties to control how the hardware will convert stuff.
> 
> No, every userspace knows that encoding of the output buffer before 
> going to the wire format is RGB.
> 
> It's the only way you can have planes alpha-blend, or mix and match RGB 
> and NV12, etc.
> 
> >>>
> >>> Anyways, sounds like what you're basically proposing is
> >>> getting rid of this property and starting from scratch.
> >>
> >> Hmm. I guess one option would be to add that property to
> >> control the output encoding, but include a few extra
> >> "automagic" values to it which would retain the kernel's
> >> freedom to select whether to do the RGB->YCbCr conversion
> >> or not.
> >>
> >> enum output_encoding {
> >> 	auto rgb=default/nodata,ycbcr=bt601
> >> 	auto rgb=default/nodata,ycbcr=bt709
> >> 	auto rgb=bt2020,ycbcr=bt2020
> >> 	passthrough,
> >> 	rgb->ycbcr bt601,
> >> 	rgb->ycbcr bt709,
> >> 	rgb->ycbcr bt2020,
> >> }
> > 
> > In fact there should perhaps be a lot more of the explicit
> > options to get all subsamlings and quantizations ranges
> > coverted. That might actually be really nice for an igt
> > to get more full test coverage.
> > 
> The choice of encoding of the pixel on the wire should be unrelated to 
> the overall output colorspace from the userspace side -- but how the 
> display engine converts the output to that wire format *is* dependent on 
> the colorspace.
> eg. picking a rec.709 ctc vs a rec.2020 ctc matrix.
> 
> I see you are proposing a "passthrough" but that wouldn't work at all as 
> you still need to at know if you are RGB or YCbCr for the infoframe and 
> to perform chroma subsampling in the display engine.

The passthrough (and other knobs after it) were meant for 
explicit control, which means they wouldn't affect infoframes.

But probably we should have seprate properties for explicit
control of each knob vs. some kind of easier to use property.
And I suppose we can still leave the explicit control stuff
for later (apart from the one property we already have).

> 
> I perused the initial patches that added this property, and it seems 
> there were no IGT tests or userspace implementation, so I am not 
> entirely sure why it was committed in the first place.

I presume at least the kodi HDR stuff uses ths. There may
have also been some chromeos stuff going on. Can't recall
anymore.

As for IGT, there's nothing we can really test since we 
have no way to get the inforframes/etc. back from the sink.
Hence nothing beyond the normal kms_property sanity checks
really makes sense.

> 
> Nobody can safely use Colorspace because of this problem right now.
> 
> If nobody is using this property, perhaps we could just get a fresh 
> start, and either re-purpose it with new enum values, or obsolete it and 
> make a new property.
> If we do this, let's start with the absolute bare minimum, such as:
> "Default/Rec.709 (sRGB), BT.2020"
> and then grow as we need, making sure we have the full circle from 
> userspace->output complete and working for each new value we add.

Yeah, I think a fresh property is what we want.

> 
> Please don't take this as me saying we shouldn't add all these other 
> options like opRGB, etc, I just want us to progress to a solid base for 
> expanding further here, which we really don't have right now.
> 
> - Joshie 

Harry Wentland Feb. 6, 2023, 5:16 p.m. UTC | #21

On 2/6/23 04:47, Ville Syrjälä wrote:
> On Sat, Feb 04, 2023 at 06:09:45AM +0000, Joshua Ashton wrote:
>>
>>
>> On 2/3/23 19:34, Ville Syrjälä wrote:
>>> On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:
>>>> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:
>>>>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:
>>>>>>
>>>>>>
>>>>>> On 2/3/23 11:00, Ville Syrjälä wrote:
>>>>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>>>>>> 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.
>>>>>>>>>>>>
>>>>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>>>>> directly feed in correct kind of data).
>>>>>>>>>>>
>>>>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>>>>> understand the intent better.
>>>>>>>>>
>>>>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>>>>> nothing more. No idea what real userspace there was, if any.
>>
>> Controlling the infoframe alone isn't useful at all unless you can 
>> guarantee the wire encoding, which we cannot do.
>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>>>>> values is the right thing to do.
>>
>> +1
>>
>>>>>>>>>
>>>>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>>>>> generate them.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>>>>> is currently no way for userspace to know that and I don't
>>>>>>>> think it makes sense.
>>>>>>>
>>>>>>> People want that control as well for whatever reason. We've
>>>>>>> been asked to allow YCbCr 4:4:4 output many times.
>>>>>>>
>>>>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>>>>> incompatible with fancy color management. How would we even
>>>>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>>>>> that stuff is just always BT.709 limited range, no questions
>>>>>>> asked.
>>
>> That's what the Colorspace property *should* be determining here.
>> That's what we have it set up to do in SteamOS/my tree right now.
>>
>>>>>>>
>>>>>>
>>>>>> We use what we're telling the display, i.e., the value in the
>>>>>> colorspace property. That way we know whether to use a BT.2020
>>>>>> or BT.709 matrix.
>>>>>
>>>>> And given how these things have gone in the past I think
>>>>> that is likey to bite someone at in the future. Also not
>>>>> what this property was meant to do nor does on any other
>>>>> driver AFAIK.
>>>>>
>>>>>> I don't see how it's fundamentally incompatible with fancy
>>>>>> color management stuff.
>>>>>>
>>>>>> If we start forbidding drivers from falling back to YCbCr
>>>>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
>>>>>> amdgpu and will see bug reports.
>>>>>
>>>>> The compositors could deal with that if/when they start doing
>>>>> the full color management stuff. The current stuff only really
>>>>> works when the kernel is allowed to do whatever it wants.
>>>>>
>>>>>>
>>>>>>> So I think if userspace wants real color management it's
>>>>>>> going to have to set up the whole pipeline. And for that
>>>>>>> we need at least one new property to control the RGB->YCbCr
>>>>>>> conversion (or to explicitly avoid it).
>>
>> I mentioned this in my commit description, we absolutely should offer 
>> fine control here eventually.
>>
>> I don't think we need to solve that problem here though.
>>
>>>>>>>
>>>>>>> And given that the proposed patch just swept all the
>>>>>>> non-BT.2020 issues under the rug makes me think no
>>>>>>> one has actually come up with any kind of consistent
>>>>>>> plan for anything else really.
>>>>>>>
>>>>>>
>>>>>> Does anyone actually use the non-BT.2020 colorspace stuff?
>>>>>
>>>>> No idea if anyone is using any of it. It's a bit hard to do
>>>>> right now outside the full passthrough case since we have no
>>>>> properties to control how the hardware will convert stuff.
>>
>> No, every userspace knows that encoding of the output buffer before 
>> going to the wire format is RGB.
>>
>> It's the only way you can have planes alpha-blend, or mix and match RGB 
>> and NV12, etc.
>>
>>>>>
>>>>> Anyways, sounds like what you're basically proposing is
>>>>> getting rid of this property and starting from scratch.
>>>>
>>>> Hmm. I guess one option would be to add that property to
>>>> control the output encoding, but include a few extra
>>>> "automagic" values to it which would retain the kernel's
>>>> freedom to select whether to do the RGB->YCbCr conversion
>>>> or not.
>>>>
>>>> enum output_encoding {
>>>> 	auto rgb=default/nodata,ycbcr=bt601
>>>> 	auto rgb=default/nodata,ycbcr=bt709
>>>> 	auto rgb=bt2020,ycbcr=bt2020
>>>> 	passthrough,
>>>> 	rgb->ycbcr bt601,
>>>> 	rgb->ycbcr bt709,
>>>> 	rgb->ycbcr bt2020,
>>>> }
>>>
>>> In fact there should perhaps be a lot more of the explicit
>>> options to get all subsamlings and quantizations ranges
>>> coverted. That might actually be really nice for an igt
>>> to get more full test coverage.
>>>
>> The choice of encoding of the pixel on the wire should be unrelated to 
>> the overall output colorspace from the userspace side -- but how the 
>> display engine converts the output to that wire format *is* dependent on 
>> the colorspace.
>> eg. picking a rec.709 ctc vs a rec.2020 ctc matrix.
>>
>> I see you are proposing a "passthrough" but that wouldn't work at all as 
>> you still need to at know if you are RGB or YCbCr for the infoframe and 
>> to perform chroma subsampling in the display engine.
> 
> The passthrough (and other knobs after it) were meant for 
> explicit control, which means they wouldn't affect infoframes.
> 
> But probably we should have seprate properties for explicit
> control of each knob vs. some kind of easier to use property.
> And I suppose we can still leave the explicit control stuff
> for later (apart from the one property we already have).
> 
>>
>> I perused the initial patches that added this property, and it seems 
>> there were no IGT tests or userspace implementation, so I am not 
>> entirely sure why it was committed in the first place.
> 
> I presume at least the kodi HDR stuff uses ths. There may
> have also been some chromeos stuff going on. Can't recall
> anymore.

I can't find mention of "colorspace" in kodi. Its SetHDR() [1]
function only sets HDR_OUTPUT_METADATA.

[1] https://github.com/xbmc/xbmc/blob/122916890a2b82ad8defaf2fd1934076387df84d/xbmc/windowing/gbm/WinSystemGbm.cpp#L316

> 
> As for IGT, there's nothing we can really test since we 
> have no way to get the inforframes/etc. back from the sink.
> Hence nothing beyond the normal kms_property sanity checks
> really makes sense.
> 

True, though a generic infoframe readback through debugfs might
be a nice-to-have for testing things that are expected to modify
the infoframe.

>>
>> Nobody can safely use Colorspace because of this problem right now.
>>
>> If nobody is using this property, perhaps we could just get a fresh 
>> start, and either re-purpose it with new enum values, or obsolete it and 
>> make a new property.
>> If we do this, let's start with the absolute bare minimum, such as:
>> "Default/Rec.709 (sRGB), BT.2020"
>> and then grow as we need, making sure we have the full circle from 
>> userspace->output complete and working for each new value we add.
> 

I agree. This leaves room for API that can be extensible but let's
not define things unless they're actually used in a full-stack
implementation.

> Yeah, I think a fresh property is what we want.
> 

Agreed. And if this new property is also updating the infoframe it needs
to be clear it's mutually exclusive with the existing property.

Harry

>>
>> Please don't take this as me saying we shouldn't add all these other 
>> options like opRGB, etc, I just want us to progress to a solid base for 
>> expanding further here, which we really don't have right now.
>>
>> - Joshie 

Pekka Paalanen Feb. 8, 2023, 9:18 a.m. UTC | #22

On Fri, 3 Feb 2023 16:02:51 +0200
Ville Syrjälä <[e-mail]> wrote:

> On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote:
> > On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä
> > <[e-mail]> wrote:  
> > >
> > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:  
> > > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > > <[e-mail]> wrote:  
> > > > >
> > > > > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> > > > > > 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.  
> > > > >
> > > > > That applies to a lot of the other values as well (they are
> > > > > explicitly RGB or YCC). The idea was that this property sets the
> > > > > infoframe/MSA/SDP value exactly, and other properties should be
> > > > > added to for use userspace to control the pixel encoding/colorspace
> > > > > conversion(if desired, or userspace just makes sure to
> > > > > directly feed in correct kind of data).  
> > > >
> > > > I'm all for getting userspace control over pixel encoding but even
> > > > then the kernel always knows which pixel encoding is selected and
> > > > which InfoFrame has to be sent. Is there a reason why userspace would
> > > > want to control the variant explicitly to the wrong value?  
> > >
> > > What do you mean wrong value? Userspace sets it based on what
> > > kind of data it has generated (or asked the display hardware
> > > to generate if/when we get explicit control over that part).  
> > 
> > Wrong in the sense of sending the YCC variant when the pixel encoding
> > is RGB for example.
> > 
> > Maybe I'm missing something here but my assumption is that the kernel
> > always has to know the pixel encoding anyway. The color pipeline also
> > assumes that the pixel values are RGB. User space might be able to
> > generate YCC content but for subsampling etc the pixel encoding still
> > has to be explicitly set.  
> 
> The kernel doesn't really know much atm. In theory you can just
> configure the thing to do a straight passthough and put anything you
> want into your pixels.

But it's impossible to use a YCbCr framebuffer and have that *not*
converted to RGB for the KMS color pipeline even if userspace wanted it
to be strictly pass-through, only to be converted again to YCbCr for
the cable, is it not?

Even more so with 4:2:0.

How could it be possible to stop the driver from doing those two
YUV-to-RGB and RGB-to-YCbCr conversions at the beginning and at the end
of the KMS color pipeline?

From uAPI point of view:

"Colorspace" currently defines (or does it? see my patch 2 review) the
colorimetry and the color model encoding. If a driver chooses the cable
encoding independently, the "Colorspace" color model encoding is often
wrong. If we have another KMS property to choose the cable encoding,
then it is possible to still set "Colorspace" to disagree with the
actual cable encoding. What's the use of that possibility to configure
things wrong?


Thanks,
pq

> 
> > 
> > So with the kernel always knowing exactly what pixel encoding is sent,
> > why do we need those variants? I just don't see why this is necessary.
> >   
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > >  
>

Pekka Paalanen Feb. 8, 2023, 9:25 a.m. UTC | #23

On Fri, 3 Feb 2023 18:00:44 +0200
Ville Syrjälä <[e-mail]> wrote:

> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > 
> > 
> > On 2/3/23 10:19, Ville Syrjälä wrote:  
> > > On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:  
> > >>
> > >>
> > >> On 2/3/23 07:59, Sebastian Wick wrote:  
> > >>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > >>> <[e-mail]> wrote:  
> > >>>>
> > >>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> > >>>>> 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.  
> > >>>>
> > >>>> That applies to a lot of the other values as well (they are
> > >>>> explicitly RGB or YCC). The idea was that this property sets the
> > >>>> infoframe/MSA/SDP value exactly, and other properties should be
> > >>>> added to for use userspace to control the pixel encoding/colorspace
> > >>>> conversion(if desired, or userspace just makes sure to
> > >>>> directly feed in correct kind of data).  
> > >>>
> > >>> I'm all for getting userspace control over pixel encoding but even
> > >>> then the kernel always knows which pixel encoding is selected and
> > >>> which InfoFrame has to be sent. Is there a reason why userspace would
> > >>> want to control the variant explicitly to the wrong value?
> > >>>  
> > >>
> > >> I've asked this before but haven't seen an answer: Is there an existing
> > >> upstream userspace project that makes use of this property (other than
> > >> what Joshua is working on in gamescope right now)? That would help us
> > >> understand the intent better.  
> > > 
> > > The intent was to control the infoframe colorimetry bits,
> > > nothing more. No idea what real userspace there was, if any.
> > >   
> > >>
> > >> I don't think giving userspace explicit control over the exact infoframe
> > >> values is the right thing to do.  
> > > 
> > > Only userspace knows what kind of data it's stuffing into
> > > the pixels (and/or how it configures the csc units/etc.) to
> > > generate them.
> > >   
> > 
> > Yes, but userspace doesn't control or know whether we drive
> > RGB or YCbCr on the wire. In fact, in some cases our driver
> > needs to fallback to YCbCr420 for bandwidth reasons. There
> > is currently no way for userspace to know that and I don't
> > think it makes sense.  
> 
> People want that control as well for whatever reason. We've
> been asked to allow YCbCr 4:4:4 output many times.
> 
> The automagic 4:2:0 fallback I think is rather fundementally
> incompatible with fancy color management. How would we even
> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> that stuff is just always BT.709 limited range, no questions
> asked.

The difference between 4:4:4 and 4:2:0 is purely the sub-sampling. It
has absolutely no implication to colorimetry nor MatrixCoefficients at
all.


Thanks,
pq

Pekka Paalanen Feb. 8, 2023, 9:30 a.m. UTC | #24

On Fri, 3 Feb 2023 20:56:55 +0200
Ville Syrjälä <[e-mail]> wrote:

> Anyways, sounds like what you're basically proposing is
> getting rid of this property and starting from scratch.

I would be happy with that (throwing "Colorspace" out and defining
something new). Then we can start with enum values we care and know
about.


Thanks,
pq

Pekka Paalanen Feb. 8, 2023, 9:30 a.m. UTC | #25

On Fri,  3 Feb 2023 02:07:44 +0000
Joshua Ashton <[e-mail]> wrote:

> 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.
> 
> Note: _CYCC was effectively 'removed' by this change, but that was not
> possible to be taken advantage of anyway, as there is currently no
> pixel_encoding control so it would not be possible to output
> linear YCbCr.
> 
> Signed-off-by: Joshua Ashton <[e-mail]>
> 
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: [e-mail]
> Cc: Uma Shankar <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Joshua Ashton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
>  drivers/gpu/drm/drm_connector.c           | 12 ++++++------
>  drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
>  include/drm/drm_connector.h               | 19 ++++++++++---------
>  4 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 0264abe55278..c85860600395 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -99,8 +99,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))
>  
> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
>  	[DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
>  	[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_1] = HDMI_COLORIMETRY_BT2020,
> +	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 */
>  	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>  	/* 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 */
> @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>  	/* Colorimetry based on IEC 61966-2-5 [33] */
>  	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>  	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>  };

Hi,

do these not rename the old uAPI strings?

Shouldn't the old strings be kept? It's much easier to scream "kernel
regression" when the expected string is no longer found than a subtle
change in behaviour that might not even be a change. ;-)

If there is not going to be a difference in behaviour, the enum could
expose e.g. all of "BT2020_RGB", "BT2020_CYCC" and "BT2020_YCC" as the
same integer value. If old userspace exists, it would not notice any
difference.

I mean, the *strings* are the uAPI, not the integers, right?


Thanks,
pq

>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c9be61d2348e..1aa5dedeec7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1763,14 +1763,12 @@ static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
>  	case DRM_MODE_COLORIMETRY_OPYCC_601:
>  		vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
>  		break;
> -	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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	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 +3041,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_1:
> +	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> +	case DRM_MODE_COLORIMETRY_BT2020:
>  		return true;
>  	default:
>  		break;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index eb4cc9076e16..42a3cf43168c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -390,12 +390,13 @@ enum drm_privacy_screen_status {
>   *   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
> - * @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
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
> + * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
> + * @DRM_MODE_COLORIMETRY_BT2020:
> + *   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)
> + *   colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
>   *   DCI-P3 (SMPTE RP 431-2) colorimetry format
>   * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> @@ -420,9 +421,9 @@ enum drm_colorspace {
>  	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,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
> +	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
> +	DRM_MODE_COLORIMETRY_BT2020,
>  	/* Additional Colorimetry extension added as part of CTA 861.G */
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
>  	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,

Pekka Paalanen Feb. 8, 2023, 10:03 a.m. UTC | #26

On Fri, 3 Feb 2023 14:33:46 -0500
Harry Wentland <[e-mail]> wrote:

> On 2/3/23 14:25, Ville Syrjälä wrote:
> > On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:  
> >> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:  
> >>>
> >>>
> >>> On 2/3/23 11:00, Ville Syrjälä wrote:  
> >>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:  
> >>>>>
> >>>>>
> >>>>> On 2/3/23 10:19, Ville Syrjälä wrote:  
> >>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:  
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:  
> >>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>>>>> <[e-mail]> wrote:  
> >>>>>>>>>
> >>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> >>>>>>>>>> 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.  
> >>>>>>>>>
> >>>>>>>>> That applies to a lot of the other values as well (they are
> >>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>>>>> directly feed in correct kind of data).  
> >>>>>>>>
> >>>>>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>>>>> then the kernel always knows which pixel encoding is selected and
> >>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>>>>> want to control the variant explicitly to the wrong value?
> >>>>>>>>  
> >>>>>>>
> >>>>>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>>>>> upstream userspace project that makes use of this property (other than
> >>>>>>> what Joshua is working on in gamescope right now)? That would help us
> >>>>>>> understand the intent better.  
> >>>>>>
> >>>>>> The intent was to control the infoframe colorimetry bits,
> >>>>>> nothing more. No idea what real userspace there was, if any.
> >>>>>>  
> >>>>>>>
> >>>>>>> I don't think giving userspace explicit control over the exact infoframe
> >>>>>>> values is the right thing to do.  
> >>>>>>
> >>>>>> Only userspace knows what kind of data it's stuffing into
> >>>>>> the pixels (and/or how it configures the csc units/etc.) to
> >>>>>> generate them.
> >>>>>>  
> >>>>>
> >>>>> Yes, but userspace doesn't control or know whether we drive
> >>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
> >>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
> >>>>> is currently no way for userspace to know that and I don't
> >>>>> think it makes sense.  
> >>>>
> >>>> People want that control as well for whatever reason. We've
> >>>> been asked to allow YCbCr 4:4:4 output many times.
> >>>>
> >>>> The automagic 4:2:0 fallback I think is rather fundementally
> >>>> incompatible with fancy color management. How would we even
> >>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> >>>> that stuff is just always BT.709 limited range, no questions
> >>>> asked.
> >>>>  
> >>>
> >>> We use what we're telling the display, i.e., the value in the
> >>> colorspace property. That way we know whether to use a BT.2020
> >>> or BT.709 matrix.  
> >>
> >> And given how these things have gone in the past I think
> >> that is likey to bite someone at in the future. Also not
> >> what this property was meant to do nor does on any other
> >> driver AFAIK.
> >>  
> >>> I don't see how it's fundamentally incompatible with fancy
> >>> color management stuff.
> >>>
> >>> If we start forbidding drivers from falling back to YCbCr
> >>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
> >>> amdgpu and will see bug reports.  
> >>
> >> The compositors could deal with that if/when they start doing
> >> the full color management stuff. The current stuff only really
> >> works when the kernel is allowed to do whatever it wants.
> >>  
> >>>  
> >>>> So I think if userspace wants real color management it's
> >>>> going to have to set up the whole pipeline. And for that
> >>>> we need at least one new property to control the RGB->YCbCr
> >>>> conversion (or to explicitly avoid it).
> >>>>
> >>>> And given that the proposed patch just swept all the
> >>>> non-BT.2020 issues under the rug makes me think no
> >>>> one has actually come up with any kind of consistent
> >>>> plan for anything else really.
> >>>>  
> >>>
> >>> Does anyone actually use the non-BT.2020 colorspace stuff?  
> >>
> >> No idea if anyone is using any of it. It's a bit hard to do
> >> right now outside the full passthrough case since we have no
> >> properties to control how the hardware will convert stuff.
> >>
> >> Anyways, sounds like what you're basically proposing is
> >> getting rid of this property and starting from scratch.  
> > 
> > Hmm. I guess one option would be to add that property to
> > control the output encoding, but include a few extra
> > "automagic" values to it which would retain the kernel's
> > freedom to select whether to do the RGB->YCbCr conversion
> > or not.
> > 
> > enum output_encoding {
> > 	auto rgb=default/nodata,ycbcr=bt601
> > 	auto rgb=default/nodata,ycbcr=bt709
> > 	auto rgb=bt2020,ycbcr=bt2020
> > 	passthrough
> > 	rgb->ycbcr bt601,
> > 	rgb->ycbcr bt709,
> > 	rgb->ycbcr bt2020,
> > }
> >   
> 
> Is there a good reason to decouple the YCbCr encoding format
> from the colorspace?

Well, they are two completely different things. Even more.

Using the CICP terms:
- ColourPrimaries
- TransferCharacteristics
- MatrixCoefficients
- VideoFullRangeFlag

Might also need to consider even the other ones:
https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/cicp_h273.md

In the narrow color gamut, SDR, cases, I believe getting the
MatrixCoefficients right was the important thing that people would
notice if it was wrong. ColourPrimaries did not really matter, who'd
use those anyway. VideoFullRangeFlag was really important to get right.
TransferCharacteristics was just about getting gamma in the right
ballpark.

Introduce WCG and especially HDR, TransferCharacteristics became really
important, and ColourPrimaries could not be ignored anymore.

The problem is that most of the relevant BT specs define all of these,
or even multiple options like BT.601 ColourPrimaries or BT.2020
MatrixCoefficients. Well, that's not really the problem, but the
practise of combining one thing from one spec and another from a
different spec.

Even if we restrict to BT.2020 only, we can choose between non-constant
luminance (simple) and constant luminance (complicated)
MatrixCoefficients.

Then we have BT.2100 ICtCp which uses BT.2020 color gamut but different
other things.


It's really tricky to come up with properties/enums that both fully
describe things and do not hit internal inconsistency.

One approach is to have single property enumerating all the practical
combinations of everything. The other approach is to have one property
for each independent aspect and accept that not all combinations are
practical (the CICP approach). Any kind of middle ground or combination
just falls apart.


Thanks,
pq

> > and then if you leave the colorspae property to "default"
> > the kernel will pick the "right" value based on the
> > output_encoding prop.
> >   
> 
> How would you fill in the AVI infoframe? Userspace would still
> need to set that to BT.2020 if the pixels are in BT.2020.
> 
> Harry
> 
> > That would leave all the weird stuff in the colorspace
> > property alone and thus would still allow someone to
> > do more than just the basic stuff explicitly.
> >   
>

Pekka Paalanen Feb. 8, 2023, 10:32 a.m. UTC | #27

On Mon, 6 Feb 2023 12:16:28 -0500
Harry Wentland <[e-mail]> wrote:

> On 2/6/23 04:47, Ville Syrjälä wrote:
> > On Sat, Feb 04, 2023 at 06:09:45AM +0000, Joshua Ashton wrote:  
> >>
> >>
> >> On 2/3/23 19:34, Ville Syrjälä wrote:  
> >>> On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote:  
> >>>> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote:  
> >>>>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote:  
> >>>>>>
> >>>>>>
> >>>>>> On 2/3/23 11:00, Ville Syrjälä wrote:  
> >>>>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:  
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:  
> >>>>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:  
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:  
> >>>>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>>>>>>>> <[e-mail]> wrote:  
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> >>>>>>>>>>>>> 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.  
> >>>>>>>>>>>>
> >>>>>>>>>>>> That applies to a lot of the other values as well (they are
> >>>>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>>>>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>>>>>>>> directly feed in correct kind of data).  
> >>>>>>>>>>>
> >>>>>>>>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>>>>>>>> then the kernel always knows which pixel encoding is selected and
> >>>>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>>>>>>>> want to control the variant explicitly to the wrong value?
> >>>>>>>>>>>  
> >>>>>>>>>>
> >>>>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>>>>>>>> upstream userspace project that makes use of this property (other than
> >>>>>>>>>> what Joshua is working on in gamescope right now)? That would help us
> >>>>>>>>>> understand the intent better.  
> >>>>>>>>>
> >>>>>>>>> The intent was to control the infoframe colorimetry bits,
> >>>>>>>>> nothing more. No idea what real userspace there was, if any.  
> >>
> >> Controlling the infoframe alone isn't useful at all unless you can 
> >> guarantee the wire encoding, which we cannot do.
> >>  
> >>>>>>>>>  
> >>>>>>>>>>
> >>>>>>>>>> I don't think giving userspace explicit control over the exact infoframe
> >>>>>>>>>> values is the right thing to do.  
> >>
> >> +1
> >>  
> >>>>>>>>>
> >>>>>>>>> Only userspace knows what kind of data it's stuffing into
> >>>>>>>>> the pixels (and/or how it configures the csc units/etc.) to
> >>>>>>>>> generate them.
> >>>>>>>>>  
> >>>>>>>>
> >>>>>>>> Yes, but userspace doesn't control or know whether we drive
> >>>>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
> >>>>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
> >>>>>>>> is currently no way for userspace to know that and I don't
> >>>>>>>> think it makes sense.  
> >>>>>>>
> >>>>>>> People want that control as well for whatever reason. We've
> >>>>>>> been asked to allow YCbCr 4:4:4 output many times.
> >>>>>>>
> >>>>>>> The automagic 4:2:0 fallback I think is rather fundementally
> >>>>>>> incompatible with fancy color management. How would we even
> >>>>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> >>>>>>> that stuff is just always BT.709 limited range, no questions
> >>>>>>> asked.  
> >>
> >> That's what the Colorspace property *should* be determining here.
> >> That's what we have it set up to do in SteamOS/my tree right now.
> >>  
> >>>>>>>  
> >>>>>>
> >>>>>> We use what we're telling the display, i.e., the value in the
> >>>>>> colorspace property. That way we know whether to use a BT.2020
> >>>>>> or BT.709 matrix.  
> >>>>>
> >>>>> And given how these things have gone in the past I think
> >>>>> that is likey to bite someone at in the future. Also not
> >>>>> what this property was meant to do nor does on any other
> >>>>> driver AFAIK.
> >>>>>  
> >>>>>> I don't see how it's fundamentally incompatible with fancy
> >>>>>> color management stuff.
> >>>>>>
> >>>>>> If we start forbidding drivers from falling back to YCbCr
> >>>>>> (whether 4:4:4 or 4:2:0) we will break existing behavior on
> >>>>>> amdgpu and will see bug reports.  
> >>>>>
> >>>>> The compositors could deal with that if/when they start doing
> >>>>> the full color management stuff. The current stuff only really
> >>>>> works when the kernel is allowed to do whatever it wants.
> >>>>>  
> >>>>>>  
> >>>>>>> So I think if userspace wants real color management it's
> >>>>>>> going to have to set up the whole pipeline. And for that
> >>>>>>> we need at least one new property to control the RGB->YCbCr
> >>>>>>> conversion (or to explicitly avoid it).  
> >>
> >> I mentioned this in my commit description, we absolutely should offer 
> >> fine control here eventually.
> >>
> >> I don't think we need to solve that problem here though.
> >>  
> >>>>>>>
> >>>>>>> And given that the proposed patch just swept all the
> >>>>>>> non-BT.2020 issues under the rug makes me think no
> >>>>>>> one has actually come up with any kind of consistent
> >>>>>>> plan for anything else really.
> >>>>>>>  
> >>>>>>
> >>>>>> Does anyone actually use the non-BT.2020 colorspace stuff?  
> >>>>>
> >>>>> No idea if anyone is using any of it. It's a bit hard to do
> >>>>> right now outside the full passthrough case since we have no
> >>>>> properties to control how the hardware will convert stuff.  
> >>
> >> No, every userspace knows that encoding of the output buffer before 
> >> going to the wire format is RGB.
> >>
> >> It's the only way you can have planes alpha-blend, or mix and match RGB 
> >> and NV12, etc.
> >>  
> >>>>>
> >>>>> Anyways, sounds like what you're basically proposing is
> >>>>> getting rid of this property and starting from scratch.  
> >>>>
> >>>> Hmm. I guess one option would be to add that property to
> >>>> control the output encoding, but include a few extra
> >>>> "automagic" values to it which would retain the kernel's
> >>>> freedom to select whether to do the RGB->YCbCr conversion
> >>>> or not.
> >>>>
> >>>> enum output_encoding {
> >>>> 	auto rgb=default/nodata,ycbcr=bt601
> >>>> 	auto rgb=default/nodata,ycbcr=bt709
> >>>> 	auto rgb=bt2020,ycbcr=bt2020
> >>>> 	passthrough,
> >>>> 	rgb->ycbcr bt601,
> >>>> 	rgb->ycbcr bt709,
> >>>> 	rgb->ycbcr bt2020,
> >>>> }  
> >>>
> >>> In fact there should perhaps be a lot more of the explicit
> >>> options to get all subsamlings and quantizations ranges
> >>> coverted. That might actually be really nice for an igt
> >>> to get more full test coverage.
> >>>  
> >> The choice of encoding of the pixel on the wire should be unrelated to 
> >> the overall output colorspace from the userspace side -- but how the 
> >> display engine converts the output to that wire format *is* dependent on 
> >> the colorspace.
> >> eg. picking a rec.709 ctc vs a rec.2020 ctc matrix.
> >>
> >> I see you are proposing a "passthrough" but that wouldn't work at all as 
> >> you still need to at know if you are RGB or YCbCr for the infoframe and 
> >> to perform chroma subsampling in the display engine.  
> > 
> > The passthrough (and other knobs after it) were meant for 
> > explicit control, which means they wouldn't affect infoframes.
> > 
> > But probably we should have seprate properties for explicit
> > control of each knob vs. some kind of easier to use property.
> > And I suppose we can still leave the explicit control stuff
> > for later (apart from the one property we already have).
> >   
> >>
> >> I perused the initial patches that added this property, and it seems 
> >> there were no IGT tests or userspace implementation, so I am not 
> >> entirely sure why it was committed in the first place.  
> > 
> > I presume at least the kodi HDR stuff uses ths. There may
> > have also been some chromeos stuff going on. Can't recall
> > anymore.  
> 
> I can't find mention of "colorspace" in kodi. Its SetHDR() [1]
> function only sets HDR_OUTPUT_METADATA.
> 
> [1] https://github.com/xbmc/xbmc/blob/122916890a2b82ad8defaf2fd1934076387df84d/xbmc/windowing/gbm/WinSystemGbm.cpp#L316
> 
> > 
> > As for IGT, there's nothing we can really test since we 
> > have no way to get the inforframes/etc. back from the sink.
> > Hence nothing beyond the normal kms_property sanity checks
> > really makes sense.
> >   
> 
> True, though a generic infoframe readback through debugfs might
> be a nice-to-have for testing things that are expected to modify
> the infoframe.
> 
> >>
> >> Nobody can safely use Colorspace because of this problem right now.
> >>
> >> If nobody is using this property, perhaps we could just get a fresh 
> >> start, and either re-purpose it with new enum values, or obsolete it and 
> >> make a new property.
> >> If we do this, let's start with the absolute bare minimum, such as:
> >> "Default/Rec.709 (sRGB), BT.2020"
> >> and then grow as we need, making sure we have the full circle from 
> >> userspace->output complete and working for each new value we add.  
> >   
> 
> I agree. This leaves room for API that can be extensible but let's
> not define things unless they're actually used in a full-stack
> implementation.
> 
> > Yeah, I think a fresh property is what we want.
> >   
> 
> Agreed. And if this new property is also updating the infoframe it needs
> to be clear it's mutually exclusive with the existing property.
> 
> Harry
> 
> >>
> >> Please don't take this as me saying we shouldn't add all these other 
> >> options like opRGB, etc, I just want us to progress to a solid base for 
> >> expanding further here, which we really don't have right now.
> >>
> >> - Joshie 

Ville Syrjala Feb. 8, 2023, 2:49 p.m. UTC | #28

On Wed, Feb 08, 2023 at 11:18:42AM +0200, Pekka Paalanen wrote:
> On Fri, 3 Feb 2023 16:02:51 +0200
> Ville Syrjälä <[e-mail]> wrote:
> 
> > On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote:
> > > On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä
> > > <[e-mail]> wrote:  
> > > >
> > > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:  
> > > > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > > > <[e-mail]> wrote:  
> > > > > >
> > > > > > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> > > > > > > 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.  
> > > > > >
> > > > > > That applies to a lot of the other values as well (they are
> > > > > > explicitly RGB or YCC). The idea was that this property sets the
> > > > > > infoframe/MSA/SDP value exactly, and other properties should be
> > > > > > added to for use userspace to control the pixel encoding/colorspace
> > > > > > conversion(if desired, or userspace just makes sure to
> > > > > > directly feed in correct kind of data).  
> > > > >
> > > > > I'm all for getting userspace control over pixel encoding but even
> > > > > then the kernel always knows which pixel encoding is selected and
> > > > > which InfoFrame has to be sent. Is there a reason why userspace would
> > > > > want to control the variant explicitly to the wrong value?  
> > > >
> > > > What do you mean wrong value? Userspace sets it based on what
> > > > kind of data it has generated (or asked the display hardware
> > > > to generate if/when we get explicit control over that part).  
> > > 
> > > Wrong in the sense of sending the YCC variant when the pixel encoding
> > > is RGB for example.
> > > 
> > > Maybe I'm missing something here but my assumption is that the kernel
> > > always has to know the pixel encoding anyway. The color pipeline also
> > > assumes that the pixel values are RGB. User space might be able to
> > > generate YCC content but for subsampling etc the pixel encoding still
> > > has to be explicitly set.  
> > 
> > The kernel doesn't really know much atm. In theory you can just
> > configure the thing to do a straight passthough and put anything you
> > want into your pixels.
> 
> But it's impossible to use a YCbCr framebuffer and have that *not*
> converted to RGB for the KMS color pipeline even if userspace wanted it
> to be strictly pass-through, only to be converted again to YCbCr for
> the cable, is it not?
> 
> Even more so with 4:2:0.
> 
> How could it be possible to stop the driver from doing those two
> YUV-to-RGB and RGB-to-YCbCr conversions at the beginning and at the end
> of the KMS color pipeline?

You can stop the conversion at the start of the pipeline by
using a "RGB" framebuffer. At the end of the pipe it's not
possible with the current props.

Pekka Paalanen Feb. 9, 2023, 10:05 a.m. UTC | #29

On Wed, 8 Feb 2023 16:49:31 +0200
Ville Syrjälä <[e-mail]> wrote:

> On Wed, Feb 08, 2023 at 11:18:42AM +0200, Pekka Paalanen wrote:
> > On Fri, 3 Feb 2023 16:02:51 +0200
> > Ville Syrjälä <[e-mail]> wrote:
> >   
> > > On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote:  
> > > > On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä
> > > > <[e-mail]> wrote:    
> > > > >
> > > > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote:    
> > > > > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > > > > <[e-mail]> wrote:    
> > > > > > >
> > > > > > > On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:    
> > > > > > > > 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.    
> > > > > > >
> > > > > > > That applies to a lot of the other values as well (they are
> > > > > > > explicitly RGB or YCC). The idea was that this property sets the
> > > > > > > infoframe/MSA/SDP value exactly, and other properties should be
> > > > > > > added to for use userspace to control the pixel encoding/colorspace
> > > > > > > conversion(if desired, or userspace just makes sure to
> > > > > > > directly feed in correct kind of data).    
> > > > > >
> > > > > > I'm all for getting userspace control over pixel encoding but even
> > > > > > then the kernel always knows which pixel encoding is selected and
> > > > > > which InfoFrame has to be sent. Is there a reason why userspace would
> > > > > > want to control the variant explicitly to the wrong value?    
> > > > >
> > > > > What do you mean wrong value? Userspace sets it based on what
> > > > > kind of data it has generated (or asked the display hardware
> > > > > to generate if/when we get explicit control over that part).    
> > > > 
> > > > Wrong in the sense of sending the YCC variant when the pixel encoding
> > > > is RGB for example.
> > > > 
> > > > Maybe I'm missing something here but my assumption is that the kernel
> > > > always has to know the pixel encoding anyway. The color pipeline also
> > > > assumes that the pixel values are RGB. User space might be able to
> > > > generate YCC content but for subsampling etc the pixel encoding still
> > > > has to be explicitly set.    
> > > 
> > > The kernel doesn't really know much atm. In theory you can just
> > > configure the thing to do a straight passthough and put anything you
> > > want into your pixels.  
> > 
> > But it's impossible to use a YCbCr framebuffer and have that *not*
> > converted to RGB for the KMS color pipeline even if userspace wanted it
> > to be strictly pass-through, only to be converted again to YCbCr for
> > the cable, is it not?
> > 
> > Even more so with 4:2:0.
> > 
> > How could it be possible to stop the driver from doing those two
> > YUV-to-RGB and RGB-to-YCbCr conversions at the beginning and at the end
> > of the KMS color pipeline?  
> 
> You can stop the conversion at the start of the pipeline by
> using a "RGB" framebuffer. At the end of the pipe it's not
> possible with the current props.

But there is no such thing as a 4:2:0 sub-sampled RGB framebuffer to be
abused for YUV content. It would be possible for some kind of xYUV
4:4:4 content though, but then the pipeline wouldn't work.

Joshua had the excellent point that disabling the conversion at the end
of the pipeline is not possible for a non-RGB output signal, period.
The KMS color pipeline is defined in terms of RGB channels, that's the
only(?) way alpha-blending could work, and the LUT-like elements cannot
handle negative values.

On one hand I very much agree that the definition of "Broadcast RGB"
property was a mistake by combining pixel operations with infoframe
settings. OTOH, since the pipeline end conversion is today chosen by
the driver, then the KMS color pipeline output must be known to the
driver so that the driver can pick the right conversion. That's what
"Broadcast RGB" did: it assumed the pipeline produces full range
values, so that it is able to insert the right conversion and the right
infoframe data. It rules out possible use cases, but the infoframe
matches.

As for the pipe-end RGB-to-YCbCr conversion, the situation is partly
similar. There is the assumption that the pipeline produces RGB model
values. However, this assumption is likely never going to change,
because the pipeline is inherently RGB, always.

A better question is, does it need other assumptions as well?

Quantization range?

RGB (electrical encoding) transfer function?

Most RGB-to-YCbCr conversions are just a matrix applied to the
electrical RGB values, but not all. Particularly the constant luminance
encoding requires optical, not electrical, RGB values, and it also
needs the transfer function since it emits electrical values. I haven't
looked if e.g. BT.2100 has more cases making the RGB-to-something
conversion complex.

Even having a doubt about that really does point towards userspace
needing to configure the pipe-end conversion by mathematical formula,
not by video standard colorspace. A consequence of that is that the
infoframe settings need to be an independent property separate from the
end-conversion property.

If so, it is not even possible for a driver to automatically set the
pipe-end conversion without telling it a lot more about what the KMS
color pipeline RGB output is.

I have been advocating that we should not tell the kernel about color
spaces, and instead we need to program the mathematical operations in
the color pipeline. Following that reasoning, we cannot make it
possible for drivers to choose the pipe-end conversion automatically.

Hmm...



Thanks,
pq

Joshua Ashton Feb. 9, 2023, 4:38 p.m. UTC | #30

On 2/8/23 09:30, Pekka Paalanen wrote:
> On Fri,  3 Feb 2023 02:07:44 +0000
> Joshua Ashton <[e-mail]> wrote:
> 
>> 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.
>>
>> Note: _CYCC was effectively 'removed' by this change, but that was not
>> possible to be taken advantage of anyway, as there is currently no
>> pixel_encoding control so it would not be possible to output
>> linear YCbCr.
>>
>> Signed-off-by: Joshua Ashton <[e-mail]>
>>
>> Cc: Pekka Paalanen <[e-mail]>
>> Cc: Sebastian Wick <[e-mail]>
>> Cc: [e-mail]
>> Cc: Uma Shankar <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Joshua Ashton <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>   drivers/gpu/drm/display/drm_hdmi_helper.c |  9 ++++-----
>>   drivers/gpu/drm/drm_connector.c           | 12 ++++++------
>>   drivers/gpu/drm/i915/display/intel_dp.c   | 20 +++++++++-----------
>>   include/drm/drm_connector.h               | 19 ++++++++++---------
>>   4 files changed, 29 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> index 0264abe55278..c85860600395 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> @@ -99,8 +99,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))
>>   
>> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
>>   	[DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
>>   	[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_1] = HDMI_COLORIMETRY_BT2020,
>> +	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>>   	/* Colorimetry based on IEC 61966-2-5 */
>>   	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>>   	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>>   	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>>   	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
>>   	/* 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 */
>> @@ -1066,9 +1066,9 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>   	/* Colorimetry based on IEC 61966-2-5 [33] */
>>   	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>>   	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
>>   	/* Colorimetry based on ITU-R BT.2020 */
>> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> +	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
>>   };
> 
> Hi,
> 
> do these not rename the old uAPI strings?
> 
> Shouldn't the old strings be kept? It's much easier to scream "kernel
> regression" when the expected string is no longer found than a subtle
> change in behaviour that might not even be a change. ;-)
> 
> If there is not going to be a difference in behaviour, the enum could
> expose e.g. all of "BT2020_RGB", "BT2020_CYCC" and "BT2020_YCC" as the
> same integer value. If old userspace exists, it would not notice any
> difference.
> 
> I mean, the *strings* are the uAPI, not the integers, right?

Both are uAPI these days.

I was wrong when I did this commit either way.

- Joshie 

Simon Ser Feb. 9, 2023, 5:03 p.m. UTC | #31

On Thursday, February 9th, 2023 at 17:38, Joshua Ashton <[e-mail]> wrote:

> > I mean, the strings are the uAPI, not the integers, right?
>
> Both are uAPI these days.

Yes. The integers are uAPI, if you change them you'll break libliftoff
users. There is an old thread discussing this somewhere. The tl;dr was
that there is no use-case for exposing the same string with a different
integer, so no good reason to justify the substantial complexity of
handling this case.

Ville Syrjala Feb. 9, 2023, 6:08 p.m. UTC | #32

On Thu, Feb 09, 2023 at 05:03:19PM +0000, Simon Ser wrote:
> On Thursday, February 9th, 2023 at 17:38, Joshua Ashton <[e-mail]> wrote:
> 
> > > I mean, the strings are the uAPI, not the integers, right?
> >
> > Both are uAPI these days.
> 
> Yes. The integers are uAPI, if you change them you'll break libliftoff
> users. There is an old thread discussing this somewhere. The tl;dr was
> that there is no use-case for exposing the same string with a different
> integer, so no good reason to justify the substantial complexity of
> handling this case.

If people actually depend on that we should probably have tests to
make sure no one breaks it by accident.

Pekka Paalanen Feb. 10, 2023, 9:37 a.m. UTC | #33

On Thu, 09 Feb 2023 17:03:19 +0000
Simon Ser <[e-mail]> wrote:

> On Thursday, February 9th, 2023 at 17:38, Joshua Ashton <[e-mail]> wrote:
> 
> > > I mean, the strings are the uAPI, not the integers, right?  
> >
> > Both are uAPI these days.  
> 
> Yes. The integers are uAPI, if you change them you'll break libliftoff
> users. There is an old thread discussing this somewhere. The tl;dr was
> that there is no use-case for exposing the same string with a different
> integer, so no good reason to justify the substantial complexity of
> handling this case.

Funny, I remember exactly the opposite.

This case would have been multiple different strings with the same
integer, anyway.

But no matter. If a uAPI header or documentation has exposed the
integers, then there is no taking that back.

This won't be a problem for enums that have no meaningful string names,
like enums where the integer names a blob that describes what the value
means, and enums where the integer is an index into an array of
descriptions exposed as a blob.


Thanks,
pq

Simon Ser Feb. 10, 2023, 9:44 a.m. UTC | #34

On Friday, February 10th, 2023 at 10:37, Pekka Paalanen <[e-mail]> wrote:

> On Thu, 09 Feb 2023 17:03:19 +0000
> Simon Ser [e-mail] wrote:
> 
> > On Thursday, February 9th, 2023 at 17:38, Joshua Ashton [e-mail] wrote:
> > 
> > > > I mean, the strings are the uAPI, not the integers, right?
> > > 
> > > Both are uAPI these days.
> > 
> > Yes. The integers are uAPI, if you change them you'll break libliftoff
> > users. There is an old thread discussing this somewhere. The tl;dr was
> > that there is no use-case for exposing the same string with a different
> > integer, so no good reason to justify the substantial complexity of
> > handling this case.
> 
> Funny, I remember exactly the opposite.

There was a bacxk-and-forth on this topic.

> This case would have been multiple different strings with the same
> integer, anyway.

That would be fine. As long as the meaning of an integer doesn't change
across planes, it's all fine.

> But no matter. If a uAPI header or documentation has exposed the
> integers, then there is no taking that back.

uAPI headers/docs don't expose the integers for this property.
Nevertheless, one cannot break user-space.

> This won't be a problem for enums that have no meaningful string names,
> like enums where the integer names a blob that describes what the value
> means, and enums where the integer is an index into an array of
> descriptions exposed as a blob.

This would be pretty tricky to handle.

Sebastian Wick Feb. 14, 2023, 3:49 p.m. UTC | #35

On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> >
> >
> > On 2/3/23 10:19, Ville Syrjälä wrote:
> > > On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > >>
> > >>
> > >> On 2/3/23 07:59, Sebastian Wick wrote:
> > >>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > >>> <[e-mail]> wrote:
> > >>>>
> > >>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > >>>>> 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.
> > >>>>
> > >>>> That applies to a lot of the other values as well (they are
> > >>>> explicitly RGB or YCC). The idea was that this property sets the
> > >>>> infoframe/MSA/SDP value exactly, and other properties should be
> > >>>> added to for use userspace to control the pixel encoding/colorspace
> > >>>> conversion(if desired, or userspace just makes sure to
> > >>>> directly feed in correct kind of data).
> > >>>
> > >>> I'm all for getting userspace control over pixel encoding but even
> > >>> then the kernel always knows which pixel encoding is selected and
> > >>> which InfoFrame has to be sent. Is there a reason why userspace would
> > >>> want to control the variant explicitly to the wrong value?
> > >>>
> > >>
> > >> I've asked this before but haven't seen an answer: Is there an existing
> > >> upstream userspace project that makes use of this property (other than
> > >> what Joshua is working on in gamescope right now)? That would help us
> > >> understand the intent better.
> > >
> > > The intent was to control the infoframe colorimetry bits,
> > > nothing more. No idea what real userspace there was, if any.
> > >
> > >>
> > >> I don't think giving userspace explicit control over the exact infoframe
> > >> values is the right thing to do.
> > >
> > > Only userspace knows what kind of data it's stuffing into
> > > the pixels (and/or how it configures the csc units/etc.) to
> > > generate them.
> > >
> >
> > Yes, but userspace doesn't control or know whether we drive
> > RGB or YCbCr on the wire. In fact, in some cases our driver
> > needs to fallback to YCbCr420 for bandwidth reasons. There
> > is currently no way for userspace to know that and I don't
> > think it makes sense.
>
> People want that control as well for whatever reason. We've
> been asked to allow YCbCr 4:4:4 output many times.

I don't really think it's a question of if we want it but rather how
we get there. Harry is completely right that if we would make the
subsampling controllable by user space instead of the kernel handling
it magically, user space which does not adapt to the new control won't
be able to light up some modes which worked before.

This is obviously a problem and not one we can easily fix. We would
need a new cap for user space to signal "I know that I can control
bpc, subsampling and compression to lower the bandwidth and light up
modes which otherwise fail". That cap would also remove all the
properties which require kernel magic to work (that's also what I
proposed for my KMS color pipeline API).

We all want to expose more of the scanout capability and give user
space more control but I don't think an incremental approach works
here and we would all do better if we accept that the current API
requires kernel magic to work and has a few implicit assumptions baked
in.

With all that being said, I think the right decision here is to

1. Ignore subsampling for now
2. Let the kernel select YCC or RGB on the cable
3. Let the kernel figure out the conversion between RGB and YCC based
on the color space selected
4. Let the kernel send the correct infoframe based on the selected
color space and cable encoding
5. Only expose color spaces for which the kernel can do the conversion
and send the infoframe
6. Work on the new API which is hidden behind a cap

> The automagic 4:2:0 fallback I think is rather fundementally
> incompatible with fancy color management. How would we even
> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> that stuff is just always BT.709 limited range, no questions
> asked.
>
> So I think if userspace wants real color management it's
> going to have to set up the whole pipeline. And for that
> we need at least one new property to control the RGB->YCbCr
> conversion (or to explicitly avoid it).
>
> And given that the proposed patch just swept all the
> non-BT.2020 issues under the rug makes me think no
> one has actually come up with any kind of consistent
> plan for anything else really.
>
> >
> > Userspace needs full control of framebuffer pixel formats,
> > as well as control over DEGAMMA, GAMMA, CTM color operations.
> > It also needs to be able to select whether to drive the panel
> > as sRGB or BT.2020/PQ but it doesn't make sense for it to
> > control the pixel encoding on the wire (RGB vs YCbCr).
> >
> > > I really don't want a repeat of the disaster of the
> > > 'Broadcast RGB' which has coupled together the infoframe
> > > and automagic conversion stuff. And I think this one would
> > > be about 100x worse given this property has something
> > > to do with actual colorspaces as well.
> > >
> >
> > I'm unaware of this disaster. Could you elaborate?
>
> The property now controls both the infoframe stuff (and
> whatever super vague stuff DP has for it in MSA) and
> full->limited range compression in the display pipeline.
> And as a result  there is no way to eg. allow already
> limited range input, which is what some people wanted.
>
> And naturally it's all made a lot more terrible by all
> the displays that fail to implement the spec correctly,
> but that's another topic.
>
> --
> Ville Syrjälä
> Intel
>

Harry Wentland Feb. 14, 2023, 4:56 p.m. UTC | #36

On 2/14/23 10:49, Sebastian Wick wrote:
> On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> <[e-mail]> wrote:
>>
>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>
>>>
>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>
>>>>>
>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>> directly feed in correct kind of data).
>>>>>>
>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>
>>>>>
>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>> upstream userspace project that makes use of this property (other than
>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>> understand the intent better.
>>>>
>>>> The intent was to control the infoframe colorimetry bits,
>>>> nothing more. No idea what real userspace there was, if any.
>>>>
>>>>>
>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>> values is the right thing to do.
>>>>
>>>> Only userspace knows what kind of data it's stuffing into
>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>> generate them.
>>>>
>>>
>>> Yes, but userspace doesn't control or know whether we drive
>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>> is currently no way for userspace to know that and I don't
>>> think it makes sense.
>>
>> People want that control as well for whatever reason. We've
>> been asked to allow YCbCr 4:4:4 output many times.
> 
> I don't really think it's a question of if we want it but rather how
> we get there. Harry is completely right that if we would make the
> subsampling controllable by user space instead of the kernel handling
> it magically, user space which does not adapt to the new control won't
> be able to light up some modes which worked before.
> 

Thanks for continuing this discussion and touching on the model of how
we get to where we want to go.

> This is obviously a problem and not one we can easily fix. We would
> need a new cap for user space to signal "I know that I can control
> bpc, subsampling and compression to lower the bandwidth and light up
> modes which otherwise fail". That cap would also remove all the
> properties which require kernel magic to work (that's also what I
> proposed for my KMS color pipeline API).
> 
> We all want to expose more of the scanout capability and give user
> space more control but I don't think an incremental approach works
> here and we would all do better if we accept that the current API
> requires kernel magic to work and has a few implicit assumptions baked
> in.
> 
> With all that being said, I think the right decision here is to
> 
> 1. Ignore subsampling for now
> 2. Let the kernel select YCC or RGB on the cable
> 3. Let the kernel figure out the conversion between RGB and YCC based
> on the color space selected
> 4. Let the kernel send the correct infoframe based on the selected
> color space and cable encoding
> 5. Only expose color spaces for which the kernel can do the conversion
> and send the infoframe

I agree. We don't want to break or change existing behavior (that is
used by userspace) and this will get us far without breaking things.

> 6. Work on the new API which is hidden behind a cap
> 

I assume you mean something like
https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11

Above you say that you don't think an incremental approach works
here. Can you elaborate?

From what I've seen recently I am inclined to favor an incremental
approach more. The reason is that any API, or portion thereof, is
useless unless it's enabled full stack. When it isn't it becomes
dead code quickly, or never really works because we overlooked
one thing. The colorspace debacle shows how even something as
simple as extra enum values in KMS APIs shouldn't be added unless
someone in a canonical upstream project actually uses them. I
would argue that such a canonical upstream project actually has
to be a production environment and not something like Weston.

I could see us getting to a fully new color pipeline API but
the only way to do that is with a development model that supports
it. While upstream needs to be our ultimate goal, a good way
to bring in new APIs and ensure a full-stack implementation is
to develop them in a downstream production kernel, alongside
userspace that makes use of it. Once the implementation is
proven in the downstream repos it can then go upstream. This
brings new challenges, though, as things don't get wide
testing and get out of sync with upstream quickly. The
alternative is the incremental approach.

We should look at this from a use-case angle, similar to what
the gamescope guys are doing. Small steps, like:
1) Add HDR10 output (PQ, BT.2020) to the display
2) Add ability to do sRGB linear blending
3) Add ability to do sRGB and PQ linear blending
4) Post-blending 3D LUT
5) Pre-blending 3D LUT

At each stage the whole stack needs to work together in production.

If we go to a new color pipeline programming model it might
make sense to enable this as an "experimental" API that is
under development. I don't know if we've ever done that in
DRM/KMS. One way to do this might be with a new CONFIG option
that only exposes the new color pipeline API when enabled and
defaults to off, alongside a client cap for clients that
are advertising a desire to use the (experimental) API.

If we have that we could then look at porting all existing
use-cases over and verifying them (including IGT tests) before
moving on to HDR and wide-gamut use-cases. It's a large
undertaking and while I'm not opposed to it I don't know
if there are enough people willing to invest a large amount
of effort to make this happen.

Harry

>> The automagic 4:2:0 fallback I think is rather fundementally
>> incompatible with fancy color management. How would we even
>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>> that stuff is just always BT.709 limited range, no questions
>> asked.
>>
>> So I think if userspace wants real color management it's
>> going to have to set up the whole pipeline. And for that
>> we need at least one new property to control the RGB->YCbCr
>> conversion (or to explicitly avoid it).
>>
>> And given that the proposed patch just swept all the
>> non-BT.2020 issues under the rug makes me think no
>> one has actually come up with any kind of consistent
>> plan for anything else really.
>>
>>>
>>> Userspace needs full control of framebuffer pixel formats,
>>> as well as control over DEGAMMA, GAMMA, CTM color operations.
>>> It also needs to be able to select whether to drive the panel
>>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
>>> control the pixel encoding on the wire (RGB vs YCbCr).
>>>
>>>> I really don't want a repeat of the disaster of the
>>>> 'Broadcast RGB' which has coupled together the infoframe
>>>> and automagic conversion stuff. And I think this one would
>>>> be about 100x worse given this property has something
>>>> to do with actual colorspaces as well.
>>>>
>>>
>>> I'm unaware of this disaster. Could you elaborate?
>>
>> The property now controls both the infoframe stuff (and
>> whatever super vague stuff DP has for it in MSA) and
>> full->limited range compression in the display pipeline.
>> And as a result  there is no way to eg. allow already
>> limited range input, which is what some people wanted.
>>
>> And naturally it's all made a lot more terrible by all
>> the displays that fail to implement the spec correctly,
>> but that's another topic.
>>
>> --
>> Ville Syrjälä
>> Intel
>>
>

Sebastian Wick Feb. 14, 2023, 7:45 p.m. UTC | #37

On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:
>
>
>
> On 2/14/23 10:49, Sebastian Wick wrote:
> > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> > <[e-mail]> wrote:
> >>
> >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> >>>
> >>>
> >>> On 2/3/23 10:19, Ville Syrjälä wrote:
> >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> >>>>>
> >>>>>
> >>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>>> <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> That applies to a lot of the other values as well (they are
> >>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>>> directly feed in correct kind of data).
> >>>>>>
> >>>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>>> then the kernel always knows which pixel encoding is selected and
> >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>>> want to control the variant explicitly to the wrong value?
> >>>>>>
> >>>>>
> >>>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>>> upstream userspace project that makes use of this property (other than
> >>>>> what Joshua is working on in gamescope right now)? That would help us
> >>>>> understand the intent better.
> >>>>
> >>>> The intent was to control the infoframe colorimetry bits,
> >>>> nothing more. No idea what real userspace there was, if any.
> >>>>
> >>>>>
> >>>>> I don't think giving userspace explicit control over the exact infoframe
> >>>>> values is the right thing to do.
> >>>>
> >>>> Only userspace knows what kind of data it's stuffing into
> >>>> the pixels (and/or how it configures the csc units/etc.) to
> >>>> generate them.
> >>>>
> >>>
> >>> Yes, but userspace doesn't control or know whether we drive
> >>> RGB or YCbCr on the wire. In fact, in some cases our driver
> >>> needs to fallback to YCbCr420 for bandwidth reasons. There
> >>> is currently no way for userspace to know that and I don't
> >>> think it makes sense.
> >>
> >> People want that control as well for whatever reason. We've
> >> been asked to allow YCbCr 4:4:4 output many times.
> >
> > I don't really think it's a question of if we want it but rather how
> > we get there. Harry is completely right that if we would make the
> > subsampling controllable by user space instead of the kernel handling
> > it magically, user space which does not adapt to the new control won't
> > be able to light up some modes which worked before.
> >
>
> Thanks for continuing this discussion and touching on the model of how
> we get to where we want to go.
>
> > This is obviously a problem and not one we can easily fix. We would
> > need a new cap for user space to signal "I know that I can control
> > bpc, subsampling and compression to lower the bandwidth and light up
> > modes which otherwise fail". That cap would also remove all the
> > properties which require kernel magic to work (that's also what I
> > proposed for my KMS color pipeline API).
> >
> > We all want to expose more of the scanout capability and give user
> > space more control but I don't think an incremental approach works
> > here and we would all do better if we accept that the current API
> > requires kernel magic to work and has a few implicit assumptions baked
> > in.
> >
> > With all that being said, I think the right decision here is to
> >
> > 1. Ignore subsampling for now
> > 2. Let the kernel select YCC or RGB on the cable
> > 3. Let the kernel figure out the conversion between RGB and YCC based
> > on the color space selected
> > 4. Let the kernel send the correct infoframe based on the selected
> > color space and cable encoding
> > 5. Only expose color spaces for which the kernel can do the conversion
> > and send the infoframe
>
> I agree. We don't want to break or change existing behavior (that is
> used by userspace) and this will get us far without breaking things.
>
> > 6. Work on the new API which is hidden behind a cap
> >
>
> I assume you mean something like
> https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11

Something like that, yes. The main point being a cap which removes a
lot of properties and sets new expectations between user space and
kernel. The actual API is not that important.

> Above you say that you don't think an incremental approach works
> here. Can you elaborate?

Backwards compatibility is really hard. If we add another property to
control e.g. the color range infoframe which doesn't magically convert
colors, we now have to define how it interacts with the existing
property. We also have to figure out how a user space which doesn't
know about the new property behaves when another client has set that
property. If any property which currently might change the pixel
values is used, we can't expose the entire color pipeline because the
kernel might have to use some element in it to achieve its magic
conversion. So essentially you already have this hard device between
"old" and "new" and you can't use the new stuff incrementally.

> From what I've seen recently I am inclined to favor an incremental
> approach more. The reason is that any API, or portion thereof, is
> useless unless it's enabled full stack. When it isn't it becomes
> dead code quickly, or never really works because we overlooked
> one thing. The colorspace debacle shows how even something as
> simple as extra enum values in KMS APIs shouldn't be added unless
> someone in a canonical upstream project actually uses them. I
> would argue that such a canonical upstream project actually has
> to be a production environment and not something like Weston.

I agree that it's very easy to design something that doesn't work in
the real world but weston is a real production environment. Even a new
project can be a real production environment imo. The goals here are
not entirely undefined: we have color transformations and we want to
offload them.

> I could see us getting to a fully new color pipeline API but
> the only way to do that is with a development model that supports
> it. While upstream needs to be our ultimate goal, a good way
> to bring in new APIs and ensure a full-stack implementation is
> to develop them in a downstream production kernel, alongside
> userspace that makes use of it. Once the implementation is
> proven in the downstream repos it can then go upstream. This
> brings new challenges, though, as things don't get wide
> testing and get out of sync with upstream quickly. The
> alternative is the incremental approach.

I also agree pretty much with everything here. My current idea is that
we would add support for the new API in a downstream kernel for at
least VKMS (one real driver probably makes sense as well) while in
parallel developing a user space library for color conversions. That
library might be a rewrite of libplacebo, which in its current form
does all the color conversions we want to do but wasn't designed to
allow for offloading. One of the developers expressed interest in
rewriting the library in rust which would be a good opportunity to
also take offloading into account.

No doubt all of that will take a significant amount of effort and time
but we can still get HDR working in the old model without offloading
and just sRGB and PQ/Rec2020 code paths.

> We should look at this from a use-case angle, similar to what
> the gamescope guys are doing. Small steps, like:
> 1) Add HDR10 output (PQ, BT.2020) to the display
> 2) Add ability to do sRGB linear blending
> 3) Add ability to do sRGB and PQ linear blending
> 4) Post-blending 3D LUT
> 5) Pre-blending 3D LUT

Sure, having a target in sight is a good idea.

> At each stage the whole stack needs to work together in production.
>
> If we go to a new color pipeline programming model it might
> make sense to enable this as an "experimental" API that is
> under development. I don't know if we've ever done that in
> DRM/KMS. One way to do this might be with a new CONFIG option
> that only exposes the new color pipeline API when enabled and
> defaults to off, alongside a client cap for clients that
> are advertising a desire to use the (experimental) API.

Yeah, that's a bit tricky. I also don't know how upstream would like
this approach. Not even sure who to talk to.

> If we have that we could then look at porting all existing
> use-cases over and verifying them (including IGT tests) before
> moving on to HDR and wide-gamut use-cases. It's a large
> undertaking and while I'm not opposed to it I don't know
> if there are enough people willing to invest a large amount
> of effort to make this happen.
>
> Harry
>
> >> The automagic 4:2:0 fallback I think is rather fundementally
> >> incompatible with fancy color management. How would we even
> >> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> >> that stuff is just always BT.709 limited range, no questions
> >> asked.
> >>
> >> So I think if userspace wants real color management it's
> >> going to have to set up the whole pipeline. And for that
> >> we need at least one new property to control the RGB->YCbCr
> >> conversion (or to explicitly avoid it).
> >>
> >> And given that the proposed patch just swept all the
> >> non-BT.2020 issues under the rug makes me think no
> >> one has actually come up with any kind of consistent
> >> plan for anything else really.
> >>
> >>>
> >>> Userspace needs full control of framebuffer pixel formats,
> >>> as well as control over DEGAMMA, GAMMA, CTM color operations.
> >>> It also needs to be able to select whether to drive the panel
> >>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
> >>> control the pixel encoding on the wire (RGB vs YCbCr).
> >>>
> >>>> I really don't want a repeat of the disaster of the
> >>>> 'Broadcast RGB' which has coupled together the infoframe
> >>>> and automagic conversion stuff. And I think this one would
> >>>> be about 100x worse given this property has something
> >>>> to do with actual colorspaces as well.
> >>>>
> >>>
> >>> I'm unaware of this disaster. Could you elaborate?
> >>
> >> The property now controls both the infoframe stuff (and
> >> whatever super vague stuff DP has for it in MSA) and
> >> full->limited range compression in the display pipeline.
> >> And as a result  there is no way to eg. allow already
> >> limited range input, which is what some people wanted.
> >>
> >> And naturally it's all made a lot more terrible by all
> >> the displays that fail to implement the spec correctly,
> >> but that's another topic.
> >>
> >> --
> >> Ville Syrjälä
> >> Intel
> >>
> >
>

Harry Wentland Feb. 14, 2023, 8:04 p.m. UTC | #38

On 2/14/23 14:45, Sebastian Wick wrote:
> On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:
>>
>>
>>
>> On 2/14/23 10:49, Sebastian Wick wrote:
>>> On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
>>> <[e-mail]> wrote:
>>>>
>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
>>>>>
>>>>>
>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:
>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:
>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>> directly feed in correct kind of data).
>>>>>>>>
>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>
>>>>>>>
>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>> understand the intent better.
>>>>>>
>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>> nothing more. No idea what real userspace there was, if any.
>>>>>>
>>>>>>>
>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>> values is the right thing to do.
>>>>>>
>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>> generate them.
>>>>>>
>>>>>
>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>> is currently no way for userspace to know that and I don't
>>>>> think it makes sense.
>>>>
>>>> People want that control as well for whatever reason. We've
>>>> been asked to allow YCbCr 4:4:4 output many times.
>>>
>>> I don't really think it's a question of if we want it but rather how
>>> we get there. Harry is completely right that if we would make the
>>> subsampling controllable by user space instead of the kernel handling
>>> it magically, user space which does not adapt to the new control won't
>>> be able to light up some modes which worked before.
>>>
>>
>> Thanks for continuing this discussion and touching on the model of how
>> we get to where we want to go.
>>
>>> This is obviously a problem and not one we can easily fix. We would
>>> need a new cap for user space to signal "I know that I can control
>>> bpc, subsampling and compression to lower the bandwidth and light up
>>> modes which otherwise fail". That cap would also remove all the
>>> properties which require kernel magic to work (that's also what I
>>> proposed for my KMS color pipeline API).
>>>
>>> We all want to expose more of the scanout capability and give user
>>> space more control but I don't think an incremental approach works
>>> here and we would all do better if we accept that the current API
>>> requires kernel magic to work and has a few implicit assumptions baked
>>> in.
>>>
>>> With all that being said, I think the right decision here is to
>>>
>>> 1. Ignore subsampling for now
>>> 2. Let the kernel select YCC or RGB on the cable
>>> 3. Let the kernel figure out the conversion between RGB and YCC based
>>> on the color space selected
>>> 4. Let the kernel send the correct infoframe based on the selected
>>> color space and cable encoding
>>> 5. Only expose color spaces for which the kernel can do the conversion
>>> and send the infoframe
>>
>> I agree. We don't want to break or change existing behavior (that is
>> used by userspace) and this will get us far without breaking things.
>>
>>> 6. Work on the new API which is hidden behind a cap
>>>
>>
>> I assume you mean something like
>> https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> 
> Something like that, yes. The main point being a cap which removes a
> lot of properties and sets new expectations between user space and
> kernel. The actual API is not that important.
> 
>> Above you say that you don't think an incremental approach works
>> here. Can you elaborate?
> 
> Backwards compatibility is really hard. If we add another property to
> control e.g. the color range infoframe which doesn't magically convert
> colors, we now have to define how it interacts with the existing
> property. We also have to figure out how a user space which doesn't
> know about the new property behaves when another client has set that
> property. If any property which currently might change the pixel
> values is used, we can't expose the entire color pipeline because the
> kernel might have to use some element in it to achieve its magic
> conversion. So essentially you already have this hard device between
> "old" and "new" and you can't use the new stuff incrementally.
> 

True. If we go toward a new color API that gives userspace explicit
control of the entire pipeline it is by definition incompatible with
a "legacy" API that touches the same HW.

>>  From what I've seen recently I am inclined to favor an incremental
>> approach more. The reason is that any API, or portion thereof, is
>> useless unless it's enabled full stack. When it isn't it becomes
>> dead code quickly, or never really works because we overlooked
>> one thing. The colorspace debacle shows how even something as
>> simple as extra enum values in KMS APIs shouldn't be added unless
>> someone in a canonical upstream project actually uses them. I
>> would argue that such a canonical upstream project actually has
>> to be a production environment and not something like Weston.
> 
> I agree that it's very easy to design something that doesn't work in
> the real world but weston is a real production environment. Even a new
> project can be a real production environment imo. The goals here are
> not entirely undefined: we have color transformations and we want to
> offload them.
> 
>> I could see us getting to a fully new color pipeline API but
>> the only way to do that is with a development model that supports
>> it. While upstream needs to be our ultimate goal, a good way
>> to bring in new APIs and ensure a full-stack implementation is
>> to develop them in a downstream production kernel, alongside
>> userspace that makes use of it. Once the implementation is
>> proven in the downstream repos it can then go upstream. This
>> brings new challenges, though, as things don't get wide
>> testing and get out of sync with upstream quickly. The
>> alternative is the incremental approach.
> 
> I also agree pretty much with everything here. My current idea is that
> we would add support for the new API in a downstream kernel for at
> least VKMS (one real driver probably makes sense as well) while in
> parallel developing a user space library for color conversions. That
> library might be a rewrite of libplacebo, which in its current form
> does all the color conversions we want to do but wasn't designed to
> allow for offloading. One of the developers expressed interest in
> rewriting the library in rust which would be a good opportunity to
> also take offloading into account.
> 

Doesn't libplacebo hook into video players, i.e., above the Wayland
protocol layer? Is the idea to bring it into a Wayland compositor
and teach it how to talk to DRM/KMS?

I wonder if it makes sense to somehow combine it with libliftoff for HW
offloading, since that library is already tackling the problem of
deciding whether to offload to KMS.

> No doubt all of that will take a significant amount of effort and time
> but we can still get HDR working in the old model without offloading
> and just sRGB and PQ/Rec2020 code paths.
> 

I would like to get to some form of HDR including offloading by adding
new per-plane LUTs or enumerated transfer functions as "legacy" 
properties. This would likely be much more tailored to specific 
use-cases than what Weston needs but would allow us to enable multi-plane
HDR in a more reasonable timeframe on applicable HW. These new
properties can educate an all-encompassing new DRM color API.

>> We should look at this from a use-case angle, similar to what
>> the gamescope guys are doing. Small steps, like:
>> 1) Add HDR10 output (PQ, BT.2020) to the display
>> 2) Add ability to do sRGB linear blending
>> 3) Add ability to do sRGB and PQ linear blending
>> 4) Post-blending 3D LUT
>> 5) Pre-blending 3D LUT
> 
> Sure, having a target in sight is a good idea.
> 
>> At each stage the whole stack needs to work together in production.
>>
>> If we go to a new color pipeline programming model it might
>> make sense to enable this as an "experimental" API that is
>> under development. I don't know if we've ever done that in
>> DRM/KMS. One way to do this might be with a new CONFIG option
>> that only exposes the new color pipeline API when enabled and
>> defaults to off, alongside a client cap for clients that
>> are advertising a desire to use the (experimental) API.
> 
> Yeah, that's a bit tricky. I also don't know how upstream would like
> this approach. Not even sure who to talk to.
> 

Agreed, I'm also not sure whether this would fly. airlied or danvet
might have an opinion.

This thought was inspired by "Blink Intents", which is a mechanism
how new full-stack features land in the Chromium browsers:
https://www.youtube.com/watch?v=9cvzZ5J_DTg

Harry

>> If we have that we could then look at porting all existing
>> use-cases over and verifying them (including IGT tests) before
>> moving on to HDR and wide-gamut use-cases. It's a large
>> undertaking and while I'm not opposed to it I don't know
>> if there are enough people willing to invest a large amount
>> of effort to make this happen.
>>
>> Harry
>>
>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>> incompatible with fancy color management. How would we even
>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>> that stuff is just always BT.709 limited range, no questions
>>>> asked.
>>>>
>>>> So I think if userspace wants real color management it's
>>>> going to have to set up the whole pipeline. And for that
>>>> we need at least one new property to control the RGB->YCbCr
>>>> conversion (or to explicitly avoid it).
>>>>
>>>> And given that the proposed patch just swept all the
>>>> non-BT.2020 issues under the rug makes me think no
>>>> one has actually come up with any kind of consistent
>>>> plan for anything else really.
>>>>
>>>>>
>>>>> Userspace needs full control of framebuffer pixel formats,
>>>>> as well as control over DEGAMMA, GAMMA, CTM color operations.
>>>>> It also needs to be able to select whether to drive the panel
>>>>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
>>>>> control the pixel encoding on the wire (RGB vs YCbCr).
>>>>>
>>>>>> I really don't want a repeat of the disaster of the
>>>>>> 'Broadcast RGB' which has coupled together the infoframe
>>>>>> and automagic conversion stuff. And I think this one would
>>>>>> be about 100x worse given this property has something
>>>>>> to do with actual colorspaces as well.
>>>>>>
>>>>>
>>>>> I'm unaware of this disaster. Could you elaborate?
>>>>
>>>> The property now controls both the infoframe stuff (and
>>>> whatever super vague stuff DP has for it in MSA) and
>>>> full->limited range compression in the display pipeline.
>>>> And as a result  there is no way to eg. allow already
>>>> limited range input, which is what some people wanted.
>>>>
>>>> And naturally it's all made a lot more terrible by all
>>>> the displays that fail to implement the spec correctly,
>>>> but that's another topic.
>>>>
>>>> --
>>>> Ville Syrjälä
>>>> Intel
>>>>
>>>
>>
>

Ville Syrjala Feb. 14, 2023, 8:10 p.m. UTC | #39

On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:
> On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:
> >
> >
> >
> > On 2/14/23 10:49, Sebastian Wick wrote:
> > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> > > <[e-mail]> wrote:
> > >>
> > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > >>>
> > >>>
> > >>> On 2/3/23 10:19, Ville Syrjälä wrote:
> > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > >>>>>
> > >>>>>
> > >>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > >>>>>> <[e-mail]> wrote:
> > >>>>>>>
> > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > >>>>>>>> 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.
> > >>>>>>>
> > >>>>>>> That applies to a lot of the other values as well (they are
> > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > >>>>>>> added to for use userspace to control the pixel encoding/colorspace
> > >>>>>>> conversion(if desired, or userspace just makes sure to
> > >>>>>>> directly feed in correct kind of data).
> > >>>>>>
> > >>>>>> I'm all for getting userspace control over pixel encoding but even
> > >>>>>> then the kernel always knows which pixel encoding is selected and
> > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > >>>>>> want to control the variant explicitly to the wrong value?
> > >>>>>>
> > >>>>>
> > >>>>> I've asked this before but haven't seen an answer: Is there an existing
> > >>>>> upstream userspace project that makes use of this property (other than
> > >>>>> what Joshua is working on in gamescope right now)? That would help us
> > >>>>> understand the intent better.
> > >>>>
> > >>>> The intent was to control the infoframe colorimetry bits,
> > >>>> nothing more. No idea what real userspace there was, if any.
> > >>>>
> > >>>>>
> > >>>>> I don't think giving userspace explicit control over the exact infoframe
> > >>>>> values is the right thing to do.
> > >>>>
> > >>>> Only userspace knows what kind of data it's stuffing into
> > >>>> the pixels (and/or how it configures the csc units/etc.) to
> > >>>> generate them.
> > >>>>
> > >>>
> > >>> Yes, but userspace doesn't control or know whether we drive
> > >>> RGB or YCbCr on the wire. In fact, in some cases our driver
> > >>> needs to fallback to YCbCr420 for bandwidth reasons. There
> > >>> is currently no way for userspace to know that and I don't
> > >>> think it makes sense.
> > >>
> > >> People want that control as well for whatever reason. We've
> > >> been asked to allow YCbCr 4:4:4 output many times.
> > >
> > > I don't really think it's a question of if we want it but rather how
> > > we get there. Harry is completely right that if we would make the
> > > subsampling controllable by user space instead of the kernel handling
> > > it magically, user space which does not adapt to the new control won't
> > > be able to light up some modes which worked before.
> > >
> >
> > Thanks for continuing this discussion and touching on the model of how
> > we get to where we want to go.
> >
> > > This is obviously a problem and not one we can easily fix. We would
> > > need a new cap for user space to signal "I know that I can control
> > > bpc, subsampling and compression to lower the bandwidth and light up
> > > modes which otherwise fail". That cap would also remove all the
> > > properties which require kernel magic to work (that's also what I
> > > proposed for my KMS color pipeline API).
> > >
> > > We all want to expose more of the scanout capability and give user
> > > space more control but I don't think an incremental approach works
> > > here and we would all do better if we accept that the current API
> > > requires kernel magic to work and has a few implicit assumptions baked
> > > in.
> > >
> > > With all that being said, I think the right decision here is to
> > >
> > > 1. Ignore subsampling for now
> > > 2. Let the kernel select YCC or RGB on the cable
> > > 3. Let the kernel figure out the conversion between RGB and YCC based
> > > on the color space selected
> > > 4. Let the kernel send the correct infoframe based on the selected
> > > color space and cable encoding
> > > 5. Only expose color spaces for which the kernel can do the conversion
> > > and send the infoframe
> >
> > I agree. We don't want to break or change existing behavior (that is
> > used by userspace) and this will get us far without breaking things.
> >
> > > 6. Work on the new API which is hidden behind a cap
> > >
> >
> > I assume you mean something like
> > https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> 
> Something like that, yes. The main point being a cap which removes a
> lot of properties and sets new expectations between user space and
> kernel. The actual API is not that important.
> 
> > Above you say that you don't think an incremental approach works
> > here. Can you elaborate?
> 
> Backwards compatibility is really hard. If we add another property to
> control e.g. the color range infoframe which doesn't magically convert
> colors, we now have to define how it interacts with the existing
> property.

I think that's easy. The old prop simply override the 
infoframe/etc. stuff. Pretty sure that's how we have it
implemented in i915 since the start.

And if you set it wrong it's you own fault.

> We also have to figure out how a user space which doesn't
> know about the new property behaves when another client has set that
> property. If any property which currently might change the pixel
> values is used, we can't expose the entire color pipeline because the
> kernel might have to use some element in it to achieve its magic
> conversion. So essentially you already have this hard device between
> "old" and "new" and you can't use the new stuff incrementally.

That problem exists with any new property. Old userspace and new
userspace may interact badly enought that nothing works right.
In that sense I think these props might even be pretty mundane
as the worst you might get from setting the infoframe wrong is
perhaps wrong colors on your display.

To solve that particular problem there has been talk (for years)
about some kind of "reset all" knob to make sure everything is
at a safe default value. I have a feeling there was even some
kind of semi-real proposal in recent times, but maybe I imgained
it?

Right now I guess what you do is either just reboot, or muck
around with modetest to manually reset properties.

I have occasionally abused this btw, to set some prop with
modetest and then run some real userspace that doesn't even
know about said property. Easy way to test stuff :P

Sebastian Wick Feb. 14, 2023, 9:18 p.m. UTC | #40

On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:
> > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:
> > >
> > >
> > >
> > > On 2/14/23 10:49, Sebastian Wick wrote:
> > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > >>
> > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > > >>>
> > > >>>
> > > >>> On 2/3/23 10:19, Ville Syrjälä wrote:
> > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > > >>>>>
> > > >>>>>
> > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > >>>>>> <[e-mail]> wrote:
> > > >>>>>>>
> > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > >>>>>>>> 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.
> > > >>>>>>>
> > > >>>>>>> That applies to a lot of the other values as well (they are
> > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > > >>>>>>> added to for use userspace to control the pixel encoding/colorspace
> > > >>>>>>> conversion(if desired, or userspace just makes sure to
> > > >>>>>>> directly feed in correct kind of data).
> > > >>>>>>
> > > >>>>>> I'm all for getting userspace control over pixel encoding but even
> > > >>>>>> then the kernel always knows which pixel encoding is selected and
> > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > > >>>>>> want to control the variant explicitly to the wrong value?
> > > >>>>>>
> > > >>>>>
> > > >>>>> I've asked this before but haven't seen an answer: Is there an existing
> > > >>>>> upstream userspace project that makes use of this property (other than
> > > >>>>> what Joshua is working on in gamescope right now)? That would help us
> > > >>>>> understand the intent better.
> > > >>>>
> > > >>>> The intent was to control the infoframe colorimetry bits,
> > > >>>> nothing more. No idea what real userspace there was, if any.
> > > >>>>
> > > >>>>>
> > > >>>>> I don't think giving userspace explicit control over the exact infoframe
> > > >>>>> values is the right thing to do.
> > > >>>>
> > > >>>> Only userspace knows what kind of data it's stuffing into
> > > >>>> the pixels (and/or how it configures the csc units/etc.) to
> > > >>>> generate them.
> > > >>>>
> > > >>>
> > > >>> Yes, but userspace doesn't control or know whether we drive
> > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver
> > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There
> > > >>> is currently no way for userspace to know that and I don't
> > > >>> think it makes sense.
> > > >>
> > > >> People want that control as well for whatever reason. We've
> > > >> been asked to allow YCbCr 4:4:4 output many times.
> > > >
> > > > I don't really think it's a question of if we want it but rather how
> > > > we get there. Harry is completely right that if we would make the
> > > > subsampling controllable by user space instead of the kernel handling
> > > > it magically, user space which does not adapt to the new control won't
> > > > be able to light up some modes which worked before.
> > > >
> > >
> > > Thanks for continuing this discussion and touching on the model of how
> > > we get to where we want to go.
> > >
> > > > This is obviously a problem and not one we can easily fix. We would
> > > > need a new cap for user space to signal "I know that I can control
> > > > bpc, subsampling and compression to lower the bandwidth and light up
> > > > modes which otherwise fail". That cap would also remove all the
> > > > properties which require kernel magic to work (that's also what I
> > > > proposed for my KMS color pipeline API).
> > > >
> > > > We all want to expose more of the scanout capability and give user
> > > > space more control but I don't think an incremental approach works
> > > > here and we would all do better if we accept that the current API
> > > > requires kernel magic to work and has a few implicit assumptions baked
> > > > in.
> > > >
> > > > With all that being said, I think the right decision here is to
> > > >
> > > > 1. Ignore subsampling for now
> > > > 2. Let the kernel select YCC or RGB on the cable
> > > > 3. Let the kernel figure out the conversion between RGB and YCC based
> > > > on the color space selected
> > > > 4. Let the kernel send the correct infoframe based on the selected
> > > > color space and cable encoding
> > > > 5. Only expose color spaces for which the kernel can do the conversion
> > > > and send the infoframe
> > >
> > > I agree. We don't want to break or change existing behavior (that is
> > > used by userspace) and this will get us far without breaking things.
> > >
> > > > 6. Work on the new API which is hidden behind a cap
> > > >
> > >
> > > I assume you mean something like
> > > https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> >
> > Something like that, yes. The main point being a cap which removes a
> > lot of properties and sets new expectations between user space and
> > kernel. The actual API is not that important.
> >
> > > Above you say that you don't think an incremental approach works
> > > here. Can you elaborate?
> >
> > Backwards compatibility is really hard. If we add another property to
> > control e.g. the color range infoframe which doesn't magically convert
> > colors, we now have to define how it interacts with the existing
> > property.
>
> I think that's easy. The old prop simply override the
> infoframe/etc. stuff. Pretty sure that's how we have it
> implemented in i915 since the start.
>
> And if you set it wrong it's you own fault.

We need the "old" property to control both the conversion and the
infoframe and we need the "new" property to control only the
infoframe. So if we don't want to old property to to any conversion we
now also need a new value for the old property which disables the
property. So why not just go for a cap and remove the property
altogether?

And again, as long as user space doesn't have complete control over
the entire pipeline we can't use the property which only sets the
infoframe anyway so you can't even have an incremental approach here.

Let's turn the question around: what do we gain by keeping all the
properties and implicit assumptions in the current API around? They
make everything more complex so what's the benefit?

I also want to be able to clearly document all the requirements of
user space and the guarantees from the kernel in the new API which we
can't do when we just incrementally change some properties here and
there. The documentation right now is basically a joke and almost
nothing has been defined properly and when things work, they work by
accident. Purely from a documentation and usability POV the
incremental approach is already going to be horrible IMO.

>
> > We also have to figure out how a user space which doesn't
> > know about the new property behaves when another client has set that
> > property. If any property which currently might change the pixel
> > values is used, we can't expose the entire color pipeline because the
> > kernel might have to use some element in it to achieve its magic
> > conversion. So essentially you already have this hard device between
> > "old" and "new" and you can't use the new stuff incrementally.
>
> That problem exists with any new property. Old userspace and new
> userspace may interact badly enought that nothing works right.
> In that sense I think these props might even be pretty mundane
> as the worst you might get from setting the infoframe wrong is
> perhaps wrong colors on your display.
>
> To solve that particular problem there has been talk (for years)
> about some kind of "reset all" knob to make sure everything is
> at a safe default value. I have a feeling there was even some
> kind of semi-real proposal in recent times, but maybe I imgained
> it?

I'm one of the people who argued for this but met only resistance. It
sure would be nice to have but also doesn't help with a lot of the
issues.

> Right now I guess what you do is either just reboot, or muck
> around with modetest to manually reset properties.
>
> I have occasionally abused this btw, to set some prop with
> modetest and then run some real userspace that doesn't even
> know about said property. Easy way to test stuff :P
>
> --
> Ville Syrjälä
> Intel
>

Ville Syrjala Feb. 14, 2023, 9:27 p.m. UTC | #41

On Tue, Feb 14, 2023 at 10:18:49PM +0100, Sebastian Wick wrote:
> On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:
> > > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:
> > > >
> > > >
> > > >
> > > > On 2/14/23 10:49, Sebastian Wick wrote:
> > > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> > > > > <[e-mail]> wrote:
> > > > >>
> > > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:
> > > > >>>
> > > > >>>
> > > > >>> On 2/3/23 10:19, Ville Syrjälä wrote:
> > > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote:
> > > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> > > > >>>>>> <[e-mail]> wrote:
> > > > >>>>>>>
> > > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:
> > > > >>>>>>>> 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.
> > > > >>>>>>>
> > > > >>>>>>> That applies to a lot of the other values as well (they are
> > > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> > > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> > > > >>>>>>> added to for use userspace to control the pixel encoding/colorspace
> > > > >>>>>>> conversion(if desired, or userspace just makes sure to
> > > > >>>>>>> directly feed in correct kind of data).
> > > > >>>>>>
> > > > >>>>>> I'm all for getting userspace control over pixel encoding but even
> > > > >>>>>> then the kernel always knows which pixel encoding is selected and
> > > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> > > > >>>>>> want to control the variant explicitly to the wrong value?
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>> I've asked this before but haven't seen an answer: Is there an existing
> > > > >>>>> upstream userspace project that makes use of this property (other than
> > > > >>>>> what Joshua is working on in gamescope right now)? That would help us
> > > > >>>>> understand the intent better.
> > > > >>>>
> > > > >>>> The intent was to control the infoframe colorimetry bits,
> > > > >>>> nothing more. No idea what real userspace there was, if any.
> > > > >>>>
> > > > >>>>>
> > > > >>>>> I don't think giving userspace explicit control over the exact infoframe
> > > > >>>>> values is the right thing to do.
> > > > >>>>
> > > > >>>> Only userspace knows what kind of data it's stuffing into
> > > > >>>> the pixels (and/or how it configures the csc units/etc.) to
> > > > >>>> generate them.
> > > > >>>>
> > > > >>>
> > > > >>> Yes, but userspace doesn't control or know whether we drive
> > > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver
> > > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There
> > > > >>> is currently no way for userspace to know that and I don't
> > > > >>> think it makes sense.
> > > > >>
> > > > >> People want that control as well for whatever reason. We've
> > > > >> been asked to allow YCbCr 4:4:4 output many times.
> > > > >
> > > > > I don't really think it's a question of if we want it but rather how
> > > > > we get there. Harry is completely right that if we would make the
> > > > > subsampling controllable by user space instead of the kernel handling
> > > > > it magically, user space which does not adapt to the new control won't
> > > > > be able to light up some modes which worked before.
> > > > >
> > > >
> > > > Thanks for continuing this discussion and touching on the model of how
> > > > we get to where we want to go.
> > > >
> > > > > This is obviously a problem and not one we can easily fix. We would
> > > > > need a new cap for user space to signal "I know that I can control
> > > > > bpc, subsampling and compression to lower the bandwidth and light up
> > > > > modes which otherwise fail". That cap would also remove all the
> > > > > properties which require kernel magic to work (that's also what I
> > > > > proposed for my KMS color pipeline API).
> > > > >
> > > > > We all want to expose more of the scanout capability and give user
> > > > > space more control but I don't think an incremental approach works
> > > > > here and we would all do better if we accept that the current API
> > > > > requires kernel magic to work and has a few implicit assumptions baked
> > > > > in.
> > > > >
> > > > > With all that being said, I think the right decision here is to
> > > > >
> > > > > 1. Ignore subsampling for now
> > > > > 2. Let the kernel select YCC or RGB on the cable
> > > > > 3. Let the kernel figure out the conversion between RGB and YCC based
> > > > > on the color space selected
> > > > > 4. Let the kernel send the correct infoframe based on the selected
> > > > > color space and cable encoding
> > > > > 5. Only expose color spaces for which the kernel can do the conversion
> > > > > and send the infoframe
> > > >
> > > > I agree. We don't want to break or change existing behavior (that is
> > > > used by userspace) and this will get us far without breaking things.
> > > >
> > > > > 6. Work on the new API which is hidden behind a cap
> > > > >
> > > >
> > > > I assume you mean something like
> > > > https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> > >
> > > Something like that, yes. The main point being a cap which removes a
> > > lot of properties and sets new expectations between user space and
> > > kernel. The actual API is not that important.
> > >
> > > > Above you say that you don't think an incremental approach works
> > > > here. Can you elaborate?
> > >
> > > Backwards compatibility is really hard. If we add another property to
> > > control e.g. the color range infoframe which doesn't magically convert
> > > colors, we now have to define how it interacts with the existing
> > > property.
> >
> > I think that's easy. The old prop simply override the
> > infoframe/etc. stuff. Pretty sure that's how we have it
> > implemented in i915 since the start.
> >
> > And if you set it wrong it's you own fault.
> 
> We need the "old" property to control both the conversion and the
> infoframe and we need the "new" property to control only the
> infoframe.

Wait what? The old property only controls the infoframe.
The new property (last I looked) was supposed to do more.
Or did that plan change already when I wasn't paying attention.

> So if we don't want to old property to to any conversion we
> now also need a new value for the old property which disables the
> property. So why not just go for a cap and remove the property
> altogether?
> 
> And again, as long as user space doesn't have complete control over
> the entire pipeline we can't use the property which only sets the
> infoframe anyway so you can't even have an incremental approach here.
> 
> Let's turn the question around: what do we gain by keeping all the
> properties and implicit assumptions in the current API around? They
> make everything more complex so what's the benefit?
> 
> I also want to be able to clearly document all the requirements of
> user space and the guarantees from the kernel in the new API which we
> can't do when we just incrementally change some properties here and
> there. The documentation right now is basically a joke and almost
> nothing has been defined properly and when things work, they work by
> accident. Purely from a documentation and usability POV the
> incremental approach is already going to be horrible IMO.
> 
> >
> > > We also have to figure out how a user space which doesn't
> > > know about the new property behaves when another client has set that
> > > property. If any property which currently might change the pixel
> > > values is used, we can't expose the entire color pipeline because the
> > > kernel might have to use some element in it to achieve its magic
> > > conversion. So essentially you already have this hard device between
> > > "old" and "new" and you can't use the new stuff incrementally.
> >
> > That problem exists with any new property. Old userspace and new
> > userspace may interact badly enought that nothing works right.
> > In that sense I think these props might even be pretty mundane
> > as the worst you might get from setting the infoframe wrong is
> > perhaps wrong colors on your display.
> >
> > To solve that particular problem there has been talk (for years)
> > about some kind of "reset all" knob to make sure everything is
> > at a safe default value. I have a feeling there was even some
> > kind of semi-real proposal in recent times, but maybe I imgained
> > it?
> 
> I'm one of the people who argued for this but met only resistance. It
> sure would be nice to have but also doesn't help with a lot of the
> issues.
> 
> > Right now I guess what you do is either just reboot, or muck
> > around with modetest to manually reset properties.
> >
> > I have occasionally abused this btw, to set some prop with
> > modetest and then run some real userspace that doesn't even
> > know about said property. Easy way to test stuff :P
> >
> > --
> > Ville Syrjälä
> > Intel
> >

Pekka Paalanen Feb. 15, 2023, 9:40 a.m. UTC | #42

On Tue, 14 Feb 2023 15:04:52 -0500
Harry Wentland <[e-mail]> wrote:

> On 2/14/23 14:45, Sebastian Wick wrote:
> > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:  
> >>
> >>
> >>
> >> On 2/14/23 10:49, Sebastian Wick wrote:  
> >>> On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
> >>> <[e-mail]> wrote:  
> >>>>
> >>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:  
> >>>>>
> >>>>>
> >>>>> On 2/3/23 10:19, Ville Syrjälä wrote:  
> >>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:  
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:  
> >>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
> >>>>>>>> <[e-mail]> wrote:  
> >>>>>>>>>
> >>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
> >>>>>>>>>> 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.  
> >>>>>>>>>
> >>>>>>>>> That applies to a lot of the other values as well (they are
> >>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
> >>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
> >>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
> >>>>>>>>> conversion(if desired, or userspace just makes sure to
> >>>>>>>>> directly feed in correct kind of data).  
> >>>>>>>>
> >>>>>>>> I'm all for getting userspace control over pixel encoding but even
> >>>>>>>> then the kernel always knows which pixel encoding is selected and
> >>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
> >>>>>>>> want to control the variant explicitly to the wrong value?
> >>>>>>>>  
> >>>>>>>
> >>>>>>> I've asked this before but haven't seen an answer: Is there an existing
> >>>>>>> upstream userspace project that makes use of this property (other than
> >>>>>>> what Joshua is working on in gamescope right now)? That would help us
> >>>>>>> understand the intent better.  
> >>>>>>
> >>>>>> The intent was to control the infoframe colorimetry bits,
> >>>>>> nothing more. No idea what real userspace there was, if any.
> >>>>>>  
> >>>>>>>
> >>>>>>> I don't think giving userspace explicit control over the exact infoframe
> >>>>>>> values is the right thing to do.  
> >>>>>>
> >>>>>> Only userspace knows what kind of data it's stuffing into
> >>>>>> the pixels (and/or how it configures the csc units/etc.) to
> >>>>>> generate them.
> >>>>>>  
> >>>>>
> >>>>> Yes, but userspace doesn't control or know whether we drive
> >>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
> >>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
> >>>>> is currently no way for userspace to know that and I don't
> >>>>> think it makes sense.  
> >>>>
> >>>> People want that control as well for whatever reason. We've
> >>>> been asked to allow YCbCr 4:4:4 output many times.  
> >>>
> >>> I don't really think it's a question of if we want it but rather how
> >>> we get there. Harry is completely right that if we would make the
> >>> subsampling controllable by user space instead of the kernel handling
> >>> it magically, user space which does not adapt to the new control won't
> >>> be able to light up some modes which worked before.
> >>>  
> >>
> >> Thanks for continuing this discussion and touching on the model of how
> >> we get to where we want to go.
> >>  
> >>> This is obviously a problem and not one we can easily fix. We would
> >>> need a new cap for user space to signal "I know that I can control
> >>> bpc, subsampling and compression to lower the bandwidth and light up
> >>> modes which otherwise fail". That cap would also remove all the
> >>> properties which require kernel magic to work (that's also what I
> >>> proposed for my KMS color pipeline API).
> >>>
> >>> We all want to expose more of the scanout capability and give user
> >>> space more control but I don't think an incremental approach works
> >>> here and we would all do better if we accept that the current API
> >>> requires kernel magic to work and has a few implicit assumptions baked
> >>> in.
> >>>
> >>> With all that being said, I think the right decision here is to
> >>>
> >>> 1. Ignore subsampling for now
> >>> 2. Let the kernel select YCC or RGB on the cable
> >>> 3. Let the kernel figure out the conversion between RGB and YCC based
> >>> on the color space selected
> >>> 4. Let the kernel send the correct infoframe based on the selected
> >>> color space and cable encoding
> >>> 5. Only expose color spaces for which the kernel can do the conversion
> >>> and send the infoframe  
> >>
> >> I agree. We don't want to break or change existing behavior (that is
> >> used by userspace) and this will get us far without breaking things.
> >>  
> >>> 6. Work on the new API which is hidden behind a cap

Hi,

I agree on all that, too.

> >>>  
> >>
> >> I assume you mean something like
> >> https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11  
> > 
> > Something like that, yes. The main point being a cap which removes a
> > lot of properties and sets new expectations between user space and
> > kernel. The actual API is not that important.
> >   
> >> Above you say that you don't think an incremental approach works
> >> here. Can you elaborate?  
> > 
> > Backwards compatibility is really hard. If we add another property to
> > control e.g. the color range infoframe which doesn't magically convert
> > colors, we now have to define how it interacts with the existing
> > property. We also have to figure out how a user space which doesn't
> > know about the new property behaves when another client has set that
> > property. If any property which currently might change the pixel
> > values is used, we can't expose the entire color pipeline because the
> > kernel might have to use some element in it to achieve its magic
> > conversion. So essentially you already have this hard device between
> > "old" and "new" and you can't use the new stuff incrementally.
> >   
> 
> True. If we go toward a new color API that gives userspace explicit
> control of the entire pipeline it is by definition incompatible with
> a "legacy" API that touches the same HW.
> 
> >>  From what I've seen recently I am inclined to favor an incremental
> >> approach more. The reason is that any API, or portion thereof, is
> >> useless unless it's enabled full stack. When it isn't it becomes
> >> dead code quickly, or never really works because we overlooked
> >> one thing. The colorspace debacle shows how even something as
> >> simple as extra enum values in KMS APIs shouldn't be added unless
> >> someone in a canonical upstream project actually uses them. I
> >> would argue that such a canonical upstream project actually has
> >> to be a production environment and not something like Weston.  

That's an interesting opinion about Weston.

Harry, are you perhaps assuming that Weston refuses to use experimental
UAPIs from downstream kernels, or where does that come from?

I think there is a lot of gray area that has not really been discussed
for Weston's policy. One thing is certain that Weston does not want to
go against upstream kernel policy, but that's a problem you need to
solve anyway if you want to use downstream kernel experimental UAPI.

> > 
> > I agree that it's very easy to design something that doesn't work in
> > the real world but weston is a real production environment. Even a new
> > project can be a real production environment imo. The goals here are
> > not entirely undefined: we have color transformations and we want to
> > offload them.
> >   
> >> I could see us getting to a fully new color pipeline API but
> >> the only way to do that is with a development model that supports
> >> it. While upstream needs to be our ultimate goal, a good way
> >> to bring in new APIs and ensure a full-stack implementation is
> >> to develop them in a downstream production kernel, alongside
> >> userspace that makes use of it. Once the implementation is
> >> proven in the downstream repos it can then go upstream. This
> >> brings new challenges, though, as things don't get wide
> >> testing and get out of sync with upstream quickly. The
> >> alternative is the incremental approach.  
> > 
> > I also agree pretty much with everything here. My current idea is that
> > we would add support for the new API in a downstream kernel for at
> > least VKMS (one real driver probably makes sense as well) while in
> > parallel developing a user space library for color conversions. That
> > library might be a rewrite of libplacebo, which in its current form
> > does all the color conversions we want to do but wasn't designed to
> > allow for offloading. One of the developers expressed interest in
> > rewriting the library in rust which would be a good opportunity to
> > also take offloading into account.
> >   
> 
> Doesn't libplacebo hook into video players, i.e., above the Wayland
> protocol layer? Is the idea to bring it into a Wayland compositor
> and teach it how to talk to DRM/KMS?
> 
> I wonder if it makes sense to somehow combine it with libliftoff for HW
> offloading, since that library is already tackling the problem of
> deciding whether to offload to KMS.
> 
> > No doubt all of that will take a significant amount of effort and time
> > but we can still get HDR working in the old model without offloading
> > and just sRGB and PQ/Rec2020 code paths.
> >   
> 
> I would like to get to some form of HDR including offloading by adding
> new per-plane LUTs or enumerated transfer functions as "legacy" 
> properties. This would likely be much more tailored to specific 
> use-cases than what Weston needs but would allow us to enable multi-plane
> HDR in a more reasonable timeframe on applicable HW. These new
> properties can educate an all-encompassing new DRM color API.

I think Weston should be fine with those legacy style KMS properties.
The problem might be that the required per-plane operations are
different than what KMS can express.

I have designed the Weston internals to carry the highest level
information about an operational element like a curve set to allow
everything from enumerated fixed curves and down to an arbitrary LUT,
including falling back to lower level description when necessary. E.g.
if there is no enumerated curve for something and it doesn't match
parameterised curve either, Weston can always fall back to a LUT if
precision is acceptable.

There will not be inference to go from LUT to a higher level
description, so all that depends on applications using the highest
level description they can, which is what we are designing into
color-representation and color-management.

That does pose a challenge for Weston's color pipeline optimiser, but I
do very much want it to get there. Converting everything into a LUT is
losing precision and performance.

However.

I am not in any hurry to make use of KMS color pipeline off-loading
features in Weston. I am approaching the API problem from the
application direction: what do applications and compositors need from
the color pipeline, and not how to expose everything of all existing
hardware. This does mean that it will take a long time before Weston is
ready to provide guidance on what an off-loading library API should
look like.

That's my opinion: I need to know what I want before I start asking for
it. Maybe someone who has a strong background in color science and
experience with video systems knows what the answer will be, but I
don't.

Still, I do need to be able to drive a monitor in known configuration
and correctly, which is why the "Colorspace" property discussion is
relevant to me right now.


Thanks,
pq

> 
> >> We should look at this from a use-case angle, similar to what
> >> the gamescope guys are doing. Small steps, like:
> >> 1) Add HDR10 output (PQ, BT.2020) to the display
> >> 2) Add ability to do sRGB linear blending
> >> 3) Add ability to do sRGB and PQ linear blending
> >> 4) Post-blending 3D LUT
> >> 5) Pre-blending 3D LUT  
> > 
> > Sure, having a target in sight is a good idea.
> >   
> >> At each stage the whole stack needs to work together in production.
> >>
> >> If we go to a new color pipeline programming model it might
> >> make sense to enable this as an "experimental" API that is
> >> under development. I don't know if we've ever done that in
> >> DRM/KMS. One way to do this might be with a new CONFIG option
> >> that only exposes the new color pipeline API when enabled and
> >> defaults to off, alongside a client cap for clients that
> >> are advertising a desire to use the (experimental) API.  
> > 
> > Yeah, that's a bit tricky. I also don't know how upstream would like
> > this approach. Not even sure who to talk to.
> >   
> 
> Agreed, I'm also not sure whether this would fly. airlied or danvet
> might have an opinion.
> 
> This thought was inspired by "Blink Intents", which is a mechanism
> how new full-stack features land in the Chromium browsers:
> https://www.youtube.com/watch?v=9cvzZ5J_DTg
> 
> Harry
> 
> >> If we have that we could then look at porting all existing
> >> use-cases over and verifying them (including IGT tests) before
> >> moving on to HDR and wide-gamut use-cases. It's a large
> >> undertaking and while I'm not opposed to it I don't know
> >> if there are enough people willing to invest a large amount
> >> of effort to make this happen.
> >>
> >> Harry
> >>  
> >>>> The automagic 4:2:0 fallback I think is rather fundementally
> >>>> incompatible with fancy color management. How would we even
> >>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
> >>>> that stuff is just always BT.709 limited range, no questions
> >>>> asked.
> >>>>
> >>>> So I think if userspace wants real color management it's
> >>>> going to have to set up the whole pipeline. And for that
> >>>> we need at least one new property to control the RGB->YCbCr
> >>>> conversion (or to explicitly avoid it).
> >>>>
> >>>> And given that the proposed patch just swept all the
> >>>> non-BT.2020 issues under the rug makes me think no
> >>>> one has actually come up with any kind of consistent
> >>>> plan for anything else really.
> >>>>  
> >>>>>
> >>>>> Userspace needs full control of framebuffer pixel formats,
> >>>>> as well as control over DEGAMMA, GAMMA, CTM color operations.
> >>>>> It also needs to be able to select whether to drive the panel
> >>>>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
> >>>>> control the pixel encoding on the wire (RGB vs YCbCr).
> >>>>>  
> >>>>>> I really don't want a repeat of the disaster of the
> >>>>>> 'Broadcast RGB' which has coupled together the infoframe
> >>>>>> and automagic conversion stuff. And I think this one would
> >>>>>> be about 100x worse given this property has something
> >>>>>> to do with actual colorspaces as well.
> >>>>>>  
> >>>>>
> >>>>> I'm unaware of this disaster. Could you elaborate?  
> >>>>
> >>>> The property now controls both the infoframe stuff (and
> >>>> whatever super vague stuff DP has for it in MSA) and
> >>>> full->limited range compression in the display pipeline.
> >>>> And as a result  there is no way to eg. allow already
> >>>> limited range input, which is what some people wanted.
> >>>>
> >>>> And naturally it's all made a lot more terrible by all
> >>>> the displays that fail to implement the spec correctly,
> >>>> but that's another topic.
> >>>>
> >>>> --
> >>>> Ville Syrjälä
> >>>> Intel
> >>>>  
> >>>  
> >>  
> >

Pekka Paalanen Feb. 15, 2023, 10:01 a.m. UTC | #43

On Tue, 14 Feb 2023 22:10:35 +0200
Ville Syrjälä <[e-mail]> wrote:

> On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:

...

> > We also have to figure out how a user space which doesn't
> > know about the new property behaves when another client has set that
> > property. If any property which currently might change the pixel
> > values is used, we can't expose the entire color pipeline because the
> > kernel might have to use some element in it to achieve its magic
> > conversion. So essentially you already have this hard device between
> > "old" and "new" and you can't use the new stuff incrementally.  
> 
> That problem exists with any new property. Old userspace and new
> userspace may interact badly enought that nothing works right.
> In that sense I think these props might even be pretty mundane
> as the worst you might get from setting the infoframe wrong is
> perhaps wrong colors on your display.
> 
> To solve that particular problem there has been talk (for years)
> about some kind of "reset all" knob to make sure everything is
> at a safe default value. I have a feeling there was even some
> kind of semi-real proposal in recent times, but maybe I imgained
> it?

I've been talking about that too, but I think it all collapsed into
"let's just fix all KMS apps to always set all KMS properties" which
results in patches like
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/952

It does not seem to be a serious enough problem for anyone to put in
the work. And why would it be, when you can easily fix it in your own
project like that Weston example. The Weston example is not even
representative, because I did it before I saw any real problems.

Other musings have been in the direction that maybe logind (since it
opens DRM devices for you) should save the full KMS state on the very
first open after a reboot, and then KMS applications can ask logind
what the boot-up state was. This is a variation of "save all KMS state
from the moment you launch, and use that as the base if you ever let
something else touch KMS in between".

You also never see the problem to begin with, if you never let
something else touch KMS in between, so that already makes the problem
rare outside of the tiny set of compositor developers.


Thanks,
pq

Ville Syrjala Feb. 15, 2023, 10:33 a.m. UTC | #44

On Wed, Feb 15, 2023 at 12:01:25PM +0200, Pekka Paalanen wrote:
> On Tue, 14 Feb 2023 22:10:35 +0200
> Ville Syrjälä <[e-mail]> wrote:
> 
> > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote:
> 
> ...
> 
> > > We also have to figure out how a user space which doesn't
> > > know about the new property behaves when another client has set that
> > > property. If any property which currently might change the pixel
> > > values is used, we can't expose the entire color pipeline because the
> > > kernel might have to use some element in it to achieve its magic
> > > conversion. So essentially you already have this hard device between
> > > "old" and "new" and you can't use the new stuff incrementally.  
> > 
> > That problem exists with any new property. Old userspace and new
> > userspace may interact badly enought that nothing works right.
> > In that sense I think these props might even be pretty mundane
> > as the worst you might get from setting the infoframe wrong is
> > perhaps wrong colors on your display.
> > 
> > To solve that particular problem there has been talk (for years)
> > about some kind of "reset all" knob to make sure everything is
> > at a safe default value. I have a feeling there was even some
> > kind of semi-real proposal in recent times, but maybe I imgained
> > it?
> 
> I've been talking about that too, but I think it all collapsed into
> "let's just fix all KMS apps to always set all KMS properties" which
> results in patches like
> https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/952

That requires some knowledge about the property in question to
pick the value. I think for some prop types (enums at least)
we could guarantee that the first value is always the safe default,
but for eg. range properties there is no way to know. So doing
that fully blind is not possible atm.

I guess one option might be to include a "reset value" in the
props somehow, and just have everyclient set all unknown props
to that. But there are of course other options too (reset
flag to atomic ioctl, etc.).

> 
> It does not seem to be a serious enough problem for anyone to put in
> the work. And why would it be, when you can easily fix it in your own
> project like that Weston example. The Weston example is not even
> representative, because I did it before I saw any real problems.
> 
> Other musings have been in the direction that maybe logind (since it
> opens DRM devices for you) should save the full KMS state on the very
> first open after a reboot, and then KMS applications can ask logind
> what the boot-up state was. This is a variation of "save all KMS state
> from the moment you launch, and use that as the base if you ever let
> something else touch KMS in between".
> 
> You also never see the problem to begin with, if you never let
> something else touch KMS in between, so that already makes the problem
> rare outside of the tiny set of compositor developers.

Yeah, it's a pretty rare problem so not much interest I guess.

Daniel Stone Feb. 15, 2023, 11:46 a.m. UTC | #45

Hi,

On Tue, 14 Feb 2023 at 16:57, Harry Wentland <[e-mail]> wrote:
> On 2/14/23 10:49, Sebastian Wick wrote:
> From what I've seen recently I am inclined to favor an incremental
> approach more. The reason is that any API, or portion thereof, is
> useless unless it's enabled full stack. When it isn't it becomes
> dead code quickly, or never really works because we overlooked
> one thing. The colorspace debacle shows how even something as
> simple as extra enum values in KMS APIs shouldn't be added unless
> someone in a canonical upstream project actually uses them. I
> would argue that such a canonical upstream project actually has
> to be a production environment and not something like Weston.

Just to chime in as well that it is a real production environment;
it's probably actually shipped the most of any compositor by a long
way. It doesn't have much place on the desktop, but it does live in
planes, trains, automobiles, digital signage, kiosks, STBs/TVs, and
about a billion other places you might not have expected.

Probably the main factor that joins all these together - apart from
not having much desktop-style click-and-drag reconfigurable UI - is
that we need to use the hardware pipeline as efficiently as possible,
because either we don't have the memory bandwidth to burn like
desktops, or we need to minimise it for power/thermal reasons.

Given that, we don't really want to paint ourselves into a corner with
incremental solutions that mean we can't do fully efficient things
later. We're also somewhat undermanned, and we've been using our
effort to try to make sure that the full solution - including full
colour-managed pathways for things like movie and TV post-prod
composition, design, etc - is possible at some point through the full
Wayland ecosystem at some point. The X11 experience was so horribly
botched that it wasn't really possible without a complete professional
setup, and that's something I personally don't want to see. However
...

> I could see us getting to a fully new color pipeline API but
> the only way to do that is with a development model that supports
> it. While upstream needs to be our ultimate goal, a good way
> to bring in new APIs and ensure a full-stack implementation is
> to develop them in a downstream production kernel, alongside
> userspace that makes use of it. Once the implementation is
> proven in the downstream repos it can then go upstream. This
> brings new challenges, though, as things don't get wide
> testing and get out of sync with upstream quickly. The
> alternative is the incremental approach.
>
> We should look at this from a use-case angle, similar to what
> the gamescope guys are doing. Small steps, like:
> 1) Add HDR10 output (PQ, BT.2020) to the display
> 2) Add ability to do sRGB linear blending
> 3) Add ability to do sRGB and PQ linear blending
> 4) Post-blending 3D LUT
> 5) Pre-blending 3D LUT
>
> At each stage the whole stack needs to work together in production.

Personally, I do think at this stage we probably have enough of an
understanding to be able to work with an intermediate solution. We
just need to think hard about what that intermediate solution is -
making sure that we don't end up in the same tangle of impossible
semantics like the old 'broadcast RGB' / colorspace / HDR properties
which were never thought through - so that it is something we can
build on rather than something we have to work around. But it would be
really good to make HDR10/HDR10+ media and HDR games work on HDR
displays, yeah.

Cheers,
Daniel

Harry Wentland Feb. 15, 2023, 8:45 p.m. UTC | #46

On 2/15/23 04:40, Pekka Paalanen wrote:
> On Tue, 14 Feb 2023 15:04:52 -0500
> Harry Wentland <[e-mail]> wrote:
> 
>> On 2/14/23 14:45, Sebastian Wick wrote:
>>> On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <[e-mail]> wrote:  
>>>>
>>>>
>>>>
>>>> On 2/14/23 10:49, Sebastian Wick wrote:  
>>>>> On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä
>>>>> <[e-mail]> wrote:  
>>>>>>
>>>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote:  
>>>>>>>
>>>>>>>
>>>>>>> On 2/3/23 10:19, Ville Syrjälä wrote:  
>>>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote:  
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote:  
>>>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
>>>>>>>>>> <[e-mail]> wrote:  
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote:  
>>>>>>>>>>>> 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.  
>>>>>>>>>>>
>>>>>>>>>>> That applies to a lot of the other values as well (they are
>>>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the
>>>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be
>>>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace
>>>>>>>>>>> conversion(if desired, or userspace just makes sure to
>>>>>>>>>>> directly feed in correct kind of data).  
>>>>>>>>>>
>>>>>>>>>> I'm all for getting userspace control over pixel encoding but even
>>>>>>>>>> then the kernel always knows which pixel encoding is selected and
>>>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace would
>>>>>>>>>> want to control the variant explicitly to the wrong value?
>>>>>>>>>>  
>>>>>>>>>
>>>>>>>>> I've asked this before but haven't seen an answer: Is there an existing
>>>>>>>>> upstream userspace project that makes use of this property (other than
>>>>>>>>> what Joshua is working on in gamescope right now)? That would help us
>>>>>>>>> understand the intent better.  
>>>>>>>>
>>>>>>>> The intent was to control the infoframe colorimetry bits,
>>>>>>>> nothing more. No idea what real userspace there was, if any.
>>>>>>>>  
>>>>>>>>>
>>>>>>>>> I don't think giving userspace explicit control over the exact infoframe
>>>>>>>>> values is the right thing to do.  
>>>>>>>>
>>>>>>>> Only userspace knows what kind of data it's stuffing into
>>>>>>>> the pixels (and/or how it configures the csc units/etc.) to
>>>>>>>> generate them.
>>>>>>>>  
>>>>>>>
>>>>>>> Yes, but userspace doesn't control or know whether we drive
>>>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver
>>>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There
>>>>>>> is currently no way for userspace to know that and I don't
>>>>>>> think it makes sense.  
>>>>>>
>>>>>> People want that control as well for whatever reason. We've
>>>>>> been asked to allow YCbCr 4:4:4 output many times.  
>>>>>
>>>>> I don't really think it's a question of if we want it but rather how
>>>>> we get there. Harry is completely right that if we would make the
>>>>> subsampling controllable by user space instead of the kernel handling
>>>>> it magically, user space which does not adapt to the new control won't
>>>>> be able to light up some modes which worked before.
>>>>>  
>>>>
>>>> Thanks for continuing this discussion and touching on the model of how
>>>> we get to where we want to go.
>>>>  
>>>>> This is obviously a problem and not one we can easily fix. We would
>>>>> need a new cap for user space to signal "I know that I can control
>>>>> bpc, subsampling and compression to lower the bandwidth and light up
>>>>> modes which otherwise fail". That cap would also remove all the
>>>>> properties which require kernel magic to work (that's also what I
>>>>> proposed for my KMS color pipeline API).
>>>>>
>>>>> We all want to expose more of the scanout capability and give user
>>>>> space more control but I don't think an incremental approach works
>>>>> here and we would all do better if we accept that the current API
>>>>> requires kernel magic to work and has a few implicit assumptions baked
>>>>> in.
>>>>>
>>>>> With all that being said, I think the right decision here is to
>>>>>
>>>>> 1. Ignore subsampling for now
>>>>> 2. Let the kernel select YCC or RGB on the cable
>>>>> 3. Let the kernel figure out the conversion between RGB and YCC based
>>>>> on the color space selected
>>>>> 4. Let the kernel send the correct infoframe based on the selected
>>>>> color space and cable encoding
>>>>> 5. Only expose color spaces for which the kernel can do the conversion
>>>>> and send the infoframe  
>>>>
>>>> I agree. We don't want to break or change existing behavior (that is
>>>> used by userspace) and this will get us far without breaking things.
>>>>  
>>>>> 6. Work on the new API which is hidden behind a cap
> 
> Hi,
> 
> I agree on all that, too.
> 
>>>>>  
>>>>
>>>> I assume you mean something like
>>>> https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11  
>>>
>>> Something like that, yes. The main point being a cap which removes a
>>> lot of properties and sets new expectations between user space and
>>> kernel. The actual API is not that important.
>>>   
>>>> Above you say that you don't think an incremental approach works
>>>> here. Can you elaborate?  
>>>
>>> Backwards compatibility is really hard. If we add another property to
>>> control e.g. the color range infoframe which doesn't magically convert
>>> colors, we now have to define how it interacts with the existing
>>> property. We also have to figure out how a user space which doesn't
>>> know about the new property behaves when another client has set that
>>> property. If any property which currently might change the pixel
>>> values is used, we can't expose the entire color pipeline because the
>>> kernel might have to use some element in it to achieve its magic
>>> conversion. So essentially you already have this hard device between
>>> "old" and "new" and you can't use the new stuff incrementally.
>>>   
>>
>> True. If we go toward a new color API that gives userspace explicit
>> control of the entire pipeline it is by definition incompatible with
>> a "legacy" API that touches the same HW.
>>
>>>>  From what I've seen recently I am inclined to favor an incremental
>>>> approach more. The reason is that any API, or portion thereof, is
>>>> useless unless it's enabled full stack. When it isn't it becomes
>>>> dead code quickly, or never really works because we overlooked
>>>> one thing. The colorspace debacle shows how even something as
>>>> simple as extra enum values in KMS APIs shouldn't be added unless
>>>> someone in a canonical upstream project actually uses them. I
>>>> would argue that such a canonical upstream project actually has
>>>> to be a production environment and not something like Weston.  
> 
> That's an interesting opinion about Weston.
> 
> Harry, are you perhaps assuming that Weston refuses to use experimental
> UAPIs from downstream kernels, or where does that come from?
> 

As a kernel display developer Weston looks to be an enablement vehicle for
new DRM/KMS and Wayland APIs and has little usefulness to me as an
end-user beyond that, unlike projects like sway or gamescope.

Part of it might be my naivete as I've heard mention of embedded projects
that are based on Weston but I'm not aware of these projects and who
uses them.

There is a hint of frustration here as the line-of-sight for landing
features for users is fuzzy (to me) with Weston. I don't want to put
the work on Weston down and appreciate the level of discussion and
design around compositor development (for color and otherwise) that
I've seen from you. Long-term this is definitely the right
direction.

> I think there is a lot of gray area that has not really been discussed
> for Weston's policy. One thing is certain that Weston does not want to
> go against upstream kernel policy, but that's a problem you need to
> solve anyway if you want to use downstream kernel experimental UAPI.
> 

I can appreciate it on some level but on another level there is a real
chicken and egg problem with new feature development that requires kernel
and compositor work. I can appreciate the initatives from, say wlroots
devs, to cut through this. Often implementing a solution step-by-step
with full vertical integration can teach more than lengthy discussions.
Again, maybe there is something that I'm missing here that is obvious
to you.

It's a difficult problem and I would think I'm not the only who struggles
with this.

>>>
>>> I agree that it's very easy to design something that doesn't work in
>>> the real world but weston is a real production environment. Even a new
>>> project can be a real production environment imo. The goals here are
>>> not entirely undefined: we have color transformations and we want to
>>> offload them.
>>>   
>>>> I could see us getting to a fully new color pipeline API but
>>>> the only way to do that is with a development model that supports
>>>> it. While upstream needs to be our ultimate goal, a good way
>>>> to bring in new APIs and ensure a full-stack implementation is
>>>> to develop them in a downstream production kernel, alongside
>>>> userspace that makes use of it. Once the implementation is
>>>> proven in the downstream repos it can then go upstream. This
>>>> brings new challenges, though, as things don't get wide
>>>> testing and get out of sync with upstream quickly. The
>>>> alternative is the incremental approach.  
>>>
>>> I also agree pretty much with everything here. My current idea is that
>>> we would add support for the new API in a downstream kernel for at
>>> least VKMS (one real driver probably makes sense as well) while in
>>> parallel developing a user space library for color conversions. That
>>> library might be a rewrite of libplacebo, which in its current form
>>> does all the color conversions we want to do but wasn't designed to
>>> allow for offloading. One of the developers expressed interest in
>>> rewriting the library in rust which would be a good opportunity to
>>> also take offloading into account.
>>>   
>>
>> Doesn't libplacebo hook into video players, i.e., above the Wayland
>> protocol layer? Is the idea to bring it into a Wayland compositor
>> and teach it how to talk to DRM/KMS?
>>
>> I wonder if it makes sense to somehow combine it with libliftoff for HW
>> offloading, since that library is already tackling the problem of
>> deciding whether to offload to KMS.
>>
>>> No doubt all of that will take a significant amount of effort and time
>>> but we can still get HDR working in the old model without offloading
>>> and just sRGB and PQ/Rec2020 code paths.
>>>   
>>
>> I would like to get to some form of HDR including offloading by adding
>> new per-plane LUTs or enumerated transfer functions as "legacy" 
>> properties. This would likely be much more tailored to specific 
>> use-cases than what Weston needs but would allow us to enable multi-plane
>> HDR in a more reasonable timeframe on applicable HW. These new
>> properties can educate an all-encompassing new DRM color API.
> 
> I think Weston should be fine with those legacy style KMS properties.
> The problem might be that the required per-plane operations are
> different than what KMS can express.
> 
> I have designed the Weston internals to carry the highest level
> information about an operational element like a curve set to allow
> everything from enumerated fixed curves and down to an arbitrary LUT,
> including falling back to lower level description when necessary. E.g.
> if there is no enumerated curve for something and it doesn't match
> parameterised curve either, Weston can always fall back to a LUT if
> precision is acceptable.
> 
> There will not be inference to go from LUT to a higher level
> description, so all that depends on applications using the highest
> level description they can, which is what we are designing into
> color-representation and color-management.
> 
> That does pose a challenge for Weston's color pipeline optimiser, but I
> do very much want it to get there. Converting everything into a LUT is
> losing precision and performance.
> 

With Weston and the new Wayland protocols you're looking at solving all
color-and-HDR-related use-cases, which is definitely needed at that
level. For HW composition the use-cases I am interested in are far
narrower.

> However.
> 
> I am not in any hurry to make use of KMS color pipeline off-loading
> features in Weston. I am approaching the API problem from the
> application direction: what do applications and compositors need from
> the color pipeline, and not how to expose everything of all existing
> hardware. This does mean that it will take a long time before Weston is
> ready to provide guidance on what an off-loading library API should
> look like.
> 
> That's my opinion: I need to know what I want before I start asking for
> it. Maybe someone who has a strong background in color science and
> experience with video systems knows what the answer will be, but I
> don't.
> 
> Still, I do need to be able to drive a monitor in known configuration
> and correctly, which is why the "Colorspace" property discussion is
> relevant to me right now.
> 

Makes sense and I fully agree that the first (and immediately important)
step is driving a monitor in a know configuration and correctly.

Harry

> 
> Thanks,
> pq
> 
>>
>>>> We should look at this from a use-case angle, similar to what
>>>> the gamescope guys are doing. Small steps, like:
>>>> 1) Add HDR10 output (PQ, BT.2020) to the display
>>>> 2) Add ability to do sRGB linear blending
>>>> 3) Add ability to do sRGB and PQ linear blending
>>>> 4) Post-blending 3D LUT
>>>> 5) Pre-blending 3D LUT  
>>>
>>> Sure, having a target in sight is a good idea.
>>>   
>>>> At each stage the whole stack needs to work together in production.
>>>>
>>>> If we go to a new color pipeline programming model it might
>>>> make sense to enable this as an "experimental" API that is
>>>> under development. I don't know if we've ever done that in
>>>> DRM/KMS. One way to do this might be with a new CONFIG option
>>>> that only exposes the new color pipeline API when enabled and
>>>> defaults to off, alongside a client cap for clients that
>>>> are advertising a desire to use the (experimental) API.  
>>>
>>> Yeah, that's a bit tricky. I also don't know how upstream would like
>>> this approach. Not even sure who to talk to.
>>>   
>>
>> Agreed, I'm also not sure whether this would fly. airlied or danvet
>> might have an opinion.
>>
>> This thought was inspired by "Blink Intents", which is a mechanism
>> how new full-stack features land in the Chromium browsers:
>> https://www.youtube.com/watch?v=9cvzZ5J_DTg
>>
>> Harry
>>
>>>> If we have that we could then look at porting all existing
>>>> use-cases over and verifying them (including IGT tests) before
>>>> moving on to HDR and wide-gamut use-cases. It's a large
>>>> undertaking and while I'm not opposed to it I don't know
>>>> if there are enough people willing to invest a large amount
>>>> of effort to make this happen.
>>>>
>>>> Harry
>>>>  
>>>>>> The automagic 4:2:0 fallback I think is rather fundementally
>>>>>> incompatible with fancy color management. How would we even
>>>>>> know whether to use eg. BT.2020 vs. BT.709 matrix? In i915
>>>>>> that stuff is just always BT.709 limited range, no questions
>>>>>> asked.
>>>>>>
>>>>>> So I think if userspace wants real color management it's
>>>>>> going to have to set up the whole pipeline. And for that
>>>>>> we need at least one new property to control the RGB->YCbCr
>>>>>> conversion (or to explicitly avoid it).
>>>>>>
>>>>>> And given that the proposed patch just swept all the
>>>>>> non-BT.2020 issues under the rug makes me think no
>>>>>> one has actually come up with any kind of consistent
>>>>>> plan for anything else really.
>>>>>>  
>>>>>>>
>>>>>>> Userspace needs full control of framebuffer pixel formats,
>>>>>>> as well as control over DEGAMMA, GAMMA, CTM color operations.
>>>>>>> It also needs to be able to select whether to drive the panel
>>>>>>> as sRGB or BT.2020/PQ but it doesn't make sense for it to
>>>>>>> control the pixel encoding on the wire (RGB vs YCbCr).
>>>>>>>  
>>>>>>>> I really don't want a repeat of the disaster of the
>>>>>>>> 'Broadcast RGB' which has coupled together the infoframe
>>>>>>>> and automagic conversion stuff. And I think this one would
>>>>>>>> be about 100x worse given this property has something
>>>>>>>> to do with actual colorspaces as well.
>>>>>>>>  
>>>>>>>
>>>>>>> I'm unaware of this disaster. Could you elaborate?  
>>>>>>
>>>>>> The property now controls both the infoframe stuff (and
>>>>>> whatever super vague stuff DP has for it in MSA) and
>>>>>> full->limited range compression in the display pipeline.
>>>>>> And as a result  there is no way to eg. allow already
>>>>>> limited range input, which is what some people wanted.
>>>>>>
>>>>>> And naturally it's all made a lot more terrible by all
>>>>>> the displays that fail to implement the spec correctly,
>>>>>> but that's another topic.
>>>>>>
>>>>>> --
>>>>>> Ville Syrjälä
>>>>>> Intel
>>>>>>  
>>>>>  
>>>>  
>>>   
>

Harry Wentland Feb. 15, 2023, 8:54 p.m. UTC | #47

On 2/15/23 06:46, Daniel Stone wrote:
> Hi,
> 
> On Tue, 14 Feb 2023 at 16:57, Harry Wentland <[e-mail]> wrote:
>> On 2/14/23 10:49, Sebastian Wick wrote:
>> From what I've seen recently I am inclined to favor an incremental
>> approach more. The reason is that any API, or portion thereof, is
>> useless unless it's enabled full stack. When it isn't it becomes
>> dead code quickly, or never really works because we overlooked
>> one thing. The colorspace debacle shows how even something as
>> simple as extra enum values in KMS APIs shouldn't be added unless
>> someone in a canonical upstream project actually uses them. I
>> would argue that such a canonical upstream project actually has
>> to be a production environment and not something like Weston.
> 
> Just to chime in as well that it is a real production environment;
> it's probably actually shipped the most of any compositor by a long
> way. It doesn't have much place on the desktop, but it does live in
> planes, trains, automobiles, digital signage, kiosks, STBs/TVs, and
> about a billion other places you might not have expected.
> 

Understood.

Curious if there's a list of some concrete examples.

> Probably the main factor that joins all these together - apart from
> not having much desktop-style click-and-drag reconfigurable UI - is
> that we need to use the hardware pipeline as efficiently as possible,
> because either we don't have the memory bandwidth to burn like
> desktops, or we need to minimise it for power/thermal reasons.
> 

I think we're very much aligned here.

> Given that, we don't really want to paint ourselves into a corner with
> incremental solutions that mean we can't do fully efficient things
> later. We're also somewhat undermanned, and we've been using our
> effort to try to make sure that the full solution - including full
> colour-managed pathways for things like movie and TV post-prod
> composition, design, etc - is possible at some point through the full
> Wayland ecosystem at some point. The X11 experience was so horribly
> botched that it wasn't really possible without a complete professional
> setup, and that's something I personally don't want to see. However
> ...

Agreed.

> 
>> I could see us getting to a fully new color pipeline API but
>> the only way to do that is with a development model that supports
>> it. While upstream needs to be our ultimate goal, a good way
>> to bring in new APIs and ensure a full-stack implementation is
>> to develop them in a downstream production kernel, alongside
>> userspace that makes use of it. Once the implementation is
>> proven in the downstream repos it can then go upstream. This
>> brings new challenges, though, as things don't get wide
>> testing and get out of sync with upstream quickly. The
>> alternative is the incremental approach.
>>
>> We should look at this from a use-case angle, similar to what
>> the gamescope guys are doing. Small steps, like:
>> 1) Add HDR10 output (PQ, BT.2020) to the display
>> 2) Add ability to do sRGB linear blending
>> 3) Add ability to do sRGB and PQ linear blending
>> 4) Post-blending 3D LUT
>> 5) Pre-blending 3D LUT
>>
>> At each stage the whole stack needs to work together in production.
> 
> Personally, I do think at this stage we probably have enough of an
> understanding to be able to work with an intermediate solution. We
> just need to think hard about what that intermediate solution is -
> making sure that we don't end up in the same tangle of impossible
> semantics like the old 'broadcast RGB' / colorspace / HDR properties
> which were never thought through - so that it is something we can
> build on rather than something we have to work around. But it would be
> really good to make HDR10/HDR10+ media and HDR games work on HDR
> displays, yeah.
> 

I have a feeling we'll make some progress here this year. I definitely
think the whole HDR/Colour work is on the right track in Weston and
Wayland which will hopefully give us a good base to work with over
many years.

Harry

> Cheers,
> Daniel

Daniel Stone Feb. 15, 2023, 10:07 p.m. UTC | #48

On Wed, 15 Feb 2023 at 20:54, Harry Wentland <[e-mail]> wrote:
> On 2/15/23 06:46, Daniel Stone wrote:
> > On Tue, 14 Feb 2023 at 16:57, Harry Wentland <[e-mail]> wrote:
> >> On 2/14/23 10:49, Sebastian Wick wrote:
> >> From what I've seen recently I am inclined to favor an incremental
> >> approach more. The reason is that any API, or portion thereof, is
> >> useless unless it's enabled full stack. When it isn't it becomes
> >> dead code quickly, or never really works because we overlooked
> >> one thing. The colorspace debacle shows how even something as
> >> simple as extra enum values in KMS APIs shouldn't be added unless
> >> someone in a canonical upstream project actually uses them. I
> >> would argue that such a canonical upstream project actually has
> >> to be a production environment and not something like Weston.
> >
> > Just to chime in as well that it is a real production environment;
> > it's probably actually shipped the most of any compositor by a long
> > way. It doesn't have much place on the desktop, but it does live in
> > planes, trains, automobiles, digital signage, kiosks, STBs/TVs, and
> > about a billion other places you might not have expected.
> >
>
> Understood.
>
> Curious if there's a list of some concrete examples.

If I was allowed to name them, I'd definitely be doing a much better
job of promoting it ... but if you've bought a car in the last 7-8
years, it's much more likely than not that its console display is
using Weston. Probably about 50% odds that you've flown on a plane
whose IFE is driven by Weston. You've definitely walked past a lot of
digital signage advertisements and display walls which are driven by
Weston. There are a huge number of consumer products (and other modes
of transport, would you believe?) that are too, but I can't name them
because it gets too specific.

The cars are probably using a 10+ year old (and frankly awful) SoC.
The display walls are probably using a 6ish-year-old SoC with
notoriously poor memory bandwidth. Or TVs trying to make 4K UIs fly on
an ancient (pre-unified-shader) GPU. The hits go on. We do ship things
on nice and capable new hardware as well, but keeping old hardware
working with new software stacks is non-negotiable for us, and we have
to bend over backwards to make that happen.

> >> We should look at this from a use-case angle, similar to what
> >> the gamescope guys are doing. Small steps, like:
> >> 1) Add HDR10 output (PQ, BT.2020) to the display
> >> 2) Add ability to do sRGB linear blending
> >> 3) Add ability to do sRGB and PQ linear blending
> >> 4) Post-blending 3D LUT
> >> 5) Pre-blending 3D LUT
> >>
> >> At each stage the whole stack needs to work together in production.
> >
> > Personally, I do think at this stage we probably have enough of an
> > understanding to be able to work with an intermediate solution. We
> > just need to think hard about what that intermediate solution is -
> > making sure that we don't end up in the same tangle of impossible
> > semantics like the old 'broadcast RGB' / colorspace / HDR properties
> > which were never thought through - so that it is something we can
> > build on rather than something we have to work around. But it would be
> > really good to make HDR10/HDR10+ media and HDR games work on HDR
> > displays, yeah.
>
> I have a feeling we'll make some progress here this year. I definitely
> think the whole HDR/Colour work is on the right track in Weston and
> Wayland which will hopefully give us a good base to work with over
> many years.

Yep!

Coming to the point you were making in the other mail - Weston was
traditionally used as _the_ enablement vehicle for KMS, because we
cared about using the depth of hardware much more than anyone else
(e.g. being years ahead on planes), and the vendor who wanted to
enable it either wanted to enable Weston specifically or just didn't
have an open userspace stack for it. The other compositors couldn't be
that vehicle, either because they were more focused on desktop UI, or
they could just afford to throw the GPU at it and suck up the
occasional frame hitch / thermal burn / etc. I like to think we had a
reputation for being pretty thoughtful and careful with our review as
well, and didn't give it lightly to misguided ideas which caused
long-term problems.

But we've got a greater diversity in userspace these days, and that's
no bad thing. If the best vehicle to demonstrate HDR GPU rendering is
gamescope, then use gamescope as that vehicle. We'll be there if we
can, and if it makes sense for us, but it's not a requirement.

Cheers,
Daniel

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index 0264abe55278..c85860600395 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -99,8 +99,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))
 
@@ -113,9 +112,9 @@  static const u32 hdmi_colorimetry_val[] = {
 	[DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
 	[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_1] = HDMI_COLORIMETRY_BT2020,
+	[DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = 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..58699ab15a6a 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1029,11 +1029,11 @@  static const struct drm_prop_enum_list hdmi_colorspaces[] = {
 	/* Colorimetry based on IEC 61966-2-5 */
 	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
 	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
+	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
 	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
+	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
 	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
+	{ DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
 	/* 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 */
@@ -1066,9 +1066,9 @@  static const struct drm_prop_enum_list dp_colorspaces[] = {
 	/* Colorimetry based on IEC 61966-2-5 [33] */
 	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
 	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
+	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
 	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
+	{ DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index c9be61d2348e..1aa5dedeec7b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1763,14 +1763,12 @@  static void intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
 	case DRM_MODE_COLORIMETRY_OPYCC_601:
 		vsc->colorimetry = DP_COLORIMETRY_OPYCC_601;
 		break;
-	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_1:
+	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
+	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 +3041,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_1:
+	case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
+	case DRM_MODE_COLORIMETRY_BT2020:
 		return true;
 	default:
 		break;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index eb4cc9076e16..42a3cf43168c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -390,12 +390,13 @@  enum drm_privacy_screen_status {
  *   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
- * @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
+ * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1:
+ * @DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2:
+ * @DRM_MODE_COLORIMETRY_BT2020:
+ *   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)
+ *   colorimetry format
  * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
  *   DCI-P3 (SMPTE RP 431-2) colorimetry format
  * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
@@ -420,9 +421,9 @@  enum drm_colorspace {
 	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,
+	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1,
+	DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2,
+	DRM_MODE_COLORIMETRY_BT2020,
 	/* Additional Colorimetry extension added as part of CTA 861.G */
 	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
 	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,