[v2,1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

From: Jyri Sarha <jsarha@ti.com>

Add a standard optional properties to support different non RGB color
encodings in DRM planes. COLOR_ENCODING select the supported non RGB
color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
the value ranges within the selected color encoding. The properties
are stored to drm_plane object to allow different set of supported
encoding for different planes on the device.

v2: Add/fix kerneldocs, verify bitmasks (danvet)

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
[vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c     |   8 +++
 drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     |  19 ++++++++
 include/drm/drm_plane.h          |  32 ++++++++++++
 4 files changed, 162 insertions(+)

On Mon, Feb 19, 2018 at 10:28:23PM +0200, Ville Syrjala wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Add a standard optional properties to support different non RGB color
> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> the value ranges within the selected color encoding. The properties
> are stored to drm_plane object to allow different set of supported
> encoding for different planes on the device.
> 
> v2: Add/fix kerneldocs, verify bitmasks (danvet)
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c     |   8 +++
>  drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  19 ++++++++
>  include/drm/drm_plane.h          |  32 ++++++++++++
>  4 files changed, 162 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 46733d534587..452a0b0bafbc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->rotation = val;
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
> +	} else if (property == plane->color_encoding_property) {
> +		state->color_encoding = val;
> +	} else if (property == plane->color_range_property) {
> +		state->color_range = val;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
>  		*val = state->zpos;
> +	} else if (property == plane->color_encoding_property) {
> +		*val = state->color_encoding;
> +	} else if (property == plane->color_range_property) {
> +		*val = state->color_range;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 0d002b045bd2..4b83e078d3e9 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -88,6 +88,20 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>   * "GAMMA_LUT" property above.
> + *
> + * Support for different non RGB color encodings is controlled through
> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
> + * are set up by calling drm_plane_create_color_properties().
> + *
> + * "COLOR_ENCODING"
> + * 	Optional plane enum property to support different non RGB
> + * 	color encodings. The driver can provide a subset of standard
> + * 	enum values supported by the DRM plane.
> + *
> + * "COLOR_RANGE"
> + * 	Optional plane enum property to support different non RGB
> + * 	color parameter ranges. The driver can provide a subset of
> + * 	standard enum values supported by the DRM plane.
>   */
>  
>  /**
> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static const char * const color_encoding_name[] = {
> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> +};
> +
> +static const char * const color_range_name[] = {
> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> +};
> +
> +/**
> + * drm_plane_create_color_properties - color encoding related plane properties
> + * @plane: plane object
> + * @supported_encodings: bitfield indicating supported color encodings
> + * @supported_ranges: bitfileld indicating supported color ranges
> + * @default_encoding: default color encoding
> + * @default_range: default color range
> + *
> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> + * properties to @plane. The supported encodings and ranges should
> + * be provided in supported_encodings and supported_ranges bitmasks.
> + * Each bit set in the bitmask indicates that its number as enum
> + * value is supported.
> + */
> +int drm_plane_create_color_properties(struct drm_plane *plane,
> +				      u32 supported_encodings,
> +				      u32 supported_ranges,
> +				      enum drm_color_encoding default_encoding,
> +				      enum drm_color_range default_range)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> +						DRM_COLOR_RANGE_MAX)];
> +	int i, len;
> +
> +	if (WARN_ON(supported_encodings == 0 ||
> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
> +		    (supported_encodings & BIT(default_encoding)) == 0))
> +		return -EINVAL;
> +
> +	if (WARN_ON(supported_ranges == 0 ||
> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> +		    (supported_ranges & BIT(default_range)) == 0))
> +		return -EINVAL;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> +		if ((supported_encodings & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_encoding_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> +	if (plane->state)
> +		plane->state->color_encoding = default_encoding;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> +		if ((supported_ranges & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_range_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_range_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_range);
> +	if (plane->state)
> +		plane->state->color_range = default_range;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_properties);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cbce621..b3b6d302ca8c 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,7 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  				 int gamma_size);
>  
> +enum drm_color_encoding {
> +	DRM_COLOR_YCBCR_BT601,
> +	DRM_COLOR_YCBCR_BT709,
> +	DRM_COLOR_YCBCR_BT2020,
> +	DRM_COLOR_ENCODING_MAX,
> +};
> +
> +enum drm_color_range {
> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	DRM_COLOR_YCBCR_FULL_RANGE,
> +	DRM_COLOR_RANGE_MAX,
> +};
> +
> +int drm_plane_create_color_properties(struct drm_plane *plane,
> +				      u32 supported_encodings,
> +				      u32 supported_ranges,
> +				      enum drm_color_encoding default_encoding,
> +				      enum drm_color_range default_range);
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..f7bf4a48b1c3 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/ctype.h>
>  #include <drm/drm_mode_object.h>
> +#include <drm/drm_color_mgmt.h>
>  
>  struct drm_crtc;
>  struct drm_printer;
> @@ -112,6 +113,20 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/**
> +	 * @color_encoding:
> +	 *
> +	 * Color encoding for non RGB formats
> +	 */
> +	enum drm_color_encoding color_encoding;
> +
> +	/**
> +	 * @color_range:
> +	 *
> +	 * Color range for non RGB formats
> +	 */
> +	enum drm_color_range color_range;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -558,6 +573,23 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	/**
> +	 * @color_encoding_property:
> +	 *
> +	 * Optional "COLOR_ENCODING" enum property for specifying
> +	 * color encoding for non RGB formats.
> +	 * See drm_plane_create_color_properties().
> +	 */
> +	struct drm_property *color_encoding_property;
> +	/**
> +	 * @color_range_property:
> +	 *
> +	 * Optional "COLOR_RANGE" enum property for specifying
> +	 * color range for non RGB formats.
> +	 * See drm_plane_create_color_properties().
> +	 */
> +	struct drm_property *color_range_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> -- 
> 2.13.6
>

Hi,

I am looking for a way to export the color encoding and range selection
to user space. I came across those properties and am wondering, why
they are meant only for non RGB color encodings. Would it be okay, to
modify them and use with RGB formats as well?

Regards,
Chris


On 02/19/2018 09:28 PM, Ville Syrjala wrote:
> From: Jyri Sarha <jsarha@ti.com>
>
> Add a standard optional properties to support different non RGB color
> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> the value ranges within the selected color encoding. The properties
> are stored to drm_plane object to allow different set of supported
> encoding for different planes on the device.
>
> v2: Add/fix kerneldocs, verify bitmasks (danvet)
>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/drm_atomic.c     |   8 +++
>   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_color_mgmt.h     |  19 ++++++++
>   include/drm/drm_plane.h          |  32 ++++++++++++
>   4 files changed, 162 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 46733d534587..452a0b0bafbc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->rotation = val;
>   	} else if (property == plane->zpos_property) {
>   		state->zpos = val;
> +	} else if (property == plane->color_encoding_property) {
> +		state->color_encoding = val;
> +	} else if (property == plane->color_range_property) {
> +		state->color_range = val;
>   	} else if (plane->funcs->atomic_set_property) {
>   		return plane->funcs->atomic_set_property(plane, state,
>   				property, val);
> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   		*val = state->rotation;
>   	} else if (property == plane->zpos_property) {
>   		*val = state->zpos;
> +	} else if (property == plane->color_encoding_property) {
> +		*val = state->color_encoding;
> +	} else if (property == plane->color_range_property) {
> +		*val = state->color_range;
>   	} else if (plane->funcs->atomic_get_property) {
>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>   	} else {
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 0d002b045bd2..4b83e078d3e9 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -88,6 +88,20 @@
>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>    * "GAMMA_LUT" property above.
> + *
> + * Support for different non RGB color encodings is controlled through
> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
> + * are set up by calling drm_plane_create_color_properties().
> + *
> + * "COLOR_ENCODING"
> + * 	Optional plane enum property to support different non RGB
> + * 	color encodings. The driver can provide a subset of standard
> + * 	enum values supported by the DRM plane.
> + *
> + * "COLOR_RANGE"
> + * 	Optional plane enum property to support different non RGB
> + * 	color parameter ranges. The driver can provide a subset of
> + * 	standard enum values supported by the DRM plane.
>    */
>   
>   /**
> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>   	drm_modeset_unlock(&crtc->mutex);
>   	return ret;
>   }
> +
> +static const char * const color_encoding_name[] = {
> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> +};
> +
> +static const char * const color_range_name[] = {
> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> +};
> +
> +/**
> + * drm_plane_create_color_properties - color encoding related plane properties
> + * @plane: plane object
> + * @supported_encodings: bitfield indicating supported color encodings
> + * @supported_ranges: bitfileld indicating supported color ranges
> + * @default_encoding: default color encoding
> + * @default_range: default color range
> + *
> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> + * properties to @plane. The supported encodings and ranges should
> + * be provided in supported_encodings and supported_ranges bitmasks.
> + * Each bit set in the bitmask indicates that its number as enum
> + * value is supported.
> + */
> +int drm_plane_create_color_properties(struct drm_plane *plane,
> +				      u32 supported_encodings,
> +				      u32 supported_ranges,
> +				      enum drm_color_encoding default_encoding,
> +				      enum drm_color_range default_range)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> +						DRM_COLOR_RANGE_MAX)];
> +	int i, len;
> +
> +	if (WARN_ON(supported_encodings == 0 ||
> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
> +		    (supported_encodings & BIT(default_encoding)) == 0))
> +		return -EINVAL;
> +
> +	if (WARN_ON(supported_ranges == 0 ||
> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> +		    (supported_ranges & BIT(default_range)) == 0))
> +		return -EINVAL;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> +		if ((supported_encodings & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_encoding_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> +	if (plane->state)
> +		plane->state->color_encoding = default_encoding;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> +		if ((supported_ranges & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_range_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_range_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_range);
> +	if (plane->state)
> +		plane->state->color_range = default_range;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_properties);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cbce621..b3b6d302ca8c 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,7 @@
>   #include <linux/ctype.h>
>   
>   struct drm_crtc;
> +struct drm_plane;
>   
>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>   
> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>   				 int gamma_size);
>   
> +enum drm_color_encoding {
> +	DRM_COLOR_YCBCR_BT601,
> +	DRM_COLOR_YCBCR_BT709,
> +	DRM_COLOR_YCBCR_BT2020,
> +	DRM_COLOR_ENCODING_MAX,
> +};
> +
> +enum drm_color_range {
> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	DRM_COLOR_YCBCR_FULL_RANGE,
> +	DRM_COLOR_RANGE_MAX,
> +};
> +
> +int drm_plane_create_color_properties(struct drm_plane *plane,
> +				      u32 supported_encodings,
> +				      u32 supported_ranges,
> +				      enum drm_color_encoding default_encoding,
> +				      enum drm_color_range default_range);
>   #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..f7bf4a48b1c3 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -26,6 +26,7 @@
>   #include <linux/list.h>
>   #include <linux/ctype.h>
>   #include <drm/drm_mode_object.h>
> +#include <drm/drm_color_mgmt.h>
>   
>   struct drm_crtc;
>   struct drm_printer;
> @@ -112,6 +113,20 @@ struct drm_plane_state {
>   	unsigned int zpos;
>   	unsigned int normalized_zpos;
>   
> +	/**
> +	 * @color_encoding:
> +	 *
> +	 * Color encoding for non RGB formats
> +	 */
> +	enum drm_color_encoding color_encoding;
> +
> +	/**
> +	 * @color_range:
> +	 *
> +	 * Color range for non RGB formats
> +	 */
> +	enum drm_color_range color_range;
> +
>   	/* Clipped coordinates */
>   	struct drm_rect src, dst;
>   
> @@ -558,6 +573,23 @@ struct drm_plane {
>   
>   	struct drm_property *zpos_property;
>   	struct drm_property *rotation_property;
> +
> +	/**
> +	 * @color_encoding_property:
> +	 *
> +	 * Optional "COLOR_ENCODING" enum property for specifying
> +	 * color encoding for non RGB formats.
> +	 * See drm_plane_create_color_properties().
> +	 */
> +	struct drm_property *color_encoding_property;
> +	/**
> +	 * @color_range_property:
> +	 *
> +	 * Optional "COLOR_RANGE" enum property for specifying
> +	 * color range for non RGB formats.
> +	 * See drm_plane_create_color_properties().
> +	 */
> +	struct drm_property *color_range_property;
>   };
>   
>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)

On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
> Hi,
> 
> I am looking for a way to export the color encoding and range selection
> to user space. I came across those properties and am wondering, why
> they are meant only for non RGB color encodings. Would it be okay, to
> modify them and use with RGB formats as well?

What you trying to do? Input limited range RGB data and expand to full
range?

> 
> Regards,
> Chris
> 
> 
> On 02/19/2018 09:28 PM, Ville Syrjala wrote:
> > From: Jyri Sarha <jsarha@ti.com>
> >
> > Add a standard optional properties to support different non RGB color
> > encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> > color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> > the value ranges within the selected color encoding. The properties
> > are stored to drm_plane object to allow different set of supported
> > encoding for different planes on the device.
> >
> > v2: Add/fix kerneldocs, verify bitmasks (danvet)
> >
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >   drivers/gpu/drm/drm_atomic.c     |   8 +++
> >   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
> >   include/drm/drm_color_mgmt.h     |  19 ++++++++
> >   include/drm/drm_plane.h          |  32 ++++++++++++
> >   4 files changed, 162 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 46733d534587..452a0b0bafbc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >   		state->rotation = val;
> >   	} else if (property == plane->zpos_property) {
> >   		state->zpos = val;
> > +	} else if (property == plane->color_encoding_property) {
> > +		state->color_encoding = val;
> > +	} else if (property == plane->color_range_property) {
> > +		state->color_range = val;
> >   	} else if (plane->funcs->atomic_set_property) {
> >   		return plane->funcs->atomic_set_property(plane, state,
> >   				property, val);
> > @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >   		*val = state->rotation;
> >   	} else if (property == plane->zpos_property) {
> >   		*val = state->zpos;
> > +	} else if (property == plane->color_encoding_property) {
> > +		*val = state->color_encoding;
> > +	} else if (property == plane->color_range_property) {
> > +		*val = state->color_range;
> >   	} else if (plane->funcs->atomic_get_property) {
> >   		return plane->funcs->atomic_get_property(plane, state, property, val);
> >   	} else {
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 0d002b045bd2..4b83e078d3e9 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -88,6 +88,20 @@
> >    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
> >    * "GAMMA_LUT" property above.
> > + *
> > + * Support for different non RGB color encodings is controlled through
> > + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
> > + * are set up by calling drm_plane_create_color_properties().
> > + *
> > + * "COLOR_ENCODING"
> > + * 	Optional plane enum property to support different non RGB
> > + * 	color encodings. The driver can provide a subset of standard
> > + * 	enum values supported by the DRM plane.
> > + *
> > + * "COLOR_RANGE"
> > + * 	Optional plane enum property to support different non RGB
> > + * 	color parameter ranges. The driver can provide a subset of
> > + * 	standard enum values supported by the DRM plane.
> >    */
> >   
> >   /**
> > @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >   	drm_modeset_unlock(&crtc->mutex);
> >   	return ret;
> >   }
> > +
> > +static const char * const color_encoding_name[] = {
> > +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> > +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> > +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> > +};
> > +
> > +static const char * const color_range_name[] = {
> > +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> > +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> > +};
> > +
> > +/**
> > + * drm_plane_create_color_properties - color encoding related plane properties
> > + * @plane: plane object
> > + * @supported_encodings: bitfield indicating supported color encodings
> > + * @supported_ranges: bitfileld indicating supported color ranges
> > + * @default_encoding: default color encoding
> > + * @default_range: default color range
> > + *
> > + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> > + * properties to @plane. The supported encodings and ranges should
> > + * be provided in supported_encodings and supported_ranges bitmasks.
> > + * Each bit set in the bitmask indicates that its number as enum
> > + * value is supported.
> > + */
> > +int drm_plane_create_color_properties(struct drm_plane *plane,
> > +				      u32 supported_encodings,
> > +				      u32 supported_ranges,
> > +				      enum drm_color_encoding default_encoding,
> > +				      enum drm_color_range default_range)
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct drm_property *prop;
> > +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> > +						DRM_COLOR_RANGE_MAX)];
> > +	int i, len;
> > +
> > +	if (WARN_ON(supported_encodings == 0 ||
> > +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
> > +		    (supported_encodings & BIT(default_encoding)) == 0))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(supported_ranges == 0 ||
> > +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> > +		    (supported_ranges & BIT(default_range)) == 0))
> > +		return -EINVAL;
> > +
> > +	len = 0;
> > +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> > +		if ((supported_encodings & BIT(i)) == 0)
> > +			continue;
> > +
> > +		enum_list[len].type = i;
> > +		enum_list[len].name = color_encoding_name[i];
> > +		len++;
> > +	}
> > +
> > +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> > +					enum_list, len);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	plane->color_encoding_property = prop;
> > +	drm_object_attach_property(&plane->base, prop, default_encoding);
> > +	if (plane->state)
> > +		plane->state->color_encoding = default_encoding;
> > +
> > +	len = 0;
> > +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> > +		if ((supported_ranges & BIT(i)) == 0)
> > +			continue;
> > +
> > +		enum_list[len].type = i;
> > +		enum_list[len].name = color_range_name[i];
> > +		len++;
> > +	}
> > +
> > +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> > +					enum_list, len);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	plane->color_range_property = prop;
> > +	drm_object_attach_property(&plane->base, prop, default_range);
> > +	if (plane->state)
> > +		plane->state->color_range = default_range;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_plane_create_color_properties);
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 03a59cbce621..b3b6d302ca8c 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -26,6 +26,7 @@
> >   #include <linux/ctype.h>
> >   
> >   struct drm_crtc;
> > +struct drm_plane;
> >   
> >   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> >   
> > @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> >   				 int gamma_size);
> >   
> > +enum drm_color_encoding {
> > +	DRM_COLOR_YCBCR_BT601,
> > +	DRM_COLOR_YCBCR_BT709,
> > +	DRM_COLOR_YCBCR_BT2020,
> > +	DRM_COLOR_ENCODING_MAX,
> > +};
> > +
> > +enum drm_color_range {
> > +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> > +	DRM_COLOR_YCBCR_FULL_RANGE,
> > +	DRM_COLOR_RANGE_MAX,
> > +};
> > +
> > +int drm_plane_create_color_properties(struct drm_plane *plane,
> > +				      u32 supported_encodings,
> > +				      u32 supported_ranges,
> > +				      enum drm_color_encoding default_encoding,
> > +				      enum drm_color_range default_range);
> >   #endif
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8185e3468a23..f7bf4a48b1c3 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -26,6 +26,7 @@
> >   #include <linux/list.h>
> >   #include <linux/ctype.h>
> >   #include <drm/drm_mode_object.h>
> > +#include <drm/drm_color_mgmt.h>
> >   
> >   struct drm_crtc;
> >   struct drm_printer;
> > @@ -112,6 +113,20 @@ struct drm_plane_state {
> >   	unsigned int zpos;
> >   	unsigned int normalized_zpos;
> >   
> > +	/**
> > +	 * @color_encoding:
> > +	 *
> > +	 * Color encoding for non RGB formats
> > +	 */
> > +	enum drm_color_encoding color_encoding;
> > +
> > +	/**
> > +	 * @color_range:
> > +	 *
> > +	 * Color range for non RGB formats
> > +	 */
> > +	enum drm_color_range color_range;
> > +
> >   	/* Clipped coordinates */
> >   	struct drm_rect src, dst;
> >   
> > @@ -558,6 +573,23 @@ struct drm_plane {
> >   
> >   	struct drm_property *zpos_property;
> >   	struct drm_property *rotation_property;
> > +
> > +	/**
> > +	 * @color_encoding_property:
> > +	 *
> > +	 * Optional "COLOR_ENCODING" enum property for specifying
> > +	 * color encoding for non RGB formats.
> > +	 * See drm_plane_create_color_properties().
> > +	 */
> > +	struct drm_property *color_encoding_property;
> > +	/**
> > +	 * @color_range_property:
> > +	 *
> > +	 * Optional "COLOR_RANGE" enum property for specifying
> > +	 * color range for non RGB formats.
> > +	 * See drm_plane_create_color_properties().
> > +	 */
> > +	struct drm_property *color_range_property;
> >   };
> >   
> >   #define obj_to_plane(x) container_of(x, struct drm_plane, base)

Hi Ville,

As Christoph cannot respond till middle next week I can try to respond
in his absence, as I am familiar with the subject.

On 30.11.2018 14:25, Ville Syrjälä wrote:
> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>> Hi,
>>
>> I am looking for a way to export the color encoding and range selection
>> to user space. I came across those properties and am wondering, why
>> they are meant only for non RGB color encodings. Would it be okay, to
>> modify them and use with RGB formats as well?
> What you trying to do? Input limited range RGB data and expand to full
> range?


For example. But there are two more general questions, which
surprisingly we have not found answer for.

1. What color encoding and range drm should expect on its input RGB
buffers by default?

2. How userspace should inform drm that given buffer has specified
non-default color encoding and range?


Hopefully this patch introduces such properties but only for YCbCr
formats, the question is what should be the best way to expand it to RGB
formats:

A. Add another enums: DRM_COLOR_RGB_BT601 and friends.

B. Reuse current enums, but remove format information from them:
DRM_COLOR_YCBCR_BT601 => DRM_COLOR_BT601.


Regards

Andrzej

>
>> Regards,
>> Chris
>>
>>
>> On 02/19/2018 09:28 PM, Ville Syrjala wrote:
>>> From: Jyri Sarha <jsarha@ti.com>
>>>
>>> Add a standard optional properties to support different non RGB color
>>> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
>>> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
>>> the value ranges within the selected color encoding. The properties
>>> are stored to drm_plane object to allow different set of supported
>>> encoding for different planes on the device.
>>>
>>> v2: Add/fix kerneldocs, verify bitmasks (danvet)
>>>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Daniel Stone <daniel@fooishbar.org>
>>> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>   drivers/gpu/drm/drm_atomic.c     |   8 +++
>>>   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_color_mgmt.h     |  19 ++++++++
>>>   include/drm/drm_plane.h          |  32 ++++++++++++
>>>   4 files changed, 162 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 46733d534587..452a0b0bafbc 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>   		state->rotation = val;
>>>   	} else if (property == plane->zpos_property) {
>>>   		state->zpos = val;
>>> +	} else if (property == plane->color_encoding_property) {
>>> +		state->color_encoding = val;
>>> +	} else if (property == plane->color_range_property) {
>>> +		state->color_range = val;
>>>   	} else if (plane->funcs->atomic_set_property) {
>>>   		return plane->funcs->atomic_set_property(plane, state,
>>>   				property, val);
>>> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>   		*val = state->rotation;
>>>   	} else if (property == plane->zpos_property) {
>>>   		*val = state->zpos;
>>> +	} else if (property == plane->color_encoding_property) {
>>> +		*val = state->color_encoding;
>>> +	} else if (property == plane->color_range_property) {
>>> +		*val = state->color_range;
>>>   	} else if (plane->funcs->atomic_get_property) {
>>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>>   	} else {
>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>> index 0d002b045bd2..4b83e078d3e9 100644
>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>> @@ -88,6 +88,20 @@
>>>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>>>    * "GAMMA_LUT" property above.
>>> + *
>>> + * Support for different non RGB color encodings is controlled through
>>> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
>>> + * are set up by calling drm_plane_create_color_properties().
>>> + *
>>> + * "COLOR_ENCODING"
>>> + * 	Optional plane enum property to support different non RGB
>>> + * 	color encodings. The driver can provide a subset of standard
>>> + * 	enum values supported by the DRM plane.
>>> + *
>>> + * "COLOR_RANGE"
>>> + * 	Optional plane enum property to support different non RGB
>>> + * 	color parameter ranges. The driver can provide a subset of
>>> + * 	standard enum values supported by the DRM plane.
>>>    */
>>>   
>>>   /**
>>> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>>   	drm_modeset_unlock(&crtc->mutex);
>>>   	return ret;
>>>   }
>>> +
>>> +static const char * const color_encoding_name[] = {
>>> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
>>> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
>>> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
>>> +};
>>> +
>>> +static const char * const color_range_name[] = {
>>> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
>>> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
>>> +};
>>> +
>>> +/**
>>> + * drm_plane_create_color_properties - color encoding related plane properties
>>> + * @plane: plane object
>>> + * @supported_encodings: bitfield indicating supported color encodings
>>> + * @supported_ranges: bitfileld indicating supported color ranges
>>> + * @default_encoding: default color encoding
>>> + * @default_range: default color range
>>> + *
>>> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
>>> + * properties to @plane. The supported encodings and ranges should
>>> + * be provided in supported_encodings and supported_ranges bitmasks.
>>> + * Each bit set in the bitmask indicates that its number as enum
>>> + * value is supported.
>>> + */
>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>> +				      u32 supported_encodings,
>>> +				      u32 supported_ranges,
>>> +				      enum drm_color_encoding default_encoding,
>>> +				      enum drm_color_range default_range)
>>> +{
>>> +	struct drm_device *dev = plane->dev;
>>> +	struct drm_property *prop;
>>> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
>>> +						DRM_COLOR_RANGE_MAX)];
>>> +	int i, len;
>>> +
>>> +	if (WARN_ON(supported_encodings == 0 ||
>>> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
>>> +		    (supported_encodings & BIT(default_encoding)) == 0))
>>> +		return -EINVAL;
>>> +
>>> +	if (WARN_ON(supported_ranges == 0 ||
>>> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
>>> +		    (supported_ranges & BIT(default_range)) == 0))
>>> +		return -EINVAL;
>>> +
>>> +	len = 0;
>>> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
>>> +		if ((supported_encodings & BIT(i)) == 0)
>>> +			continue;
>>> +
>>> +		enum_list[len].type = i;
>>> +		enum_list[len].name = color_encoding_name[i];
>>> +		len++;
>>> +	}
>>> +
>>> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
>>> +					enum_list, len);
>>> +	if (!prop)
>>> +		return -ENOMEM;
>>> +	plane->color_encoding_property = prop;
>>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
>>> +	if (plane->state)
>>> +		plane->state->color_encoding = default_encoding;
>>> +
>>> +	len = 0;
>>> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
>>> +		if ((supported_ranges & BIT(i)) == 0)
>>> +			continue;
>>> +
>>> +		enum_list[len].type = i;
>>> +		enum_list[len].name = color_range_name[i];
>>> +		len++;
>>> +	}
>>> +
>>> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
>>> +					enum_list, len);
>>> +	if (!prop)
>>> +		return -ENOMEM;
>>> +	plane->color_range_property = prop;
>>> +	drm_object_attach_property(&plane->base, prop, default_range);
>>> +	if (plane->state)
>>> +		plane->state->color_range = default_range;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_plane_create_color_properties);
>>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>>> index 03a59cbce621..b3b6d302ca8c 100644
>>> --- a/include/drm/drm_color_mgmt.h
>>> +++ b/include/drm/drm_color_mgmt.h
>>> @@ -26,6 +26,7 @@
>>>   #include <linux/ctype.h>
>>>   
>>>   struct drm_crtc;
>>> +struct drm_plane;
>>>   
>>>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>>   
>>> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>>>   				 int gamma_size);
>>>   
>>> +enum drm_color_encoding {
>>> +	DRM_COLOR_YCBCR_BT601,
>>> +	DRM_COLOR_YCBCR_BT709,
>>> +	DRM_COLOR_YCBCR_BT2020,
>>> +	DRM_COLOR_ENCODING_MAX,
>>> +};
>>> +
>>> +enum drm_color_range {
>>> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
>>> +	DRM_COLOR_YCBCR_FULL_RANGE,
>>> +	DRM_COLOR_RANGE_MAX,
>>> +};
>>> +
>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>> +				      u32 supported_encodings,
>>> +				      u32 supported_ranges,
>>> +				      enum drm_color_encoding default_encoding,
>>> +				      enum drm_color_range default_range);
>>>   #endif
>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>> index 8185e3468a23..f7bf4a48b1c3 100644
>>> --- a/include/drm/drm_plane.h
>>> +++ b/include/drm/drm_plane.h
>>> @@ -26,6 +26,7 @@
>>>   #include <linux/list.h>
>>>   #include <linux/ctype.h>
>>>   #include <drm/drm_mode_object.h>
>>> +#include <drm/drm_color_mgmt.h>
>>>   
>>>   struct drm_crtc;
>>>   struct drm_printer;
>>> @@ -112,6 +113,20 @@ struct drm_plane_state {
>>>   	unsigned int zpos;
>>>   	unsigned int normalized_zpos;
>>>   
>>> +	/**
>>> +	 * @color_encoding:
>>> +	 *
>>> +	 * Color encoding for non RGB formats
>>> +	 */
>>> +	enum drm_color_encoding color_encoding;
>>> +
>>> +	/**
>>> +	 * @color_range:
>>> +	 *
>>> +	 * Color range for non RGB formats
>>> +	 */
>>> +	enum drm_color_range color_range;
>>> +
>>>   	/* Clipped coordinates */
>>>   	struct drm_rect src, dst;
>>>   
>>> @@ -558,6 +573,23 @@ struct drm_plane {
>>>   
>>>   	struct drm_property *zpos_property;
>>>   	struct drm_property *rotation_property;
>>> +
>>> +	/**
>>> +	 * @color_encoding_property:
>>> +	 *
>>> +	 * Optional "COLOR_ENCODING" enum property for specifying
>>> +	 * color encoding for non RGB formats.
>>> +	 * See drm_plane_create_color_properties().
>>> +	 */
>>> +	struct drm_property *color_encoding_property;
>>> +	/**
>>> +	 * @color_range_property:
>>> +	 *
>>> +	 * Optional "COLOR_RANGE" enum property for specifying
>>> +	 * color range for non RGB formats.
>>> +	 * See drm_plane_create_color_properties().
>>> +	 */
>>> +	struct drm_property *color_range_property;
>>>   };
>>>   
>>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)

On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
> Hi Ville,
> 
> As Christoph cannot respond till middle next week I can try to respond
> in his absence, as I am familiar with the subject.
> 
> On 30.11.2018 14:25, Ville Syrjälä wrote:
> > On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
> >> Hi,
> >>
> >> I am looking for a way to export the color encoding and range selection
> >> to user space. I came across those properties and am wondering, why
> >> they are meant only for non RGB color encodings. Would it be okay, to
> >> modify them and use with RGB formats as well?
> > What you trying to do? Input limited range RGB data and expand to full
> > range?
> 
> 
> For example. But there are two more general questions, which
> surprisingly we have not found answer for.
> 
> 1. What color encoding and range drm should expect on its input RGB
> buffers by default?

RGB is just RGB. There is no encoding. It's assumed to be full range
because no one really uses anything else.

> 
> 2. How userspace should inform drm that given buffer has specified
> non-default color encoding and range?
> 
> 
> Hopefully this patch introduces such properties but only for YCbCr
> formats, the question is what should be the best way to expand it to RGB
> formats:
> 
> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.

BT.601 specifies how to encoder RGB data as YCbCr. So without
YCbCr BT.601 does not mean anything. Well, the standard does
contain other things as well I suppose, but for the purposes
of the color encoding prop only that one part is relevant.

> 
> B. Reuse current enums, but remove format information from them:
> DRM_COLOR_YCBCR_BT601 => DRM_COLOR_BT601.
> 
> 
> Regards
> 
> Andrzej
> 
> >
> >> Regards,
> >> Chris
> >>
> >>
> >> On 02/19/2018 09:28 PM, Ville Syrjala wrote:
> >>> From: Jyri Sarha <jsarha@ti.com>
> >>>
> >>> Add a standard optional properties to support different non RGB color
> >>> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> >>> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> >>> the value ranges within the selected color encoding. The properties
> >>> are stored to drm_plane object to allow different set of supported
> >>> encoding for different planes on the device.
> >>>
> >>> v2: Add/fix kerneldocs, verify bitmasks (danvet)
> >>>
> >>> Cc: Harry Wentland <harry.wentland@amd.com>
> >>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>> Cc: Daniel Stone <daniel@fooishbar.org>
> >>> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> >>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> >>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> >>> Cc: Uma Shankar <uma.shankar@intel.com>
> >>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> ---
> >>>   drivers/gpu/drm/drm_atomic.c     |   8 +++
> >>>   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
> >>>   include/drm/drm_color_mgmt.h     |  19 ++++++++
> >>>   include/drm/drm_plane.h          |  32 ++++++++++++
> >>>   4 files changed, 162 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 46733d534587..452a0b0bafbc 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >>>   		state->rotation = val;
> >>>   	} else if (property == plane->zpos_property) {
> >>>   		state->zpos = val;
> >>> +	} else if (property == plane->color_encoding_property) {
> >>> +		state->color_encoding = val;
> >>> +	} else if (property == plane->color_range_property) {
> >>> +		state->color_range = val;
> >>>   	} else if (plane->funcs->atomic_set_property) {
> >>>   		return plane->funcs->atomic_set_property(plane, state,
> >>>   				property, val);
> >>> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >>>   		*val = state->rotation;
> >>>   	} else if (property == plane->zpos_property) {
> >>>   		*val = state->zpos;
> >>> +	} else if (property == plane->color_encoding_property) {
> >>> +		*val = state->color_encoding;
> >>> +	} else if (property == plane->color_range_property) {
> >>> +		*val = state->color_range;
> >>>   	} else if (plane->funcs->atomic_get_property) {
> >>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
> >>>   	} else {
> >>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> >>> index 0d002b045bd2..4b83e078d3e9 100644
> >>> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >>> @@ -88,6 +88,20 @@
> >>>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >>>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
> >>>    * "GAMMA_LUT" property above.
> >>> + *
> >>> + * Support for different non RGB color encodings is controlled through
> >>> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
> >>> + * are set up by calling drm_plane_create_color_properties().
> >>> + *
> >>> + * "COLOR_ENCODING"
> >>> + * 	Optional plane enum property to support different non RGB
> >>> + * 	color encodings. The driver can provide a subset of standard
> >>> + * 	enum values supported by the DRM plane.
> >>> + *
> >>> + * "COLOR_RANGE"
> >>> + * 	Optional plane enum property to support different non RGB
> >>> + * 	color parameter ranges. The driver can provide a subset of
> >>> + * 	standard enum values supported by the DRM plane.
> >>>    */
> >>>   
> >>>   /**
> >>> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >>>   	drm_modeset_unlock(&crtc->mutex);
> >>>   	return ret;
> >>>   }
> >>> +
> >>> +static const char * const color_encoding_name[] = {
> >>> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> >>> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> >>> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> >>> +};
> >>> +
> >>> +static const char * const color_range_name[] = {
> >>> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> >>> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> >>> +};
> >>> +
> >>> +/**
> >>> + * drm_plane_create_color_properties - color encoding related plane properties
> >>> + * @plane: plane object
> >>> + * @supported_encodings: bitfield indicating supported color encodings
> >>> + * @supported_ranges: bitfileld indicating supported color ranges
> >>> + * @default_encoding: default color encoding
> >>> + * @default_range: default color range
> >>> + *
> >>> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> >>> + * properties to @plane. The supported encodings and ranges should
> >>> + * be provided in supported_encodings and supported_ranges bitmasks.
> >>> + * Each bit set in the bitmask indicates that its number as enum
> >>> + * value is supported.
> >>> + */
> >>> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >>> +				      u32 supported_encodings,
> >>> +				      u32 supported_ranges,
> >>> +				      enum drm_color_encoding default_encoding,
> >>> +				      enum drm_color_range default_range)
> >>> +{
> >>> +	struct drm_device *dev = plane->dev;
> >>> +	struct drm_property *prop;
> >>> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> >>> +						DRM_COLOR_RANGE_MAX)];
> >>> +	int i, len;
> >>> +
> >>> +	if (WARN_ON(supported_encodings == 0 ||
> >>> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
> >>> +		    (supported_encodings & BIT(default_encoding)) == 0))
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (WARN_ON(supported_ranges == 0 ||
> >>> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> >>> +		    (supported_ranges & BIT(default_range)) == 0))
> >>> +		return -EINVAL;
> >>> +
> >>> +	len = 0;
> >>> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> >>> +		if ((supported_encodings & BIT(i)) == 0)
> >>> +			continue;
> >>> +
> >>> +		enum_list[len].type = i;
> >>> +		enum_list[len].name = color_encoding_name[i];
> >>> +		len++;
> >>> +	}
> >>> +
> >>> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> >>> +					enum_list, len);
> >>> +	if (!prop)
> >>> +		return -ENOMEM;
> >>> +	plane->color_encoding_property = prop;
> >>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> >>> +	if (plane->state)
> >>> +		plane->state->color_encoding = default_encoding;
> >>> +
> >>> +	len = 0;
> >>> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> >>> +		if ((supported_ranges & BIT(i)) == 0)
> >>> +			continue;
> >>> +
> >>> +		enum_list[len].type = i;
> >>> +		enum_list[len].name = color_range_name[i];
> >>> +		len++;
> >>> +	}
> >>> +
> >>> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> >>> +					enum_list, len);
> >>> +	if (!prop)
> >>> +		return -ENOMEM;
> >>> +	plane->color_range_property = prop;
> >>> +	drm_object_attach_property(&plane->base, prop, default_range);
> >>> +	if (plane->state)
> >>> +		plane->state->color_range = default_range;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_plane_create_color_properties);
> >>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> >>> index 03a59cbce621..b3b6d302ca8c 100644
> >>> --- a/include/drm/drm_color_mgmt.h
> >>> +++ b/include/drm/drm_color_mgmt.h
> >>> @@ -26,6 +26,7 @@
> >>>   #include <linux/ctype.h>
> >>>   
> >>>   struct drm_crtc;
> >>> +struct drm_plane;
> >>>   
> >>>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> >>>   
> >>> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >>>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> >>>   				 int gamma_size);
> >>>   
> >>> +enum drm_color_encoding {
> >>> +	DRM_COLOR_YCBCR_BT601,
> >>> +	DRM_COLOR_YCBCR_BT709,
> >>> +	DRM_COLOR_YCBCR_BT2020,
> >>> +	DRM_COLOR_ENCODING_MAX,
> >>> +};
> >>> +
> >>> +enum drm_color_range {
> >>> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> >>> +	DRM_COLOR_YCBCR_FULL_RANGE,
> >>> +	DRM_COLOR_RANGE_MAX,
> >>> +};
> >>> +
> >>> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >>> +				      u32 supported_encodings,
> >>> +				      u32 supported_ranges,
> >>> +				      enum drm_color_encoding default_encoding,
> >>> +				      enum drm_color_range default_range);
> >>>   #endif
> >>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >>> index 8185e3468a23..f7bf4a48b1c3 100644
> >>> --- a/include/drm/drm_plane.h
> >>> +++ b/include/drm/drm_plane.h
> >>> @@ -26,6 +26,7 @@
> >>>   #include <linux/list.h>
> >>>   #include <linux/ctype.h>
> >>>   #include <drm/drm_mode_object.h>
> >>> +#include <drm/drm_color_mgmt.h>
> >>>   
> >>>   struct drm_crtc;
> >>>   struct drm_printer;
> >>> @@ -112,6 +113,20 @@ struct drm_plane_state {
> >>>   	unsigned int zpos;
> >>>   	unsigned int normalized_zpos;
> >>>   
> >>> +	/**
> >>> +	 * @color_encoding:
> >>> +	 *
> >>> +	 * Color encoding for non RGB formats
> >>> +	 */
> >>> +	enum drm_color_encoding color_encoding;
> >>> +
> >>> +	/**
> >>> +	 * @color_range:
> >>> +	 *
> >>> +	 * Color range for non RGB formats
> >>> +	 */
> >>> +	enum drm_color_range color_range;
> >>> +
> >>>   	/* Clipped coordinates */
> >>>   	struct drm_rect src, dst;
> >>>   
> >>> @@ -558,6 +573,23 @@ struct drm_plane {
> >>>   
> >>>   	struct drm_property *zpos_property;
> >>>   	struct drm_property *rotation_property;
> >>> +
> >>> +	/**
> >>> +	 * @color_encoding_property:
> >>> +	 *
> >>> +	 * Optional "COLOR_ENCODING" enum property for specifying
> >>> +	 * color encoding for non RGB formats.
> >>> +	 * See drm_plane_create_color_properties().
> >>> +	 */
> >>> +	struct drm_property *color_encoding_property;
> >>> +	/**
> >>> +	 * @color_range_property:
> >>> +	 *
> >>> +	 * Optional "COLOR_RANGE" enum property for specifying
> >>> +	 * color range for non RGB formats.
> >>> +	 * See drm_plane_create_color_properties().
> >>> +	 */
> >>> +	struct drm_property *color_range_property;
> >>>   };
> >>>   
> >>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>

On 11/30/18 15:20, Andrzej Hajda wrote:
> Hi Ville,
> 
> As Christoph cannot respond till middle next week I can try to respond
> in his absence, as I am familiar with the subject.
> 
> On 30.11.2018 14:25, Ville Syrjälä wrote:
>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>> Hi,
>>>
>>> I am looking for a way to export the color encoding and range selection
>>> to user space. I came across those properties and am wondering, why
>>> they are meant only for non RGB color encodings. Would it be okay, to
>>> modify them and use with RGB formats as well?
>> What you trying to do? Input limited range RGB data and expand to full
>> range?
> 
> 
> For example. But there are two more general questions, which
> surprisingly we have not found answer for.
> 
> 1. What color encoding and range drm should expect on its input RGB
> buffers by default?

While I am not a drm expert, I am pretty certain it always expects
full range RGB.

There is a real use-case for being able to give drm limited range RGB:
if the image was filled from an HDMI receiver and that receiver got
limited range RGB. That said, most (but not all) receivers can expand
it to full range before writing to memory.

> 2. How userspace should inform drm that given buffer has specified
> non-default color encoding and range?
> 
> 
> Hopefully this patch introduces such properties but only for YCbCr
> formats, the question is what should be the best way to expand it to RGB
> formats:
> 
> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
> 
> B. Reuse current enums, but remove format information from them:
> DRM_COLOR_YCBCR_BT601 => DRM_COLOR_BT601.

The colorspace (BT601, REC709, BT2020) is independent of the quantization
range (full/limited) and of the color encoding (YCbCr, RGB). There is also
the transfer function, another independent setting.

More background info is here:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html

If all you use is a desktop, then it is all simple: full range sRGB. But if
you start mixing video captured from an HDMI receiver, then it can become
much more complex.

Regards,

	Hans

> 
> 
> Regards
> 
> Andrzej
> 
>>
>>> Regards,
>>> Chris
>>>
>>>
>>> On 02/19/2018 09:28 PM, Ville Syrjala wrote:
>>>> From: Jyri Sarha <jsarha@ti.com>
>>>>
>>>> Add a standard optional properties to support different non RGB color
>>>> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
>>>> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
>>>> the value ranges within the selected color encoding. The properties
>>>> are stored to drm_plane object to allow different set of supported
>>>> encoding for different planes on the device.
>>>>
>>>> v2: Add/fix kerneldocs, verify bitmasks (danvet)
>>>>
>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: Daniel Stone <daniel@fooishbar.org>
>>>> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
>>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic.c     |   8 +++
>>>>   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
>>>>   include/drm/drm_color_mgmt.h     |  19 ++++++++
>>>>   include/drm/drm_plane.h          |  32 ++++++++++++
>>>>   4 files changed, 162 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index 46733d534587..452a0b0bafbc 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>   		state->rotation = val;
>>>>   	} else if (property == plane->zpos_property) {
>>>>   		state->zpos = val;
>>>> +	} else if (property == plane->color_encoding_property) {
>>>> +		state->color_encoding = val;
>>>> +	} else if (property == plane->color_range_property) {
>>>> +		state->color_range = val;
>>>>   	} else if (plane->funcs->atomic_set_property) {
>>>>   		return plane->funcs->atomic_set_property(plane, state,
>>>>   				property, val);
>>>> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>   		*val = state->rotation;
>>>>   	} else if (property == plane->zpos_property) {
>>>>   		*val = state->zpos;
>>>> +	} else if (property == plane->color_encoding_property) {
>>>> +		*val = state->color_encoding;
>>>> +	} else if (property == plane->color_range_property) {
>>>> +		*val = state->color_range;
>>>>   	} else if (plane->funcs->atomic_get_property) {
>>>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>>>   	} else {
>>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>>> index 0d002b045bd2..4b83e078d3e9 100644
>>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>>> @@ -88,6 +88,20 @@
>>>>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>>>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>>>>    * "GAMMA_LUT" property above.
>>>> + *
>>>> + * Support for different non RGB color encodings is controlled through
>>>> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
>>>> + * are set up by calling drm_plane_create_color_properties().
>>>> + *
>>>> + * "COLOR_ENCODING"
>>>> + * 	Optional plane enum property to support different non RGB
>>>> + * 	color encodings. The driver can provide a subset of standard
>>>> + * 	enum values supported by the DRM plane.
>>>> + *
>>>> + * "COLOR_RANGE"
>>>> + * 	Optional plane enum property to support different non RGB
>>>> + * 	color parameter ranges. The driver can provide a subset of
>>>> + * 	standard enum values supported by the DRM plane.
>>>>    */
>>>>   
>>>>   /**
>>>> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>>>   	drm_modeset_unlock(&crtc->mutex);
>>>>   	return ret;
>>>>   }
>>>> +
>>>> +static const char * const color_encoding_name[] = {
>>>> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
>>>> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
>>>> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
>>>> +};
>>>> +
>>>> +static const char * const color_range_name[] = {
>>>> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
>>>> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
>>>> +};
>>>> +
>>>> +/**
>>>> + * drm_plane_create_color_properties - color encoding related plane properties
>>>> + * @plane: plane object
>>>> + * @supported_encodings: bitfield indicating supported color encodings
>>>> + * @supported_ranges: bitfileld indicating supported color ranges
>>>> + * @default_encoding: default color encoding
>>>> + * @default_range: default color range
>>>> + *
>>>> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
>>>> + * properties to @plane. The supported encodings and ranges should
>>>> + * be provided in supported_encodings and supported_ranges bitmasks.
>>>> + * Each bit set in the bitmask indicates that its number as enum
>>>> + * value is supported.
>>>> + */
>>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>>> +				      u32 supported_encodings,
>>>> +				      u32 supported_ranges,
>>>> +				      enum drm_color_encoding default_encoding,
>>>> +				      enum drm_color_range default_range)
>>>> +{
>>>> +	struct drm_device *dev = plane->dev;
>>>> +	struct drm_property *prop;
>>>> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
>>>> +						DRM_COLOR_RANGE_MAX)];
>>>> +	int i, len;
>>>> +
>>>> +	if (WARN_ON(supported_encodings == 0 ||
>>>> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
>>>> +		    (supported_encodings & BIT(default_encoding)) == 0))
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (WARN_ON(supported_ranges == 0 ||
>>>> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
>>>> +		    (supported_ranges & BIT(default_range)) == 0))
>>>> +		return -EINVAL;
>>>> +
>>>> +	len = 0;
>>>> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
>>>> +		if ((supported_encodings & BIT(i)) == 0)
>>>> +			continue;
>>>> +
>>>> +		enum_list[len].type = i;
>>>> +		enum_list[len].name = color_encoding_name[i];
>>>> +		len++;
>>>> +	}
>>>> +
>>>> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
>>>> +					enum_list, len);
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	plane->color_encoding_property = prop;
>>>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
>>>> +	if (plane->state)
>>>> +		plane->state->color_encoding = default_encoding;
>>>> +
>>>> +	len = 0;
>>>> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
>>>> +		if ((supported_ranges & BIT(i)) == 0)
>>>> +			continue;
>>>> +
>>>> +		enum_list[len].type = i;
>>>> +		enum_list[len].name = color_range_name[i];
>>>> +		len++;
>>>> +	}
>>>> +
>>>> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
>>>> +					enum_list, len);
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	plane->color_range_property = prop;
>>>> +	drm_object_attach_property(&plane->base, prop, default_range);
>>>> +	if (plane->state)
>>>> +		plane->state->color_range = default_range;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_plane_create_color_properties);
>>>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>>>> index 03a59cbce621..b3b6d302ca8c 100644
>>>> --- a/include/drm/drm_color_mgmt.h
>>>> +++ b/include/drm/drm_color_mgmt.h
>>>> @@ -26,6 +26,7 @@
>>>>   #include <linux/ctype.h>
>>>>   
>>>>   struct drm_crtc;
>>>> +struct drm_plane;
>>>>   
>>>>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>>>   
>>>> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>>>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>>>>   				 int gamma_size);
>>>>   
>>>> +enum drm_color_encoding {
>>>> +	DRM_COLOR_YCBCR_BT601,
>>>> +	DRM_COLOR_YCBCR_BT709,
>>>> +	DRM_COLOR_YCBCR_BT2020,
>>>> +	DRM_COLOR_ENCODING_MAX,
>>>> +};
>>>> +
>>>> +enum drm_color_range {
>>>> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
>>>> +	DRM_COLOR_YCBCR_FULL_RANGE,
>>>> +	DRM_COLOR_RANGE_MAX,
>>>> +};
>>>> +
>>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>>> +				      u32 supported_encodings,
>>>> +				      u32 supported_ranges,
>>>> +				      enum drm_color_encoding default_encoding,
>>>> +				      enum drm_color_range default_range);
>>>>   #endif
>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>> index 8185e3468a23..f7bf4a48b1c3 100644
>>>> --- a/include/drm/drm_plane.h
>>>> +++ b/include/drm/drm_plane.h
>>>> @@ -26,6 +26,7 @@
>>>>   #include <linux/list.h>
>>>>   #include <linux/ctype.h>
>>>>   #include <drm/drm_mode_object.h>
>>>> +#include <drm/drm_color_mgmt.h>
>>>>   
>>>>   struct drm_crtc;
>>>>   struct drm_printer;
>>>> @@ -112,6 +113,20 @@ struct drm_plane_state {
>>>>   	unsigned int zpos;
>>>>   	unsigned int normalized_zpos;
>>>>   
>>>> +	/**
>>>> +	 * @color_encoding:
>>>> +	 *
>>>> +	 * Color encoding for non RGB formats
>>>> +	 */
>>>> +	enum drm_color_encoding color_encoding;
>>>> +
>>>> +	/**
>>>> +	 * @color_range:
>>>> +	 *
>>>> +	 * Color range for non RGB formats
>>>> +	 */
>>>> +	enum drm_color_range color_range;
>>>> +
>>>>   	/* Clipped coordinates */
>>>>   	struct drm_rect src, dst;
>>>>   
>>>> @@ -558,6 +573,23 @@ struct drm_plane {
>>>>   
>>>>   	struct drm_property *zpos_property;
>>>>   	struct drm_property *rotation_property;
>>>> +
>>>> +	/**
>>>> +	 * @color_encoding_property:
>>>> +	 *
>>>> +	 * Optional "COLOR_ENCODING" enum property for specifying
>>>> +	 * color encoding for non RGB formats.
>>>> +	 * See drm_plane_create_color_properties().
>>>> +	 */
>>>> +	struct drm_property *color_encoding_property;
>>>> +	/**
>>>> +	 * @color_range_property:
>>>> +	 *
>>>> +	 * Optional "COLOR_RANGE" enum property for specifying
>>>> +	 * color range for non RGB formats.
>>>> +	 * See drm_plane_create_color_properties().
>>>> +	 */
>>>> +	struct drm_property *color_range_property;
>>>>   };
>>>>   
>>>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> 
>

On 11/30/18 15:29, Ville Syrjälä wrote:
> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
>> Hi Ville,
>>
>> As Christoph cannot respond till middle next week I can try to respond
>> in his absence, as I am familiar with the subject.
>>
>> On 30.11.2018 14:25, Ville Syrjälä wrote:
>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>>> Hi,
>>>>
>>>> I am looking for a way to export the color encoding and range selection
>>>> to user space. I came across those properties and am wondering, why
>>>> they are meant only for non RGB color encodings. Would it be okay, to
>>>> modify them and use with RGB formats as well?
>>> What you trying to do? Input limited range RGB data and expand to full
>>> range?
>>
>>
>> For example. But there are two more general questions, which
>> surprisingly we have not found answer for.
>>
>> 1. What color encoding and range drm should expect on its input RGB
>> buffers by default?
> 
> RGB is just RGB. There is no encoding. It's assumed to be full range
> because no one really uses anything else.

For simple desktop usage that's true. When dealing with video inputs,
this becomes much more complicated.

> 
>>
>> 2. How userspace should inform drm that given buffer has specified
>> non-default color encoding and range?
>>
>>
>> Hopefully this patch introduces such properties but only for YCbCr
>> formats, the question is what should be the best way to expand it to RGB
>> formats:
>>
>> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
> 
> BT.601 specifies how to encoder RGB data as YCbCr. So without
> YCbCr BT.601 does not mean anything. Well, the standard does
> contain other things as well I suppose, but for the purposes
> of the color encoding prop only that one part is relevant.

Ah, I misunderstood the meaning of DRM_COLOR_RGB_BT601.
This is the equivalent of V4L2_YCBCR_ENC_601, and that's indeed
only defined for Y'CbCr. But it is often (ab)used as an alias for
the SMPTE170M colorspace (used by SDTV).

V4L2 has the following defines for colorspaces, transfer functions,
Y'CbCr (and HSV) encodings and quantization ranges:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html

Missing in this list is the color encoding (RGB vs YCbCr vs HSV) which
is set through the pixelformat fourcc.

And indeed, we don't have an RGB encoding define since RGB is just RGB :-)

Regards,

	Hans

> 
>>
>> B. Reuse current enums, but remove format information from them:
>> DRM_COLOR_YCBCR_BT601 => DRM_COLOR_BT601.
>>
>>
>> Regards
>>
>> Andrzej
>>
>>>
>>>> Regards,
>>>> Chris
>>>>
>>>>
>>>> On 02/19/2018 09:28 PM, Ville Syrjala wrote:
>>>>> From: Jyri Sarha <jsarha@ti.com>
>>>>>
>>>>> Add a standard optional properties to support different non RGB color
>>>>> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
>>>>> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
>>>>> the value ranges within the selected color encoding. The properties
>>>>> are stored to drm_plane object to allow different set of supported
>>>>> encoding for different planes on the device.
>>>>>
>>>>> v2: Add/fix kerneldocs, verify bitmasks (danvet)
>>>>>
>>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: Daniel Stone <daniel@fooishbar.org>
>>>>> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
>>>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>>>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>>> [vsyrjala v2: Add/fix kerneldocs, verify bitmasks]
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic.c     |   8 +++
>>>>>   drivers/gpu/drm/drm_color_mgmt.c | 103 +++++++++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_color_mgmt.h     |  19 ++++++++
>>>>>   include/drm/drm_plane.h          |  32 ++++++++++++
>>>>>   4 files changed, 162 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index 46733d534587..452a0b0bafbc 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -759,6 +759,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>>   		state->rotation = val;
>>>>>   	} else if (property == plane->zpos_property) {
>>>>>   		state->zpos = val;
>>>>> +	} else if (property == plane->color_encoding_property) {
>>>>> +		state->color_encoding = val;
>>>>> +	} else if (property == plane->color_range_property) {
>>>>> +		state->color_range = val;
>>>>>   	} else if (plane->funcs->atomic_set_property) {
>>>>>   		return plane->funcs->atomic_set_property(plane, state,
>>>>>   				property, val);
>>>>> @@ -818,6 +822,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>>   		*val = state->rotation;
>>>>>   	} else if (property == plane->zpos_property) {
>>>>>   		*val = state->zpos;
>>>>> +	} else if (property == plane->color_encoding_property) {
>>>>> +		*val = state->color_encoding;
>>>>> +	} else if (property == plane->color_range_property) {
>>>>> +		*val = state->color_range;
>>>>>   	} else if (plane->funcs->atomic_get_property) {
>>>>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>>>>   	} else {
>>>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>>>> index 0d002b045bd2..4b83e078d3e9 100644
>>>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>>>> @@ -88,6 +88,20 @@
>>>>>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>>>>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>>>>>    * "GAMMA_LUT" property above.
>>>>> + *
>>>>> + * Support for different non RGB color encodings is controlled through
>>>>> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
>>>>> + * are set up by calling drm_plane_create_color_properties().
>>>>> + *
>>>>> + * "COLOR_ENCODING"
>>>>> + * 	Optional plane enum property to support different non RGB
>>>>> + * 	color encodings. The driver can provide a subset of standard
>>>>> + * 	enum values supported by the DRM plane.
>>>>> + *
>>>>> + * "COLOR_RANGE"
>>>>> + * 	Optional plane enum property to support different non RGB
>>>>> + * 	color parameter ranges. The driver can provide a subset of
>>>>> + * 	standard enum values supported by the DRM plane.
>>>>>    */
>>>>>   
>>>>>   /**
>>>>> @@ -339,3 +353,92 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>>>>   	drm_modeset_unlock(&crtc->mutex);
>>>>>   	return ret;
>>>>>   }
>>>>> +
>>>>> +static const char * const color_encoding_name[] = {
>>>>> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
>>>>> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
>>>>> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
>>>>> +};
>>>>> +
>>>>> +static const char * const color_range_name[] = {
>>>>> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
>>>>> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * drm_plane_create_color_properties - color encoding related plane properties
>>>>> + * @plane: plane object
>>>>> + * @supported_encodings: bitfield indicating supported color encodings
>>>>> + * @supported_ranges: bitfileld indicating supported color ranges
>>>>> + * @default_encoding: default color encoding
>>>>> + * @default_range: default color range
>>>>> + *
>>>>> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
>>>>> + * properties to @plane. The supported encodings and ranges should
>>>>> + * be provided in supported_encodings and supported_ranges bitmasks.
>>>>> + * Each bit set in the bitmask indicates that its number as enum
>>>>> + * value is supported.
>>>>> + */
>>>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>>>> +				      u32 supported_encodings,
>>>>> +				      u32 supported_ranges,
>>>>> +				      enum drm_color_encoding default_encoding,
>>>>> +				      enum drm_color_range default_range)
>>>>> +{
>>>>> +	struct drm_device *dev = plane->dev;
>>>>> +	struct drm_property *prop;
>>>>> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
>>>>> +						DRM_COLOR_RANGE_MAX)];
>>>>> +	int i, len;
>>>>> +
>>>>> +	if (WARN_ON(supported_encodings == 0 ||
>>>>> +		    (supported_encodings & -BIT(DRM_COLOR_ENCODING_MAX)) != 0 ||
>>>>> +		    (supported_encodings & BIT(default_encoding)) == 0))
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (WARN_ON(supported_ranges == 0 ||
>>>>> +		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
>>>>> +		    (supported_ranges & BIT(default_range)) == 0))
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	len = 0;
>>>>> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
>>>>> +		if ((supported_encodings & BIT(i)) == 0)
>>>>> +			continue;
>>>>> +
>>>>> +		enum_list[len].type = i;
>>>>> +		enum_list[len].name = color_encoding_name[i];
>>>>> +		len++;
>>>>> +	}
>>>>> +
>>>>> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
>>>>> +					enum_list, len);
>>>>> +	if (!prop)
>>>>> +		return -ENOMEM;
>>>>> +	plane->color_encoding_property = prop;
>>>>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
>>>>> +	if (plane->state)
>>>>> +		plane->state->color_encoding = default_encoding;
>>>>> +
>>>>> +	len = 0;
>>>>> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
>>>>> +		if ((supported_ranges & BIT(i)) == 0)
>>>>> +			continue;
>>>>> +
>>>>> +		enum_list[len].type = i;
>>>>> +		enum_list[len].name = color_range_name[i];
>>>>> +		len++;
>>>>> +	}
>>>>> +
>>>>> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
>>>>> +					enum_list, len);
>>>>> +	if (!prop)
>>>>> +		return -ENOMEM;
>>>>> +	plane->color_range_property = prop;
>>>>> +	drm_object_attach_property(&plane->base, prop, default_range);
>>>>> +	if (plane->state)
>>>>> +		plane->state->color_range = default_range;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_plane_create_color_properties);
>>>>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>>>>> index 03a59cbce621..b3b6d302ca8c 100644
>>>>> --- a/include/drm/drm_color_mgmt.h
>>>>> +++ b/include/drm/drm_color_mgmt.h
>>>>> @@ -26,6 +26,7 @@
>>>>>   #include <linux/ctype.h>
>>>>>   
>>>>>   struct drm_crtc;
>>>>> +struct drm_plane;
>>>>>   
>>>>>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>>>>   
>>>>> @@ -37,4 +38,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>>>>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>>>>>   				 int gamma_size);
>>>>>   
>>>>> +enum drm_color_encoding {
>>>>> +	DRM_COLOR_YCBCR_BT601,
>>>>> +	DRM_COLOR_YCBCR_BT709,
>>>>> +	DRM_COLOR_YCBCR_BT2020,
>>>>> +	DRM_COLOR_ENCODING_MAX,
>>>>> +};
>>>>> +
>>>>> +enum drm_color_range {
>>>>> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
>>>>> +	DRM_COLOR_YCBCR_FULL_RANGE,
>>>>> +	DRM_COLOR_RANGE_MAX,
>>>>> +};
>>>>> +
>>>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>>>> +				      u32 supported_encodings,
>>>>> +				      u32 supported_ranges,
>>>>> +				      enum drm_color_encoding default_encoding,
>>>>> +				      enum drm_color_range default_range);
>>>>>   #endif
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 8185e3468a23..f7bf4a48b1c3 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -26,6 +26,7 @@
>>>>>   #include <linux/list.h>
>>>>>   #include <linux/ctype.h>
>>>>>   #include <drm/drm_mode_object.h>
>>>>> +#include <drm/drm_color_mgmt.h>
>>>>>   
>>>>>   struct drm_crtc;
>>>>>   struct drm_printer;
>>>>> @@ -112,6 +113,20 @@ struct drm_plane_state {
>>>>>   	unsigned int zpos;
>>>>>   	unsigned int normalized_zpos;
>>>>>   
>>>>> +	/**
>>>>> +	 * @color_encoding:
>>>>> +	 *
>>>>> +	 * Color encoding for non RGB formats
>>>>> +	 */
>>>>> +	enum drm_color_encoding color_encoding;
>>>>> +
>>>>> +	/**
>>>>> +	 * @color_range:
>>>>> +	 *
>>>>> +	 * Color range for non RGB formats
>>>>> +	 */
>>>>> +	enum drm_color_range color_range;
>>>>> +
>>>>>   	/* Clipped coordinates */
>>>>>   	struct drm_rect src, dst;
>>>>>   
>>>>> @@ -558,6 +573,23 @@ struct drm_plane {
>>>>>   
>>>>>   	struct drm_property *zpos_property;
>>>>>   	struct drm_property *rotation_property;
>>>>> +
>>>>> +	/**
>>>>> +	 * @color_encoding_property:
>>>>> +	 *
>>>>> +	 * Optional "COLOR_ENCODING" enum property for specifying
>>>>> +	 * color encoding for non RGB formats.
>>>>> +	 * See drm_plane_create_color_properties().
>>>>> +	 */
>>>>> +	struct drm_property *color_encoding_property;
>>>>> +	/**
>>>>> +	 * @color_range_property:
>>>>> +	 *
>>>>> +	 * Optional "COLOR_RANGE" enum property for specifying
>>>>> +	 * color range for non RGB formats.
>>>>> +	 * See drm_plane_create_color_properties().
>>>>> +	 */
>>>>> +	struct drm_property *color_range_property;
>>>>>   };
>>>>>   
>>>>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>>
>

On Fri, Nov 30, 2018 at 03:48:00PM +0100, Hans Verkuil wrote:
> On 11/30/18 15:29, Ville Syrjälä wrote:
> > On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
> >> Hi Ville,
> >>
> >> As Christoph cannot respond till middle next week I can try to respond
> >> in his absence, as I am familiar with the subject.
> >>
> >> On 30.11.2018 14:25, Ville Syrjälä wrote:
> >>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
> >>>> Hi,
> >>>>
> >>>> I am looking for a way to export the color encoding and range selection
> >>>> to user space. I came across those properties and am wondering, why
> >>>> they are meant only for non RGB color encodings. Would it be okay, to
> >>>> modify them and use with RGB formats as well?
> >>> What you trying to do? Input limited range RGB data and expand to full
> >>> range?
> >>
> >>
> >> For example. But there are two more general questions, which
> >> surprisingly we have not found answer for.
> >>
> >> 1. What color encoding and range drm should expect on its input RGB
> >> buffers by default?
> > 
> > RGB is just RGB. There is no encoding. It's assumed to be full range
> > because no one really uses anything else.
> 
> For simple desktop usage that's true. When dealing with video inputs,
> this becomes much more complicated.
> 
> > 
> >>
> >> 2. How userspace should inform drm that given buffer has specified
> >> non-default color encoding and range?
> >>
> >>
> >> Hopefully this patch introduces such properties but only for YCbCr
> >> formats, the question is what should be the best way to expand it to RGB
> >> formats:
> >>
> >> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
> > 
> > BT.601 specifies how to encoder RGB data as YCbCr. So without
> > YCbCr BT.601 does not mean anything. Well, the standard does
> > contain other things as well I suppose, but for the purposes
> > of the color encoding prop only that one part is relevant.
> 
> Ah, I misunderstood the meaning of DRM_COLOR_RGB_BT601.
> This is the equivalent of V4L2_YCBCR_ENC_601, and that's indeed
> only defined for Y'CbCr. But it is often (ab)used as an alias for
> the SMPTE170M colorspace (used by SDTV).
> 
> V4L2 has the following defines for colorspaces, transfer functions,
> Y'CbCr (and HSV) encodings and quantization ranges:
> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html

Yeah, we're going to be introducing other properties to control
colorspace and transfer function in kms as well. Actually some
patches towards that have been floated a few times already.

On 11/30/18 16:16, Ville Syrjälä wrote:
> On Fri, Nov 30, 2018 at 03:48:00PM +0100, Hans Verkuil wrote:
>> On 11/30/18 15:29, Ville Syrjälä wrote:
>>> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
>>>> Hi Ville,
>>>>
>>>> As Christoph cannot respond till middle next week I can try to respond
>>>> in his absence, as I am familiar with the subject.
>>>>
>>>> On 30.11.2018 14:25, Ville Syrjälä wrote:
>>>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I am looking for a way to export the color encoding and range selection
>>>>>> to user space. I came across those properties and am wondering, why
>>>>>> they are meant only for non RGB color encodings. Would it be okay, to
>>>>>> modify them and use with RGB formats as well?
>>>>> What you trying to do? Input limited range RGB data and expand to full
>>>>> range?
>>>>
>>>>
>>>> For example. But there are two more general questions, which
>>>> surprisingly we have not found answer for.
>>>>
>>>> 1. What color encoding and range drm should expect on its input RGB
>>>> buffers by default?
>>>
>>> RGB is just RGB. There is no encoding. It's assumed to be full range
>>> because no one really uses anything else.
>>
>> For simple desktop usage that's true. When dealing with video inputs,
>> this becomes much more complicated.
>>
>>>
>>>>
>>>> 2. How userspace should inform drm that given buffer has specified
>>>> non-default color encoding and range?
>>>>
>>>>
>>>> Hopefully this patch introduces such properties but only for YCbCr
>>>> formats, the question is what should be the best way to expand it to RGB
>>>> formats:
>>>>
>>>> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
>>>
>>> BT.601 specifies how to encoder RGB data as YCbCr. So without
>>> YCbCr BT.601 does not mean anything. Well, the standard does
>>> contain other things as well I suppose, but for the purposes
>>> of the color encoding prop only that one part is relevant.
>>
>> Ah, I misunderstood the meaning of DRM_COLOR_RGB_BT601.
>> This is the equivalent of V4L2_YCBCR_ENC_601, and that's indeed
>> only defined for Y'CbCr. But it is often (ab)used as an alias for
>> the SMPTE170M colorspace (used by SDTV).
>>
>> V4L2 has the following defines for colorspaces, transfer functions,
>> Y'CbCr (and HSV) encodings and quantization ranges:
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
> 
> Yeah, we're going to be introducing other properties to control
> colorspace and transfer function in kms as well. Actually some
> patches towards that have been floated a few times already.
> 

Great. Let's try to keep drm and V4L2 in sync for this. It should be
possible to convert from one to the other without having to do weird
things.

I'll try to pay attention to these patches, but just ping me if you
want me to take a look at something.

I put a lot of effort into the V4L2 colorspace documentation, trying to
put all the information in one place, esp. all the formulas.

Regards,

	Hans

Hi,

On Fri, Nov 30, 2018 at 04:34:54PM +0100, Hans Verkuil wrote:
> On 11/30/18 16:16, Ville Syrjälä wrote:
> > On Fri, Nov 30, 2018 at 03:48:00PM +0100, Hans Verkuil wrote:
> >> On 11/30/18 15:29, Ville Syrjälä wrote:
> >>> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
> >>>> Hi Ville,
> >>>>
> >>>> As Christoph cannot respond till middle next week I can try to respond
> >>>> in his absence, as I am familiar with the subject.
> >>>>
> >>>> On 30.11.2018 14:25, Ville Syrjälä wrote:
> >>>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I am looking for a way to export the color encoding and range selection
> >>>>>> to user space. I came across those properties and am wondering, why
> >>>>>> they are meant only for non RGB color encodings. Would it be okay, to
> >>>>>> modify them and use with RGB formats as well?
> >>>>> What you trying to do? Input limited range RGB data and expand to full
> >>>>> range?
> >>>>
> >>>>
> >>>> For example. But there are two more general questions, which
> >>>> surprisingly we have not found answer for.
> >>>>
> >>>> 1. What color encoding and range drm should expect on its input RGB
> >>>> buffers by default?

This is where I personally think we've got an unfortunate disconnect
in the KMS UAPI.

For YCbCr buffers, these properties specify the encoding and range of
the data in the buffer. But everything else in the pipe is described
in terms of the processing to apply - i.e. the KMS driver doesn't know
what transfer function the data uses, it only knows the degamma LUT
it's told to apply to it.

It would have been more uniform if the COLOR_ENCODING/COLOR_RANGE
properties were a single "ENCODING_CONVERSION" property stating what
conversion should be applied.

> >>>
> >>> RGB is just RGB. There is no encoding. It's assumed to be full range
> >>> because no one really uses anything else.
> >>
> >> For simple desktop usage that's true. When dealing with video inputs,
> >> this becomes much more complicated.
> >>

When the plane degamma/ctm/gamma properties land, those could be used
to convert limited range to whatever the pipe-internal format is, I
think.

That pipe-internal format would be whatever userspace decides it is,
via converting input buffers using the various color conversion
properties.

> >>>
> >>>>
> >>>> 2. How userspace should inform drm that given buffer has specified
> >>>> non-default color encoding and range?
> >>>>

My understanding is that DRM would never be informed of this - only
what to do with the data (which does of-course imply an encoding, but
it's not told to DRM explicitly).

> >>>>
> >>>> Hopefully this patch introduces such properties but only for YCbCr
> >>>> formats, the question is what should be the best way to expand it to RGB
> >>>> formats:
> >>>>
> >>>> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
> >>>
> >>> BT.601 specifies how to encoder RGB data as YCbCr. So without
> >>> YCbCr BT.601 does not mean anything. Well, the standard does
> >>> contain other things as well I suppose, but for the purposes
> >>> of the color encoding prop only that one part is relevant.
> >>
> >> Ah, I misunderstood the meaning of DRM_COLOR_RGB_BT601.
> >> This is the equivalent of V4L2_YCBCR_ENC_601, and that's indeed
> >> only defined for Y'CbCr. But it is often (ab)used as an alias for
> >> the SMPTE170M colorspace (used by SDTV).
> >>
> >> V4L2 has the following defines for colorspaces, transfer functions,
> >> Y'CbCr (and HSV) encodings and quantization ranges:
> >>
> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
> > 
> > Yeah, we're going to be introducing other properties to control
> > colorspace and transfer function in kms as well. Actually some
> > patches towards that have been floated a few times already.
> > 
> 
> Great. Let's try to keep drm and V4L2 in sync for this. It should be
> possible to convert from one to the other without having to do weird
> things.
> 
> I'll try to pay attention to these patches, but just ping me if you
> want me to take a look at something.
> 
> I put a lot of effort into the V4L2 colorspace documentation, trying to
> put all the information in one place, esp. all the formulas.

There's always going to be a bit of a disconnect here - in KMS, it's
userspace which needs to handle all this stuff. It would be up to
userspace to set e.g. DEGAMMA_LUT to a LUT which corresponds to SMPTE
2084, rather than the kernel driver being told directly that the
buffer is encoded using the SMPTE 2084 transfer function.

Actually I want to put an RFC together to allow DEGAMMA_LUT/GAMMA_LUT
to be set to some pre-defined values (e.g. sRGB, PQ, HLG) to suit
hardware which has built-in hard-coded transfer functions (and
potentially also save userspace some effort of coming up with LUTs).

Cheers,
-Brian

> 
> Regards,
> 
> 	Hans
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On 30.11.2018 15:48, Hans Verkuil wrote:
> On 11/30/18 15:29, Ville Syrjälä wrote:
>> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
>>> Hi Ville,
>>>
>>> As Christoph cannot respond till middle next week I can try to respond
>>> in his absence, as I am familiar with the subject.
>>>
>>> On 30.11.2018 14:25, Ville Syrjälä wrote:
>>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>>>> Hi,
>>>>>
>>>>> I am looking for a way to export the color encoding and range selection
>>>>> to user space. I came across those properties and am wondering, why
>>>>> they are meant only for non RGB color encodings. Would it be okay, to
>>>>> modify them and use with RGB formats as well?
>>>> What you trying to do? Input limited range RGB data and expand to full
>>>> range?
>>>
>>> For example. But there are two more general questions, which
>>> surprisingly we have not found answer for.
>>>
>>> 1. What color encoding and range drm should expect on its input RGB
>>> buffers by default?
>> RGB is just RGB. There is no encoding. 


OK, Now I see I have confused encoding with colorspace, Hans thanks for
the documentation.

As I understand drm by default should expect sRGB colorspace, also for
Y'CbCr buffers, and currently there are no standard ways to specify
buffer colorspace.

Looking at current users of drm_plane_create_color_properties for Y'CbCr
buffers it seems default range should be LIMITED.

But default encoding is different: mali, armada, nouveau use
DRM_COLOR_YCBCR_BT601, i915 uses DRM_COLOR_YCBCR_BT709 - shouldn't this
be somehow standardized?


What I still do not understand is colorspace conversion(not encoding!) -
between pipeline input (framebuffers) and output (HDMI connector for
example):

1. Since sRGB, SMPTE170M/BT.601, BT.709 colorspaces are 'almost
identical in coverage' (according to wikipedia[1]) no colorspace
conversion is performed at all.

2. It is performed somewhere by HW, just my IP documentation is not
clear about it.


Another thing which still bothers me is RGB range in case of HDMI -
CEA-861 expects than on CEA modes limited RGB range should be sent, but
the pipeline on the particular hardware I have do not perform any
conversion of input buffers. So only limited range RGB buffers are
displayed correctly. In such case property describing plane with limited
RGB would be useful.


[1]: https://en.wikipedia.org/wiki/Rec._709#Primary_chromaticities


> It's assumed to be full range 
>> because no one really uses anything else.
> For simple desktop usage that's true. When dealing with video inputs,
> this becomes much more complicated.


As I said earlier there are cases where output stream should be limited
RGB, and no conversion in pipeline - thus framebuffers also should be
'limited RGB'.


Regards

Andrzej


>
>>> 2. How userspace should inform drm that given buffer has specified
>>> non-default color encoding and range?
>>>
>>>
>>> Hopefully this patch introduces such properties but only for YCbCr
>>> formats, the question is what should be the best way to expand it to RGB
>>> formats:
>>>
>>> A. Add another enums: DRM_COLOR_RGB_BT601 and friends.
>> BT.601 specifies how to encoder RGB data as YCbCr. So without
>> YCbCr BT.601 does not mean anything. Well, the standard does
>> contain other things as well I suppose, but for the purposes
>> of the color encoding prop only that one part is relevant.
> Ah, I misunderstood the meaning of DRM_COLOR_RGB_BT601.
> This is the equivalent of V4L2_YCBCR_ENC_601, and that's indeed
> only defined for Y'CbCr. But it is often (ab)used as an alias for
> the SMPTE170M colorspace (used by SDTV).
>
> V4L2 has the following defines for colorspaces, transfer functions,
> Y'CbCr (and HSV) encodings and quantization ranges:
>
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
>
> Missing in this list is the color encoding (RGB vs YCbCr vs HSV) which
> is set through the pixelformat fourcc.
>
> And indeed, we don't have an RGB encoding define since RGB is just RGB :-)
>
> Regards,
>
> 	Hans
>
>>> B. Reuse current enums, but remove format information from them:
>>> DRM_COLOR_YCBCR_BT601 => DRM_COLOR_BT601.
>>>
>>>
>>> Regards
>>>
>>> Andrzej
>>>
...

On 12/03/2018 12:23 PM, Andrzej Hajda wrote:
> On 30.11.2018 15:48, Hans Verkuil wrote:
>> On 11/30/18 15:29, Ville Syrjälä wrote:
>>> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
>>>> Hi Ville,
>>>>
>>>> As Christoph cannot respond till middle next week I can try to respond
>>>> in his absence, as I am familiar with the subject.
>>>>
>>>> On 30.11.2018 14:25, Ville Syrjälä wrote:
>>>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I am looking for a way to export the color encoding and range selection
>>>>>> to user space. I came across those properties and am wondering, why
>>>>>> they are meant only for non RGB color encodings. Would it be okay, to
>>>>>> modify them and use with RGB formats as well?
>>>>> What you trying to do? Input limited range RGB data and expand to full
>>>>> range?
>>>>
>>>> For example. But there are two more general questions, which
>>>> surprisingly we have not found answer for.
>>>>
>>>> 1. What color encoding and range drm should expect on its input RGB
>>>> buffers by default?
>>> RGB is just RGB. There is no encoding. 
> 
> 
> OK, Now I see I have confused encoding with colorspace, Hans thanks for
> the documentation.
> 
> As I understand drm by default should expect sRGB colorspace, also for
> Y'CbCr buffers, and currently there are no standard ways to specify
> buffer colorspace.
> 
> Looking at current users of drm_plane_create_color_properties for Y'CbCr
> buffers it seems default range should be LIMITED.

Right. There is support for YCbCr quantization range signaling in HDMI,
but that is for all intents and purposes broken in the standard. Don't
use it, just output limited range YCbCr.

On the other hand, RGB Quantization range signaling should *always* be
used if the EDID signals support for it. I believe that's what drm does
today already (this certainly works for i915).

> 
> But default encoding is different: mali, armada, nouveau use
> DRM_COLOR_YCBCR_BT601, i915 uses DRM_COLOR_YCBCR_BT709 - shouldn't this
> be somehow standardized?

According to CEA-861 bt.601 should be used for SDTV (PAL/NTSC), 709 for
everything else. So this is resolution dependent.

Although strictly speaking it is only userspace that knows the right
encoding, since it depends on the source material. You can have SDTV
video encoded with 709, or HDTV encoded with 601. But in the absence
of any information, the default rule above is what should be used.

> 
> 
> What I still do not understand is colorspace conversion(not encoding!) -
> between pipeline input (framebuffers) and output (HDMI connector for
> example):
> 
> 1. Since sRGB, SMPTE170M/BT.601, BT.709 colorspaces are 'almost
> identical in coverage' (according to wikipedia[1]) no colorspace
> conversion is performed at all.

That's correct. There is a slight difference between SMPTE170M and
sRGB/Rec709, but basically invisible to the naked eye unless you see
it side-by-side, and even then it's is hard to detect.

However, sRGB uses a different transfer function compared to SMPTE170M
and Rec709, and that difference *is* quite visible. Not all hardware
(certainly not video capture hardware) is capable of converting between
different transfer functions, though. I gather that this is more common
for HDMI output hardware.

> 
> 2. It is performed somewhere by HW, just my IP documentation is not
> clear about it.
> 
> 
> Another thing which still bothers me is RGB range in case of HDMI -
> CEA-861 expects than on CEA modes limited RGB range should be sent, but
> the pipeline on the particular hardware I have do not perform any
> conversion of input buffers. So only limited range RGB buffers are
> displayed correctly. In such case property describing plane with limited
> RGB would be useful.

Are you sure? Usually the same block that does YCbCr encoding conversion
(601 to 709 or vice versa), also deals with limited/full range conversion.

It is typically a 3x3 matrix + a vector adding the needed offsets. It is
dead cheap to implement in hardware, so it would be very unusual if there
is no support for this.

> 
> 
> [1]: https://en.wikipedia.org/wiki/Rec._709#Primary_chromaticities
> 
> 
>> It's assumed to be full range 
>>> because no one really uses anything else.
>> For simple desktop usage that's true. When dealing with video inputs,
>> this becomes much more complicated.
> 
> 
> As I said earlier there are cases where output stream should be limited
> RGB, and no conversion in pipeline - thus framebuffers also should be
> 'limited RGB'.

Whether RGB output should be full or limited range depends entirely on
the resolution and the EDID. I have tested i915 w.r.t. RGB quantization
range, and that does it right.

The reality with RGB Quantization Range handling is that 50% of all
devices do this wrong. So if the EDID signals selectable quantization range
support, then use it and signal full range RGB.

If it is not available, then the i915 implementation is one I can point
to that does it 100% correct. (This might be based on core drm code, I'm
not sure if it is i915 specific or not)

Kudos to the i915 driver developers: the linux implementation is the
only one that handles this correctly. The Windows implementation doesn't
honor RGB Selectable Quantization and the macos driver seems to always
output full range.

The only way to support all three variants in an HDMI receiver is to
enable YCbCr support in the EDID: the macos driver will now output
YCbCr, while all the others output RGB (Windows using default RGB
Quantization rules and Linux explicitly signaling the RGB range).

It's insane.

Regards,

	Hans

On 03.12.2018 12:52, Hans Verkuil wrote:
> On 12/03/2018 12:23 PM, Andrzej Hajda wrote:
>> On 30.11.2018 15:48, Hans Verkuil wrote:
>>> On 11/30/18 15:29, Ville Syrjälä wrote:
>>>> On Fri, Nov 30, 2018 at 03:20:59PM +0100, Andrzej Hajda wrote:
>>>>> Hi Ville,
>>>>>
>>>>> As Christoph cannot respond till middle next week I can try to respond
>>>>> in his absence, as I am familiar with the subject.
>>>>>
>>>>> On 30.11.2018 14:25, Ville Syrjälä wrote:
>>>>>> On Fri, Nov 30, 2018 at 02:08:11PM +0100, Christoph Manszewski wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I am looking for a way to export the color encoding and range selection
>>>>>>> to user space. I came across those properties and am wondering, why
>>>>>>> they are meant only for non RGB color encodings. Would it be okay, to
>>>>>>> modify them and use with RGB formats as well?
>>>>>> What you trying to do? Input limited range RGB data and expand to full
>>>>>> range?
>>>>> For example. But there are two more general questions, which
>>>>> surprisingly we have not found answer for.
>>>>>
>>>>> 1. What color encoding and range drm should expect on its input RGB
>>>>> buffers by default?
>>>> RGB is just RGB. There is no encoding. 
>>
>> OK, Now I see I have confused encoding with colorspace, Hans thanks for
>> the documentation.
>>
>> As I understand drm by default should expect sRGB colorspace, also for
>> Y'CbCr buffers, and currently there are no standard ways to specify
>> buffer colorspace.
>>
>> Looking at current users of drm_plane_create_color_properties for Y'CbCr
>> buffers it seems default range should be LIMITED.
> Right. There is support for YCbCr quantization range signaling in HDMI,
> but that is for all intents and purposes broken in the standard. Don't
> use it, just output limited range YCbCr.
>
> On the other hand, RGB Quantization range signaling should *always* be
> used if the EDID signals support for it. I believe that's what drm does
> today already (this certainly works for i915).
>
>> But default encoding is different: mali, armada, nouveau use
>> DRM_COLOR_YCBCR_BT601, i915 uses DRM_COLOR_YCBCR_BT709 - shouldn't this
>> be somehow standardized?
> According to CEA-861 bt.601 should be used for SDTV (PAL/NTSC), 709 for
> everything else. So this is resolution dependent.
>
> Although strictly speaking it is only userspace that knows the right
> encoding, since it depends on the source material. You can have SDTV
> video encoded with 709, or HDTV encoded with 601. But in the absence
> of any information, the default rule above is what should be used.


Since drm plane is not tied to specific crtc default encoding will not
be always known prior to setting mode on crtc and attaching plane to
crtc, probably DRM_COLOR_YCBCR_DEFAULT or similar would solve the issue.


>
>>
>> What I still do not understand is colorspace conversion(not encoding!) -
>> between pipeline input (framebuffers) and output (HDMI connector for
>> example):
>>
>> 1. Since sRGB, SMPTE170M/BT.601, BT.709 colorspaces are 'almost
>> identical in coverage' (according to wikipedia[1]) no colorspace
>> conversion is performed at all.
> That's correct. There is a slight difference between SMPTE170M and
> sRGB/Rec709, but basically invisible to the naked eye unless you see
> it side-by-side, and even then it's is hard to detect.
>
> However, sRGB uses a different transfer function compared to SMPTE170M
> and Rec709, and that difference *is* quite visible. Not all hardware
> (certainly not video capture hardware) is capable of converting between
> different transfer functions, though. I gather that this is more common
> for HDMI output hardware.
>
>> 2. It is performed somewhere by HW, just my IP documentation is not
>> clear about it.
>>
>>
>> Another thing which still bothers me is RGB range in case of HDMI -
>> CEA-861 expects than on CEA modes limited RGB range should be sent, but
>> the pipeline on the particular hardware I have do not perform any
>> conversion of input buffers. So only limited range RGB buffers are
>> displayed correctly. In such case property describing plane with limited
>> RGB would be useful.
> Are you sure? Usually the same block that does YCbCr encoding conversion
> (601 to 709 or vice versa), also deals with limited/full range conversion.
>
> It is typically a 3x3 matrix + a vector adding the needed offsets. It is
> dead cheap to implement in hardware, so it would be very unusual if there
> is no support for this.


Yay, I have unusual hardware :)

There is register in HDMI output block for RGB->YCbCr conversion and I
can specify there input RGB range and encoding, but it is only for cases
where output is YCbCr4:4:4, In case RGB -> RGB no conversion is performed.


Regards

Andrzej


>>
>> [1]: https://en.wikipedia.org/wiki/Rec._709#Primary_chromaticities
>>
>>
>>> It's assumed to be full range 
>>>> because no one really uses anything else.
>>> For simple desktop usage that's true. When dealing with video inputs,
>>> this becomes much more complicated.
>>
>> As I said earlier there are cases where output stream should be limited
>> RGB, and no conversion in pipeline - thus framebuffers also should be
>> 'limited RGB'.
> Whether RGB output should be full or limited range depends entirely on
> the resolution and the EDID. I have tested i915 w.r.t. RGB quantization
> range, and that does it right.
>
> The reality with RGB Quantization Range handling is that 50% of all
> devices do this wrong. So if the EDID signals selectable quantization range
> support, then use it and signal full range RGB.
>
> If it is not available, then the i915 implementation is one I can point
> to that does it 100% correct. (This might be based on core drm code, I'm
> not sure if it is i915 specific or not)
>
> Kudos to the i915 driver developers: the linux implementation is the
> only one that handles this correctly. The Windows implementation doesn't
> honor RGB Selectable Quantization and the macos driver seems to always
> output full range.
>
> The only way to support all three variants in an HDMI receiver is to
> enable YCbCr support in the EDID: the macos driver will now output
> YCbCr, while all the others output RGB (Windows using default RGB
> Quantization rules and Linux explicitly signaling the RGB range).
>
> It's insane.
>
> Regards,
>
> 	Hans
>
>