On Fri, Apr 21, 2017 at 02:47:44PM +0300, Laurent Pinchart wrote:

Hi Jyri,

Thank you for the patch.

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 jsarha@ti.com

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

First two patches applied for 4.13, thanks. -Daniel

...

---

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..f881319 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -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,
  

-- Regards,

Laurent Pinchart

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:

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 jsarha@ti.com

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(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f881319..d1512aa 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -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 {

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c3994b4..89fd826 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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);

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -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",
  

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.

• [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;

+} diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index bc71aa2..4c7e827 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -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); diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cb..a20b3ff 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -26,6 +26,8 @@ #include <linux/ctype.h>

struct drm_crtc; +struct drm_plane; +struct drm_prop_enum_list;

uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);

@@ -37,4 +39,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 diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e70..41dcde2 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -26,6 +26,7 @@ #include <linux/list.h> #include <linux/ctype.h> #include <drm/drm_mode_object.h> +#include <drm/drm_color_mgmt.h>

struct drm_crtc; struct drm_printer; @@ -112,6 +113,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) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 8c67fc0..27e0bee 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -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

1.9.1

On Fri, Apr 21, 2017 at 03:10:31PM +0300, Laurent Pinchart wrote:

Hello,

CC'ing Hans Verkuil for his knowledge on colorspace.

On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:

...

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:

...

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 jsarha@ti.com

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]

...

...

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -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",
  

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.

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.

...

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.

We can always add more values to the enum if new useful encodings crop up. I'd be more inclined to exposing the blob if some media formats would allow you to specify a custom matrix as well. Not sure any do.

I guess maybe the only real benefit from exposing the blob would be that you could then combine the encoding and colorspace conversion stages to the one matrix if you for some reason decide that degamma is not your thing. Otherwise we'd have to multiply the matrices in the kernel, which I guess we may have to anyway in some cases. At least i915 tries to do that (unsuccesfully I might add) for the output CSC stuff.

Anyways, I just wanted to point out that I think exposing the blob propably won't have any real users, so not sure it's worth the hassle. But if people find it useful I won't object to having it.

...

...

• [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;

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

...

...

• 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]

...

...

diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cb..a20b3ff 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h

[snip]

...

...

@@ -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,

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.

...

...

+};

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

...

...

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 8c67fc0..27e0bee 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -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

-- Regards,

Laurent Pinchart

On 05/02/17 11:33, Daniel Vetter wrote:

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:

...

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 jsarha@ti.com

Just to make sure there's no surprises: We need the userspace for this too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone, whatever you feel like) or drm_hwcomposer.

But yeah might be good to bikeshed the uabi first a bit more and get at least some agreement on that.

In the first phase I have been using kms++ [1]. And when/if we have an agreement about the API, I will push my patches there.

With X, wayland, and other compositors, we would in the end need some video player supporting the different YCbCr encodings according to the video stream. This sounds like a relatively big task, but surely I volunteer to assist, when we get there.

Cheer, Jyri

[1] https://github.com/tomba/kmsxx/

Thanks, Daniel

...

---

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(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f881319..d1512aa 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -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 {

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c3994b4..89fd826 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -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);

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -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;

+} diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index bc71aa2..4c7e827 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -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); diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cb..a20b3ff 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -26,6 +26,8 @@ #include <linux/ctype.h>

struct drm_crtc; +struct drm_plane; +struct drm_prop_enum_list;

uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);

@@ -37,4 +39,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 diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e70..41dcde2 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -26,6 +26,7 @@ #include <linux/list.h> #include <linux/ctype.h> #include <drm/drm_mode_object.h> +#include <drm/drm_color_mgmt.h>

struct drm_crtc; struct drm_printer; @@ -112,6 +113,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) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 8c67fc0..27e0bee 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -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

1.9.1

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:15 Jyri Sarha wrote:

From: Tomi Valkeinen tomi.valkeinen@ti.com

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.

That's quite a few changes for a single patch. I might have split the last one in a separate patch.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Jyri Sarha jsarha@ti.com

---

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 5ac0145..b53e63d 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -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));

• 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; int num_ovl = dss_feat_get_num_ovls();

• const struct color_conv_coef ctbl_bt601_5_ovl = {

• /* YUV -> RGB */
  

• 298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
  

• /* YUV -> RGB, ITU-R BT.601, limited range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298,    0,  409,	/* ry, rcb, rcr */
  

• 298, -100, -208,	/* gy, gcb, gcr */
  

• 298,  516,    0,	/* by, bcb, bcr */
  

You changed 517 to 516, was it intentional ? The commit message doesn't mention that modification.

• 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 */
  

• 112, -94, -18,		/* crr, crg, crb */
  

• false,			/* limited range */
  

  };

  for (i = 1; i < num_ovl; i++)

• dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
  

• dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
  
  

  if (dispc.feat->has_writeback)

• dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, 
  

&ctbl_bt601_5_wb);

• dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
  

}

static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:16 Jyri Sarha wrote:

From: Tomi Valkeinen tomi.valkeinen@ti.com

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 tomi.valkeinen@ti.com Signed-off-by: Jyri Sarha jsarha@ti.com

---

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index b53e63d..f2a2d08 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -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 = {

@@ -808,6 +810,14 @@ static void dispc_setup_color_conv_coef(void) false, /* limited range */ };

• /* YUV -> RGB, ITU-R BT.601, full range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {

• 256,   0,  358,		/* ry, rcb, rcr */
  

• 256, -88, -182,		/* gy, gcb, gcr */
  

• 256, 452,    0,		/* by, bcb, bcr */
  

• true,			/* full range */
  

• };

Shouldn't all those tables be static const ?

With that fixed,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• /* 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)