[PATCH RFC v2 0/2] drm: Add properties to control YCbCr to RGB conversion

[PATCH RFC v2 0/2] drm: Add properties to control YCbCr to RGB conversion

[Jyri Sarha]

Changes since first RFC version:
- Drop already merged
  - drm: drm_color_mgmt.h needs struct drm_crtc declaration
  - drm: Make drm_atomic_replace_property_blob_from_id() more generic
- Drop omapdrm specific example patches 
  - I have updated versions, for testing buy they are not relevant here
- Rename properties
- Drop bundled color space conversion enums and leave just:
  - BT.601 full, BT.601 limited, BT.709 limited, BT.2020 limited
- Split YCBCR_ENCODING property with standard encodings to a separate patch

The first RFC version can be found here:
http://www.spinics.net/lists/dri-devel/msg139440.html

The changes to this new version are not really that big. However,
especially with the first patch I try to iterate towards some minimal
common ground that we could agree on.

Jyri Sarha (2):
  drm: Add optinal YCBCR_ENCODING property to drm_plane
  drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to
    drm_plane

 drivers/gpu/drm/drm_atomic.c        |  24 ++++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 110 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c         |   9 +++
 include/drm/drm_color_mgmt.h        |  17 ++++++
 include/drm/drm_plane.h             |  10 ++++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 191 insertions(+)

-- 
1.9.1

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

[PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Jyri Sarha]

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

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

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 4033384..bcef93d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -732,6 +732,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	int ret;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
@@ -774,6 +775,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->rotation = val;
 	} else if (property == plane->zpos_property) {
 		state->zpos = val;
+	} else if (property == plane->ycbcr_encoding_property) {
+		state->ycbcr_encoding = val;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);
@@ -834,6 +837,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->rotation;
 	} else if (property == plane->zpos_property) {
 		*val = state->zpos;
+	} else if (property == plane->ycbcr_encoding_property) {
+		*val = state->ycbcr_encoding;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 533f3a3..245b14a 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -33,6 +33,11 @@
  * properties on the &drm_crtc object. They are set up by calling
  * drm_crtc_enable_color_mgmt().
  *
+ * Support for different YCbCr color encodings is controlled trough
+ * plane specific YCBCR_ENCODING property.
+ *
+ * The &drm_crtc object's properties are:
+ *
  * "DEGAMMA_LUT”:
  *	Blob property to set the degamma lookup table (LUT) mapping pixel data
  *	from the framebuffer before it is given to the transformation matrix.
@@ -85,6 +90,13 @@
  * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
  * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
  * "GAMMA_LUT" property above.
+ *
+ * The &drm_plane object's properties are:
+ *
+ * "YCBCR_ENCODING"
+ * 	Optional plane enum property to control YCbCr to RGB
+ * 	conversion. The driver provides a subset of standard
+ *	enum values supported by the DRM plane.
  */
 
 /**
@@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static char *ycbcr_encoding_name[] = {
+	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
+	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
+	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+};
+
+/**
+ * drm_plane_create_ycbcr_properties - ycbcr related plane properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default YCbCr encoding
+ *
+ * Create and attach plane specific YCBCR_ENCODING property to to the
+ * drm_plane object. The supported encodings should be provided in the
+ * enum_list parameter. The enum_list parameter should not contain the
+ * enum names, because the standard names are added by this function.
+ */
+int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_encoding default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	unsigned int i;
+
+	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_encoding_name[encoding];
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_ENCODING",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index fedd4d6..007c4d7 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->ycbcr_encoding_property)
+		drm_property_destroy(dev, plane->ycbcr_encoding_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 03a59cb..1771394 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,16 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 
+enum drm_plane_ycbcr_encoding {
+	DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0,
+	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+};
+
+int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_encoding default_mode);
+
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 9ab3e70..4d0510f 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,9 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 
@@ -523,6 +527,8 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_encoding_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
-- 
1.9.1


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

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Hans Verkuil]

Hi Jyri,

On 05/04/17 09:14, Jyri Sarha wrote:
> Add a standard optinal property to control YCbCr conversion in DRM
> planes. The property is stored to drm_plane object to allow different
> set of supported conversion modes for different planes on the device.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  3 ++
>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>  include/drm/drm_plane.h          |  6 ++++
>  5 files changed, 87 insertions(+)
> 

<snip>

> @@ -37,4 +39,16 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  				 int gamma_size);
>  
> +enum drm_plane_ycbcr_encoding {
> +	DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0,
> +	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> +};

I did a lot of work and research into colorspace handling for V4L2, and I
strongly recommend that you do not combine the ycbcr encoding with the quantization
range setting. I.e., make this two properties.

This is a good reference (but I'm biased, since I wrote it :-) ):

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/colorspaces.html
(and the two following sections as well).

The four 'parameters' that control how to interpret colors (colorspace,
transfer function, ycbcr encoding, quantization range) are all independent
of one another, and combining two or more into one property will become
a hassle to maintain.

Regards,

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Ville Syrjälä]

On Thu, May 04, 2017 at 10:14:25AM +0300, Jyri Sarha wrote:
> Add a standard optinal property to control YCbCr conversion in DRM
> planes. The property is stored to drm_plane object to allow different
> set of supported conversion modes for different planes on the device.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  3 ++
>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>  include/drm/drm_plane.h          |  6 ++++
>  5 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4033384..bcef93d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -732,6 +732,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	int ret;
>  
>  	if (property == config->prop_fb_id) {
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -774,6 +775,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->rotation = val;
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
> +	} else if (property == plane->ycbcr_encoding_property) {
> +		state->ycbcr_encoding = val;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -834,6 +837,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
>  		*val = state->zpos;
> +	} else if (property == plane->ycbcr_encoding_property) {
> +		*val = state->ycbcr_encoding;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 533f3a3..245b14a 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,6 +33,11 @@
>   * properties on the &drm_crtc object. They are set up by calling
>   * drm_crtc_enable_color_mgmt().
>   *
> + * Support for different YCbCr color encodings is controlled trough
> + * plane specific YCBCR_ENCODING property.
> + *
> + * The &drm_crtc object's properties are:
> + *
>   * "DEGAMMA_LUT”:
>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>   *	from the framebuffer before it is given to the transformation matrix.
> @@ -85,6 +90,13 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>   * "GAMMA_LUT" property above.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "YCBCR_ENCODING"
> + * 	Optional plane enum property to control YCbCr to RGB
> + * 	conversion. The driver provides a subset of standard
> + *	enum values supported by the DRM plane.
>   */
>  
>  /**
> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static char *ycbcr_encoding_name[] = {
> +	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",

Why full range for BT.601 but not the others?

I presume the full range here refers to the YCbCr data, and RGB will
always be full range? 

> +	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> +	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> +	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> +};
> +
> +/**
> + * drm_plane_create_ycbcr_properties - ycbcr related plane properties
> + * @enum_list: drm_prop_enum_list array of supported modes without names
> + * @enum_list_len: length of enum_list array
> + * @default_mode: default YCbCr encoding
> + *
> + * Create and attach plane specific YCBCR_ENCODING property to to the
> + * drm_plane object. The supported encodings should be provided in the
> + * enum_list parameter. The enum_list parameter should not contain the
> + * enum names, because the standard names are added by this function.
> + */
> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,

This API doesn't seem very nice because you have to pass in a
non-const enum list. Could we perhaps pass the supported values
as a bitmask instead? Or const int []?

> +			enum drm_plane_ycbcr_encoding default_mode)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	unsigned int i;
> +
> +	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> +		return -EEXIST;

We don't have those kinds of checks elsewhere, so why would we
want it here?

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

Missing the state->ycbcr_encoding setup.

> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d6..007c4d7 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	kfree(plane->name);
>  
> +	if (plane->ycbcr_encoding_property)
> +		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cb..1771394 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,16 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  				 int gamma_size);
>  
> +enum drm_plane_ycbcr_encoding {
> +	DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0,
> +	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> +};
> +
> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_encoding default_mode);
> +
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e70..4d0510f 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,9 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* YCbCr to RGB conversion */
> +	enum drm_plane_ycbcr_encoding ycbcr_encoding;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -523,6 +527,8 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	struct drm_property *ycbcr_encoding_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> -- 
> 1.9.1

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
> Add a standard optinal property to control YCbCr conversion in DRM
> planes. The property is stored to drm_plane object to allow different
> set of supported conversion modes for different planes on the device.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  3 ++
>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>  include/drm/drm_plane.h          |  6 ++++
>  5 files changed, 87 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c

[snip]

> @@ -85,6 +90,13 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with
> the
>   * "GAMMA_LUT" property above.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "YCBCR_ENCODING"
> + * 	Optional plane enum property to control YCbCr to RGB
> + * 	conversion. The driver provides a subset of standard
> + *	enum values supported by the DRM plane.
>   */

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

>  /**
> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static char *ycbcr_encoding_name[] = {
> +	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> +	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> +	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> +};
> +
> +/**
> + * drm_plane_create_ycbcr_properties - ycbcr related plane properties
> + * @enum_list: drm_prop_enum_list array of supported modes without names
> + * @enum_list_len: length of enum_list array
> + * @default_mode: default YCbCr encoding
> + *
> + * Create and attach plane specific YCBCR_ENCODING property to to the
> + * drm_plane object. The supported encodings should be provided in the
> + * enum_list parameter. The enum_list parameter should not contain the
> + * enum names, because the standard names are added by this function.
> + */
> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_encoding default_mode)

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

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

> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	unsigned int i;
> +
> +	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> +		return -EEXIST;
> +
> +	for (i = 0; i < enum_list_len; i++) {
> +		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> +
> +		enum_list[i].name = ycbcr_encoding_name[encoding];
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"YCBCR_ENCODING",
> +					enum_list, enum_list_len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->ycbcr_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_mode);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d6..007c4d7 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
> 
>  	kfree(plane->name);
> 
> +	if (plane->ycbcr_encoding_property)
> +		drm_property_destroy(dev, plane->ycbcr_encoding_property);

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

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

[snip]

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Jyri Sarha]

On 05/05/17 12:07, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
>> Add a standard optinal property to control YCbCr conversion in DRM
>> planes. The property is stored to drm_plane object to allow different
>> set of supported conversion modes for different planes on the device.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_plane.c      |  3 ++
>>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>>  include/drm/drm_plane.h          |  6 ++++
>>  5 files changed, 87 insertions(+)
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
>> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> 
> [snip]
> 
>> @@ -85,6 +90,13 @@
>>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with
>> the
>>   * "GAMMA_LUT" property above.
>> + *
>> + * The &drm_plane object's properties are:
>> + *
>> + * "YCBCR_ENCODING"
>> + * 	Optional plane enum property to control YCbCr to RGB
>> + * 	conversion. The driver provides a subset of standard
>> + *	enum values supported by the DRM plane.
>>   */
> 
> As already mentioned by Hans Verkuil, I also recommend not mixing the encoding 
> and quantization in a single property. If you split them, I would then drop 
> the YCBCR_ prefix (or replace it by something more generic) at least for the 
> quantization property, as it would apply to RGB as well. For the encoding 
> property, we have support in V4L2 for a two HSV encodings, so it could make 
> sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt 
> we'll see any display hardware with native support for HSV :-)
> 

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

>>  /**
>> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>  	drm_modeset_unlock(&crtc->mutex);
>>  	return ret;
>>  }
>> +
>> +static char *ycbcr_encoding_name[] = {
>> +	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
>> +	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
>> +};
>> +
>> +/**
>> + * drm_plane_create_ycbcr_properties - ycbcr related plane properties
>> + * @enum_list: drm_prop_enum_list array of supported modes without names
>> + * @enum_list_len: length of enum_list array
>> + * @default_mode: default YCbCr encoding
>> + *
>> + * Create and attach plane specific YCBCR_ENCODING property to to the
>> + * drm_plane object. The supported encodings should be provided in the
>> + * enum_list parameter. The enum_list parameter should not contain the
>> + * enum names, because the standard names are added by this function.
>> + */
>> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>> +			struct drm_prop_enum_list *enum_list,
>> +			unsigned int enum_list_len,
>> +			enum drm_plane_ycbcr_encoding default_mode)
> 
> I wonder whether we shouldn't simplify the API by passing a bitmask of 
> supported encodings. Sure, you would have to allocate the array of 
> drm_prop_enum_list internally in the function, but driver code would be 
> simpler. Even if you don't like bitmasks, I think we should pass a const 
> pointer and duplicate the array internally to fill the names. Drivers will in 
> many cases pass the same array for all planes, modifying it in the function is 
> asking for trouble (even if it should be OK with the current implementation).
> 

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

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

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

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

> 
>> +{
>> +	struct drm_device *dev = plane->dev;
>> +	struct drm_property *prop;
>> +	unsigned int i;
>> +
>> +	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
>> +		return -EEXIST;
>> +
>> +	for (i = 0; i < enum_list_len; i++) {
>> +		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
>> +
>> +		enum_list[i].name = ycbcr_encoding_name[encoding];
>> +	}
>> +
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +					"YCBCR_ENCODING",
>> +					enum_list, enum_list_len);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	plane->ycbcr_encoding_property = prop;
>> +	drm_object_attach_property(&plane->base, prop, default_mode);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index fedd4d6..007c4d7 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
>>
>>  	kfree(plane->name);
>>
>> +	if (plane->ycbcr_encoding_property)
>> +		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> 
> There's lots of similar code all over the place, I wonder whether we shouldn't 
> move the NULL check to drm_property_destroy().
> 

Absolutely.

>> +
>>  	memset(plane, 0, sizeof(*plane));
>>  }
>>  EXPORT_SYMBOL(drm_plane_cleanup);
> 
> [snip]
> 

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Laurent Pinchart]

Hi Jyri,

On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:
> On 05/05/17 12:07, Laurent Pinchart wrote:
> > On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
> >> Add a standard optinal property to control YCbCr conversion in DRM
> >> planes. The property is stored to drm_plane object to allow different
> >> set of supported conversion modes for different planes on the device.
> >> 
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
> >>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_plane.c      |  3 ++
> >>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
> >>  include/drm/drm_plane.h          |  6 ++++
> >>  5 files changed, 87 insertions(+)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> >> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
> >> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > 
> > [snip]
> > 
> >> @@ -85,6 +90,13 @@
> >>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> >>   with the
> >>   * "GAMMA_LUT" property above.
> >> + *
> >> + * The &drm_plane object's properties are:
> >> + *
> >> + * "YCBCR_ENCODING"
> >> + * 	Optional plane enum property to control YCbCr to RGB
> >> + * 	conversion. The driver provides a subset of standard
> >> + *	enum values supported by the DRM plane.
> >>   */
> > 
> > As already mentioned by Hans Verkuil, I also recommend not mixing the
> > encoding and quantization in a single property. If you split them, I
> > would then drop the YCBCR_ prefix (or replace it by something more
> > generic) at least for the quantization property, as it would apply to RGB
> > as well. For the encoding property, we have support in V4L2 for a two HSV
> > encodings, so it could make sense to drop or replace the YCBCR_ prefix,
> > but on the other hand I doubt we'll see any display hardware with native
> > support for HSV :-)
> 
> COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.

Sounds good to me.

> >>  /**
> >> 
> >> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >>  	drm_modeset_unlock(&crtc->mutex);
> >>  	return ret;
> >>  }
> >> +
> >> +static char *ycbcr_encoding_name[] = {
> >> +	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
> >> +	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> >> +	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> >> +	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> >> +};
> >> +
> >> +/**
> >> + * drm_plane_create_ycbcr_properties - ycbcr related plane properties
> >> + * @enum_list: drm_prop_enum_list array of supported modes without names
> >> + * @enum_list_len: length of enum_list array
> >> + * @default_mode: default YCbCr encoding
> >> + *
> >> + * Create and attach plane specific YCBCR_ENCODING property to to the
> >> + * drm_plane object. The supported encodings should be provided in the
> >> + * enum_list parameter. The enum_list parameter should not contain the
> >> + * enum names, because the standard names are added by this function.
> >> + */
> >> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> >> +			struct drm_prop_enum_list *enum_list,
> >> +			unsigned int enum_list_len,
> >> +			enum drm_plane_ycbcr_encoding default_mode)
> > 
> > I wonder whether we shouldn't simplify the API by passing a bitmask of
> > supported encodings. Sure, you would have to allocate the array of
> > drm_prop_enum_list internally in the function, but driver code would be
> > simpler. Even if you don't like bitmasks, I think we should pass a const
> > pointer and duplicate the array internally to fill the names. Drivers will
> > in many cases pass the same array for all planes, modifying it in the
> > function is asking for trouble (even if it should be OK with the current
> > implementation).
>
> I used a bitmask property first, but abandoned it because the encodings
> do not behave like bitmasks. You can not have BT.601 | BT.709 at the
> same time.

Sorry, I should have expressed myself more clearly, I meant an enum property 
with a bitmask in the function API. I agree that a bitmask property isn't a 
good match.

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

I think we're getting there with 1/2, so feel free to update that in the next 
version :-)

> > By the way, for drivers that support the same encodings for all planes,
> > would it be worth it to support allocation of a single property instead
> > of allocating one per plane ?
> 
> I was thinking of that, but AFAIK there is really not that many planes
> on the know HW that it would justify the complexity.
> 
> >> +{
> >> +	struct drm_device *dev = plane->dev;
> >> +	struct drm_property *prop;
> >> +	unsigned int i;
> >> +
> >> +	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> >> +		return -EEXIST;
> >> +
> >> +	for (i = 0; i < enum_list_len; i++) {
> >> +		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> >> +
> >> +		enum_list[i].name = ycbcr_encoding_name[encoding];
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> +					"YCBCR_ENCODING",
> >> +					enum_list, enum_list_len);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	plane->ycbcr_encoding_property = prop;
> >> +	drm_object_attach_property(&plane->base, prop, default_mode);
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index fedd4d6..007c4d7 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >> 
> >>  	kfree(plane->name);
> >> 
> >> +	if (plane->ycbcr_encoding_property)
> >> +		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> > 
> > There's lots of similar code all over the place, I wonder whether we
> > shouldn't move the NULL check to drm_property_destroy().
> 
> Absolutely.
> 
> >> +
> >> 
> >>  	memset(plane, 0, sizeof(*plane));
> >>  
> >>  }
> >>  EXPORT_SYMBOL(drm_plane_cleanup);
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Jyri Sarha]

On 05/05/17 16:03, Laurent Pinchart wrote:
> Hi Jyri,
> 
> On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:
>> On 05/05/17 12:07, Laurent Pinchart wrote:
>>> On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
>>>> Add a standard optinal property to control YCbCr conversion in DRM
>>>> planes. The property is stored to drm_plane object to allow different
>>>> set of supported conversion modes for different planes on the device.
>>>>
>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>>>>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/drm_plane.c      |  3 ++
>>>>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>>>>  include/drm/drm_plane.h          |  6 ++++
>>>>  5 files changed, 87 insertions(+)
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
>>>> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
>>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>>
>>> [snip]
>>>
>>>> @@ -85,6 +90,13 @@
>>>>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>>>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
>>>>   with the
>>>>   * "GAMMA_LUT" property above.
>>>> + *
>>>> + * The &drm_plane object's properties are:
>>>> + *
>>>> + * "YCBCR_ENCODING"
>>>> + * 	Optional plane enum property to control YCbCr to RGB
>>>> + * 	conversion. The driver provides a subset of standard
>>>> + *	enum values supported by the DRM plane.
>>>>   */
>>>
>>> As already mentioned by Hans Verkuil, I also recommend not mixing the
>>> encoding and quantization in a single property. If you split them, I
>>> would then drop the YCBCR_ prefix (or replace it by something more
>>> generic) at least for the quantization property, as it would apply to RGB
>>> as well. For the encoding property, we have support in V4L2 for a two HSV
>>> encodings, so it could make sense to drop or replace the YCBCR_ prefix,
>>> but on the other hand I doubt we'll see any display hardware with native
>>> support for HSV :-)
>>
>> COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.
> 
> Sounds good to me.
> 

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

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

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

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

Re: [PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Laurent Pinchart]

Hi Jyri,

On Thursday 11 May 2017 17:30:01 Jyri Sarha wrote:
> On 05/05/17 16:03, Laurent Pinchart wrote:
> > On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:
> >> On 05/05/17 12:07, Laurent Pinchart wrote:
> >>> On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
> >>>> Add a standard optinal property to control YCbCr conversion in DRM
> >>>> planes. The property is stored to drm_plane object to allow different
> >>>> set of supported conversion modes for different planes on the device.
> >>>> 
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
> >>>>  drivers/gpu/drm/drm_color_mgmt.c | 59 +++++++++++++++++++++++++++++++
> >>>>  drivers/gpu/drm/drm_plane.c      |  3 ++
> >>>>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
> >>>>  include/drm/drm_plane.h          |  6 ++++
> >>>>  5 files changed, 87 insertions(+)
> >>> 
> >>> [snip]
> >>> 
> >>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> >>>> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
> >>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >>> 
> >>> [snip]
> >>> 
> >>>> @@ -85,6 +90,13 @@
> >>>>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should
> >>>>   use
> >>>>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> >>>>   with the
> >>>>   * "GAMMA_LUT" property above.
> >>>> + *
> >>>> + * The &drm_plane object's properties are:
> >>>> + *
> >>>> + * "YCBCR_ENCODING"
> >>>> + * 	Optional plane enum property to control YCbCr to RGB
> >>>> + * 	conversion. The driver provides a subset of standard
> >>>> + *	enum values supported by the DRM plane.
> >>>>   */
> >>> 
> >>> As already mentioned by Hans Verkuil, I also recommend not mixing the
> >>> encoding and quantization in a single property. If you split them, I
> >>> would then drop the YCBCR_ prefix (or replace it by something more
> >>> generic) at least for the quantization property, as it would apply to
> >>> RGB as well. For the encoding property, we have support in V4L2 for a
> >>> two HSV encodings, so it could make sense to drop or replace the YCBCR_
> >>> prefix, but on the other hand I doubt we'll see any display hardware
> >>> with native support for HSV :-)
> >> 
> >> COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.
> > 
> > Sounds good to me.
> 
> I am now implementing this. However, there is a logical challenge in
> putting the two suggestions together, splitting the encoding and range
> to separate properties, and changing the property names to generic
> COLOR_ENCODING and COLOR_RANGE.
> 
> In general COLOR_RANGE enum values are only defined within the selected
> COLOR_ENCODING. With the standard YCbCr encodings this is not a problem,
> since they all define a "full range" and a "limted range". But if we are
> preparing for some unknown color ecodings, I am not sure how likely it
> is that "full range" and "limited range" options make sense there.
> 
> I can implement the properties for currently known YCbCr color encodings
> either way, either as YCbCr specific properties, or as generic COLOR_*
> properties for all non RGB encodings. I am just not sure if defining the
> generic properties would make any sense now that we (or at least I) have
> no idea what the other non RGB encodings could be. What do you think?

I won't claim to know what quantization methods will make sense for non-YCbCr 
encodings in the future (or, for that matter, even for YCbCr encodings). We 
might even not need any new quantization method. However I don't think this is 
an argument to not standardize a quantization method property. We can start 
with an enumerated property with two values, clearly defined in the API 
documentation as applying to YCbCr only. If and when we'll need quantization 
methods for other encodings, we can just add values to that enumeration. I 
would have proposed to even reuse the enumerated values for a different 
purpose depending on the colour encoding, but as the DRM API exposes 
enumerated values as strings, we unfortunately can't do that.

-- 
Regards,

Laurent Pinchart

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

[PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Jyri Sarha]

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

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
 drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_plane.c         |  6 ++++
 include/drm/drm_color_mgmt.h        |  3 ++
 include/drm/drm_plane.h             |  4 +++
 include/uapi/drm/drm_mode.h         | 12 ++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index bcef93d..87a2d55 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
@@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->zpos = val;
 	} else if (property == plane->ycbcr_encoding_property) {
 		state->ycbcr_encoding = val;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_decode_csc, val,
+				sizeof(struct drm_ycbcr_decode_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_csc_preoffset, val,
+				sizeof(struct drm_ycbcr_csc_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);
@@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->zpos;
 	} else if (property == plane->ycbcr_encoding_property) {
 		*val = state->ycbcr_encoding;
+	} else if (property == plane->ycbcr_decode_csc_property) {
+		*val = state->ycbcr_decode_csc ?
+			state->ycbcr_decode_csc->base.id : 0;
+	} else if (property == plane->ycbcr_csc_preoffset_property) {
+		*val = state->ycbcr_csc_preoffset ?
+			state->ycbcr_csc_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8be9719..6ecc32f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_decode_csc)
+		drm_property_blob_get(state->ycbcr_decode_csc);
+
+	if (state->ycbcr_csc_preoffset)
+		drm_property_blob_get(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 
@@ -3278,6 +3284,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_decode_csc);
+	drm_property_blob_put(state->ycbcr_csc_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 245b14a..319ed46 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -95,8 +95,23 @@
  *
  * "YCBCR_ENCODING"
  * 	Optional plane enum property to control YCbCr to RGB
- * 	conversion. The driver provides a subset of standard
- *	enum values supported by the DRM plane.
+ * 	conversion. The driver provides a subset of standard enum
+ * 	values supported by the DRM plane. The possible encodings
+ * 	include the standard conversions and a possibility to select a
+ * 	custom conversion matrix and preoffset vector.
+ *
+ * "YCBCR_DECODE_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	used depends on the value of YCBCR_ENCODING property.
+ *
+ * "YCBCR_CSC_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_csc_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is used depends on
+ *	the value of YCBCR_ENCODING property.
  */
 
 /**
@@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
 	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
 	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
+	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
+	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
+	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
 };
 
 /**
@@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
  * drm_plane object. The supported encodings should be provided in the
  * enum_list parameter. The enum_list parameter should not contain the
  * enum names, because the standard names are added by this function.
+ * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
+ * based on the provided enum_list.
  */
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 			struct drm_prop_enum_list *enum_list,
@@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
+	bool ycbcr_decode_csc_create = false;
+	bool ycbcr_csc_preoffset_create = false;
 	unsigned int i;
 
 	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
@@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
 
 		enum_list[i].name = ycbcr_encoding_name[encoding];
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
+		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
+			ycbcr_decode_csc_create = true;
+
+		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
+			ycbcr_decode_csc_create = true;
+			ycbcr_csc_preoffset_create = true;
+		}
 	}
 
 	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
@@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
 	plane->ycbcr_encoding_property = prop;
 	drm_object_attach_property(&plane->base, prop, default_mode);
 
+	if (ycbcr_decode_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_DECODE_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_decode_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_csc_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_CSC_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_csc_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 007c4d7..d10c942 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	if (plane->ycbcr_encoding_property)
 		drm_property_destroy(dev, plane->ycbcr_encoding_property);
 
+	if (plane->ycbcr_decode_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
+
+	if (plane->ycbcr_csc_preoffset_property)
+		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 1771394..bdfd37e 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
 	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
 	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
+	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
+	DRM_PLANE_YCBCR_CSC_PREOFFSET,
 };
 
 int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 4d0510f..63b1e0c 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -115,6 +115,8 @@ struct drm_plane_state {
 
 	/* YCbCr to RGB conversion */
 	enum drm_plane_ycbcr_encoding ycbcr_encoding;
+	struct drm_property_blob *ycbcr_decode_csc;
+	struct drm_property_blob *ycbcr_csc_preoffset;
 
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
@@ -529,6 +531,8 @@ struct drm_plane {
 	struct drm_property *rotation_property;
 
 	struct drm_property *ycbcr_encoding_property;
+	struct drm_property *ycbcr_decode_csc_property;
+	struct drm_property *ycbcr_csc_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 8c67fc0..8c9568d 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_decode_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_csc_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
1.9.1

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Daniel Vetter]

On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> Add standard optinal property blobs for YCbCr to RGB conversion CSC
> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> to YCBCR_ENCODING property to activate the CSC and preoffset
> properties. For simplicity the new properties are stored in the
> drm_plane object, because the YCBCR_ENCODING is already there. The
> blob contents are defined in the uapi/drm/drm_mode.h header.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

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

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

> ---
>  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
>  drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_plane.c         |  6 ++++
>  include/drm/drm_color_mgmt.h        |  3 ++
>  include/drm/drm_plane.h             |  4 +++
>  include/uapi/drm/drm_mode.h         | 12 ++++++++
>  7 files changed, 106 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index bcef93d..87a2d55 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
> +	bool dummy;
>  
>  	if (property == config->prop_fb_id) {
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->zpos = val;
>  	} else if (property == plane->ycbcr_encoding_property) {
>  		state->ycbcr_encoding = val;
> +	} else if (property == plane->ycbcr_decode_csc_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_decode_csc, val,
> +				sizeof(struct drm_ycbcr_decode_csc),
> +				&dummy);
> +		return ret;
> +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_csc_preoffset, val,
> +				sizeof(struct drm_ycbcr_csc_preoffset),
> +				&dummy);
> +		return ret;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		*val = state->zpos;
>  	} else if (property == plane->ycbcr_encoding_property) {
>  		*val = state->ycbcr_encoding;
> +	} else if (property == plane->ycbcr_decode_csc_property) {
> +		*val = state->ycbcr_decode_csc ?
> +			state->ycbcr_decode_csc->base.id : 0;
> +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> +		*val = state->ycbcr_csc_preoffset ?
> +			state->ycbcr_csc_preoffset->base.id : 0;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8be9719..6ecc32f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  {
>  	memcpy(state, plane->state, sizeof(*state));
>  
> +	if (state->ycbcr_decode_csc)
> +		drm_property_blob_get(state->ycbcr_decode_csc);
> +
> +	if (state->ycbcr_csc_preoffset)
> +		drm_property_blob_get(state->ycbcr_csc_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_get(state->fb);
>  
> @@ -3278,6 +3284,9 @@ struct drm_plane_state *
>   */
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
> +	drm_property_blob_put(state->ycbcr_decode_csc);
> +	drm_property_blob_put(state->ycbcr_csc_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_put(state->fb);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 245b14a..319ed46 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -95,8 +95,23 @@
>   *
>   * "YCBCR_ENCODING"
>   * 	Optional plane enum property to control YCbCr to RGB
> - * 	conversion. The driver provides a subset of standard
> - *	enum values supported by the DRM plane.
> + * 	conversion. The driver provides a subset of standard enum
> + * 	values supported by the DRM plane. The possible encodings
> + * 	include the standard conversions and a possibility to select a
> + * 	custom conversion matrix and preoffset vector.
> + *
> + * "YCBCR_DECODE_CSC"
> + *	Optional plane property blob to set YCbCr to RGB conversion
> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> + *	defined in uapi/drm/drm_mode.h. Whether this property is
> + *	used depends on the value of YCBCR_ENCODING property.
> + *
> + * "YCBCR_CSC_PREOFFSET"
> + *	Optional plane property blob to configure YCbCr offset before
> + *	YCbCr to RGB CSC is applied. The blob contains struct
> + *	drm_ycbcr_csc_preoffset which is defined in
> + *	uapi/drm/drm_mode.h. Whether this property is used depends on
> + *	the value of YCBCR_ENCODING property.
>   */
>  
>  /**
> @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
>  	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
>  	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> +	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
> +	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
> +	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
>  };
>  
>  /**
> @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>   * drm_plane object. The supported encodings should be provided in the
>   * enum_list parameter. The enum_list parameter should not contain the
>   * enum names, because the standard names are added by this function.
> + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
> + * based on the provided enum_list.
>   */
>  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>  			struct drm_prop_enum_list *enum_list,
> @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_property *prop;
> +	bool ycbcr_decode_csc_create = false;
> +	bool ycbcr_csc_preoffset_create = false;
>  	unsigned int i;
>  
>  	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>  		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
>  
>  		enum_list[i].name = ycbcr_encoding_name[encoding];
> +
> +		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
> +		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
> +			ycbcr_decode_csc_create = true;
> +
> +		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
> +			ycbcr_decode_csc_create = true;
> +			ycbcr_csc_preoffset_create = true;
> +		}
>  	}
>  
>  	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>  	plane->ycbcr_encoding_property = prop;
>  	drm_object_attach_property(&plane->base, prop, default_mode);
>  
> +	if (ycbcr_decode_csc_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_DECODE_CSC", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_decode_csc_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
> +	if (ycbcr_csc_preoffset_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_CSC_PREOFFSET", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_csc_preoffset_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 007c4d7..d10c942 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  	if (plane->ycbcr_encoding_property)
>  		drm_property_destroy(dev, plane->ycbcr_encoding_property);
>  
> +	if (plane->ycbcr_decode_csc_property)
> +		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
> +
> +	if (plane->ycbcr_csc_preoffset_property)
> +		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 1771394..bdfd37e 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
>  	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
>  	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
>  	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
> +	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
> +	DRM_PLANE_YCBCR_CSC_PREOFFSET,
>  };
>  
>  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 4d0510f..63b1e0c 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -115,6 +115,8 @@ struct drm_plane_state {
>  
>  	/* YCbCr to RGB conversion */
>  	enum drm_plane_ycbcr_encoding ycbcr_encoding;
> +	struct drm_property_blob *ycbcr_decode_csc;
> +	struct drm_property_blob *ycbcr_csc_preoffset;
>  
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
> @@ -529,6 +531,8 @@ struct drm_plane {
>  	struct drm_property *rotation_property;
>  
>  	struct drm_property *ycbcr_encoding_property;
> +	struct drm_property *ycbcr_decode_csc_property;
> +	struct drm_property *ycbcr_csc_preoffset_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc0..8c9568d 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_decode_csc {
> +	/* Conversion matrix in 2-complement s32.32 format. */
> +	__s64 ry, rcb, rcr;
> +	__s64 gy, gcb, gcr;
> +	__s64 by, bcb, bcr;
> +};
> +
> +struct drm_ycbcr_csc_preoffset {
> +	/* Offset vector in 2-complement s.32 format. */
> +	__s32 y, cb, cr;
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Ville Syrjälä]

On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> > Add standard optinal property blobs for YCbCr to RGB conversion CSC
> > matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> > to YCBCR_ENCODING property to activate the CSC and preoffset
> > properties. For simplicity the new properties are stored in the
> > drm_plane object, because the YCBCR_ENCODING is already there. The
> > blob contents are defined in the uapi/drm/drm_mode.h header.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> 
> Not sure we want to make this yuv2rgb specific, plenty of planes have
> generic degamma/offset, csc, gamme/offset hw. I think what we want instead
> is the generic csc support, plus a ycbcr2rgb mode of "bypass".

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

> 
> And as usual, this needs some userspace compositor which actually uses it
> (not just a demo, since there's a huge difference between a demo and
> something that has to Just Work like a real compositor).
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
> >  drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/drm_plane.c         |  6 ++++
> >  include/drm/drm_color_mgmt.h        |  3 ++
> >  include/drm/drm_plane.h             |  4 +++
> >  include/uapi/drm/drm_mode.h         | 12 ++++++++
> >  7 files changed, 106 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index bcef93d..87a2d55 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  	struct drm_device *dev = plane->dev;
> >  	struct drm_mode_config *config = &dev->mode_config;
> >  	int ret;
> > +	bool dummy;
> >  
> >  	if (property == config->prop_fb_id) {
> >  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		state->zpos = val;
> >  	} else if (property == plane->ycbcr_encoding_property) {
> >  		state->ycbcr_encoding = val;
> > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > +				&state->ycbcr_decode_csc, val,
> > +				sizeof(struct drm_ycbcr_decode_csc),
> > +				&dummy);
> > +		return ret;
> > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > +				&state->ycbcr_csc_preoffset, val,
> > +				sizeof(struct drm_ycbcr_csc_preoffset),
> > +				&dummy);
> > +		return ret;
> >  	} else if (plane->funcs->atomic_set_property) {
> >  		return plane->funcs->atomic_set_property(plane, state,
> >  				property, val);
> > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		*val = state->zpos;
> >  	} else if (property == plane->ycbcr_encoding_property) {
> >  		*val = state->ycbcr_encoding;
> > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > +		*val = state->ycbcr_decode_csc ?
> > +			state->ycbcr_decode_csc->base.id : 0;
> > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > +		*val = state->ycbcr_csc_preoffset ?
> > +			state->ycbcr_csc_preoffset->base.id : 0;
> >  	} else if (plane->funcs->atomic_get_property) {
> >  		return plane->funcs->atomic_get_property(plane, state, property, val);
> >  	} else {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8be9719..6ecc32f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> >  {
> >  	memcpy(state, plane->state, sizeof(*state));
> >  
> > +	if (state->ycbcr_decode_csc)
> > +		drm_property_blob_get(state->ycbcr_decode_csc);
> > +
> > +	if (state->ycbcr_csc_preoffset)
> > +		drm_property_blob_get(state->ycbcr_csc_preoffset);
> > +
> >  	if (state->fb)
> >  		drm_framebuffer_get(state->fb);
> >  
> > @@ -3278,6 +3284,9 @@ struct drm_plane_state *
> >   */
> >  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> >  {
> > +	drm_property_blob_put(state->ycbcr_decode_csc);
> > +	drm_property_blob_put(state->ycbcr_csc_preoffset);
> > +
> >  	if (state->fb)
> >  		drm_framebuffer_put(state->fb);
> >  
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 245b14a..319ed46 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -95,8 +95,23 @@
> >   *
> >   * "YCBCR_ENCODING"
> >   * 	Optional plane enum property to control YCbCr to RGB
> > - * 	conversion. The driver provides a subset of standard
> > - *	enum values supported by the DRM plane.
> > + * 	conversion. The driver provides a subset of standard enum
> > + * 	values supported by the DRM plane. The possible encodings
> > + * 	include the standard conversions and a possibility to select a
> > + * 	custom conversion matrix and preoffset vector.
> > + *
> > + * "YCBCR_DECODE_CSC"
> > + *	Optional plane property blob to set YCbCr to RGB conversion
> > + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > + *	defined in uapi/drm/drm_mode.h. Whether this property is
> > + *	used depends on the value of YCBCR_ENCODING property.
> > + *
> > + * "YCBCR_CSC_PREOFFSET"
> > + *	Optional plane property blob to configure YCbCr offset before
> > + *	YCbCr to RGB CSC is applied. The blob contains struct
> > + *	drm_ycbcr_csc_preoffset which is defined in
> > + *	uapi/drm/drm_mode.h. Whether this property is used depends on
> > + *	the value of YCBCR_ENCODING property.
> >   */
> >  
> >  /**
> > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >  	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> >  	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> >  	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> > +	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
> > +	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
> > +	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
> >  };
> >  
> >  /**
> > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >   * drm_plane object. The supported encodings should be provided in the
> >   * enum_list parameter. The enum_list parameter should not contain the
> >   * enum names, because the standard names are added by this function.
> > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
> > + * based on the provided enum_list.
> >   */
> >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> >  			struct drm_prop_enum_list *enum_list,
> > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> >  {
> >  	struct drm_device *dev = plane->dev;
> >  	struct drm_property *prop;
> > +	bool ycbcr_decode_csc_create = false;
> > +	bool ycbcr_csc_preoffset_create = false;
> >  	unsigned int i;
> >  
> >  	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> >  		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> >  
> >  		enum_list[i].name = ycbcr_encoding_name[encoding];
> > +
> > +		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
> > +		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
> > +			ycbcr_decode_csc_create = true;
> > +
> > +		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
> > +			ycbcr_decode_csc_create = true;
> > +			ycbcr_csc_preoffset_create = true;
> > +		}
> >  	}
> >  
> >  	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> >  	plane->ycbcr_encoding_property = prop;
> >  	drm_object_attach_property(&plane->base, prop, default_mode);
> >  
> > +	if (ycbcr_decode_csc_create) {
> > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > +					   DRM_MODE_PROP_BLOB,
> > +					   "YCBCR_DECODE_CSC", 0);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		plane->ycbcr_decode_csc_property = prop;
> > +		drm_object_attach_property(&plane->base, prop, 0);
> > +	}
> > +
> > +	if (ycbcr_csc_preoffset_create) {
> > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > +					   DRM_MODE_PROP_BLOB,
> > +					   "YCBCR_CSC_PREOFFSET", 0);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		plane->ycbcr_csc_preoffset_property = prop;
> > +		drm_object_attach_property(&plane->base, prop, 0);
> > +	}
> > +
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 007c4d7..d10c942 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >  	if (plane->ycbcr_encoding_property)
> >  		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> >  
> > +	if (plane->ycbcr_decode_csc_property)
> > +		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
> > +
> > +	if (plane->ycbcr_csc_preoffset_property)
> > +		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
> > +
> >  	memset(plane, 0, sizeof(*plane));
> >  }
> >  EXPORT_SYMBOL(drm_plane_cleanup);
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 1771394..bdfd37e 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
> >  	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> >  	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> >  	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> > +	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
> > +	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
> > +	DRM_PLANE_YCBCR_CSC_PREOFFSET,
> >  };
> >  
> >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 4d0510f..63b1e0c 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -115,6 +115,8 @@ struct drm_plane_state {
> >  
> >  	/* YCbCr to RGB conversion */
> >  	enum drm_plane_ycbcr_encoding ycbcr_encoding;
> > +	struct drm_property_blob *ycbcr_decode_csc;
> > +	struct drm_property_blob *ycbcr_csc_preoffset;
> >  
> >  	/* Clipped coordinates */
> >  	struct drm_rect src, dst;
> > @@ -529,6 +531,8 @@ struct drm_plane {
> >  	struct drm_property *rotation_property;
> >  
> >  	struct drm_property *ycbcr_encoding_property;
> > +	struct drm_property *ycbcr_decode_csc_property;
> > +	struct drm_property *ycbcr_csc_preoffset_property;
> >  };
> >  
> >  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 8c67fc0..8c9568d 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_decode_csc {
> > +	/* Conversion matrix in 2-complement s32.32 format. */
> > +	__s64 ry, rcb, rcr;
> > +	__s64 gy, gcb, gcr;
> > +	__s64 by, bcb, bcr;
> > +};
> > +
> > +struct drm_ycbcr_csc_preoffset {
> > +	/* Offset vector in 2-complement s.32 format. */
> > +	__s32 y, cb, cr;
> > +};
> > +
> >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Jyri Sarha]

On 05/04/17 17:51, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
>>> to YCBCR_ENCODING property to activate the CSC and preoffset
>>> properties. For simplicity the new properties are stored in the
>>> drm_plane object, because the YCBCR_ENCODING is already there. The
>>> blob contents are defined in the uapi/drm/drm_mode.h header.
>>>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> Not sure we want to make this yuv2rgb specific, plenty of planes have
>> generic degamma/offset, csc, gamme/offset hw. I think what we want instead
>> is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> My idea is to not expose this. And instead just expose a normal
> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> that can't be done by the hw.
> 

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

YCBCR_RANGE
 - Full
 - Limited

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

CTM (blob)

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



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

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

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Laurent Pinchart]

Hi Jyri,

On Thursday 04 May 2017 18:23:21 Jyri Sarha wrote:
> On 05/04/17 17:51, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> >> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> >>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
> >>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> >>> to YCBCR_ENCODING property to activate the CSC and preoffset
> >>> properties. For simplicity the new properties are stored in the
> >>> drm_plane object, because the YCBCR_ENCODING is already there. The
> >>> blob contents are defined in the uapi/drm/drm_mode.h header.
> >>> 
> >>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> 
> >> Not sure we want to make this yuv2rgb specific, plenty of planes have
> >> generic degamma/offset, csc, gamme/offset hw. I think what we want
> >> instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> > 
> > My idea is to not expose this. And instead just expose a normal
> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> > that can't be done by the hw.
> 
> So, pulling the suggestions together the properties would now look like
> this:
> 
> YCBCR_RANGE
>  - Full
>  - Limited
> 
> YCBCR_ENCODING
>  - BT.601
>  - BT.709
>  - BT.2020
> 
> CTM (blob)
> 
> And all these properties could be added to individual planes. I'll try
> to come up with a new patch to add simple enum properties for
> YCBCR_PREOFFSET and YCBCR_ENCODING for planes. Should we need any more
> enum values for those?
> 
> The only functional thing that is missing from the above proposal is the
> possibility to define an arbitrary YCbCr preoffset vector, but that can
> be added later if there ever is any real need.
> 
> The other thing that could be added for special purposes would be adding
> an "Identity" enum to YCBCR_ENCODING, that would allow setting the YCbCr
> CSC trough CTM without any driver level multiplication getting in way,
> but that can also be added if there is a need.

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

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

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Daniel Vetter]

On Fri, May 5, 2017 at 4:36 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the
> content of the framebuffer is encoded. If I understand correctly, you're
> proposing adding an enumeration value that tells the driver not to try to be
> clever and multiply the CTM matrix by the CSC matrix corresponding to the
> encoding. That would probably work in most cases, but it would combine two
> pieces of information in a single property. The driver would then lose the
> knowledge of how the plane framebuffer is encoded. Couldn't there be cases
> where that knowledge is needed for other purposes than picking the right CSC
> matrix ? If so, it might be better to always set the YCBCR_ENCODING property
> to the actual encoding, and have another property to tell the driver to skip
> multiplication by the CSC matrix. Or could that be conveyed through the CTM
> blob property ? Some kind of flag that would essentially tell that the CTM
> matrix has been pre-multiplied already ?
>
> Before I forget, how do you plan to handle backward compatibility with
> userspace that won't set the YCBCR_ENCODING property ? Is that done by picking
> a driver-specific default value ? Do you think there would be a need for
> drivers to know that the property has not been set ?

Where do we need this? Afaik the encoding is to spec the yuv2rbg
conversion function, and that's it. But I'm fairly ignorant about
video and yuv and all these things. so does this specify something
else? If not, I don't see any possibilities that someone could
retrofit more meaning onto these conversion rules.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Ville Syrjälä]

On Fri, May 05, 2017 at 07:17:43PM +0200, Daniel Vetter wrote:
> On Fri, May 5, 2017 at 4:36 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the
> > content of the framebuffer is encoded. If I understand correctly, you're
> > proposing adding an enumeration value that tells the driver not to try to be
> > clever and multiply the CTM matrix by the CSC matrix corresponding to the
> > encoding. That would probably work in most cases, but it would combine two
> > pieces of information in a single property. The driver would then lose the
> > knowledge of how the plane framebuffer is encoded. Couldn't there be cases
> > where that knowledge is needed for other purposes than picking the right CSC
> > matrix ? If so, it might be better to always set the YCBCR_ENCODING property
> > to the actual encoding, and have another property to tell the driver to skip
> > multiplication by the CSC matrix. Or could that be conveyed through the CTM
> > blob property ? Some kind of flag that would essentially tell that the CTM
> > matrix has been pre-multiplied already ?
> >
> > Before I forget, how do you plan to handle backward compatibility with
> > userspace that won't set the YCBCR_ENCODING property ? Is that done by picking
> > a driver-specific default value ? Do you think there would be a need for
> > drivers to know that the property has not been set ?
> 
> Where do we need this? Afaik the encoding is to spec the yuv2rbg
> conversion function, and that's it. But I'm fairly ignorant about
> video and yuv and all these things. so does this specify something
> else? If not, I don't see any possibilities that someone could
> retrofit more meaning onto these conversion rules.

The question, I believe, is how do we deal with existing
userspace that doesn't know about this knob.

I think BT.709 is probably what we should use as the default
since I'm expecting that would match most non-ancient source
material out there. i915 currently uses BT.601 for no
particular good reason, but I'm very happy to change that
to BT.709. I think for the old video overlay (which isn't
exposed as a plane currently) we actually default to BT.709.

The only problatic case is when the hardware can't to BT.709
but I'm not sure if that's relevant for any currently supported
hardware. I guess we could have the core pick the default based
on some priority list eg. BT.709->BT.601->BT.2020.

The other option of course being that we just let each driver
pick their own default. I guess that might make sense if there's
some userspace already out there that expects eg. BT.601. But
frankly the difference between 601 and 709 is small enough that
I wouldn't expect anyone to scream too loudly if we end up
changing the default for someone.

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Daniel Vetter]

On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> > > Add standard optinal property blobs for YCbCr to RGB conversion CSC
> > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> > > to YCBCR_ENCODING property to activate the CSC and preoffset
> > > properties. For simplicity the new properties are stored in the
> > > drm_plane object, because the YCBCR_ENCODING is already there. The
> > > blob contents are defined in the uapi/drm/drm_mode.h header.
> > > 
> > > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > 
> > Not sure we want to make this yuv2rgb specific, plenty of planes have
> > generic degamma/offset, csc, gamme/offset hw. I think what we want instead
> > is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> 
> My idea is to not expose this. And instead just expose a normal
> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> that can't be done by the hw.

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

> 
> > 
> > And as usual, this needs some userspace compositor which actually uses it
> > (not just a demo, since there's a huge difference between a demo and
> > something that has to Just Work like a real compositor).
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
> > >  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
> > >  drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/drm_plane.c         |  6 ++++
> > >  include/drm/drm_color_mgmt.h        |  3 ++
> > >  include/drm/drm_plane.h             |  4 +++
> > >  include/uapi/drm/drm_mode.h         | 12 ++++++++
> > >  7 files changed, 106 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index bcef93d..87a2d55 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > >  	struct drm_device *dev = plane->dev;
> > >  	struct drm_mode_config *config = &dev->mode_config;
> > >  	int ret;
> > > +	bool dummy;
> > >  
> > >  	if (property == config->prop_fb_id) {
> > >  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > >  		state->zpos = val;
> > >  	} else if (property == plane->ycbcr_encoding_property) {
> > >  		state->ycbcr_encoding = val;
> > > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > > +				&state->ycbcr_decode_csc, val,
> > > +				sizeof(struct drm_ycbcr_decode_csc),
> > > +				&dummy);
> > > +		return ret;
> > > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > > +				&state->ycbcr_csc_preoffset, val,
> > > +				sizeof(struct drm_ycbcr_csc_preoffset),
> > > +				&dummy);
> > > +		return ret;
> > >  	} else if (plane->funcs->atomic_set_property) {
> > >  		return plane->funcs->atomic_set_property(plane, state,
> > >  				property, val);
> > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > >  		*val = state->zpos;
> > >  	} else if (property == plane->ycbcr_encoding_property) {
> > >  		*val = state->ycbcr_encoding;
> > > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > > +		*val = state->ycbcr_decode_csc ?
> > > +			state->ycbcr_decode_csc->base.id : 0;
> > > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > > +		*val = state->ycbcr_csc_preoffset ?
> > > +			state->ycbcr_csc_preoffset->base.id : 0;
> > >  	} else if (plane->funcs->atomic_get_property) {
> > >  		return plane->funcs->atomic_get_property(plane, state, property, val);
> > >  	} else {
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 8be9719..6ecc32f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > >  {
> > >  	memcpy(state, plane->state, sizeof(*state));
> > >  
> > > +	if (state->ycbcr_decode_csc)
> > > +		drm_property_blob_get(state->ycbcr_decode_csc);
> > > +
> > > +	if (state->ycbcr_csc_preoffset)
> > > +		drm_property_blob_get(state->ycbcr_csc_preoffset);
> > > +
> > >  	if (state->fb)
> > >  		drm_framebuffer_get(state->fb);
> > >  
> > > @@ -3278,6 +3284,9 @@ struct drm_plane_state *
> > >   */
> > >  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > >  {
> > > +	drm_property_blob_put(state->ycbcr_decode_csc);
> > > +	drm_property_blob_put(state->ycbcr_csc_preoffset);
> > > +
> > >  	if (state->fb)
> > >  		drm_framebuffer_put(state->fb);
> > >  
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index 245b14a..319ed46 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -95,8 +95,23 @@
> > >   *
> > >   * "YCBCR_ENCODING"
> > >   * 	Optional plane enum property to control YCbCr to RGB
> > > - * 	conversion. The driver provides a subset of standard
> > > - *	enum values supported by the DRM plane.
> > > + * 	conversion. The driver provides a subset of standard enum
> > > + * 	values supported by the DRM plane. The possible encodings
> > > + * 	include the standard conversions and a possibility to select a
> > > + * 	custom conversion matrix and preoffset vector.
> > > + *
> > > + * "YCBCR_DECODE_CSC"
> > > + *	Optional plane property blob to set YCbCr to RGB conversion
> > > + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > + *	defined in uapi/drm/drm_mode.h. Whether this property is
> > > + *	used depends on the value of YCBCR_ENCODING property.
> > > + *
> > > + * "YCBCR_CSC_PREOFFSET"
> > > + *	Optional plane property blob to configure YCbCr offset before
> > > + *	YCbCr to RGB CSC is applied. The blob contains struct
> > > + *	drm_ycbcr_csc_preoffset which is defined in
> > > + *	uapi/drm/drm_mode.h. Whether this property is used depends on
> > > + *	the value of YCBCR_ENCODING property.
> > >   */
> > >  
> > >  /**
> > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > >  	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> > >  	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> > >  	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> > > +	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
> > > +	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
> > > +	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
> > >  };
> > >  
> > >  /**
> > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > >   * drm_plane object. The supported encodings should be provided in the
> > >   * enum_list parameter. The enum_list parameter should not contain the
> > >   * enum names, because the standard names are added by this function.
> > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
> > > + * based on the provided enum_list.
> > >   */
> > >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > >  			struct drm_prop_enum_list *enum_list,
> > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > >  {
> > >  	struct drm_device *dev = plane->dev;
> > >  	struct drm_property *prop;
> > > +	bool ycbcr_decode_csc_create = false;
> > > +	bool ycbcr_csc_preoffset_create = false;
> > >  	unsigned int i;
> > >  
> > >  	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > >  		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> > >  
> > >  		enum_list[i].name = ycbcr_encoding_name[encoding];
> > > +
> > > +		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
> > > +		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
> > > +			ycbcr_decode_csc_create = true;
> > > +
> > > +		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
> > > +			ycbcr_decode_csc_create = true;
> > > +			ycbcr_csc_preoffset_create = true;
> > > +		}
> > >  	}
> > >  
> > >  	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > >  	plane->ycbcr_encoding_property = prop;
> > >  	drm_object_attach_property(&plane->base, prop, default_mode);
> > >  
> > > +	if (ycbcr_decode_csc_create) {
> > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > +					   DRM_MODE_PROP_BLOB,
> > > +					   "YCBCR_DECODE_CSC", 0);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		plane->ycbcr_decode_csc_property = prop;
> > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > +	}
> > > +
> > > +	if (ycbcr_csc_preoffset_create) {
> > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > +					   DRM_MODE_PROP_BLOB,
> > > +					   "YCBCR_CSC_PREOFFSET", 0);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		plane->ycbcr_csc_preoffset_property = prop;
> > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 007c4d7..d10c942 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> > >  	if (plane->ycbcr_encoding_property)
> > >  		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> > >  
> > > +	if (plane->ycbcr_decode_csc_property)
> > > +		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
> > > +
> > > +	if (plane->ycbcr_csc_preoffset_property)
> > > +		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
> > > +
> > >  	memset(plane, 0, sizeof(*plane));
> > >  }
> > >  EXPORT_SYMBOL(drm_plane_cleanup);
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index 1771394..bdfd37e 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
> > >  	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> > >  	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> > >  	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> > > +	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
> > > +	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
> > > +	DRM_PLANE_YCBCR_CSC_PREOFFSET,
> > >  };
> > >  
> > >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index 4d0510f..63b1e0c 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -115,6 +115,8 @@ struct drm_plane_state {
> > >  
> > >  	/* YCbCr to RGB conversion */
> > >  	enum drm_plane_ycbcr_encoding ycbcr_encoding;
> > > +	struct drm_property_blob *ycbcr_decode_csc;
> > > +	struct drm_property_blob *ycbcr_csc_preoffset;
> > >  
> > >  	/* Clipped coordinates */
> > >  	struct drm_rect src, dst;
> > > @@ -529,6 +531,8 @@ struct drm_plane {
> > >  	struct drm_property *rotation_property;
> > >  
> > >  	struct drm_property *ycbcr_encoding_property;
> > > +	struct drm_property *ycbcr_decode_csc_property;
> > > +	struct drm_property *ycbcr_csc_preoffset_property;
> > >  };
> > >  
> > >  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 8c67fc0..8c9568d 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_decode_csc {
> > > +	/* Conversion matrix in 2-complement s32.32 format. */
> > > +	__s64 ry, rcb, rcr;
> > > +	__s64 gy, gcb, gcr;
> > > +	__s64 by, bcb, bcr;
> > > +};
> > > +
> > > +struct drm_ycbcr_csc_preoffset {
> > > +	/* Offset vector in 2-complement s.32 format. */
> > > +	__s32 y, cb, cr;
> > > +};
> > > +
> > >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> > > -- 
> > > 1.9.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Sharma, Shashank]

Regards

Shashank


On 5/5/2017 12:28 PM, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
>> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
>>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
>>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
>>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
>>>> to YCBCR_ENCODING property to activate the CSC and preoffset
>>>> properties. For simplicity the new properties are stored in the
>>>> drm_plane object, because the YCBCR_ENCODING is already there. The
>>>> blob contents are defined in the uapi/drm/drm_mode.h header.
>>>>
>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> Not sure we want to make this yuv2rgb specific, plenty of planes have
>>> generic degamma/offset, csc, gamme/offset hw. I think what we want instead
>>> is the generic csc support, plus a ycbcr2rgb mode of "bypass".
>> My idea is to not expose this. And instead just expose a normal
>> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
>> that can't be done by the hw.
> But that means we'd need to have a bit-perfect match with a canonical
> conversion matrix (even if maybe the hw has a rounding bug and implements
> something slightly different than the standard). That seems a bit an
> awkward interface. Or is your idea that hw with a ctm would simply not
> expose the colorspace enum, if it doesn't also have fixed-function bits on
> top of the ctm?
> -Daniel
I think the idea is to have separate properties for CTM and Gamut 
Mapping, so that we can have more control
over linear and non-linear data blocks. All transformation should happen 
in one property whereas all gamut
mapping should go into other, which IMHO seems to be the correct way to 
operate.

- Shashank
>>> And as usual, this needs some userspace compositor which actually uses it
>>> (not just a demo, since there's a huge difference between a demo and
>>> something that has to Just Work like a real compositor).
>>> -Daniel
>>>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
>>>>   drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
>>>>   drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
>>>>   drivers/gpu/drm/drm_plane.c         |  6 ++++
>>>>   include/drm/drm_color_mgmt.h        |  3 ++
>>>>   include/drm/drm_plane.h             |  4 +++
>>>>   include/uapi/drm/drm_mode.h         | 12 ++++++++
>>>>   7 files changed, 106 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index bcef93d..87a2d55 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>   	struct drm_device *dev = plane->dev;
>>>>   	struct drm_mode_config *config = &dev->mode_config;
>>>>   	int ret;
>>>> +	bool dummy;
>>>>   
>>>>   	if (property == config->prop_fb_id) {
>>>>   		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>>>> @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>   		state->zpos = val;
>>>>   	} else if (property == plane->ycbcr_encoding_property) {
>>>>   		state->ycbcr_encoding = val;
>>>> +	} else if (property == plane->ycbcr_decode_csc_property) {
>>>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>>>> +				&state->ycbcr_decode_csc, val,
>>>> +				sizeof(struct drm_ycbcr_decode_csc),
>>>> +				&dummy);
>>>> +		return ret;
>>>> +	} else if (property == plane->ycbcr_csc_preoffset_property) {
>>>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>>>> +				&state->ycbcr_csc_preoffset, val,
>>>> +				sizeof(struct drm_ycbcr_csc_preoffset),
>>>> +				&dummy);
>>>> +		return ret;
>>>>   	} else if (plane->funcs->atomic_set_property) {
>>>>   		return plane->funcs->atomic_set_property(plane, state,
>>>>   				property, val);
>>>> @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>   		*val = state->zpos;
>>>>   	} else if (property == plane->ycbcr_encoding_property) {
>>>>   		*val = state->ycbcr_encoding;
>>>> +	} else if (property == plane->ycbcr_decode_csc_property) {
>>>> +		*val = state->ycbcr_decode_csc ?
>>>> +			state->ycbcr_decode_csc->base.id : 0;
>>>> +	} else if (property == plane->ycbcr_csc_preoffset_property) {
>>>> +		*val = state->ycbcr_csc_preoffset ?
>>>> +			state->ycbcr_csc_preoffset->base.id : 0;
>>>>   	} else if (plane->funcs->atomic_get_property) {
>>>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>>>   	} else {
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 8be9719..6ecc32f 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>>   {
>>>>   	memcpy(state, plane->state, sizeof(*state));
>>>>   
>>>> +	if (state->ycbcr_decode_csc)
>>>> +		drm_property_blob_get(state->ycbcr_decode_csc);
>>>> +
>>>> +	if (state->ycbcr_csc_preoffset)
>>>> +		drm_property_blob_get(state->ycbcr_csc_preoffset);
>>>> +
>>>>   	if (state->fb)
>>>>   		drm_framebuffer_get(state->fb);
>>>>   
>>>> @@ -3278,6 +3284,9 @@ struct drm_plane_state *
>>>>    */
>>>>   void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>>   {
>>>> +	drm_property_blob_put(state->ycbcr_decode_csc);
>>>> +	drm_property_blob_put(state->ycbcr_csc_preoffset);
>>>> +
>>>>   	if (state->fb)
>>>>   		drm_framebuffer_put(state->fb);
>>>>   
>>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>>> index 245b14a..319ed46 100644
>>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>>> @@ -95,8 +95,23 @@
>>>>    *
>>>>    * "YCBCR_ENCODING"
>>>>    * 	Optional plane enum property to control YCbCr to RGB
>>>> - * 	conversion. The driver provides a subset of standard
>>>> - *	enum values supported by the DRM plane.
>>>> + * 	conversion. The driver provides a subset of standard enum
>>>> + * 	values supported by the DRM plane. The possible encodings
>>>> + * 	include the standard conversions and a possibility to select a
>>>> + * 	custom conversion matrix and preoffset vector.
>>>> + *
>>>> + * "YCBCR_DECODE_CSC"
>>>> + *	Optional plane property blob to set YCbCr to RGB conversion
>>>> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
>>>> + *	defined in uapi/drm/drm_mode.h. Whether this property is
>>>> + *	used depends on the value of YCBCR_ENCODING property.
>>>> + *
>>>> + * "YCBCR_CSC_PREOFFSET"
>>>> + *	Optional plane property blob to configure YCbCr offset before
>>>> + *	YCbCr to RGB CSC is applied. The blob contains struct
>>>> + *	drm_ycbcr_csc_preoffset which is defined in
>>>> + *	uapi/drm/drm_mode.h. Whether this property is used depends on
>>>> + *	the value of YCBCR_ENCODING property.
>>>>    */
>>>>   
>>>>   /**
>>>> @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>>>   	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
>>>>   	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
>>>>   	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
>>>> +	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
>>>> +	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
>>>> +	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
>>>>   };
>>>>   
>>>>   /**
>>>> @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>>>    * drm_plane object. The supported encodings should be provided in the
>>>>    * enum_list parameter. The enum_list parameter should not contain the
>>>>    * enum names, because the standard names are added by this function.
>>>> + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
>>>> + * based on the provided enum_list.
>>>>    */
>>>>   int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>>>>   			struct drm_prop_enum_list *enum_list,
>>>> @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>>>>   {
>>>>   	struct drm_device *dev = plane->dev;
>>>>   	struct drm_property *prop;
>>>> +	bool ycbcr_decode_csc_create = false;
>>>> +	bool ycbcr_csc_preoffset_create = false;
>>>>   	unsigned int i;
>>>>   
>>>>   	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
>>>> @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>>>>   		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
>>>>   
>>>>   		enum_list[i].name = ycbcr_encoding_name[encoding];
>>>> +
>>>> +		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
>>>> +		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
>>>> +			ycbcr_decode_csc_create = true;
>>>> +
>>>> +		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
>>>> +			ycbcr_decode_csc_create = true;
>>>> +			ycbcr_csc_preoffset_create = true;
>>>> +		}
>>>>   	}
>>>>   
>>>>   	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>>>> @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>>>>   	plane->ycbcr_encoding_property = prop;
>>>>   	drm_object_attach_property(&plane->base, prop, default_mode);
>>>>   
>>>> +	if (ycbcr_decode_csc_create) {
>>>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>>>> +					   DRM_MODE_PROP_BLOB,
>>>> +					   "YCBCR_DECODE_CSC", 0);
>>>> +		if (!prop)
>>>> +			return -ENOMEM;
>>>> +		plane->ycbcr_decode_csc_property = prop;
>>>> +		drm_object_attach_property(&plane->base, prop, 0);
>>>> +	}
>>>> +
>>>> +	if (ycbcr_csc_preoffset_create) {
>>>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>>>> +					   DRM_MODE_PROP_BLOB,
>>>> +					   "YCBCR_CSC_PREOFFSET", 0);
>>>> +		if (!prop)
>>>> +			return -ENOMEM;
>>>> +		plane->ycbcr_csc_preoffset_property = prop;
>>>> +		drm_object_attach_property(&plane->base, prop, 0);
>>>> +	}
>>>> +
>>>>   	return 0;
>>>>   }
>>>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>>>> index 007c4d7..d10c942 100644
>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>> @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
>>>>   	if (plane->ycbcr_encoding_property)
>>>>   		drm_property_destroy(dev, plane->ycbcr_encoding_property);
>>>>   
>>>> +	if (plane->ycbcr_decode_csc_property)
>>>> +		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
>>>> +
>>>> +	if (plane->ycbcr_csc_preoffset_property)
>>>> +		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
>>>> +
>>>>   	memset(plane, 0, sizeof(*plane));
>>>>   }
>>>>   EXPORT_SYMBOL(drm_plane_cleanup);
>>>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>>>> index 1771394..bdfd37e 100644
>>>> --- a/include/drm/drm_color_mgmt.h
>>>> +++ b/include/drm/drm_color_mgmt.h
>>>> @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
>>>>   	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
>>>>   	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
>>>>   	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
>>>> +	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
>>>> +	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
>>>> +	DRM_PLANE_YCBCR_CSC_PREOFFSET,
>>>>   };
>>>>   
>>>>   int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>> index 4d0510f..63b1e0c 100644
>>>> --- a/include/drm/drm_plane.h
>>>> +++ b/include/drm/drm_plane.h
>>>> @@ -115,6 +115,8 @@ struct drm_plane_state {
>>>>   
>>>>   	/* YCbCr to RGB conversion */
>>>>   	enum drm_plane_ycbcr_encoding ycbcr_encoding;
>>>> +	struct drm_property_blob *ycbcr_decode_csc;
>>>> +	struct drm_property_blob *ycbcr_csc_preoffset;
>>>>   
>>>>   	/* Clipped coordinates */
>>>>   	struct drm_rect src, dst;
>>>> @@ -529,6 +531,8 @@ struct drm_plane {
>>>>   	struct drm_property *rotation_property;
>>>>   
>>>>   	struct drm_property *ycbcr_encoding_property;
>>>> +	struct drm_property *ycbcr_decode_csc_property;
>>>> +	struct drm_property *ycbcr_csc_preoffset_property;
>>>>   };
>>>>   
>>>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>> index 8c67fc0..8c9568d 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_decode_csc {
>>>> +	/* Conversion matrix in 2-complement s32.32 format. */
>>>> +	__s64 ry, rcb, rcr;
>>>> +	__s64 gy, gcb, gcr;
>>>> +	__s64 by, bcb, bcr;
>>>> +};
>>>> +
>>>> +struct drm_ycbcr_csc_preoffset {
>>>> +	/* Offset vector in 2-complement s.32 format. */
>>>> +	__s32 y, cb, cr;
>>>> +};
>>>> +
>>>>   #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>>>>   #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>>>>   #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>>>> -- 
>>>> 1.9.1
>>>>
>>> -- 
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
>> -- 
>> Ville Syrjälä
>> Intel OTC

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Daniel Vetter]

On Fri, May 5, 2017 at 9:06 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> On 5/5/2017 12:28 PM, Daniel Vetter wrote:
>>
>> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
>>>
>>> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
>>>>
>>>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
>>>>>
>>>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
>>>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
>>>>> to YCBCR_ENCODING property to activate the CSC and preoffset
>>>>> properties. For simplicity the new properties are stored in the
>>>>> drm_plane object, because the YCBCR_ENCODING is already there. The
>>>>> blob contents are defined in the uapi/drm/drm_mode.h header.
>>>>>
>>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>>
>>>> Not sure we want to make this yuv2rgb specific, plenty of planes have
>>>> generic degamma/offset, csc, gamme/offset hw. I think what we want
>>>> instead
>>>> is the generic csc support, plus a ycbcr2rgb mode of "bypass".
>>>
>>> My idea is to not expose this. And instead just expose a normal
>>> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
>>> that can't be done by the hw.
>>
>> But that means we'd need to have a bit-perfect match with a canonical
>> conversion matrix (even if maybe the hw has a rounding bug and implements
>> something slightly different than the standard). That seems a bit an
>> awkward interface. Or is your idea that hw with a ctm would simply not
>> expose the colorspace enum, if it doesn't also have fixed-function bits on
>> top of the ctm?
>> -Daniel
>
> I think the idea is to have separate properties for CTM and Gamut Mapping,
> so that we can have more control
> over linear and non-linear data blocks. All transformation should happen in
> one property whereas all gamut
> mapping should go into other, which IMHO seems to be the correct way to
> operate.

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

On that topic, I think it'd be good if we put the helper functions and
property documentation into drm_color_mgmt.c, so that it is all in one
place, and to make sure we don't accidentally have different meanings
for gamma table and ctm blobs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Sharma, Shashank]

Regards

Shashank


On 5/5/2017 12:43 PM, Daniel Vetter wrote:
> On Fri, May 5, 2017 at 9:06 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> On 5/5/2017 12:28 PM, Daniel Vetter wrote:
>>> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
>>>> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
>>>>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
>>>>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
>>>>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
>>>>>> to YCBCR_ENCODING property to activate the CSC and preoffset
>>>>>> properties. For simplicity the new properties are stored in the
>>>>>> drm_plane object, because the YCBCR_ENCODING is already there. The
>>>>>> blob contents are defined in the uapi/drm/drm_mode.h header.
>>>>>>
>>>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>>> Not sure we want to make this yuv2rgb specific, plenty of planes have
>>>>> generic degamma/offset, csc, gamme/offset hw. I think what we want
>>>>> instead
>>>>> is the generic csc support, plus a ycbcr2rgb mode of "bypass".
>>>> My idea is to not expose this. And instead just expose a normal
>>>> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
>>>> that can't be done by the hw.
>>> But that means we'd need to have a bit-perfect match with a canonical
>>> conversion matrix (even if maybe the hw has a rounding bug and implements
>>> something slightly different than the standard). That seems a bit an
>>> awkward interface. Or is your idea that hw with a ctm would simply not
>>> expose the colorspace enum, if it doesn't also have fixed-function bits on
>>> top of the ctm?
>>> -Daniel
>> I think the idea is to have separate properties for CTM and Gamut Mapping,
>> so that we can have more control
>> over linear and non-linear data blocks. All transformation should happen in
>> one property whereas all gamut
>> mapping should go into other, which IMHO seems to be the correct way to
>> operate.
> Yeah, for the programmable hw we should probably go with the
> degamma/ctm/gamma combo, like we have on the CRTC already. My question
> was how this interactions with some fixed-function color space
> conversion the hw might also have, and how these two sets of
> properties are mean to interact.
Even for the fixed function HW we can have this segregation, and the 
core driver can take care of understanding the enum,
and conversion into driving a fix-function block. I had proposed one 
block diagram, which was once prepared by Ville / Me
to handle this situation, but unfortunately we could not discus much there.
https://patchwork.kernel.org/patch/9695875/
> On that topic, I think it'd be good if we put the helper functions and
> property documentation into drm_color_mgmt.c, so that it is all in one
> place, and to make sure we don't accidentally have different meanings
> for gamma table and ctm blobs.
I agree, that would be  good place.
- Shashank
> -Daniel

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Ville Syrjälä]

On Fri, May 05, 2017 at 08:58:17AM +0200, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC
> > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> > > > to YCBCR_ENCODING property to activate the CSC and preoffset
> > > > properties. For simplicity the new properties are stored in the
> > > > drm_plane object, because the YCBCR_ENCODING is already there. The
> > > > blob contents are defined in the uapi/drm/drm_mode.h header.
> > > > 
> > > > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > > 
> > > Not sure we want to make this yuv2rgb specific, plenty of planes have
> > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead
> > > is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> > 
> > My idea is to not expose this. And instead just expose a normal
> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> > that can't be done by the hw.
> 
> But that means we'd need to have a bit-perfect match with a canonical
> conversion matrix (even if maybe the hw has a rounding bug and implements
> something slightly different than the standard).

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

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

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

> That seems a bit an
> awkward interface. Or is your idea that hw with a ctm would simply not
> expose the colorspace enum, if it doesn't also have fixed-function bits on
> top of the ctm?
> -Daniel
> 
> > 
> > > 
> > > And as usual, this needs some userspace compositor which actually uses it
> > > (not just a demo, since there's a huge difference between a demo and
> > > something that has to Just Work like a real compositor).
> > > -Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++
> > > >  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++
> > > >  drivers/gpu/drm/drm_color_mgmt.c    | 55 +++++++++++++++++++++++++++++++++++--
> > > >  drivers/gpu/drm/drm_plane.c         |  6 ++++
> > > >  include/drm/drm_color_mgmt.h        |  3 ++
> > > >  include/drm/drm_plane.h             |  4 +++
> > > >  include/uapi/drm/drm_mode.h         | 12 ++++++++
> > > >  7 files changed, 106 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > index bcef93d..87a2d55 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > >  	struct drm_device *dev = plane->dev;
> > > >  	struct drm_mode_config *config = &dev->mode_config;
> > > >  	int ret;
> > > > +	bool dummy;
> > > >  
> > > >  	if (property == config->prop_fb_id) {
> > > >  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> > > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > >  		state->zpos = val;
> > > >  	} else if (property == plane->ycbcr_encoding_property) {
> > > >  		state->ycbcr_encoding = val;
> > > > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > > > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > +				&state->ycbcr_decode_csc, val,
> > > > +				sizeof(struct drm_ycbcr_decode_csc),
> > > > +				&dummy);
> > > > +		return ret;
> > > > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > > > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > +				&state->ycbcr_csc_preoffset, val,
> > > > +				sizeof(struct drm_ycbcr_csc_preoffset),
> > > > +				&dummy);
> > > > +		return ret;
> > > >  	} else if (plane->funcs->atomic_set_property) {
> > > >  		return plane->funcs->atomic_set_property(plane, state,
> > > >  				property, val);
> > > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > >  		*val = state->zpos;
> > > >  	} else if (property == plane->ycbcr_encoding_property) {
> > > >  		*val = state->ycbcr_encoding;
> > > > +	} else if (property == plane->ycbcr_decode_csc_property) {
> > > > +		*val = state->ycbcr_decode_csc ?
> > > > +			state->ycbcr_decode_csc->base.id : 0;
> > > > +	} else if (property == plane->ycbcr_csc_preoffset_property) {
> > > > +		*val = state->ycbcr_csc_preoffset ?
> > > > +			state->ycbcr_csc_preoffset->base.id : 0;
> > > >  	} else if (plane->funcs->atomic_get_property) {
> > > >  		return plane->funcs->atomic_get_property(plane, state, property, val);
> > > >  	} else {
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 8be9719..6ecc32f 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > > >  {
> > > >  	memcpy(state, plane->state, sizeof(*state));
> > > >  
> > > > +	if (state->ycbcr_decode_csc)
> > > > +		drm_property_blob_get(state->ycbcr_decode_csc);
> > > > +
> > > > +	if (state->ycbcr_csc_preoffset)
> > > > +		drm_property_blob_get(state->ycbcr_csc_preoffset);
> > > > +
> > > >  	if (state->fb)
> > > >  		drm_framebuffer_get(state->fb);
> > > >  
> > > > @@ -3278,6 +3284,9 @@ struct drm_plane_state *
> > > >   */
> > > >  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > > >  {
> > > > +	drm_property_blob_put(state->ycbcr_decode_csc);
> > > > +	drm_property_blob_put(state->ycbcr_csc_preoffset);
> > > > +
> > > >  	if (state->fb)
> > > >  		drm_framebuffer_put(state->fb);
> > > >  
> > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > > index 245b14a..319ed46 100644
> > > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > > @@ -95,8 +95,23 @@
> > > >   *
> > > >   * "YCBCR_ENCODING"
> > > >   * 	Optional plane enum property to control YCbCr to RGB
> > > > - * 	conversion. The driver provides a subset of standard
> > > > - *	enum values supported by the DRM plane.
> > > > + * 	conversion. The driver provides a subset of standard enum
> > > > + * 	values supported by the DRM plane. The possible encodings
> > > > + * 	include the standard conversions and a possibility to select a
> > > > + * 	custom conversion matrix and preoffset vector.
> > > > + *
> > > > + * "YCBCR_DECODE_CSC"
> > > > + *	Optional plane property blob to set YCbCr to RGB conversion
> > > > + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > > + *	defined in uapi/drm/drm_mode.h. Whether this property is
> > > > + *	used depends on the value of YCBCR_ENCODING property.
> > > > + *
> > > > + * "YCBCR_CSC_PREOFFSET"
> > > > + *	Optional plane property blob to configure YCbCr offset before
> > > > + *	YCbCr to RGB CSC is applied. The blob contains struct
> > > > + *	drm_ycbcr_csc_preoffset which is defined in
> > > > + *	uapi/drm/drm_mode.h. Whether this property is used depends on
> > > > + *	the value of YCBCR_ENCODING property.
> > > >   */
> > > >  
> > > >  /**
> > > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > > >  	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> > > >  	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> > > >  	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> > > > +	[DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
> > > > +	[DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
> > > > +	[DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > > >   * drm_plane object. The supported encodings should be provided in the
> > > >   * enum_list parameter. The enum_list parameter should not contain the
> > > >   * enum names, because the standard names are added by this function.
> > > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
> > > > + * based on the provided enum_list.
> > > >   */
> > > >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > >  			struct drm_prop_enum_list *enum_list,
> > > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > >  {
> > > >  	struct drm_device *dev = plane->dev;
> > > >  	struct drm_property *prop;
> > > > +	bool ycbcr_decode_csc_create = false;
> > > > +	bool ycbcr_csc_preoffset_create = false;
> > > >  	unsigned int i;
> > > >  
> > > >  	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> > > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > >  		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> > > >  
> > > >  		enum_list[i].name = ycbcr_encoding_name[encoding];
> > > > +
> > > > +		if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
> > > > +		    encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
> > > > +			ycbcr_decode_csc_create = true;
> > > > +
> > > > +		if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
> > > > +			ycbcr_decode_csc_create = true;
> > > > +			ycbcr_csc_preoffset_create = true;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > >  	plane->ycbcr_encoding_property = prop;
> > > >  	drm_object_attach_property(&plane->base, prop, default_mode);
> > > >  
> > > > +	if (ycbcr_decode_csc_create) {
> > > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > > +					   DRM_MODE_PROP_BLOB,
> > > > +					   "YCBCR_DECODE_CSC", 0);
> > > > +		if (!prop)
> > > > +			return -ENOMEM;
> > > > +		plane->ycbcr_decode_csc_property = prop;
> > > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > > +	}
> > > > +
> > > > +	if (ycbcr_csc_preoffset_create) {
> > > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > > +					   DRM_MODE_PROP_BLOB,
> > > > +					   "YCBCR_CSC_PREOFFSET", 0);
> > > > +		if (!prop)
> > > > +			return -ENOMEM;
> > > > +		plane->ycbcr_csc_preoffset_property = prop;
> > > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index 007c4d7..d10c942 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> > > >  	if (plane->ycbcr_encoding_property)
> > > >  		drm_property_destroy(dev, plane->ycbcr_encoding_property);
> > > >  
> > > > +	if (plane->ycbcr_decode_csc_property)
> > > > +		drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
> > > > +
> > > > +	if (plane->ycbcr_csc_preoffset_property)
> > > > +		drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
> > > > +
> > > >  	memset(plane, 0, sizeof(*plane));
> > > >  }
> > > >  EXPORT_SYMBOL(drm_plane_cleanup);
> > > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > > index 1771394..bdfd37e 100644
> > > > --- a/include/drm/drm_color_mgmt.h
> > > > +++ b/include/drm/drm_color_mgmt.h
> > > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
> > > >  	DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> > > >  	DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> > > >  	DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> > > > +	DRM_PLANE_YCBCR_CSC_FULL_RANGE,
> > > > +	DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
> > > > +	DRM_PLANE_YCBCR_CSC_PREOFFSET,
> > > >  };
> > > >  
> > > >  int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > index 4d0510f..63b1e0c 100644
> > > > --- a/include/drm/drm_plane.h
> > > > +++ b/include/drm/drm_plane.h
> > > > @@ -115,6 +115,8 @@ struct drm_plane_state {
> > > >  
> > > >  	/* YCbCr to RGB conversion */
> > > >  	enum drm_plane_ycbcr_encoding ycbcr_encoding;
> > > > +	struct drm_property_blob *ycbcr_decode_csc;
> > > > +	struct drm_property_blob *ycbcr_csc_preoffset;
> > > >  
> > > >  	/* Clipped coordinates */
> > > >  	struct drm_rect src, dst;
> > > > @@ -529,6 +531,8 @@ struct drm_plane {
> > > >  	struct drm_property *rotation_property;
> > > >  
> > > >  	struct drm_property *ycbcr_encoding_property;
> > > > +	struct drm_property *ycbcr_decode_csc_property;
> > > > +	struct drm_property *ycbcr_csc_preoffset_property;
> > > >  };
> > > >  
> > > >  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > index 8c67fc0..8c9568d 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_decode_csc {
> > > > +	/* Conversion matrix in 2-complement s32.32 format. */
> > > > +	__s64 ry, rcb, rcr;
> > > > +	__s64 gy, gcb, gcr;
> > > > +	__s64 by, bcb, bcr;
> > > > +};
> > > > +
> > > > +struct drm_ycbcr_csc_preoffset {
> > > > +	/* Offset vector in 2-complement s.32 format. */
> > > > +	__s32 y, cb, cr;
> > > > +};
> > > > +
> > > >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > > >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > > >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> > > > -- 
> > > > 1.9.1
> > > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Daniel Vetter]

On Fri, May 5, 2017 at 12:45 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, May 05, 2017 at 08:58:17AM +0200, Daniel Vetter wrote:
>> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
>> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
>> > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
>> > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC
>> > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined
>> > > > to YCBCR_ENCODING property to activate the CSC and preoffset
>> > > > properties. For simplicity the new properties are stored in the
>> > > > drm_plane object, because the YCBCR_ENCODING is already there. The
>> > > > blob contents are defined in the uapi/drm/drm_mode.h header.
>> > > >
>> > > > Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> > >
>> > > Not sure we want to make this yuv2rgb specific, plenty of planes have
>> > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead
>> > > is the generic csc support, plus a ycbcr2rgb mode of "bypass".
>> >
>> > My idea is to not expose this. And instead just expose a normal
>> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
>> > that can't be done by the hw.
>>
>> But that means we'd need to have a bit-perfect match with a canonical
>> conversion matrix (even if maybe the hw has a rounding bug and implements
>> something slightly different than the standard).
>
> I don't see what this has to do with bit correctness. Different hw is
> likely to use different precision for this stuff anyway, so I'm 100%
> sure we'll not get the same results from all hardware. And really,
> getting 100% same output doesn't seem all that important here
> anyway.
>
> Anyway, all I'm saying is that we should expose a standard pipeline:
> ycbcr->rgb -> degamma -> ctm -> gamma
>
> And each driver can then expose whatever parts of that they wish. If you
> want to expoe a programmable matrix you expose it as the ctm. I don't
> think having a programmable matrix as both ycbcr->rgb and ctm would
> really benefit us.

Ok, that makes sense. I was somehow assuming you're suggesting we only expose a
colors -> degamma -> ctm -> gamma pipeline and drivers with only fixed
function ycbcr2rbg conversion would then need to infer which exact
btsomethingsomething standard it should pick.

The above makes perfect sense, sorry for the confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel