[PATCH RFC v3] drm: Add properties to control YCbCr to RGB conversion

[PATCH RFC v3] drm: Add properties to control YCbCr to RGB conversion

[Jyri Sarha]

Changes since v2 RFC version:
 - Drop the second patch that exposed the raw CSC and preoffset vector
 - Rename YCBCR_ENCODING to COLOR_ENCODING, add YCbCr to enum values instead
 - Split the encoding and used value range to different properties:
   COLOR_ENCODING and COLOR_RANGE

The v2 RFC and the related discussion can be found here:
http://www.spinics.net/lists/dri-devel/msg140329.html

Jyri Sarha (1):
  drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to
    drm_plane

 drivers/gpu/drm/drm_atomic.c     |  8 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  6 +++
 include/drm/drm_color_mgmt.h     | 20 +++++++++
 include/drm/drm_plane.h          |  8 ++++
 5 files changed, 139 insertions(+)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Jyri Sarha]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 8271 bytes --]

Add a standard optinal 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.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic.c     |  8 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  6 +++
 include/drm/drm_color_mgmt.h     | 20 +++++++++
 include/drm/drm_plane.h          |  8 ++++
 5 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 4033384..f341dda 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -774,6 +774,10 @@ 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);
@@ -834,6 +838,10 @@ int drm_atomic_plane_set_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 533f3a3..2e04ebb 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -33,6 +33,11 @@
  * properties on the &drm_crtc object. They are set up by calling
  * drm_crtc_enable_color_mgmt().
  *
+ * Support for different non RGB color encodings is controlled trough
+ * plane specific COLOR_ENCODING and COLOR_RANGE properties.
+ *
+ * The &drm_crtc object's properties are:
+ *
  * "DEGAMMA_LUT”:
  *	Blob property to set the degamma lookup table (LUT) mapping pixel data
  *	from the framebuffer before it is given to the transformation matrix.
@@ -85,6 +90,18 @@
  * 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.
+ *
+ * The &drm_plane object's properties are:
+ *
+ * "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.
  */
 
 /**
@@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static char *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 char *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
+ * @supported_encodings: bitfield indicating supported color encdings
+ * @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 to the drm_plane object. The supported encodings and
+ * ranges should be provided in supported_encodings and
+ * supported_ranges bitmasks. Each bit set in the bitmast indicates
+ * the its number as enum value being 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)];
+	unsigned int i, len;
+
+
+	if (WARN_ON(plane->color_encoding_property != NULL) ||
+	    WARN_ON(plane->color_range_property != NULL))
+		return -EEXIST;
+
+	len = 0;
+	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
+		if ((BIT(i) & supported_encodings) == 0)
+			continue;
+
+		enum_list[i].type = i;
+		enum_list[i].name = color_encoding_name[i];
+		len++;
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"COLOR_ENCODING",
+					enum_list, len);
+	if (!prop)
+		return -ENOMEM;
+	plane->color_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_encoding);
+
+	len = 0;
+	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
+		if ((BIT(i) & supported_ranges) == 0)
+			continue;
+
+		enum_list[i].type = i;
+		enum_list[i].name = color_range_name[i];
+		len++;
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"COLOR_RANGE",
+					enum_list, len);
+	if (!prop)
+		return -ENOMEM;
+	plane->color_range_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_range);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index fedd4d6..a6762e8 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->color_encoding_property)
+		drm_property_destroy(dev, plane->color_encoding_property);
+
+	if (plane->color_range_property)
+		drm_property_destroy(dev, plane->color_range_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 03a59cb..7a1af63 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -26,6 +26,8 @@
 #include <linux/ctype.h>
 
 struct drm_crtc;
+struct drm_plane;
+struct drm_prop_enum_list;
 
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
 
@@ -37,4 +39,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 = 0,
+	DRM_COLOR_YCBCR_BT709,
+	DRM_COLOR_YCBCR_BT2020,
+	DRM_COLOR_ENCODING_MAX,
+};
+
+enum drm_color_range {
+	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
+	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 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* Color encoding for non RGB formats */
+	enum drm_color_encoding color_encoding;
+	enum drm_color_range color_range;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 
@@ -523,6 +528,9 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *color_encoding_property;
+	struct drm_property *color_range_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
-- 
1.9.1


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Eric Engestrom]

On Monday, 2017-05-15 14:19:09 +0300, Jyri Sarha wrote:
> Add a standard optinal properties to support different non RGB color

"optional" (same in commit title)

> 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.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  6 +++
>  include/drm/drm_color_mgmt.h     | 20 +++++++++
>  include/drm/drm_plane.h          |  8 ++++
>  5 files changed, 139 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4033384..f341dda 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -774,6 +774,10 @@ 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);
> @@ -834,6 +838,10 @@ int drm_atomic_plane_set_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 533f3a3..2e04ebb 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,6 +33,11 @@
>   * properties on the &drm_crtc object. They are set up by calling
>   * drm_crtc_enable_color_mgmt().
>   *
> + * Support for different non RGB color encodings is controlled trough

"through"

> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.
> + *
> + * The &drm_crtc object's properties are:
> + *
>   * "DEGAMMA_LUT”:
>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>   *	from the framebuffer before it is given to the transformation matrix.
> @@ -85,6 +90,18 @@
>   * 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.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "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.
>   */
>  
>  /**
> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static char *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 char *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
> + * @supported_encodings: bitfield indicating supported color encdings

"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 to the drm_plane object. The supported encodings and
> + * ranges should be provided in supported_encodings and
> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
> + * the its number as enum value being 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)];
> +	unsigned int i, len;
> +
> +
> +	if (WARN_ON(plane->color_encoding_property != NULL) ||
> +	    WARN_ON(plane->color_range_property != NULL))
> +		return -EEXIST;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> +		if ((BIT(i) & supported_encodings) == 0)
> +			continue;
> +
> +		enum_list[i].type = i;
> +		enum_list[i].name = color_encoding_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_ENCODING",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> +		if ((BIT(i) & supported_ranges) == 0)
> +			continue;
> +
> +		enum_list[i].type = i;
> +		enum_list[i].name = color_range_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_RANGE",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_range_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_range);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d6..a6762e8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	kfree(plane->name);
>  
> +	if (plane->color_encoding_property)
> +		drm_property_destroy(dev, plane->color_encoding_property);
> +
> +	if (plane->color_range_property)
> +		drm_property_destroy(dev, plane->color_range_property);
> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cb..7a1af63 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,8 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
> +struct drm_prop_enum_list;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +39,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 = 0,
> +	DRM_COLOR_YCBCR_BT709,
> +	DRM_COLOR_YCBCR_BT2020,
> +	DRM_COLOR_ENCODING_MAX,
> +};
> +
> +enum drm_color_range {
> +	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
> +	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 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* Color encoding for non RGB formats */
> +	enum drm_color_encoding color_encoding;
> +	enum drm_color_range color_range;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -523,6 +528,9 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	struct drm_property *color_encoding_property;
> +	struct drm_property *color_range_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> -- 
> 1.9.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Ville Syrjälä]

On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
> Add a standard optinal 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.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  6 +++
>  include/drm/drm_color_mgmt.h     | 20 +++++++++
>  include/drm/drm_plane.h          |  8 ++++
>  5 files changed, 139 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4033384..f341dda 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -774,6 +774,10 @@ 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);
> @@ -834,6 +838,10 @@ int drm_atomic_plane_set_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 533f3a3..2e04ebb 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,6 +33,11 @@
>   * properties on the &drm_crtc object. They are set up by calling
>   * drm_crtc_enable_color_mgmt().
>   *
> + * Support for different non RGB color encodings is controlled trough
> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.

Putting this comment in middle of the crtc stuff seems a bit strange
when the rest of the plane stuff is at the end. 

This should specify somewhere that these plane properties control
the plane input and not the output.

> + *
> + * The &drm_crtc object's properties are:
> + *
>   * "DEGAMMA_LUT”:
>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>   *	from the framebuffer before it is given to the transformation matrix.
> @@ -85,6 +90,18 @@
>   * 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.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "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.
>   */
>  
>  /**
> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static char *color_encoding_name[] = {

Missing a few consts.

> +	[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 char *color_range_name[] = {

ditto.

> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr FULL RANGE",
> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr LIMITED RANGE",

Lowercase for the strings maybe? I guess we don't have any consistent
style in this regard.

> +};
> +
> +/**
> + * drm_plane_create_color_properties - color encoding related plane properties
> + * @supported_encodings: bitfield indicating supported color encdings
> + * @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 to the drm_plane object. The supported encodings and
> + * ranges should be provided in supported_encodings and
> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
> + * the its number as enum value being 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)];
> +	unsigned int i, len;

'unsigned int i' might surprise someone if they ever try to change the
loop to go backwards. Better make it signed IMO.

> +
> +
> +	if (WARN_ON(plane->color_encoding_property != NULL) ||
> +	    WARN_ON(plane->color_range_property != NULL))
> +		return -EEXIST;

I think I already stated that we don't have these sorts of checks
elsewhere, so IMO no point in adding them here either unless you
go and change all the other places too.

Just feels like accumulating random differences that will confuse
people in the future.

> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> +		if ((BIT(i) & supported_encodings) == 0)

'foo & BIT(i)' would be the more conventional order.

> +			continue;
> +
> +		enum_list[i].type = i;
> +		enum_list[i].name = color_encoding_name[i];

enum_list[len] ?

> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_ENCODING",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_encoding);

Missing the plane->state->whatever setup here.

> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> +		if ((BIT(i) & supported_ranges) == 0)
> +			continue;
> +
> +		enum_list[i].type = i;
> +		enum_list[i].name = color_range_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_RANGE",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_range_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_range);

Same issues here

I was somewhat wondering if we should even have these two props handled
in the same function, but I suppose it does make some sense. Not much
point in adding just one but not the other.

> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d6..a6762e8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	kfree(plane->name);
>  
> +	if (plane->color_encoding_property)
> +		drm_property_destroy(dev, plane->color_encoding_property);
> +
> +	if (plane->color_range_property)
> +		drm_property_destroy(dev, plane->color_range_property);

Not needed.

> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cb..7a1af63 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,8 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
> +struct drm_prop_enum_list;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +39,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 = 0,

= 0 seems pointless.

> +	DRM_COLOR_YCBCR_BT709,
> +	DRM_COLOR_YCBCR_BT2020,
> +	DRM_COLOR_ENCODING_MAX,
> +};
> +
> +enum drm_color_range {
> +	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,

ditto

> +	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 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* Color encoding for non RGB formats */
> +	enum drm_color_encoding color_encoding;
> +	enum drm_color_range color_range;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -523,6 +528,9 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	struct drm_property *color_encoding_property;
> +	struct drm_property *color_range_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> -- 
> 1.9.1

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

Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Jyri Sarha]

On 05/15/17 16:34, Ville Syrjälä wrote:
> On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
>> Add a standard optinal 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.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
>>  drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_plane.c      |  6 +++
>>  include/drm/drm_color_mgmt.h     | 20 +++++++++
>>  include/drm/drm_plane.h          |  8 ++++
>>  5 files changed, 139 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 4033384..f341dda 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -774,6 +774,10 @@ 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);
>> @@ -834,6 +838,10 @@ int drm_atomic_plane_set_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 533f3a3..2e04ebb 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -33,6 +33,11 @@
>>   * properties on the &drm_crtc object. They are set up by calling
>>   * drm_crtc_enable_color_mgmt().
>>   *
>> + * Support for different non RGB color encodings is controlled trough
>> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.
> 
> Putting this comment in middle of the crtc stuff seems a bit strange
> when the rest of the plane stuff is at the end. 
> 

