Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
> Op 31-10-18 om 13:05 schreef Uma Shankar:
> > This patch adds a colorspace property enabling
> > userspace to switch to various supported colorspaces.
> > This will help enable BT2020 along with other colorspaces.
> >
> > v2: Addressed Maarten and Ville's review comments. Enhanced
> > the colorspace enum to incorporate both HDMI and DP supported
> > colorspaces. Also, added a default option for colorspace.
> >
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
> >  drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  7 +++++++
> >  include/drm/drm_mode_config.h     |  6 ++++++
> >  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
> >  5 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d5b7f31..9e1d46b 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		state->picture_aspect_ratio = val;
> >  	} else if (property == config->content_type_property) {
> >  		state->content_type = val;
> > +	} else if (property == config->colorspace_property) {
> > +		state->colorspace = val;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		state->scaling_mode = val;
> >  	} else if (property == connector->content_protection_property) {
> > @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		*val = state->picture_aspect_ratio;
> >  	} else if (property == config->content_type_property) {
> >  		*val = state->content_type;
> > +	} else if (property == config->colorspace_property) {
> > +		*val = state->colorspace;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		*val = state->scaling_mode;
> >  	} else if (property == connector->content_protection_property) {
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index aa18b1d..5ad52a0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> >  };
> >  DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> >  
> > +static const struct drm_prop_enum_list colorspace[] = {
> > +	/* Standard Definition Colorimetry based on CEA 861 */
> > +	{ COLORIMETRY_ITU_601, "601" },
> > +	{ COLORIMETRY_ITU_709, "709" },
> > +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > +	{ COLORIMETRY_XV_YCC_601, "601" },
> > +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> > +	{ COLORIMETRY_XV_YCC_709, "709" },
> 2 definitions that share the same name?
> All in all, I think the enum strings need to be more descriptive and self-consistent.
+1

> > +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> > +	{ COLORIMETRY_S_YCC_601, "s601" },
> > +	/* Colorimetry based on IEC 61966-2-5 [33] */
> > +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },

Hans (cc:d) recetly noted that these things aren't called
Adobe<something> anymore in the CTA-861 spec. There is some new name for
them, which I now forget.

> > +	/* Colorimetry based on IEC 61966-2-5 */
> > +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
> > +	/* DP MSA Colorimetry */
> > +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
> > +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
> > +	{ COLORIMETRY_SRGB, "SRGB" },
> sRGB
> > +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
> > +	{ COLORIMETRY_SCRGB, "SCRGB" },
> scRGB?
> > +	{ COLORIMETRY_DCI_P3, "DCIP3" },
> DCI-P3?
> > +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
> ^Typo
> > +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
> > +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
> This enum together with the code below is broken.
> 
> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> 
> I would just make it COLORIMETRY_UNSET = "Unset".

"Unset" might work. Though feels a bit strange to me. Other ideas
would be "Default", "Automatic", "Undefined" or something along those lines.
Ideally it should convey the meaning of "the driver will pick this for you",
and for that I'd lean towards "Default" or "Automatic".

> 
> To set the default colorimetry for all drivers, just make the default value 0 (preferred),
> or add it to __drm_atomic_helper_connector_reset().
> 
> > +};
> > +
> >  /**
> >   * DOC: standard connector properties
> >   *
> > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> >   *	can also expose this property to external outputs, in which case they
> >   *	must support "None", which should be the default (since external screens
> >   *	have a built-in scaler).
> > + *
> > + * colorspace:
> > + *	This property helps select a suitable colorspace based on the sink
> > + *	capability. Modern sink devices support wider gamut like BT2020.
> > + *	This helps switch to BT2020 mode if the BT2020 encoded video stream
> > + *	is being played by the user, same for any other colorspace.
> >   */
> >  
> >  int drm_connector_create_standard_properties(struct drm_device *dev)
> > @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
> >  		return -ENOMEM;
> >  	dev->mode_config.non_desktop_property = prop;
> >  
> > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
> > +					colorspace, ARRAY_SIZE(colorspace));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.colorspace_property = prop;
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index dd0552c..b7f5373 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -497,6 +497,13 @@ struct drm_connector_state {
> >  	unsigned int content_protection;
> >  
> >  	/**
> > +	 * @colorspace: Connector property to request colorspace
> > +	 * change. This is most commonly used to switch to wider color
> > +	 * gamuts like BT2020.
> > +	 */
> > +	enum encoder_colorimetry colorspace;
> > +
> > +	/**
> >  	 * @writeback_job: Writeback job for writeback connectors
> >  	 *
> >  	 * Holds the framebuffer and out-fence for a writeback connector. As
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index d643d26..a6eb0aa 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -863,6 +863,12 @@ struct drm_mode_config {
> >  	uint32_t cursor_width, cursor_height;
> >  
> >  	/**
> > +	 * @colorspace_property: Connector property to set the suitable
> > +	 * colorspace supported by the sink.
> > +	 */
> > +	struct drm_property *colorspace_property;
> > +
> > +	/**
> >  	 * @suspend_state:
> >  	 *
> >  	 * Atomic state when suspended.
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index d3e0fe3..831cc77 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -210,6 +210,30 @@
> >  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> >  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> >  
> > +enum encoder_colorimetry {
> > +	/* CEA 861 Normal Colorimetry options */
> > +	COLORIMETRY_ITU_601 = 0,
> > +	COLORIMETRY_ITU_709,
> > +	/* CEA 861 Extended Colorimetry Options */
> > +	COLORIMETRY_XV_YCC_601,
> > +	COLORIMETRY_XV_YCC_709,
> > +	COLORIMETRY_S_YCC_601,
> > +	COLORIMETRY_ADOBE_YCC_601,
> > +	COLORIMETRY_ADOBE_RGB,
> > +	COLORIMETRY_BT2020_RGB,
> > +	COLORIMETRY_BT2020_YCC,
> > +	COLORIMETRY_BT2020_CYCC,
> > +	/* DP MSA Colorimetry Options */
> > +	COLORIMETRY_Y_CBCR_ITU_601,
> > +	COLORIMETRY_Y_CBCR_ITU_709,
> > +	COLORIMETRY_SRGB,
> > +	COLORIMETRY_RGB_WIDE_GAMUT,
> > +	COLORIMETRY_SCRGB,
> > +	COLORIMETRY_DCI_P3,
> > +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
> > +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> > +};
> > +
> >  struct drm_mode_modeinfo {
> >  	__u32 clock;
> >  	__u16 hdisplay;
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel