drm: Add properties to control YCbCr to RGB conversion

Add standard properties to control YCbCr to RGB conversion in DRM planes. The created properties are stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. For simplicity the related property blobs for CSC matrix and YCbCr preoffsets are also stored in the same place. 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        |  26 +++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c         |  10 +++
 include/drm/drm_color_mgmt.h        |  23 ++++++
 include/drm/drm_plane.h             |  10 +++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 223 insertions(+), 3 deletions(-)

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

index f881319..d1512aa 100644
@@ -732,6 +732,8 @@ 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);

@@ -774,6 +776,22 @@ 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_to_rgb_mode_property) {
+		state->ycbcr_to_rgb_mode = val;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_csc,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_preoffset,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -834,6 +852,14 @@ 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_to_rgb_mode_property) {
+		*val = state->ycbcr_to_rgb_mode;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		*val = state->ycbcr_to_rgb_csc ?
+			state->ycbcr_to_rgb_csc->base.id : 0;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		*val = state->ycbcr_to_rgb_preoffset ?
+			state->ycbcr_to_rgb_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 c3994b4..89fd826 100644
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_to_rgb_csc)
+		drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+	if (state->ycbcr_to_rgb_preoffset)
+		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3236,6 +3242,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_to_rgb_csc);
+	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

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

index cc23b9a..badaddd 100644
@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
  *
  * "DEGAMMA_LUT”:
  *	Blob property to set the degamma lookup table (LUT) mapping pixel data

@@ -85,6 +90,28 @@
  * 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_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *	supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_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
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
  */
 
 /**

@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+static char *ycbcr_to_rgb_mode_name[] = {

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;
+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}

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

index bc71aa2..4c7e827 100644
@@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->ycbcr_to_rgb_mode_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
+
+	if (plane->ycbcr_to_rgb_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
+
+	if (plane->ycbcr_to_rgb_preoffset_property)
+		drm_property_destroy(dev,
+				     plane->ycbcr_to_rgb_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +23 -0

index 03a59cb..a20b3ff 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,25 @@ 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_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
 #endif

include/drm/drm_plane.h +10 -0

index 9ab3e70..41dcde2 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,11 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
+	struct drm_property_blob *ycbcr_to_rgb_csc;
+	struct drm_property_blob *ycbcr_to_rgb_preoffset;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 

@@ -523,6 +529,10 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_to_rgb_mode_property;
+	struct drm_property *ycbcr_to_rgb_csc_property;
+	struct drm_property *ycbcr_to_rgb_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

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

index 8c67fc0..27e0bee 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_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

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

---
 include/drm/drm_color_mgmt.h | 2 ++
 1 file changed, 2 insertions(+)

include/drm/drm_color_mgmt.h +2 -0

index bce4a53..03a59cb 100644
@@ -25,6 +25,8 @@
 
 #include <linux/ctype.h>
 
+struct drm_crtc;
+
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t
 bit_precision);
 

void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,

Change drm_atomic_replace_property_blob_from_id()'s first parameter from drm_crtc to drm_device, so that the function can be used for other drm_mode_objects too.

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

---
 drivers/gpu/drm/drm_atomic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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

index 9b892af..f881319 100644
@@ -425,7 +425,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
 drm_crtc_state *state, }
 
 static int
-drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
+drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
 					 struct drm_property_blob **blob,
 					 uint64_t blob_id,
 					 ssize_t expected_size,

@@ -434,7 +434,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
 drm_crtc_state *state, struct drm_property_blob *new_blob = NULL;
 
 	if (blob_id != 0) {
-		new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
+		new_blob = drm_property_lookup_blob(dev, blob_id);
 		if (new_blob == NULL)
 			return -EINVAL;
 

@@ -483,7 +483,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		drm_property_blob_put(mode);
 		return ret;
 	} else if (property == config->degamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
 					val,
 					-1,

@@ -491,7 +491,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->ctm_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->ctm,
 					val,
 					sizeof(struct drm_color_ctm),

@@ -499,7 +499,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->gamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->gamma_lut,
 					val,
 					-1,

From: Tomi Valkeinen <[email protected]>

The setup code for color space conversion is a bit messy. This patch cleans it up.

For some reason the TRM uses values in YCrCb order, which is also used in the current driver, whereas everywhere else it's YCbCr (which also matches YUV order). This patch changes the tables to use the common order to avoid confusion.

The tables are split into separate lines, and comments added for clarity.

WB color conversion registers are similar but different than non-WB, but the same function was used to write both. It worked fine because the coef table was adjusted accordingly, but that was rather confusing. This patch adds a separate function to write the WB values so that the coef table can be written in an understandable way.

Signed-off-by Tomi Valkeinen <[email protected]>

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

---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 59 +++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 15 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/dss/dispc.c index 5ac0145..b53e63d 100644

drivers/gpu/drm/omapdrm/dss/dispc.c +24 -7

@@ -293,11 +293,6 @@ struct dispc_gamma_desc {
 	},
 };
 
-struct color_conv_coef {
-	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
-	int full_range;
-};
-
 static unsigned long dispc_fclk_rate(void);
 static unsigned long dispc_core_clk_rate(void);
 static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);

@@ -757,9 +752,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id
 plane, int fir_hinc, }
 }
 
+struct csc_coef_yuv2rgb {
+	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
+	bool full_range;
+};
+
+struct csc_coef_rgb2yuv {
+	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
+	bool full_range;
+};
 
 static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
-		const struct color_conv_coef *ct)
+		const struct csc_coef_yuv2rgb *ct)
 {
 #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
 

@@ -769,7 +773,24 @@ static void dispc_ovl_write_color_conv_coef(enum
 omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
 CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
 CVAL(0, ct->bcb));
 
-	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
+
+#undef CVAL
+}
+
+static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
 *ct) +{
+	const enum omap_plane_id plane = OMAP_DSS_WB;
+
+#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
+
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct-
 crg));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct-

cbr));

drivers/gpu/drm/omapdrm/dss/dispc.c +3 -0

+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
+
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
 
 #undef CVAL
 }

@@ -778,20 +799,28 @@ static void dispc_setup_color_conv_coef(void)
 {
 	int i;

drivers/gpu/drm/omapdrm/dss/dispc.c +6 -3

+		false,			/* limited range */
 	};
-	const struct color_conv_coef ctbl_bt601_5_wb = {
-		/* RGB -> YUV */
-		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
+
+	/* RGB -> YUV, ITU-R BT.601, limited range */
+	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
+		 66, 129,  25,		/* yr,   yg,  yb */
+		-38, -74, 112,		/* cbr, cbg, cbb */

drivers/gpu/drm/omapdrm/dss/dispc.c +1 -0

+		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
 }
 
 static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

From: Tomi Valkeinen <[email protected]>

At the moment the driver always uses limited range when doing YUV-RGB conversions. This patch adds full-range tables, and makes the code to always use full-range tables.

In the future we should allow the user to select the color range instead of hardcoding it.

Signed-off-by Tomi Valkeinen <[email protected]>

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

---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/dss/dispc.c index b53e63d..f2a2d08 100644

drivers/gpu/drm/omapdrm/dss/dispc.c +2 -0

@@ -799,6 +799,8 @@ static void dispc_setup_color_conv_coef(void)
 {
 	int i;
 	int num_ovl = dss_feat_get_num_ovls();
+	/* always use full range for now */
+	bool use_full_range = true;
 
 	/* YUV -> RGB, ITU-R BT.601, limited range */
 	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

drivers/gpu/drm/omapdrm/dss/dispc.c +22 -2

+
 	/* RGB -> YUV, ITU-R BT.601, limited range */
 	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
 		 66, 129,  25,		/* yr,   yg,  yb */

@@ -816,11 +826,30 @@ static void dispc_setup_color_conv_coef(void)
 		false,			/* limited range */
 	};
 
+	/* RGB -> YUV, ITU-R BT.601, full range */
+	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {
+		 77,  150,  29,		/* yr,   yg,  yb */
+		-43,  -85, 128,		/* cbr, cbg, cbb */
+		128, -107, -21,		/* crr, crg, crb */
+		true,			/* full range */
+	};
+
+	const struct csc_coef_yuv2rgb *yuv2rgb;
+	const struct csc_coef_rgb2yuv *rgb2yuv;
+
+	if (use_full_range) {
+		yuv2rgb = &coefs_yuv2rgb_bt601_full;
+		rgb2yuv = &coefs_rgb2yuv_bt601_full;
+	} else {
+		yuv2rgb = &coefs_yuv2rgb_bt601_lim;
+		rgb2yuv = &coefs_rgb2yuv_bt601_lim;
+	}
+
 	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
+		dispc_ovl_write_color_conv_coef(i, yuv2rgb);
 
 	if (dispc.feat->has_writeback)
-		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
+		dispc_wb_write_color_conv_coef(rgb2yuv);
 }
 
 static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

Quoted 18 lines · Jyri Sarha

Add standard properties to control YCbCr to RGB conversion in DRM planes. The created properties are stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. For simplicity the related property blobs for CSC matrix and YCbCr preoffsets are also stored in the same place. 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        |  26 +++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 136 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c         |  10 +++
 include/drm/drm_color_mgmt.h        |  23 ++++++
 include/drm/drm_plane.h             |  10 +++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 223 insertions(+), 3 deletions(-)

Quoted 60 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644

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

@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set

of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
*
* "DEGAMMA_LUT”:
* Blob property to set the degamma lookup table (LUT) mapping pixel data

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

@@ -85,6 +90,28 @@
  * 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_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *		supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_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
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
  */

/**

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

@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+static char *ycbcr_to_rgb_mode_name[] = {

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

Quoted 10 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING.

OTOH if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though. I don't think most people are in the habit if cooking up new ways to encode their pixel data.

Quoted 35 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related

properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */

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

+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,

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

Quoted 8 lines

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

+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;

Quoted 49 lines

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

+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}

Quoted 4 lines

include/drm/drm_color_mgmt.h +0 -0

index 03a59cb..a20b3ff 100644

Quoted 4 lines

include/drm/drm_color_mgmt.h +0 -0

@@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 

Quoted 4 lines

include/drm/drm_color_mgmt.h +4 -0

+enum drm_plane_ycbcr_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,

include/drm/drm_color_mgmt.h void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, · line 55

Quoted 10 lines

include/drm/drm_color_mgmt.h +10 -0

+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,

Quoted 8 lines

include/drm/drm_color_mgmt.h +7 -0

+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
 #endif

Quoted 24 lines

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

index 8c67fc0..27e0bee 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_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

The series adds plane specific atomic properties to control YCbCr to RGB conversions. My intention was to try to implement the plane specific (before DEGAMMA) part of the suggestion in this dri-devel post:

https://lists.freedesktop.org/archives/dri-devel/2017-March/135870.html

This series may not be ready as such. At least the kernel doc parts should be more detailed and carefully written. The purpose is merely to move the discussion to a more concrete level.

The series also includes drm/omap patches that implement the standard properties for OMAP DSS in omapdrm driver.

Best regards,
Jyri

Jyri Sarha (4):
drm: drm_color_mgmt.h needs struct drm_crtc declaration
drm: Make drm_atomic_replace_property_blob_from_id() more generic
drm: Plane YCbCr to RGB conversion related properties drm/omap: Enable ycbcr_to_rgb_properties for omapdrm planes REVISIT

Tomi Valkeinen (2):
drm/omap: cleanup color space conversion
drm/omap: csc full range support

  drivers/gpu/drm/drm_atomic.c          |  36 +++++++--
  drivers/gpu/drm/drm_atomic_helper.c   |   9 +++
  drivers/gpu/drm/drm_color_mgmt.c      | 136 +++++++++++++++++++++++++++++++-
  drivers/gpu/drm/drm_plane.c           |  10 +++
  drivers/gpu/drm/omapdrm/dss/dispc.c   | 141 +++++++++++++++++++++++++++++-----
  drivers/gpu/drm/omapdrm/dss/omapdss.h |  14 ++++
  drivers/gpu/drm/omapdrm/omap_plane.c  |  41 ++++++++++
  include/drm/drm_color_mgmt.h          |  25 ++++++
  include/drm/drm_plane.h               |  10 +++
  include/uapi/drm/drm_mode.h           |  12 +++
  10 files changed, 408 insertions(+), 26 deletions(-)

Quoted 153 lines · Jyri Sarha

Add standard properties to control YCbCr to RGB conversion in DRM planes. The created properties are stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. For simplicity the related property blobs for CSC matrix and YCbCr preoffsets are also stored in the same place. 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        |  26 +++++++
  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
  drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
  drivers/gpu/drm/drm_plane.c         |  10 +++
  include/drm/drm_color_mgmt.h        |  23 ++++++
  include/drm/drm_plane.h             |  10 +++
  include/uapi/drm/drm_mode.h         |  12 ++++
  7 files changed, 223 insertions(+), 3 deletions(-)

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

index f881319..d1512aa 100644
@@ -732,6 +732,8 @@ 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);

@@ -774,6 +776,22 @@ 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_to_rgb_mode_property) {
+		state->ycbcr_to_rgb_mode = val;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_csc,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_preoffset,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_preoffset),
+				&dummy);
+		return ret;
  	} else if (plane->funcs->atomic_set_property) {
  		return plane->funcs->atomic_set_property(plane, state,
  				property, val);

@@ -834,6 +852,14 @@ 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_to_rgb_mode_property) {
+		*val = state->ycbcr_to_rgb_mode;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		*val = state->ycbcr_to_rgb_csc ?
+			state->ycbcr_to_rgb_csc->base.id : 0;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		*val = state->ycbcr_to_rgb_preoffset ?
+			state->ycbcr_to_rgb_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 c3994b4..89fd826 100644
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
  {
  	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_to_rgb_csc)
+		drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+	if (state->ycbcr_to_rgb_preoffset)
+		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
  	if (state->fb)
  		drm_framebuffer_get(state->fb);
 

@@ -3236,6 +3242,9 @@ struct drm_plane_state *
   */
  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  {
+	drm_property_blob_put(state->ycbcr_to_rgb_csc);
+	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
  	if (state->fb)
  		drm_framebuffer_put(state->fb);
 

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

index cc23b9a..badaddd 100644
@@ -29,9 +29,14 @@
  /**
   * DOC: overview
   *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
   *
   * "DEGAMMA_LUT”:
   *	Blob property to set the degamma lookup table (LUT) mapping pixel data

@@ -85,6 +90,28 @@
   * 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_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *	supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_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
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
   */
 
  /**

@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  	drm_modeset_unlock_all(dev);
  	return ret;
  }
+
+static char *ycbcr_to_rgb_mode_name[] = {

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

Quoted 10 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though. I don't think most people are in the habit if cooking up new ways to encode their pixel data.

Quoted 203 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;
+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}

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

index bc71aa2..4c7e827 100644
@@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
  	kfree(plane->name);
 
+	if (plane->ycbcr_to_rgb_mode_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
+
+	if (plane->ycbcr_to_rgb_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
+
+	if (plane->ycbcr_to_rgb_preoffset_property)
+		drm_property_destroy(dev,
+				     plane->ycbcr_to_rgb_preoffset_property);
+
  	memset(plane, 0, sizeof(*plane));
  }
  EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +23 -0

index 03a59cb..a20b3ff 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,25 @@ 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_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
  #endif

include/drm/drm_plane.h +10 -0

index 9ab3e70..41dcde2 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,11 @@ struct drm_plane_state {
  	unsigned int zpos;
  	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
+	struct drm_property_blob *ycbcr_to_rgb_csc;
+	struct drm_property_blob *ycbcr_to_rgb_preoffset;
+
  	/* Clipped coordinates */
  	struct drm_rect src, dst;
 

@@ -523,6 +529,10 @@ struct drm_plane {
 
  	struct drm_property *zpos_property;
  	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_to_rgb_mode_property;
+	struct drm_property *ycbcr_to_rgb_csc_property;
+	struct drm_property *ycbcr_to_rgb_preoffset_property;
  };
 
  #define obj_to_plane(x) container_of(x, struct drm_plane, base)

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

index 8c67fc0..27e0bee 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
  	__u16 reserved;
  };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_preoffset {
+	/* Offset vector in 2-complement s.32 format. */

Quoted 8 lines

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

+	__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

Hello,

CC'ing Hans Verkuil for his knowledge on colorspace.

Quoted 19 lines · On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:

Quoted 18 lines · Jyri Sarha

Add standard properties to control YCbCr to RGB conversion in DRM planes. The created properties are stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. For simplicity the related property blobs for CSC matrix and YCbCr preoffsets are also stored in the same place. 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        |  26 +++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 136 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c         |  10 +++
 include/drm/drm_color_mgmt.h        |  23 ++++++
 include/drm/drm_plane.h             |  10 +++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 223 insertions(+), 3 deletions(-)

[snip]

Quoted 77 lines

Quoted 60 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644

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

@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set

of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
*
* "DEGAMMA_LUT”:
* Blob property to set the degamma lookup table (LUT) mapping pixel data

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

@@ -85,6 +90,28 @@
  * 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_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *		supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_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
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
  */

/**

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

@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+static char *ycbcr_to_rgb_mode_name[] = {

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

Quoted 10 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 3 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING.

For quite obvious reasons I agree with this partial reasoning :-) I would also copy how V4L2 splits color space information into a transfer function, an encoding and a quantization. If you group all three in a single enum you will end up with lots of possible combinations.

Quoted 4 lines

OTOH if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though. I don't think most people are in the habit if cooking up new ways to encode their pixel data.

I believe it would be more about supporting weird color encodings that are in the wild out there in various media sources. I'm not an expert in the field of color spaces though.

Quoted 36 lines

Quoted 36 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related

properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */

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

+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,

Quoted 8 lines

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

Quoted 8 lines

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

+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;

i takes positive values only, you can make it an unsigned int.

Quoted 49 lines

Quoted 49 lines

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

+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}

[snip]

Quoted 4 lines

Quoted 4 lines

include/drm/drm_color_mgmt.h +0 -0

index 03a59cb..a20b3ff 100644

[snip]

Quoted 4 lines

Quoted 4 lines

include/drm/drm_color_mgmt.h +0 -0

@@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 

Quoted 14 lines

Quoted 4 lines

include/drm/drm_color_mgmt.h +4 -0

+enum drm_plane_ycbcr_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,

include/drm/drm_color_mgmt.h void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, · line 55

Quoted 10 lines

include/drm/drm_color_mgmt.h +10 -0

+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,

All those values should be documented.

Should the CSC coefficient tables be centralized in core code ? Patch 5/6 adds tables to the omapdrm driver, for standard conversions it would make sense to share that data.

Quoted 8 lines

Quoted 8 lines

include/drm/drm_color_mgmt.h +7 -0

+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
 #endif

[snip]

Quoted 24 lines

Quoted 24 lines

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

index 8c67fc0..27e0bee 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_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 · 3 lines

Regards,

Laurent Pinchart

Quoted 8 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =

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

Quoted 3 lines

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

+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

The series adds plane specific atomic properties to control YCbCr to RGB conversions. My intention was to try to implement the plane specific (before DEGAMMA) part of the suggestion in this dri-devel post:

https://lists.freedesktop.org/archives/dri-devel/2017-March/135870.html

This series may not be ready as such. At least the kernel doc parts should be more detailed and carefully written. The purpose is merely to move the discussion to a more concrete level.

The series also includes drm/omap patches that implement the standard properties for OMAP DSS in omapdrm driver.

Best regards,
Jyri

Jyri Sarha (4):
drm: drm_color_mgmt.h needs struct drm_crtc declaration
drm: Make drm_atomic_replace_property_blob_from_id() more generic
drm: Plane YCbCr to RGB conversion related properties drm/omap: Enable ycbcr_to_rgb_properties for omapdrm planes REVISIT

Tomi Valkeinen (2):
drm/omap: cleanup color space conversion
drm/omap: csc full range support

 drivers/gpu/drm/drm_atomic.c          |  36 +++++++--
 drivers/gpu/drm/drm_atomic_helper.c   |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c      | 136 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c           |  10 +++
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 141 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  14 ++++
 drivers/gpu/drm/omapdrm/omap_plane.c  |  41 ++++++++++
 include/drm/drm_color_mgmt.h          |  25 ++++++
 include/drm/drm_plane.h               |  10 +++
 include/uapi/drm/drm_mode.h           |  12 +++
 10 files changed, 408 insertions(+), 26 deletions(-)

Signature · 1 line

1.9.1

Quoted 8 lines · Ville Syrjälä

Quoted 8 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =

Quoted 9 lines

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

Quoted 3 lines

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

+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

Quoted 10 lines · Jyri Sarha

Quoted 9 lines · Ville Syrjälä

Quoted 9 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",

Quoted 8 lines

Quoted 8 lines

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

Quoted 2 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 6 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 5 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

Quoted 4 lines

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

Signature · 2 lines

Ville Syrjälä
Intel OTC

Hi,

Thanks for picking this up Jyri.

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Jyri Sarha

Quoted 9 lines · Ville Syrjälä

Quoted 9 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 2 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

I thought the conclusion of the last round was that on some hardware this would be conflated as a conscious choice. If the HW doesn't have the required degamma->csc->gamma stages, and the implementor/user doesn't care about being a little incorrect, then it can all be described in this single property.

If there is more capable hardware with the additional stages, then they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
Does YCbCr->RGB conversion on non-linear YCbCr data, only the enum values which don't have a CSC are exposed.

DEGAMMA:
Does the non-linear to linear conversion on the RGB data output from the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC on the linear data.

GAMMA:
Convert the CSC'd RGB data back in to non-linear for blending (if blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their hardware supports.

Quoted 7 lines

Quoted 6 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Quoted 9 lines

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

I'm not sure the custom blob is worth having either. It can easily be added later if we decide we want it after all.

Thanks,
-Brian

Quoted 40 lines

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 9 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 25 lines · Brian Starkey

Hi,

Thanks for picking this up Jyri.

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Jyri Sarha

Quoted 9 lines · Ville Syrjälä

Quoted 9 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 2 lines

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

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 5 lines

I thought the conclusion of the last round was that on some hardware this would be conflated as a conscious choice. If the HW doesn't have the required degamma->csc->gamma stages, and the implementor/user doesn't care about being a little incorrect, then it can all be described in this single property.

I was proposing the single prop approach initially, but now I think it might just lead to more confusion. So a dedicated property for each stage is the clearer design I think. We do lose potentially a bit of discoverability when not all combinations are supported, but we have that problem in many other places as well, so not a big deal I think.

Quoted 7 lines

If there is more capable hardware with the additional stages, then they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
Does YCbCr->RGB conversion on non-linear YCbCr data, only the enum values which don't have a CSC are exposed.

Not sure what you mean but that last part.

Quoted 8 lines

DEGAMMA:
Does the non-linear to linear conversion on the RGB data output from the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC on the linear data.

We may want to call this just CTM to match the matching crtc prop.

Quoted 65 lines

GAMMA:
Convert the CSC'd RGB data back in to non-linear for blending (if blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their hardware supports.

Quoted 7 lines

Quoted 6 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Quoted 9 lines

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

I'm not sure the custom blob is worth having either. It can easily be added later if we decide we want it after all.

Thanks,
-Brian

Quoted 34 lines

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 9 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

Quoted 6 lines

Quoted 6 lines

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

--
Ville Syrjälä
Intel OTC

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 32 lines · Ville Syrjälä

Quoted 25 lines · Brian Starkey

Hi,

Thanks for picking this up Jyri.

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Jyri Sarha

Quoted 9 lines · Ville Syrjälä

Quoted 9 lines

(quoted diff) +9 -0

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 5 lines

I thought the conclusion of the last round was that on some hardware this would be conflated as a conscious choice. If the HW doesn't have the required degamma->csc->gamma stages, and the implementor/user doesn't care about being a little incorrect, then it can all be described in this single property.

Quoted 6 lines

I was proposing the single prop approach initially, but now I think it might just lead to more confusion. So a dedicated property for each stage is the clearer design I think. We do lose potentially a bit of discoverability when not all combinations are supported, but we have that problem in many other places as well, so not a big deal I think.

Yeah you lose discoverability, but do you also lose the ability to do the non-perfect, single-stage conversions?

For HW that only has a matrix, is the driver expected to combine all of the separated stages down into a single matrix? Or it wouldn't expose the other properties, only a matrix, and userspace has to come up with a blob that does the (approximate) right thing?

Quoted 11 lines

Quoted 8 lines

If there is more capable hardware with the additional stages, then they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
Does YCbCr->RGB conversion on non-linear YCbCr data, only the enum values which don't have a CSC are exposed.

Not sure what you mean but that last part.

Just that the enum list should only contain things that are:
YCbCr BT.601 -> RGB BT.601
YCbCr BT.709 -> RGB BT.709
YcbCr BT.2020 -> RGB BT.2020

and not a those including a color space change, e.g.:
YCbCr BT.601 -> RGB BT.709

because an RGB BT.601 -> RGB BT.709 conversion can be performed later with the RGB_TO_RGB/CTM/whatever property.

Quoted 11 lines

Quoted 8 lines

DEGAMMA:
Does the non-linear to linear conversion on the RGB data output from the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC on the linear data.

We may want to call this just CTM to match the matching crtc prop.

Good call.

-Brian

Quoted 75 lines

Quoted 71 lines

GAMMA:
Convert the CSC'd RGB data back in to non-linear for blending (if blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their hardware supports.

Quoted 7 lines

Quoted 6 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Quoted 9 lines

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

I'm not sure the custom blob is worth having either. It can easily be added later if we decide we want it after all.

Thanks,
-Brian

Quoted 40 lines

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 9 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

--
Ville Syrjälä
Intel OTC

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 40 lines · Brian Starkey

Quoted 32 lines · Ville Syrjälä

Quoted 25 lines · Brian Starkey

Hi,

Thanks for picking this up Jyri.

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Jyri Sarha

Quoted 9 lines · Ville Syrjälä

Quoted 9 lines

(quoted diff) +9 -0

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 5 lines

I thought the conclusion of the last round was that on some hardware this would be conflated as a conscious choice. If the HW doesn't have the required degamma->csc->gamma stages, and the implementor/user doesn't care about being a little incorrect, then it can all be described in this single property.

Quoted 6 lines

I was proposing the single prop approach initially, but now I think it might just lead to more confusion. So a dedicated property for each stage is the clearer design I think. We do lose potentially a bit of discoverability when not all combinations are supported, but we have that problem in many other places as well, so not a big deal I think.

Quoted 2 lines

Yeah you lose discoverability, but do you also lose the ability to do the non-perfect, single-stage conversions?

Not sure why one vs. multiple props should matter here. Either you accept the less than perfect pipeline or you don't. Whether you ask it via one of multiple props doesn't seem all that different to me.

Quoted 5 lines

For HW that only has a matrix, is the driver expected to combine all of the separated stages down into a single matrix? Or it wouldn't expose the other properties, only a matrix, and userspace has to come up with a blob that does the (approximate) right thing?

If you're not happy with exposing just one or the other, then I guess you would expose both but indicate that there's no degamma in between and userspace can then choose whether it's happy with that solution or or not.

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

If there is more capable hardware with the additional stages, then they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
Does YCbCr->RGB conversion on non-linear YCbCr data, only the enum values which don't have a CSC are exposed.

Not sure what you mean but that last part.

Quoted 7 lines

Just that the enum list should only contain things that are:
YCbCr BT.601 -> RGB BT.601
YCbCr BT.709 -> RGB BT.709
YcbCr BT.2020 -> RGB BT.2020

and not a those including a color space change, e.g.:
YCbCr BT.601 -> RGB BT.709

Right. We probably shouldn't even have the "BT.whatever" on both sides since it's not really a colorspace, just the encoding, and once you decoded the YCbCr into RGB it's just RGB. We could actually just define the enum values as "BT.601","BT.709" etc.

Quoted 88 lines

because an RGB BT.601 -> RGB BT.709 conversion can be performed later with the RGB_TO_RGB/CTM/whatever property.

Quoted 11 lines

Quoted 8 lines

DEGAMMA:
Does the non-linear to linear conversion on the RGB data output from the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC on the linear data.

We may want to call this just CTM to match the matching crtc prop.

Good call.

-Brian

Quoted 68 lines

Quoted 68 lines

GAMMA:
Convert the CSC'd RGB data back in to non-linear for blending (if blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their hardware supports.

Quoted 7 lines

Quoted 6 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Quoted 9 lines

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

I'm not sure the custom blob is worth having either. It can easily be added later if we decide we want it after all.

Thanks,
-Brian

Quoted 37 lines

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 9 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

--
Ville Syrjälä
Intel OTC

--
Ville Syrjälä
Intel OTC

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 19 lines · Jyri Sarha

Quoted 8 lines · Ville Syrjälä

Quoted 8 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =

Quoted 9 lines

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

Quoted 3 lines

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

+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 5 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

In the future we should be getting a more fully fleged pipeline.

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

Quoted 4 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

Quoted 5 lines

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

Quoted 5 lines

Quoted 4 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

Quoted 4 lines

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

Quoted 29 lines · Ville Syrjälä

Quoted 19 lines · Jyri Sarha

Quoted 8 lines · Ville Syrjälä

Quoted 8 lines

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

+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =

Quoted 9 lines

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

Quoted 3 lines

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

+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",

We probably don't want to conflate the YCbCr->RGB part with the colorspace conversion because the YCbCr->RGB part should be performed on gamma encoded data and the colorspace conversion on linear data. So we need a degamma stage in between. At least that seemed to be the general concencus after the last round of mails on this topic.

Quoted 5 lines

I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them.

Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages.

Quoted 8 lines

On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it.

In the future we should be getting a more fully fleged pipeline.

If going forward with these patches, then maybe it is better to stick with the enum options that we are sure of, e.g. BT.601, BT.709, and BT.2020 to full range RGB. It is easy enough to add more enum values in the future.

What is more important is the naming approach, whether we keep the conversion mode naming explicit about the output format, or just specify the output format implicitly to be always full range (non linear) RGB?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

After staring at the v4l docs on this stuff I kinda like their "encoding" terminology to describe the YCbCr->RGB part, so I'm now a little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

No strong opinions here. I am fine with any of the names that have been suggested so far.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

if we want to expose the raw matrix as a blob then maybe calling it a CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction.

Quoted 3 lines

Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level.

No, but we need such a property to CRTC output, and there we certainly need to expose the CSC, because we do not have CTM (before gamma) matrix, and may have to use the output CSC in its place.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

I don't think most people are in the habit if cooking up new ways to encode their pixel data.

In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area.

Quoted 3 lines

I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.

I am on thin ice here :), but surely there is no harm in specifying an optional property for exposing the CSC as many display HWs provide a way to set an arbitrary CSC.

What about the uapi structs? In the patch there is an explicit struct naming each coefficient for what they are for in YCbCr to RGB conversion. Is this Ok, or should we go with a generic (CTM style) matrix, that could be used for RGB to YCbCr conversion too?

Quoted 4 lines

What about the uapi structs? In the patch there is an explicit struct naming each coefficient for what they are for in YCbCr to RGB conversion. Is this Ok, or should we go with a generic (CTM style) matrix, that could be used for RGB to YCbCr conversion too?

Not sure what we're talking about here, but like I said I think we should stick to a fairly limited set of standard props and just have each driver map the hardware resources to them as best they can.

If you just have csc+(de)gamma then I guess it might make sense to just expose the YCbCr->RGB and degamma. If you have degamma+csc+gamma then it might make sense to expose both YCbCr->RGB, degamma, CTM, and gamma, and just refuse any combination that can't be done. Eg. can't do YCbCr->RGB if degamma is used, and YCbCr->RGB + CTM would require multiplying the matrices together which you may or may not want to bother with, I guess we could try to put some matrix math helpers into the core to make such things less painful for drivers?

Quoted 8 lines · Ville Syrjälä

Quoted 4 lines

What about the uapi structs? In the patch there is an explicit struct naming each coefficient for what they are for in YCbCr to RGB conversion. Is this Ok, or should we go with a generic (CTM style) matrix, that could be used for RGB to YCbCr conversion too?

Not sure what we're talking about here, but like I said I think we should stick to a fairly limited set of standard props and just have each driver map the hardware resources to them as best they can.

Just about the implementation detail, if we should have a separate uapi struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use the same struct, then we could as well use the already existing CTM struct.

Quoted 9 lines

If you just have csc+(de)gamma then I guess it might make sense to just expose the YCbCr->RGB and degamma. If you have degamma+csc+gamma then it might make sense to expose both YCbCr->RGB, degamma, CTM, and gamma, and just refuse any combination that can't be done. Eg. can't do YCbCr->RGB if degamma is used, and YCbCr->RGB + CTM would require multiplying the matrices together which you may or may not want to bother with, I guess we could try to put some matrix math helpers into the core to make such things less painful for drivers?

In fact we have plane specific YCbCr to RGB CSC (only preoffset possible), then (per crtc) gamma table, and finally a (per crtc) RGB to YCbCr CSC with optional post offset (so it can be used either as CSC or CTM).

Quoted 4 lines

In fact we have plane specific YCbCr to RGB CSC (only preoffset possible), then (per crtc) gamma table, and finally a (per crtc) RGB to YCbCr CSC with optional post offset (so it can be used either as CSC or CTM).

So with that plane hw you could perhaps do:
- YCbCr->RGB if you input is not linear, but then you must blend using non-linear data
- colorspace conversion if your input is alredy linear

And with your crtc hw you could do:
- degamma + CTM
- gamma + RGB->YCbCr

Quoted 4 lines · Ville Syrjälä

Quoted 4 lines

In fact we have plane specific YCbCr to RGB CSC (only preoffset possible), then (per crtc) gamma table, and finally a (per crtc) RGB to YCbCr CSC with optional post offset (so it can be used either as CSC or CTM).

Quoted 8 lines

So with that plane hw you could perhaps do:
- YCbCr->RGB if you input is not linear, but then you must blend using non-linear data
- colorspace conversion if your input is alredy linear

And with your crtc hw you could do:
- degamma + CTM
- gamma + RGB->YCbCr

Just a generic question. Shouldn't - in an ideal HW - the degamma phase and the CTM be a plane specific property?

I mean, isn't the purpose of normalizing the non linear RGB to linear (and possibly converting the color space) to have same format for all plane data before blending and composing them together?

Of course it does not matter if all the planes use the same color space, which then should be converted to something else for the output.

... or have I misunderstood something?

Cheers,
Jyri

Quoted 4 lines · Ville Syrjälä

Quoted 4 lines

In fact we have plane specific YCbCr to RGB CSC (only preoffset possible), then (per crtc) gamma table, and finally a (per crtc) RGB to YCbCr CSC with optional post offset (so it can be used either as CSC or CTM).

Quoted 8 lines

So with that plane hw you could perhaps do:
- YCbCr->RGB if you input is not linear, but then you must blend using non-linear data
- colorspace conversion if your input is alredy linear

And with your crtc hw you could do:
- degamma + CTM
- gamma + RGB->YCbCr

Just a generic question. Shouldn't - in an ideal HW - the degamma phase and the CTM be a plane specific property?

I mean, isn't the purpose of normalizing the non linear RGB to linear (and possibly converting the color space) to have same format for all plane data before blending and composing them together?

Of course it does not matter if all the planes use the same color space, which then should be converted to something else for the output.

... or have I misunderstood something?

Hi Jyri,

Thank you for the patch.

Quoted 5 lines · On Friday 21 Apr 2017 12:51:13 Jyri Sarha wrote:

Change drm_atomic_replace_property_blob_from_id()'s first parameter from drm_crtc to drm_device, so that the function can be used for other drm_mode_objects too.

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

Reviewed-by Laurent Pinchart <[email protected]>

Quoted 54 lines

---
 drivers/gpu/drm/drm_atomic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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

index 9b892af..f881319 100644
@@ -425,7 +425,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
 drm_crtc_state *state, }
 
 static int
-drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
+drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
 					 struct drm_property_blob **blob,
 					 uint64_t blob_id,
 					 ssize_t expected_size,

@@ -434,7 +434,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
 drm_crtc_state *state, struct drm_property_blob *new_blob = NULL;
 
 	if (blob_id != 0) {
-		new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
+		new_blob = drm_property_lookup_blob(dev, blob_id);
 		if (new_blob == NULL)
 			return -EINVAL;
 

@@ -483,7 +483,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		drm_property_blob_put(mode);
 		return ret;
 	} else if (property == config->degamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
 					val,
 					-1,

@@ -491,7 +491,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->ctm_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->ctm,
 					val,
 					sizeof(struct drm_color_ctm),

@@ -499,7 +499,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->gamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->gamma_lut,
 					val,
 					-1,

Signature · 3 lines

Regards,

Laurent Pinchart

Add standard properties to control YCbCr to RGB conversion in DRM planes. The created properties are stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. For simplicity the related property blobs for CSC matrix and YCbCr preoffsets are also stored in the same place. 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        |  26 +++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c         |  10 +++
 include/drm/drm_color_mgmt.h        |  23 ++++++
 include/drm/drm_plane.h             |  10 +++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 223 insertions(+), 3 deletions(-)

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

index f881319..d1512aa 100644
@@ -732,6 +732,8 @@ 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);

@@ -774,6 +776,22 @@ 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_to_rgb_mode_property) {
+		state->ycbcr_to_rgb_mode = val;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_csc,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_preoffset,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);

@@ -834,6 +852,14 @@ 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_to_rgb_mode_property) {
+		*val = state->ycbcr_to_rgb_mode;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		*val = state->ycbcr_to_rgb_csc ?
+			state->ycbcr_to_rgb_csc->base.id : 0;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		*val = state->ycbcr_to_rgb_preoffset ?
+			state->ycbcr_to_rgb_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 c3994b4..89fd826 100644
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_to_rgb_csc)
+		drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+	if (state->ycbcr_to_rgb_preoffset)
+		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 

@@ -3236,6 +3242,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_to_rgb_csc);
+	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 

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

index cc23b9a..badaddd 100644
@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
  *
  * "DEGAMMA_LUT”:
  *	Blob property to set the degamma lookup table (LUT) mapping pixel data

@@ -85,6 +90,28 @@
  * 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_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *	supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_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
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
  */
 
 /**

@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;
+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}

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

index bc71aa2..4c7e827 100644
@@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->ycbcr_to_rgb_mode_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
+
+	if (plane->ycbcr_to_rgb_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
+
+	if (plane->ycbcr_to_rgb_preoffset_property)
+		drm_property_destroy(dev,
+				     plane->ycbcr_to_rgb_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);

include/drm/drm_color_mgmt.h +23 -0

index 03a59cb..a20b3ff 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,25 @@ 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_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
 #endif

include/drm/drm_plane.h +10 -0

index 9ab3e70..41dcde2 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,11 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
+	struct drm_property_blob *ycbcr_to_rgb_csc;
+	struct drm_property_blob *ycbcr_to_rgb_preoffset;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 

@@ -523,6 +529,10 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_to_rgb_mode_property;
+	struct drm_property *ycbcr_to_rgb_csc_property;
+	struct drm_property *ycbcr_to_rgb_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)

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

index 8c67fc0..27e0bee 100644
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_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