Ok. I remove them. Of course in the future some one may want to attach
those proerties to crtcs too, but that is a different story.

> This should specify somewhere that these plane properties control
> the plane input and not the output.
> 

Ok. I'll make the documentation more verbose.

>> + *
>> + * The &drm_crtc object's properties are:
>> + *
>>   * "DEGAMMA_LUT”:
>>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>>   *	from the framebuffer before it is given to the transformation matrix.
>> @@ -85,6 +90,18 @@
>>   * 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.
>> + *
>> + * The &drm_plane object's properties are:
>> + *
>> + * "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.
>>   */
>>  
>>  /**
>> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>  	drm_modeset_unlock(&crtc->mutex);
>>  	return ret;
>>  }
>> +
>> +static char *color_encoding_name[] = {
> 
> Missing a few consts.
> 
>> +	[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 char *color_range_name[] = {
> 
> ditto.
> 

Will fix.

>> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr FULL RANGE",
>> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr LIMITED RANGE",
> 
> Lowercase for the strings maybe? I guess we don't have any consistent
> style in this regard.
> 

Lower case of everything but YCbCr, right?

>> +};
>> +
>> +/**
>> + * drm_plane_create_color_properties - color encoding related plane properties
>> + * @supported_encodings: bitfield indicating supported color encdings
>> + * @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 to the drm_plane object. The supported encodings and
>> + * ranges should be provided in supported_encodings and
>> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
>> + * the its number as enum value being 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)];
>> +	unsigned int i, len;
> 
> 'unsigned int i' might surprise someone if they ever try to change the
> loop to go backwards. Better make it signed IMO.
> 

Ok, will fix.

>> +
>> +
>> +	if (WARN_ON(plane->color_encoding_property != NULL) ||
>> +	    WARN_ON(plane->color_range_property != NULL))
>> +		return -EEXIST;
> 
> I think I already stated that we don't have these sorts of checks
> elsewhere, so IMO no point in adding them here either unless you
> go and change all the other places too.
> 
> Just feels like accumulating random differences that will confuse
> people in the future.
> 
>> +
>> +	len = 0;
>> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
>> +		if ((BIT(i) & supported_encodings) == 0)
> 
> 'foo & BIT(i)' would be the more conventional order.
> 

Ok, I'll swap the order.

>> +			continue;
>> +
>> +		enum_list[i].type = i;
>> +		enum_list[i].name = color_encoding_name[i];
> 
> enum_list[len] ?
> 

Argh. That's a real bug. Thanks, I'll fix that.

>> +		len++;
>> +	}
>> +
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +					"COLOR_ENCODING",
>> +					enum_list, len);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	plane->color_encoding_property = prop;
>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> 
> Missing the plane->state->whatever setup here.

What do you mean? Initialize the plane->state->color_encoding with the
default value?

There is no state allocated for a plane at initialization phase, where I
attach the properties, at least in omapdrm. I somehow expected this to
be taken care by the default value in the attach call, but now I see it
does not do that.

> 
>> +
>> +	len = 0;
>> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
>> +		if ((BIT(i) & supported_ranges) == 0)
>> +			continue;
>> +
>> +		enum_list[i].type = i;
>> +		enum_list[i].name = color_range_name[i];
>> +		len++;
>> +	}
>> +
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +					"COLOR_RANGE",
>> +					enum_list, len);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	plane->color_range_property = prop;
>> +	drm_object_attach_property(&plane->base, prop, default_range);
> 
> Same issues here
> 
> I was somewhat wondering if we should even have these two props handled
> in the same function, but I suppose it does make some sense. Not much
> point in adding just one but not the other.
> 

I was thinking about the same thing, and decided that if only one range
is supported, it is better to expose that as a singleton enum as well.

>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index fedd4d6..a6762e8 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
>>  
>>  	kfree(plane->name);
>>  
>> +	if (plane->color_encoding_property)
>> +		drm_property_destroy(dev, plane->color_encoding_property);
>> +
>> +	if (plane->color_range_property)
>> +		drm_property_destroy(dev, plane->color_range_property);
> 
> Not needed.
> 

Is that some late addition? I remember having some trouble earlier and
added these because of that. Maybe I remember wrong...

>> +
>>  	memset(plane, 0, sizeof(*plane));
>>  }
>>  EXPORT_SYMBOL(drm_plane_cleanup);
>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>> index 03a59cb..7a1af63 100644
>> --- a/include/drm/drm_color_mgmt.h
>> +++ b/include/drm/drm_color_mgmt.h
>> @@ -26,6 +26,8 @@
>>  #include <linux/ctype.h>
>>  
>>  struct drm_crtc;
>> +struct drm_plane;
>> +struct drm_prop_enum_list;
>>  
>>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>  
>> @@ -37,4 +39,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 = 0,
> 
> = 0 seems pointless.
> 

I added them just for stating that the code breaks if the enums do not
start from 0, but I can remove that.

>> +	DRM_COLOR_YCBCR_BT709,
>> +	DRM_COLOR_YCBCR_BT2020,
>> +	DRM_COLOR_ENCODING_MAX,
>> +};
>> +
>> +enum drm_color_range {
>> +	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
> 
> ditto
> 
>> +	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 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
>>  	unsigned int zpos;
>>  	unsigned int normalized_zpos;
>>  
>> +	/* Color encoding for non RGB formats */
>> +	enum drm_color_encoding color_encoding;
>> +	enum drm_color_range color_range;
>> +
>>  	/* Clipped coordinates */
>>  	struct drm_rect src, dst;
>>  
>> @@ -523,6 +528,9 @@ struct drm_plane {
>>  
>>  	struct drm_property *zpos_property;
>>  	struct drm_property *rotation_property;
>> +
>> +	struct drm_property *color_encoding_property;
>> +	struct drm_property *color_range_property;
>>  };
>>  
>>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>> -- 
>> 1.9.1
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Ville Syrjälä]

On Mon, May 15, 2017 at 05:25:02PM +0300, Jyri Sarha wrote:
> On 05/15/17 16:34, Ville Syrjälä wrote:
> > On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
> >> Add a standard optinal 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.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
> >>  drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_plane.c      |  6 +++
> >>  include/drm/drm_color_mgmt.h     | 20 +++++++++
> >>  include/drm/drm_plane.h          |  8 ++++
> >>  5 files changed, 139 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 4033384..f341dda 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -774,6 +774,10 @@ 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);
> >> @@ -834,6 +838,10 @@ int drm_atomic_plane_set_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 533f3a3..2e04ebb 100644
> >> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >> @@ -33,6 +33,11 @@
> >>   * properties on the &drm_crtc object. They are set up by calling
> >>   * drm_crtc_enable_color_mgmt().
> >>   *
> >> + * Support for different non RGB color encodings is controlled trough
> >> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.
> > 
> > Putting this comment in middle of the crtc stuff seems a bit strange
> > when the rest of the plane stuff is at the end. 
> > 
> 
> Ok. I remove them. Of course in the future some one may want to attach
> those proerties to crtcs too, but that is a different story.

True. But at that point I guess we'll have to reword the comment anyway.

> 
> > This should specify somewhere that these plane properties control
> > the plane input and not the output.
> > 
> 
> Ok. I'll make the documentation more verbose.
> 
> >> + *
> >> + * The &drm_crtc object's properties are:
> >> + *
> >>   * "DEGAMMA_LUT”:
> >>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> >>   *	from the framebuffer before it is given to the transformation matrix.
> >> @@ -85,6 +90,18 @@
> >>   * 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.
> >> + *
> >> + * The &drm_plane object's properties are:
> >> + *
> >> + * "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.
> >>   */
> >>  
> >>  /**
> >> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >>  	drm_modeset_unlock(&crtc->mutex);
> >>  	return ret;
> >>  }
> >> +
> >> +static char *color_encoding_name[] = {
> > 
> > Missing a few consts.
> > 
> >> +	[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 char *color_range_name[] = {
> > 
> > ditto.
> > 
> 
> Will fix.
> 
> >> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr FULL RANGE",
> >> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr LIMITED RANGE",
> > 
> > Lowercase for the strings maybe? I guess we don't have any consistent
> > style in this regard.
> > 
> 
> Lower case of everything but YCbCr, right?

That would work for me. But as stated, we already totally lack a
consistent style so I guess it doesn't matter too much in the end.
Same goes for the prop names, basic atomic props are all caps, 
zpos/rotation are all lowercase, and many other props are somewhere
in between.

> 
> >> +};
> >> +
> >> +/**
> >> + * drm_plane_create_color_properties - color encoding related plane properties
> >> + * @supported_encodings: bitfield indicating supported color encdings
> >> + * @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 to the drm_plane object. The supported encodings and
> >> + * ranges should be provided in supported_encodings and
> >> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
> >> + * the its number as enum value being 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)];
> >> +	unsigned int i, len;
> > 
> > 'unsigned int i' might surprise someone if they ever try to change the
> > loop to go backwards. Better make it signed IMO.
> > 
> 
> Ok, will fix.
> 
> >> +
> >> +
> >> +	if (WARN_ON(plane->color_encoding_property != NULL) ||
> >> +	    WARN_ON(plane->color_range_property != NULL))
> >> +		return -EEXIST;
> > 
> > I think I already stated that we don't have these sorts of checks
> > elsewhere, so IMO no point in adding them here either unless you
> > go and change all the other places too.
> > 
> > Just feels like accumulating random differences that will confuse
> > people in the future.
> > 
> >> +
> >> +	len = 0;
> >> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> >> +		if ((BIT(i) & supported_encodings) == 0)
> > 
> > 'foo & BIT(i)' would be the more conventional order.
> > 
> 
> Ok, I'll swap the order.
> 
> >> +			continue;
> >> +
> >> +		enum_list[i].type = i;
> >> +		enum_list[i].name = color_encoding_name[i];
> > 
> > enum_list[len] ?
> > 
> 
> Argh. That's a real bug. Thanks, I'll fix that.
> 
> >> +		len++;
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> +					"COLOR_ENCODING",
> >> +					enum_list, len);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	plane->color_encoding_property = prop;
> >> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> > 
> > Missing the plane->state->whatever setup here.
> 
> What do you mean? Initialize the plane->state->color_encoding with the
> default value?
> 
> There is no state allocated for a plane at initialization phase, where I
> attach the properties, at least in omapdrm. I somehow expected this to
> be taken care by the default value in the attach call, but now I see it
> does not do that.

Yeah. For the props we just do 'if (state) state->foo = bar;'.

> 
> > 
> >> +
> >> +	len = 0;
> >> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> >> +		if ((BIT(i) & supported_ranges) == 0)
> >> +			continue;
> >> +
> >> +		enum_list[i].type = i;
> >> +		enum_list[i].name = color_range_name[i];
> >> +		len++;
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> +					"COLOR_RANGE",
> >> +					enum_list, len);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	plane->color_range_property = prop;
> >> +	drm_object_attach_property(&plane->base, prop, default_range);
> > 
> > Same issues here
> > 
> > I was somewhat wondering if we should even have these two props handled
> > in the same function, but I suppose it does make some sense. Not much
> > point in adding just one but not the other.
> > 
> 
> I was thinking about the same thing, and decided that if only one range
> is supported, it is better to expose that as a singleton enum as well.
> 
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index fedd4d6..a6762e8 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >>  
> >>  	kfree(plane->name);
> >>  
> >> +	if (plane->color_encoding_property)
> >> +		drm_property_destroy(dev, plane->color_encoding_property);
> >> +
> >> +	if (plane->color_range_property)
> >> +		drm_property_destroy(dev, plane->color_range_property);
> > 
> > Not needed.
> > 
> 
> Is that some late addition? I remember having some trouble earlier and
> added these because of that. Maybe I remember wrong...

I believe all props should end up on some global list that gets
cleaned up by the core.

> 
> >> +
> >>  	memset(plane, 0, sizeof(*plane));
> >>  }
> >>  EXPORT_SYMBOL(drm_plane_cleanup);
> >> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> >> index 03a59cb..7a1af63 100644
> >> --- a/include/drm/drm_color_mgmt.h
> >> +++ b/include/drm/drm_color_mgmt.h
> >> @@ -26,6 +26,8 @@
> >>  #include <linux/ctype.h>
> >>  
> >>  struct drm_crtc;
> >> +struct drm_plane;
> >> +struct drm_prop_enum_list;
> >>  
> >>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> >>  
> >> @@ -37,4 +39,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 = 0,
> > 
> > = 0 seems pointless.
> > 
> 
> I added them just for stating that the code breaks if the enums do not
> start from 0, but I can remove that.

I'm pretty sure enums always start at 0.

> 
> >> +	DRM_COLOR_YCBCR_BT709,
> >> +	DRM_COLOR_YCBCR_BT2020,
> >> +	DRM_COLOR_ENCODING_MAX,
> >> +};
> >> +
> >> +enum drm_color_range {
> >> +	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
> > 
> > ditto
> > 
> >> +	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 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
> >>  	unsigned int zpos;
> >>  	unsigned int normalized_zpos;
> >>  
> >> +	/* Color encoding for non RGB formats */
> >> +	enum drm_color_encoding color_encoding;
> >> +	enum drm_color_range color_range;
> >> +
> >>  	/* Clipped coordinates */
> >>  	struct drm_rect src, dst;
> >>  
> >> @@ -523,6 +528,9 @@ struct drm_plane {
> >>  
> >>  	struct drm_property *zpos_property;
> >>  	struct drm_property *rotation_property;
> >> +
> >> +	struct drm_property *color_encoding_property;
> >> +	struct drm_property *color_range_property;
> >>  };
> >>  
> >>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> >> -- 
> >> 1.9.1
> > 

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

Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Jyri Sarha]

On 05/15/17 18:19, Ville Syrjälä wrote:
> On Mon, May 15, 2017 at 05:25:02PM +0300, Jyri Sarha wrote:
>> On 05/15/17 16:34, Ville Syrjälä wrote:
>>> On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
...
>>>> +		len++;
>>>> +	}
>>>> +
>>>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>>>> +					"COLOR_ENCODING",
>>>> +					enum_list, len);
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	plane->color_encoding_property = prop;
>>>> +	drm_object_attach_property(&plane->base, prop, default_encoding);
>>>
>>> Missing the plane->state->whatever setup here.
>>
>> What do you mean? Initialize the plane->state->color_encoding with the
>> default value?
>>
>> There is no state allocated for a plane at initialization phase, where I
>> attach the properties, at least in omapdrm. I somehow expected this to
>> be taken care by the default value in the attach call, but now I see it
>> does not do that.
> 
> Yeah. For the props we just do 'if (state) state->foo = bar;'.
> 

Isn't it rather useless to initialize the state variables at property
attach time, in case we happen to have them available, and then just
forget about it?

Shouldn't there be some automation to initialize a new state structures
with the default values?

Am I missing something here?

...
>>>> @@ -37,4 +39,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 = 0,
>>>
>>> = 0 seems pointless.
>>>
>>
>> I added them just for stating that the code breaks if the enums do not
>> start from 0, but I can remove that.
> 
> I'm pretty sure enums always start at 0.
> 

Sure, I was just thinking if someone gets clever and later marks the
enum to start from 1 to avoid 0 for some special purpose... but yes,
still quite pointless. I am happy to remove the assignment.

Best regards,
Jyri

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel