drm: Add properties to control YCbCr to RGB conversion

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

drivers/gpu/drm/drm_atomic.c +5 -0

index 4033384..bcef93d 100644
@@ -732,6 +732,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	int ret;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -774,6 +775,8 @@ 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->ycbcr_encoding_property) {
+		state->ycbcr_encoding = val;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -834,6 +837,8 @@ 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->ycbcr_encoding_property) {
+		*val = state->ycbcr_encoding;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {

drivers/gpu/drm/drm_color_mgmt.c +12 -0

index 533f3a3..245b14a 100644
@@ -33,6 +33,11 @@
  * properties on the &drm_crtc object. They are set up by calling
  * drm_crtc_enable_color_mgmt().
  *
+ * Support for different YCbCr color encodings is controlled trough
+ * plane specific YCBCR_ENCODING property.
+ *
+ * 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,13 @@
  * 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:
+ *
+ * "YCBCR_ENCODING"
+ * 	Optional plane enum property to control YCbCr to RGB
+ * 	conversion. The driver provides a subset of standard
+ *	enum values supported by the DRM plane.
  */
 
 /**

drivers/gpu/drm/drm_color_mgmt.c +9 -0

+	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
+	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
+	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+};
+
+/**
+ * drm_plane_create_ycbcr_properties - ycbcr related plane properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array

drivers/gpu/drm/drm_color_mgmt.c +4 -0

+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
+

drivers/gpu/drm/drm_color_mgmt.c +3 -0

+
+	return 0;
+}

drivers/gpu/drm/drm_plane.c +3 -0

index fedd4d6..007c4d7 100644
@@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->ycbcr_encoding_property)
+		drm_property_destroy(dev, plane->ycbcr_encoding_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +14 -0

index 03a59cb..1771394 100644
@@ -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,16 @@ 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_plane_ycbcr_encoding {
+	DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0,
+	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+};
+
+int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_encoding default_mode);
+
 #endif

include/drm/drm_plane.h +6 -0

index 9ab3e70..4d0510f 100644
@@ -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,9 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 

@@ -523,6 +527,8 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_encoding_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

Signature · 1 line

1.9.1

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644

drivers/gpu/drm/drm_color_mgmt.c +0 -0

drivers/gpu/drm/drm_color_mgmt.c +0 -0

@@ -85,6 +90,13 @@
  * 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:
+ *
+ * "YCBCR_ENCODING"
+ * Optional plane enum property to control YCbCr to RGB
+ * conversion. The driver provides a subset of standard
+ * enum values supported by the DRM plane.
*/

/**

drivers/gpu/drm/drm_color_mgmt.c +13 -0

@@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static char *ycbcr_encoding_name[] = {
+	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
+	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
+	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+};
+
+/**
+ * drm_plane_create_ycbcr_properties - ycbcr related plane properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default YCbCr encoding

drivers/gpu/drm/drm_color_mgmt.c +24 -0

+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	unsigned int i;
+
+	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_encoding_name[encoding];
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_ENCODING",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	return 0;
+}

drivers/gpu/drm/drm_plane.c +0 -0

index fedd4d6..007c4d7 100644

drivers/gpu/drm/drm_plane.c +1 -0

+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

Hi Jyri,

Thank you for the patch.

Quoted 12 lines · On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

[snip]

Quoted 4 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644

drivers/gpu/drm/drm_color_mgmt.c +0 -0

[snip]

Quoted 5 lines

drivers/gpu/drm/drm_color_mgmt.c +0 -0

@@ -85,6 +90,13 @@
  * 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.

Quoted 8 lines

+ *
+ * The &drm_plane object's properties are:
+ *
+ * "YCBCR_ENCODING"
+ * Optional plane enum property to control YCbCr to RGB
+ * conversion. The driver provides a subset of standard
+ * enum values supported by the DRM plane.
*/

As already mentioned by Hans Verkuil, I also recommend not mixing the encoding and quantization in a single property. If you split them, I would then drop the YCBCR_ prefix (or replace it by something more generic) at least for the quantization property, as it would apply to RGB as well. For the encoding property, we have support in V4L2 for a two HSV encodings, so it could make sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt we'll see any display hardware with native support for HSV :-)

Quoted 18 lines

/**

drivers/gpu/drm/drm_color_mgmt.c +13 -0

@@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static char *ycbcr_encoding_name[] = {
+	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
+	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
+	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+};
+
+/**
+ * drm_plane_create_ycbcr_properties - ycbcr related plane properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default YCbCr encoding

drivers/gpu/drm/drm_color_mgmt.c int drm_mode_gamma_get_ioctl(struct drm_device *dev, · line 370

Quoted 10 lines

drivers/gpu/drm/drm_color_mgmt.c +10 -0

+ *
+ * Create and attach plane specific YCBCR_ENCODING property to to the
+ * drm_plane object. The supported encodings should be provided in the
+ * enum_list parameter. The enum_list parameter should not contain the
+ * enum names, because the standard names are added by this function.
+ */
+int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_encoding default_mode)

I wonder whether we shouldn't simplify the API by passing a bitmask of supported encodings. Sure, you would have to allocate the array of drm_prop_enum_list internally in the function, but driver code would be simpler. Even if you don't like bitmasks, I think we should pass a const pointer and duplicate the array internally to fill the names. Drivers will in many cases pass the same array for all planes, modifying it in the function is asking for trouble (even if it should be OK with the current implementation).

By the way, for drivers that support the same encodings for all planes, would it be worth it to support allocation of a single property instead of allocating one per plane ?

Quoted 31 lines

drivers/gpu/drm/drm_color_mgmt.c +24 -0

+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	unsigned int i;
+
+	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_encoding_name[encoding];
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_ENCODING",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	return 0;
+}

drivers/gpu/drm/drm_plane.c +0 -0

index fedd4d6..007c4d7 100644
@@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
 

drivers/gpu/drm/drm_plane.c void drm_plane_cleanup(struct drm_plane *plane) · line 248

Quoted 4 lines

drivers/gpu/drm/drm_plane.c +2 -0

 	kfree(plane->name);
 
+	if (plane->ycbcr_encoding_property)
+		drm_property_destroy(dev, plane->ycbcr_encoding_property);

There's lots of similar code all over the place, I wonder whether we shouldn't move the NULL check to drm_property_destroy().

Quoted 4 lines

drivers/gpu/drm/drm_plane.c +1 -0

+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

[snip]

Quoted 38 lines · Laurent Pinchart

Quoted 13 lines · On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---

 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

[snip]

Quoted 4 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644

drivers/gpu/drm/drm_color_mgmt.c +0 -0

[snip]

drivers/gpu/drm/drm_color_mgmt.c line 101

Quoted 13 lines

drivers/gpu/drm/drm_color_mgmt.c +7 -0

@@ -85,6 +90,13 @@
  * 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:
+ *
+ * "YCBCR_ENCODING"
+ * 	Optional plane enum property to control YCbCr to RGB
+ * 	conversion. The driver provides a subset of standard
+ *	enum values supported by the DRM plane.
  */

Quoted 8 lines

As already mentioned by Hans Verkuil, I also recommend not mixing the encoding and quantization in a single property. If you split them, I would then drop the YCBCR_ prefix (or replace it by something more generic) at least for the quantization property, as it would apply to RGB as well. For the encoding property, we have support in V4L2 for a two HSV encodings, so it could make sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt we'll see any display hardware with native support for HSV :-)

COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.

Quoted 30 lines

Quoted 19 lines

drivers/gpu/drm/drm_color_mgmt.c +13 -0

 /**

@@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static char *ycbcr_encoding_name[] = {
+	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
+	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
+	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+};
+
+/**
+ * drm_plane_create_ycbcr_properties - ycbcr related plane properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default YCbCr encoding

drivers/gpu/drm/drm_color_mgmt.c int drm_mode_gamma_get_ioctl(struct drm_device *dev, · line 370

Quoted 10 lines

drivers/gpu/drm/drm_color_mgmt.c +10 -0

+ *
+ * Create and attach plane specific YCBCR_ENCODING property to to the
+ * drm_plane object. The supported encodings should be provided in the
+ * enum_list parameter. The enum_list parameter should not contain the
+ * enum names, because the standard names are added by this function.
+ */
+int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_encoding default_mode)

Quoted 8 lines

I wonder whether we shouldn't simplify the API by passing a bitmask of supported encodings. Sure, you would have to allocate the array of drm_prop_enum_list internally in the function, but driver code would be simpler. Even if you don't like bitmasks, I think we should pass a const pointer and duplicate the array internally to fill the names. Drivers will in many cases pass the same array for all planes, modifying it in the function is asking for trouble (even if it should be OK with the current implementation).

I used a bitmask property first, but abandoned it because the encodings do not behave like bitmasks. You can not have BT.601 | BT.709 at the same time.

A bitmask in the function API would certainly work and be probably better, but I've tried to keep the implementation simple, while we are still discussing what we should actually do.

Quoted 3 lines

By the way, for drivers that support the same encodings for all planes, would it be worth it to support allocation of a single property instead of allocating one per plane ?

I was thinking of that, but AFAIK there is really not that many planes on the know HW that it would justify the complexity.

Quoted 30 lines

Quoted 30 lines

drivers/gpu/drm/drm_color_mgmt.c +24 -0

+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	unsigned int i;
+
+	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_encoding_name[encoding];
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_ENCODING",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	return 0;
+}

drivers/gpu/drm/drm_plane.c +0 -0

index fedd4d6..007c4d7 100644
@@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
 

Quoted 7 lines

drivers/gpu/drm/drm_plane.c void drm_plane_cleanup(struct drm_plane *plane) · line 248

Quoted 4 lines

drivers/gpu/drm/drm_plane.c +2 -0

 	kfree(plane->name);
 
+	if (plane->ycbcr_encoding_property)
+		drm_property_destroy(dev, plane->ycbcr_encoding_property);

There's lots of similar code all over the place, I wonder whether we shouldn't move the NULL check to drm_property_destroy().

Absolutely.

Quoted 8 lines

Quoted 6 lines

drivers/gpu/drm/drm_plane.c +1 -0

+
 
 	memset(plane, 0, sizeof(*plane));
 

}
EXPORT_SYMBOL(drm_plane_cleanup);

[snip]

Hi Jyri,

Quoted 48 lines · On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:

Quoted 46 lines · Laurent Pinchart

Quoted 13 lines · On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---

 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

[snip]

Quoted 4 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644

drivers/gpu/drm/drm_color_mgmt.c +0 -0

[snip]

drivers/gpu/drm/drm_color_mgmt.c line 101

Quoted 13 lines

drivers/gpu/drm/drm_color_mgmt.c +7 -0

@@ -85,6 +90,13 @@
  * 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:
+ *
+ * "YCBCR_ENCODING"
+ * 	Optional plane enum property to control YCbCr to RGB
+ * 	conversion. The driver provides a subset of standard
+ *	enum values supported by the DRM plane.
  */

As already mentioned by Hans Verkuil, I also recommend not mixing the encoding and quantization in a single property. If you split them, I would then drop the YCBCR_ prefix (or replace it by something more generic) at least for the quantization property, as it would apply to RGB as well. For the encoding property, we have support in V4L2 for a two HSV encodings, so it could make sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt we'll see any display hardware with native support for HSV :-)

Quoted 1 line

COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.

Sounds good to me.

Quoted 50 lines · Laurent Pinchart

Quoted 49 lines · On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:

Quoted 47 lines · Laurent Pinchart

Quoted 13 lines · On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:

Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device.

Signed-off-by Jyri Sarha <[email protected]>

---

 drivers/gpu/drm/drm_atomic.c     |  5 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 59 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c      |  3 ++
 include/drm/drm_color_mgmt.h     | 14 ++++++++++
 include/drm/drm_plane.h          |  6 ++++
 5 files changed, 87 insertions(+)

[snip]

Quoted 4 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644

drivers/gpu/drm/drm_color_mgmt.c +0 -0

[snip]

Quoted 4 lines

drivers/gpu/drm/drm_color_mgmt.c +0 -0

@@ -85,6 +90,13 @@
  * drm_mode_crtc_set_gamma_size(). Drivers which support both should
  use
  * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp

drivers/gpu/drm/drm_color_mgmt.c line 102

Quoted 10 lines

drivers/gpu/drm/drm_color_mgmt.c +7 -0

  with the
  * "GAMMA_LUT" property above.
+ *
+ * The &drm_plane object's properties are:
+ *
+ * "YCBCR_ENCODING"
+ * 	Optional plane enum property to control YCbCr to RGB
+ * 	conversion. The driver provides a subset of standard
+ *	enum values supported by the DRM plane.
  */

As already mentioned by Hans Verkuil, I also recommend not mixing the encoding and quantization in a single property. If you split them, I would then drop the YCBCR_ prefix (or replace it by something more generic) at least for the quantization property, as it would apply to RGB as well. For the encoding property, we have support in V4L2 for a two HSV encodings, so it could make sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt we'll see any display hardware with native support for HSV :-)

Quoted 3 lines

Quoted 1 line

COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.

Sounds good to me.

I am now implementing this. However, there is a logical challenge in putting the two suggestions together, splitting the encoding and range to separate properties, and changing the property names to generic COLOR_ENCODING and COLOR_RANGE.

In general COLOR_RANGE enum values are only defined within the selected COLOR_ENCODING. With the standard YCbCr encodings this is not a problem, since they all define a "full range" and a "limted range". But if we are preparing for some unknown color ecodings, I am not sure how likely it is that "full range" and "limited range" options make sense there.

I can implement the properties for currently known YCbCr color encodings either way, either as YCbCr specific properties, or as generic COLOR_* properties for all non RGB encodings. I am just not sure if defining the generic properties would make any sense now that we (or at least I) have no idea what the other non RGB encodings could be. What do you think?

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
 drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_plane.c         |  6 ++++
 include/drm/drm_color_mgmt.h        |  3 ++
 include/drm/drm_plane.h             |  4 +++
 include/uapi/drm/drm_mode.h         | 12 ++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +19 -0

index bcef93d..87a2d55 100644
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->zpos = val;
 	} else if (property == plane->ycbcr_encoding_property) {
 		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->zpos;
 	} else if (property == plane->ycbcr_encoding_property) {
 		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {

drivers/gpu/drm/drm_atomic_helper.c +9 -0

index 8be9719..6ecc32f 100644
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3278,6 +3284,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

drivers/gpu/drm/drm_color_mgmt.c +53 -2

index 245b14a..319ed46 100644
@@ -95,8 +95,23 @@
  *
  * "YCBCR_ENCODING"
  * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
  */
 
 /**

@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
 	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
 	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
 };
 
 /**

@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  * drm_plane object. The supported encodings should be provided in the
  * enum_list parameter. The enum_list parameter should not contain the
  * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
  */
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 			struct drm_prop_enum_list *enum_list,

@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
 	unsigned int i;
 
 	if (WARN_ON(plane->ycbcr_encoding_property != NULL))

@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
 		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
 	}
 
 	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,

@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 	plane->ycbcr_encoding_property = prop;
 	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
 	return 0;
 }

drivers/gpu/drm/drm_plane.c +6 -0

index 007c4d7..d10c942 100644
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->ycbcr_encoding_property)
 		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +3 -0

index 1771394..bdfd37e 100644
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
 	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
 };
 
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,

include/drm/drm_plane.h +4 -0

index 4d0510f..63b1e0c 100644
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
 	/* YCbCr to RGB conversion */
 	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
 	/* Clipped coordinates */
 	struct drm_rect src, dst;

@@ -529,6 +531,8 @@ struct drm_plane {
 	struct drm_property *rotation_property;
 
 	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

include/uapi/drm/drm_mode.h +12 -0

index 8c67fc0..8c9568d 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Signature · 1 line

1.9.1

Quoted 7 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

And as usual, this needs some userspace compositor which actually uses it (not just a demo, since there's a huge difference between a demo and something that has to Just Work like a real compositor). -Daniel

Quoted 260 lines

---
 drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
 drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_plane.c         |  6 ++++
 include/drm/drm_color_mgmt.h        |  3 ++
 include/drm/drm_plane.h             |  4 +++
 include/uapi/drm/drm_mode.h         | 12 ++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +19 -0

index bcef93d..87a2d55 100644
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->zpos = val;
 	} else if (property == plane->ycbcr_encoding_property) {
 		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->zpos;
 	} else if (property == plane->ycbcr_encoding_property) {
 		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {

drivers/gpu/drm/drm_atomic_helper.c +9 -0

index 8be9719..6ecc32f 100644
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3278,6 +3284,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

drivers/gpu/drm/drm_color_mgmt.c +53 -2

index 245b14a..319ed46 100644
@@ -95,8 +95,23 @@
  *
  * "YCBCR_ENCODING"
  * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
  */
 
 /**

@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
 	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
 	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
 };
 
 /**

@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  * drm_plane object. The supported encodings should be provided in the
  * enum_list parameter. The enum_list parameter should not contain the
  * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
  */
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 			struct drm_prop_enum_list *enum_list,

@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
 	unsigned int i;
 
 	if (WARN_ON(plane->ycbcr_encoding_property != NULL))

@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
 		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
 	}
 
 	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,

@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 	plane->ycbcr_encoding_property = prop;
 	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
 	return 0;
 }

drivers/gpu/drm/drm_plane.c +6 -0

index 007c4d7..d10c942 100644
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->ycbcr_encoding_property)
 		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +3 -0

index 1771394..bdfd37e 100644
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
 	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
 };
 
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,

include/drm/drm_plane.h +4 -0

index 4d0510f..63b1e0c 100644
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
 	/* YCbCr to RGB conversion */
 	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
 	/* Clipped coordinates */
 	struct drm_rect src, dst;

@@ -529,6 +531,8 @@ struct drm_plane {
 	struct drm_property *rotation_property;
 
 	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

include/uapi/drm/drm_mode.h +12 -0

index 8c67fc0..8c9568d 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Signature · 1 line

1.9.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 8 lines · Daniel Vetter

Quoted 7 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Quoted 4 lines

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Daniel Vetter

Quoted 8 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

Quoted 7 lines

Quoted 3 lines

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

So, pulling the suggestions together the properties would now look like this:

YCBCR_RANGE
- Full
- Limited

YCBCR_ENCODING
- BT.601
- BT.709
- BT.2020

CTM (blob)

And all these properties could be added to individual planes. I'll try to come up with a new patch to add simple enum properties for YCBCR_PREOFFSET and YCBCR_ENCODING for planes. Should we need any more enum values for those?

The only functional thing that is missing from the above proposal is the possibility to define an arbitrary YCbCr preoffset vector, but that can be added later if there ever is any real need.

The other thing that could be added for special purposes would be adding an "Identity" enum to YCBCR_ENCODING, that would allow setting the YCbCr CSC trough CTM without any driver level multiplication getting in way, but that can also be added if there is a need.

I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the content of the framebuffer is encoded. If I understand correctly, you're proposing adding an enumeration value that tells the driver not to try to be clever and multiply the CTM matrix by the CSC matrix corresponding to the encoding. That would probably work in most cases, but it would combine two pieces of information in a single property. The driver would then lose the knowledge of how the plane framebuffer is encoded. Couldn't there be cases where that knowledge is needed for other purposes than picking the right CSC matrix ? If so, it might be better to always set the YCBCR_ENCODING property to the actual encoding, and have another property to tell the driver to skip multiplication by the CSC matrix. Or could that be conveyed through the CTM blob property ? Some kind of flag that would essentially tell that the CTM matrix has been pre-multiplied already ?

Before I forget, how do you plan to handle backward compatibility with userspace that won't set the YCBCR_ENCODING property ? Is that done by picking a driver-specific default value ? Do you think there would be a need for drivers to know that the property has not been set ?

Quoted 14 lines · Laurent Pinchart

I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the content of the framebuffer is encoded. If I understand correctly, you're proposing adding an enumeration value that tells the driver not to try to be clever and multiply the CTM matrix by the CSC matrix corresponding to the encoding. That would probably work in most cases, but it would combine two pieces of information in a single property. The driver would then lose the knowledge of how the plane framebuffer is encoded. Couldn't there be cases where that knowledge is needed for other purposes than picking the right CSC matrix ? If so, it might be better to always set the YCBCR_ENCODING property to the actual encoding, and have another property to tell the driver to skip multiplication by the CSC matrix. Or could that be conveyed through the CTM blob property ? Some kind of flag that would essentially tell that the CTM matrix has been pre-multiplied already ?

Quoted 4 lines

Before I forget, how do you plan to handle backward compatibility with userspace that won't set the YCBCR_ENCODING property ? Is that done by picking a driver-specific default value ? Do you think there would be a need for drivers to know that the property has not been set ?

Where do we need this? Afaik the encoding is to spec the yuv2rbg conversion function, and that's it. But I'm fairly ignorant about video and yuv and all these things. so does this specify something else? If not, I don't see any possibilities that someone could retrofit more meaning onto these conversion rules.

Quoted 10 lines · Daniel Vetter

Quoted 8 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

Quoted 3 lines

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

Quoted 272 lines

And as usual, this needs some userspace compositor which actually uses it (not just a demo, since there's a huge difference between a demo and something that has to Just Work like a real compositor). -Daniel

Quoted 260 lines

---
 drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
 drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_plane.c         |  6 ++++
 include/drm/drm_color_mgmt.h        |  3 ++
 include/drm/drm_plane.h             |  4 +++
 include/uapi/drm/drm_mode.h         | 12 ++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +19 -0

index bcef93d..87a2d55 100644
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->zpos = val;
 	} else if (property == plane->ycbcr_encoding_property) {
 		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->zpos;
 	} else if (property == plane->ycbcr_encoding_property) {
 		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {

drivers/gpu/drm/drm_atomic_helper.c +9 -0

index 8be9719..6ecc32f 100644
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3278,6 +3284,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

drivers/gpu/drm/drm_color_mgmt.c +53 -2

index 245b14a..319ed46 100644
@@ -95,8 +95,23 @@
  *
  * "YCBCR_ENCODING"
  * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
  */
 
 /**

@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
 	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
 	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
 };
 
 /**

@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  * drm_plane object. The supported encodings should be provided in the
  * enum_list parameter. The enum_list parameter should not contain the
  * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
  */
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 			struct drm_prop_enum_list *enum_list,

@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
 	unsigned int i;
 
 	if (WARN_ON(plane->ycbcr_encoding_property != NULL))

@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
 		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
 	}
 
 	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,

@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 	plane->ycbcr_encoding_property = prop;
 	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
 	return 0;
 }

drivers/gpu/drm/drm_plane.c +6 -0

index 007c4d7..d10c942 100644
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->ycbcr_encoding_property)
 		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +3 -0

index 1771394..bdfd37e 100644
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
 	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
 };
 
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,

include/drm/drm_plane.h +4 -0

index 4d0510f..63b1e0c 100644
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
 	/* YCbCr to RGB conversion */
 	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
 	/* Clipped coordinates */
 	struct drm_rect src, dst;

@@ -529,6 +531,8 @@ struct drm_plane {
 	struct drm_property *rotation_property;
 
 	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

include/uapi/drm/drm_mode.h +12 -0

index 8c67fc0..8c9568d 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Signature · 1 line

1.9.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 9 lines · Ville Syrjälä

Quoted 8 lines · Daniel Vetter

Quoted 7 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard). That seems a bit an awkward interface. Or is your idea that hw with a ctm would simply not expose the colorspace enum, if it doesn't also have fixed-function bits on top of the ctm?
-Daniel

Quoted 269 lines

Quoted 269 lines

And as usual, this needs some userspace compositor which actually uses it (not just a demo, since there's a huge difference between a demo and something that has to Just Work like a real compositor). -Daniel

Quoted 260 lines

---
  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
  drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
  drivers/gpu/drm/drm_plane.c         |  6 ++++
  include/drm/drm_color_mgmt.h        |  3 ++
  include/drm/drm_plane.h             |  4 +++
  include/uapi/drm/drm_mode.h         | 12 ++++++++
  7 files changed, 106 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +19 -0

index bcef93d..87a2d55 100644
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
  	struct drm_device *dev = plane->dev;
  	struct drm_mode_config *config = &dev->mode_config;
  	int ret;
+	bool dummy;
 
  	if (property == config->prop_fb_id) {
  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
  		state->zpos = val;
  	} else if (property == plane->ycbcr_encoding_property) {
  		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
  	} else if (plane->funcs->atomic_set_property) {
  		return plane->funcs->atomic_set_property(plane, state,
  				property, val);

@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
  		*val = state->zpos;
  	} else if (property == plane->ycbcr_encoding_property) {
  		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
  	} else if (plane->funcs->atomic_get_property) {
  		return plane->funcs->atomic_get_property(plane, state, property, val);
  	} else {

drivers/gpu/drm/drm_atomic_helper.c +9 -0

index 8be9719..6ecc32f 100644
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
  {
  	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
  	if (state->fb)
  		drm_framebuffer_get(state->fb);
 

@@ -3278,6 +3284,9 @@ struct drm_plane_state *
   */
  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
  	if (state->fb)
  		drm_framebuffer_put(state->fb);
 

drivers/gpu/drm/drm_color_mgmt.c +53 -2

index 245b14a..319ed46 100644
@@ -95,8 +95,23 @@
   *
   * "YCBCR_ENCODING"
   * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
   */
 
  /**

@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
  	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
  	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
  };
 
  /**

@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
   * drm_plane object. The supported encodings should be provided in the
   * enum_list parameter. The enum_list parameter should not contain the
   * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
   */
  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
  			struct drm_prop_enum_list *enum_list,

@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
  {
  	struct drm_device *dev = plane->dev;
  	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
  	unsigned int i;
 
  	if (WARN_ON(plane->ycbcr_encoding_property != NULL))

@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
  		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
  		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
  	}
 
  	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,

@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
  	plane->ycbcr_encoding_property = prop;
  	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
  	return 0;
  }

drivers/gpu/drm/drm_plane.c +6 -0

index 007c4d7..d10c942 100644
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
  	if (plane->ycbcr_encoding_property)
  		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
  	memset(plane, 0, sizeof(*plane));
  }
  EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +3 -0

index 1771394..bdfd37e 100644
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
  	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
  	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
  	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
  };
 
  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,

include/drm/drm_plane.h +4 -0

index 4d0510f..63b1e0c 100644
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
  	/* YCbCr to RGB conversion */
  	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
  	/* Clipped coordinates */
  	struct drm_rect src, dst;

@@ -529,6 +531,8 @@ struct drm_plane {
  	struct drm_property *rotation_property;
 
  	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
  };
 
  #define obj_to_plane(x) container_of(x, struct drm_plane, base)

include/uapi/drm/drm_mode.h +12 -0

index 8c67fc0..8c9568d 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
  	__u16 reserved;
  };
 
+struct drm_ycbcr_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Signature · 1 line

1.9.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 3 lines

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 25 lines · Daniel Vetter

Quoted 14 lines · Ville Syrjälä

Quoted 12 lines · Daniel Vetter

Quoted 9 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

Quoted 8 lines

Quoted 4 lines

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead
is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

Quoted 7 lines

But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard). That seems a bit an awkward interface. Or is your idea that hw with a ctm would simply not expose the colorspace enum, if it doesn't also have fixed-function bits on top of the ctm?
-Daniel

I think the idea is to have separate properties for CTM and Gamut Mapping, so that we can have more control
over linear and non-linear data blocks. All transformation should happen in one property whereas all gamut
mapping should go into other, which IMHO seems to be the correct way to operate.

Quoted 26 lines · Shashank

Quoted 18 lines · Daniel Vetter

Quoted 9 lines · Ville Syrjälä

Quoted 8 lines · Daniel Vetter

Quoted 7 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead
is the generic csc support, plus a ycbcr2rgb mode of "bypass".

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

Quoted 7 lines

But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard). That seems a bit an awkward interface. Or is your idea that hw with a ctm would simply not expose the colorspace enum, if it doesn't also have fixed-function bits on top of the ctm?
-Daniel

Quoted 6 lines

I think the idea is to have separate properties for CTM and Gamut Mapping, so that we can have more control
over linear and non-linear data blocks. All transformation should happen in one property whereas all gamut
mapping should go into other, which IMHO seems to be the correct way to operate.

Yeah, for the programmable hw we should probably go with the degamma/ctm/gamma combo, like we have on the CRTC already. My question was how this interactions with some fixed-function color space conversion the hw might also have, and how these two sets of properties are mean to interact.

On that topic, I think it'd be good if we put the helper functions and property documentation into drm_color_mgmt.c, so that it is all in one place, and to make sure we don't accidentally have different meanings for gamma table and ctm blobs.

-Daniel

Quoted 15 lines · Ville Syrjälä

Quoted 13 lines · Daniel Vetter

Quoted 8 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

Quoted 3 lines

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard).

That seems a bit an
awkward interface. Or is your idea that hw with a ctm would simply not expose the colorspace enum, if it doesn't also have fixed-function bits on top of the ctm?
-Daniel

Quoted 276 lines

Quoted 272 lines

And as usual, this needs some userspace compositor which actually uses it (not just a demo, since there's a huge difference between a demo and something that has to Just Work like a real compositor). -Daniel

Quoted 260 lines

---
 drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
 drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_plane.c         |  6 ++++
 include/drm/drm_color_mgmt.h        |  3 ++
 include/drm/drm_plane.h             |  4 +++
 include/uapi/drm/drm_mode.h         | 12 ++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +19 -0

index bcef93d..87a2d55 100644
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);

@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->zpos = val;
 	} else if (property == plane->ycbcr_encoding_property) {
 		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->zpos;
 	} else if (property == plane->ycbcr_encoding_property) {
 		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {

drivers/gpu/drm/drm_atomic_helper.c +9 -0

index 8be9719..6ecc32f 100644
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3278,6 +3284,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

drivers/gpu/drm/drm_color_mgmt.c +53 -2

index 245b14a..319ed46 100644
@@ -95,8 +95,23 @@
  *
  * "YCBCR_ENCODING"
  * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
  */
 
 /**

@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
 	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
 	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
 };
 
 /**

@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  * drm_plane object. The supported encodings should be provided in the
  * enum_list parameter. The enum_list parameter should not contain the
  * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
  */
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 			struct drm_prop_enum_list *enum_list,

@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
 	unsigned int i;
 
 	if (WARN_ON(plane->ycbcr_encoding_property != NULL))

@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
 		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
 	}
 
 	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,

@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 	plane->ycbcr_encoding_property = prop;
 	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
 	return 0;
 }

drivers/gpu/drm/drm_plane.c +6 -0

index 007c4d7..d10c942 100644
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->ycbcr_encoding_property)
 		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +3 -0

index 1771394..bdfd37e 100644
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
 	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
 };
 
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,

include/drm/drm_plane.h +4 -0

index 4d0510f..63b1e0c 100644
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
 	/* YCbCr to RGB conversion */
 	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
 	/* Clipped coordinates */
 	struct drm_rect src, dst;

@@ -529,6 +531,8 @@ struct drm_plane {
 	struct drm_property *rotation_property;
 
 	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

include/uapi/drm/drm_mode.h +12 -0

index 8c67fc0..8c9568d 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

Signature · 1 line

1.9.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Ville Syrjälä
Intel OTC

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 16 lines · Daniel Vetter

Quoted 15 lines · Ville Syrjälä

Quoted 13 lines · Daniel Vetter

Quoted 8 lines · Jyri Sarha

Add standard optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. The blob contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by Jyri Sarha <[email protected]>

Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".

Quoted 7 lines

Quoted 3 lines

My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw.

But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard).

I don't see what this has to do with bit correctness. Different hw is likely to use different precision for this stuff anyway, so I'm 100% sure we'll not get the same results from all hardware. And really, getting 100% same output doesn't seem all that important here anyway.

Anyway, all I'm saying is that we should expose a standard pipeline:
ycbcr->rgb -> degamma -> ctm -> gamma

And each driver can then expose whatever parts of that they wish. If you want to expoe a programmable matrix you expose it as the ctm. I don't think having a programmable matrix as both ycbcr->rgb and ctm would really benefit us.