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

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

[Jyri Sarha]

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

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

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

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

Best regards,
Jyri

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

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

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

-- 
1.9.1

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

[PATCH RFC 1/6] drm: drm_color_mgmt.h needs struct drm_crtc declaration

[Jyri Sarha]

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 include/drm/drm_color_mgmt.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index bce4a53..03a59cb 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -25,6 +25,8 @@
 
 #include <linux/ctype.h>
 
+struct drm_crtc;
+
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
 
 void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
-- 
1.9.1

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

Re: [PATCH RFC 1/6] drm: drm_color_mgmt.h needs struct drm_crtc declaration

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch

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

No commit message ?

> Signed-off-by: Jyri Sarha <jsarha@ti.com>

Apart from that,

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

> ---
>  include/drm/drm_color_mgmt.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index bce4a53..03a59cb 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -25,6 +25,8 @@
> 
>  #include <linux/ctype.h>
> 
> +struct drm_crtc;
> +
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t
> bit_precision);
> 
>  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,

-- 
Regards,

Laurent Pinchart

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

[PATCH RFC 2/6] drm: Make drm_atomic_replace_property_blob_from_id() more generic

[Jyri Sarha]

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

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af..f881319 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -425,7 +425,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 }
 
 static int
-drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
+drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
 					 struct drm_property_blob **blob,
 					 uint64_t blob_id,
 					 ssize_t expected_size,
@@ -434,7 +434,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 	struct drm_property_blob *new_blob = NULL;
 
 	if (blob_id != 0) {
-		new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
+		new_blob = drm_property_lookup_blob(dev, blob_id);
 		if (new_blob == NULL)
 			return -EINVAL;
 
@@ -483,7 +483,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		drm_property_blob_put(mode);
 		return ret;
 	} else if (property == config->degamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
 					val,
 					-1,
@@ -491,7 +491,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->ctm_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->ctm,
 					val,
 					sizeof(struct drm_color_ctm),
@@ -499,7 +499,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		state->color_mgmt_changed |= replaced;
 		return ret;
 	} else if (property == config->gamma_lut_property) {
-		ret = drm_atomic_replace_property_blob_from_id(crtc,
+		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->gamma_lut,
 					val,
 					-1,
-- 
1.9.1

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

Re: [PATCH RFC 2/6] drm: Make drm_atomic_replace_property_blob_from_id() more generic

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:13 Jyri Sarha wrote:
> Change drm_atomic_replace_property_blob_from_id()'s first parameter
> from drm_crtc to drm_device, so that the function can be used for other
> drm_mode_objects too.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

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

> ---
>  drivers/gpu/drm/drm_atomic.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af..f881319 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -425,7 +425,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
> drm_crtc_state *state, }
> 
>  static int
> -drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
> +drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>  					 struct drm_property_blob **blob,
>  					 uint64_t blob_id,
>  					 ssize_t expected_size,
> @@ -434,7 +434,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
> drm_crtc_state *state, struct drm_property_blob *new_blob = NULL;
> 
>  	if (blob_id != 0) {
> -		new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
> +		new_blob = drm_property_lookup_blob(dev, blob_id);
>  		if (new_blob == NULL)
>  			return -EINVAL;
> 
> @@ -483,7 +483,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		drm_property_blob_put(mode);
>  		return ret;
>  	} else if (property == config->degamma_lut_property) {
> -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->degamma_lut,
>  					val,
>  					-1,
> @@ -491,7 +491,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
>  	} else if (property == config->ctm_property) {
> -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->ctm,
>  					val,
>  					sizeof(struct drm_color_ctm),
> @@ -499,7 +499,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
>  	} else if (property == config->gamma_lut_property) {
> -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->gamma_lut,
>  					val,
>  					-1,

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH RFC 2/6] drm: Make drm_atomic_replace_property_blob_from_id() more generic

[Daniel Vetter]

On Fri, Apr 21, 2017 at 02:47:44PM +0300, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Friday 21 Apr 2017 12:51:13 Jyri Sarha wrote:
> > Change drm_atomic_replace_property_blob_from_id()'s first parameter
> > from drm_crtc to drm_device, so that the function can be used for other
> > drm_mode_objects too.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

First two patches applied for 4.13, thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 9b892af..f881319 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -425,7 +425,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
> > drm_crtc_state *state, }
> > 
> >  static int
> > -drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
> > +drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >  					 struct drm_property_blob **blob,
> >  					 uint64_t blob_id,
> >  					 ssize_t expected_size,
> > @@ -434,7 +434,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct
> > drm_crtc_state *state, struct drm_property_blob *new_blob = NULL;
> > 
> >  	if (blob_id != 0) {
> > -		new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
> > +		new_blob = drm_property_lookup_blob(dev, blob_id);
> >  		if (new_blob == NULL)
> >  			return -EINVAL;
> > 
> > @@ -483,7 +483,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  		drm_property_blob_put(mode);
> >  		return ret;
> >  	} else if (property == config->degamma_lut_property) {
> > -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> >  					&state->degamma_lut,
> >  					val,
> >  					-1,
> > @@ -491,7 +491,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  		state->color_mgmt_changed |= replaced;
> >  		return ret;
> >  	} else if (property == config->ctm_property) {
> > -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> >  					&state->ctm,
> >  					val,
> >  					sizeof(struct drm_color_ctm),
> > @@ -499,7 +499,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  		state->color_mgmt_changed |= replaced;
> >  		return ret;
> >  	} else if (property == config->gamma_lut_property) {
> > -		ret = drm_atomic_replace_property_blob_from_id(crtc,
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> >  					&state->gamma_lut,
> >  					val,
> >  					-1,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

[PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

Add standard properties to control YCbCr to RGB conversion in DRM
planes. The created properties are stored to drm_plane object to allow
different set of supported conversion modes for different planes on
the device. For simplicity the related property blobs for CSC matrix
and YCbCr preoffsets are also stored in the same place. The blob
contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic.c        |  26 +++++++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_plane.c         |  10 +++
 include/drm/drm_color_mgmt.h        |  23 ++++++
 include/drm/drm_plane.h             |  10 +++
 include/uapi/drm/drm_mode.h         |  12 ++++
 7 files changed, 223 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f881319..d1512aa 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	int ret;
+	bool dummy;
 
 	if (property == config->prop_fb_id) {
 		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
@@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->rotation = val;
 	} else if (property == plane->zpos_property) {
 		state->zpos = val;
+	} else if (property == plane->ycbcr_to_rgb_mode_property) {
+		state->ycbcr_to_rgb_mode = val;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_csc,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_csc),
+				&dummy);
+		return ret;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+				&state->ycbcr_to_rgb_preoffset,
+				val,
+				sizeof(struct drm_ycbcr_to_rgb_preoffset),
+				&dummy);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);
@@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		*val = state->rotation;
 	} else if (property == plane->zpos_property) {
 		*val = state->zpos;
+	} else if (property == plane->ycbcr_to_rgb_mode_property) {
+		*val = state->ycbcr_to_rgb_mode;
+	} else if (property == plane->ycbcr_to_rgb_csc_property) {
+		*val = state->ycbcr_to_rgb_csc ?
+			state->ycbcr_to_rgb_csc->base.id : 0;
+	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+		*val = state->ycbcr_to_rgb_preoffset ?
+			state->ycbcr_to_rgb_preoffset->base.id : 0;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c3994b4..89fd826 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 {
 	memcpy(state, plane->state, sizeof(*state));
 
+	if (state->ycbcr_to_rgb_csc)
+		drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+	if (state->ycbcr_to_rgb_preoffset)
+		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_get(state->fb);
 
@@ -3236,6 +3242,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+	drm_property_blob_put(state->ycbcr_to_rgb_csc);
+	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
 	if (state->fb)
 		drm_framebuffer_put(state->fb);
 
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index cc23b9a..badaddd 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the &drm_crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the &drm_crtc object. They are set
+ * up by calling drm_crtc_enable_color_mgmt().
+ *
+ * Color space conversions from YCbCr to RGB color space in planes is
+ * supporter trough 3 optional properties in &drm_plane object.
+ *
+ * The &drm_crtc object's properties are:
  *
  * "DEGAMMA_LUT”:
  *	Blob property to set the degamma lookup table (LUT) mapping pixel data
@@ -85,6 +90,28 @@
  * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
  * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
  * "GAMMA_LUT" property above.
+ *
+ * The &drm_plane object's properties are:
+ *
+ * "YCBCR_TO_RGB_MODE"
+ * 	Optional plane enum property to configure YCbCr to RGB
+ * 	conversion. The possible modes include a number of standard
+ * 	conversions and a possibility to select custom conversion
+ * 	matrix and preoffset vector. The driver should select the
+ *	supported subset of of the modes.
+ *
+ * "YCBCR_TO_RGB_CSC"
+ *	Optional plane property blob to set YCbCr to RGB conversion
+ *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
+ *	defined in uapi/drm/drm_mode.h. Whether this property is
+ *	active dependent on YCBCR_TO_RGB_MODE property.
+ *
+ * "YCBCR_TO_RGB_PREOFFSET"
+ *	Optional plane property blob to configure YCbCr offset before
+ *	YCbCr to RGB CSC is applied. The blob contains struct
+ *	drm_ycbcr_to_rgb_preoffset which is defined in
+ *	uapi/drm/drm_mode.h. Whether this property is active dependent
+ *	on YCBCR_TO_RGB_MODE property.
  */
 
 /**
@@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+static char *ycbcr_to_rgb_mode_name[] = {
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
+		"YCbCr BT.601 full range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
+		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
+	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
+		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
+		"YCbCr TO RGB CSC limited range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
+		"YCbCr TO RGB CSC full range preoffset",
+	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
+		"YCBCR TO RGB CSC preoffset vector",
+};
+
+/**
+ * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
+ * @enum_list: drm_prop_enum_list array of supported modes without names
+ * @enum_list_len: length of enum_list array
+ * @default_mode: default csc mode
+ *
+ * Create and attach plane specific YCbCr to RGB conversion related
+ * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
+ * created and an the supported modes should be provided the enum_list
+ * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
+ * created based on supported conversion modes. The enum_list parameter
+ * should not contain the enum names, because the standard names are
+ * added by this function.
+ */
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	bool ycbcr_to_rgb_csc_create = false;
+	bool ycbcr_to_rgb_preoffset_create = false;
+	int i;
+
+	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
+		return -EEXIST;
+
+	for (i = 0; i < enum_list_len; i++) {
+		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
+
+		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
+		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
+			ycbcr_to_rgb_csc_create = true;
+
+		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
+			ycbcr_to_rgb_csc_create = true;
+			ycbcr_to_rgb_preoffset_create = true;
+		}
+	}
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+					"YCBCR_TO_RGB_MODE",
+					enum_list, enum_list_len);
+	if (!prop)
+		return -ENOMEM;
+	plane->ycbcr_to_rgb_mode_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_mode);
+
+	if (ycbcr_to_rgb_csc_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_CSC", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_csc_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	if (ycbcr_to_rgb_preoffset_create) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_BLOB,
+					   "YCBCR_TO_RGB_PREOFFSET", 0);
+		if (!prop)
+			return -ENOMEM;
+		plane->ycbcr_to_rgb_preoffset_property = prop;
+		drm_object_attach_property(&plane->base, prop, 0);
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index bc71aa2..4c7e827 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	kfree(plane->name);
 
+	if (plane->ycbcr_to_rgb_mode_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
+
+	if (plane->ycbcr_to_rgb_csc_property)
+		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
+
+	if (plane->ycbcr_to_rgb_preoffset_property)
+		drm_property_destroy(dev,
+				     plane->ycbcr_to_rgb_preoffset_property);
+
 	memset(plane, 0, sizeof(*plane));
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 03a59cb..a20b3ff 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -26,6 +26,8 @@
 #include <linux/ctype.h>
 
 struct drm_crtc;
+struct drm_plane;
+struct drm_prop_enum_list;
 
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
 
@@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 
+enum drm_plane_ycbcr_to_rgb_mode {
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
+	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
+	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
+	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
+};
+
+int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
+			struct drm_prop_enum_list *enum_list,
+			unsigned int enum_list_len,
+			enum drm_plane_ycbcr_to_rgb_mode default_mode);
+
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 9ab3e70..41dcde2 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/ctype.h>
 #include <drm/drm_mode_object.h>
+#include <drm/drm_color_mgmt.h>
 
 struct drm_crtc;
 struct drm_printer;
@@ -112,6 +113,11 @@ struct drm_plane_state {
 	unsigned int zpos;
 	unsigned int normalized_zpos;
 
+	/* YCbCr to RGB conversion */
+	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
+	struct drm_property_blob *ycbcr_to_rgb_csc;
+	struct drm_property_blob *ycbcr_to_rgb_preoffset;
+
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 
@@ -523,6 +529,10 @@ struct drm_plane {
 
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
+
+	struct drm_property *ycbcr_to_rgb_mode_property;
+	struct drm_property *ycbcr_to_rgb_csc_property;
+	struct drm_property *ycbcr_to_rgb_preoffset_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 8c67fc0..27e0bee 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -543,6 +543,18 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+struct drm_ycbcr_to_rgb_csc {
+	/* Conversion matrix in 2-complement s32.32 format. */
+	__s64 ry, rcb, rcr;
+	__s64 gy, gcb, gcr;
+	__s64 by, bcb, bcr;
+};
+
+struct drm_ycbcr_to_rgb_preoffset {
+	/* Offset vector in 2-complement s.32 format. */
+	__s32 y, cb, cr;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
1.9.1

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> Add standard properties to control YCbCr to RGB conversion in DRM
> planes. The created properties are stored to drm_plane object to allow
> different set of supported conversion modes for different planes on
> the device. For simplicity the related property blobs for CSC matrix
> and YCbCr preoffsets are also stored in the same place. The blob
> contents are defined in the uapi/drm/drm_mode.h header.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  26 +++++++
>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>  drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_plane.c         |  10 +++
>  include/drm/drm_color_mgmt.h        |  23 ++++++
>  include/drm/drm_plane.h             |  10 +++
>  include/uapi/drm/drm_mode.h         |  12 ++++
>  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f881319..d1512aa 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	int ret;
> +	bool dummy;
>  
>  	if (property == config->prop_fb_id) {
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->rotation = val;
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
> +		state->ycbcr_to_rgb_mode = val;
> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_to_rgb_csc,
> +				val,
> +				sizeof(struct drm_ycbcr_to_rgb_csc),
> +				&dummy);
> +		return ret;
> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_to_rgb_preoffset,
> +				val,
> +				sizeof(struct drm_ycbcr_to_rgb_preoffset),
> +				&dummy);
> +		return ret;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
>  		*val = state->zpos;
> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
> +		*val = state->ycbcr_to_rgb_mode;
> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
> +		*val = state->ycbcr_to_rgb_csc ?
> +			state->ycbcr_to_rgb_csc->base.id : 0;
> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> +		*val = state->ycbcr_to_rgb_preoffset ?
> +			state->ycbcr_to_rgb_preoffset->base.id : 0;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3994b4..89fd826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  {
>  	memcpy(state, plane->state, sizeof(*state));
>  
> +	if (state->ycbcr_to_rgb_csc)
> +		drm_property_blob_get(state->ycbcr_to_rgb_csc);
> +
> +	if (state->ycbcr_to_rgb_preoffset)
> +		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_get(state->fb);
>  
> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>   */
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
> +	drm_property_blob_put(state->ycbcr_to_rgb_csc);
> +	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_put(state->fb);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index cc23b9a..badaddd 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -29,9 +29,14 @@
>  /**
>   * DOC: overview
>   *
> - * Color management or color space adjustments is supported through a set of 5
> - * properties on the &drm_crtc object. They are set up by calling
> - * drm_crtc_enable_color_mgmt().
> + * Color management or color space adjustments in CRTCs is supported
> + * through a set of 5 properties on the &drm_crtc object. They are set
> + * up by calling drm_crtc_enable_color_mgmt().
> + *
> + * Color space conversions from YCbCr to RGB color space in planes is
> + * supporter trough 3 optional properties in &drm_plane object.
> + *
> + * The &drm_crtc object's properties are:
>   *
>   * "DEGAMMA_LUT”:
>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> @@ -85,6 +90,28 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>   * "GAMMA_LUT" property above.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "YCBCR_TO_RGB_MODE"
> + * 	Optional plane enum property to configure YCbCr to RGB
> + * 	conversion. The possible modes include a number of standard
> + * 	conversions and a possibility to select custom conversion
> + * 	matrix and preoffset vector. The driver should select the
> + *	supported subset of of the modes.
> + *
> + * "YCBCR_TO_RGB_CSC"
> + *	Optional plane property blob to set YCbCr to RGB conversion
> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> + *	defined in uapi/drm/drm_mode.h. Whether this property is
> + *	active dependent on YCBCR_TO_RGB_MODE property.
> + *
> + * "YCBCR_TO_RGB_PREOFFSET"
> + *	Optional plane property blob to configure YCbCr offset before
> + *	YCbCr to RGB CSC is applied. The blob contains struct
> + *	drm_ycbcr_to_rgb_preoffset which is defined in
> + *	uapi/drm/drm_mode.h. Whether this property is active dependent
> + *	on YCBCR_TO_RGB_MODE property.
>   */
>  
>  /**
> @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock_all(dev);
>  	return ret;
>  }
> +
> +static char *ycbcr_to_rgb_mode_name[] = {
> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",

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

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

> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
> +		"YCbCr TO RGB CSC limited range preoffset",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
> +		"YCbCr TO RGB CSC full range preoffset",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
> +		"YCBCR TO RGB CSC preoffset vector",
> +};
> +
> +/**
> + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
> + * @enum_list: drm_prop_enum_list array of supported modes without names
> + * @enum_list_len: length of enum_list array
> + * @default_mode: default csc mode
> + *
> + * Create and attach plane specific YCbCr to RGB conversion related
> + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
> + * created and an the supported modes should be provided the enum_list
> + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
> + * created based on supported conversion modes. The enum_list parameter
> + * should not contain the enum names, because the standard names are
> + * added by this function.
> + */
> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_to_rgb_mode default_mode)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	bool ycbcr_to_rgb_csc_create = false;
> +	bool ycbcr_to_rgb_preoffset_create = false;
> +	int i;
> +
> +	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
> +		return -EEXIST;
> +
> +	for (i = 0; i < enum_list_len; i++) {
> +		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
> +
> +		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
> +
> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
> +		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
> +			ycbcr_to_rgb_csc_create = true;
> +
> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
> +			ycbcr_to_rgb_csc_create = true;
> +			ycbcr_to_rgb_preoffset_create = true;
> +		}
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"YCBCR_TO_RGB_MODE",
> +					enum_list, enum_list_len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->ycbcr_to_rgb_mode_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_mode);
> +
> +	if (ycbcr_to_rgb_csc_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_TO_RGB_CSC", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_to_rgb_csc_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
> +	if (ycbcr_to_rgb_preoffset_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_TO_RGB_PREOFFSET", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_to_rgb_preoffset_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index bc71aa2..4c7e827 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	kfree(plane->name);
>  
> +	if (plane->ycbcr_to_rgb_mode_property)
> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
> +
> +	if (plane->ycbcr_to_rgb_csc_property)
> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
> +
> +	if (plane->ycbcr_to_rgb_preoffset_property)
> +		drm_property_destroy(dev,
> +				     plane->ycbcr_to_rgb_preoffset_property);
> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cb..a20b3ff 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,8 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
> +struct drm_prop_enum_list;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  				 int gamma_size);
>  
> +enum drm_plane_ycbcr_to_rgb_mode {
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
> +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
> +};
> +
> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_to_rgb_mode default_mode);
> +
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e70..41dcde2 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/ctype.h>
>  #include <drm/drm_mode_object.h>
> +#include <drm/drm_color_mgmt.h>
>  
>  struct drm_crtc;
>  struct drm_printer;
> @@ -112,6 +113,11 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* YCbCr to RGB conversion */
> +	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
> +	struct drm_property_blob *ycbcr_to_rgb_csc;
> +	struct drm_property_blob *ycbcr_to_rgb_preoffset;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -523,6 +529,10 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	struct drm_property *ycbcr_to_rgb_mode_property;
> +	struct drm_property *ycbcr_to_rgb_csc_property;
> +	struct drm_property *ycbcr_to_rgb_preoffset_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc0..27e0bee 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -543,6 +543,18 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +struct drm_ycbcr_to_rgb_csc {
> +	/* Conversion matrix in 2-complement s32.32 format. */
> +	__s64 ry, rcb, rcr;
> +	__s64 gy, gcb, gcr;
> +	__s64 by, bcb, bcr;
> +};
> +
> +struct drm_ycbcr_to_rgb_preoffset {
> +	/* Offset vector in 2-complement s.32 format. */
> +	__s32 y, cb, cr;
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> -- 
> 1.9.1

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Laurent Pinchart]

Hello,

CC'ing Hans Verkuil for his knowledge on colorspace.

On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> > Add standard properties to control YCbCr to RGB conversion in DRM
> > planes. The created properties are stored to drm_plane object to allow
> > different set of supported conversion modes for different planes on
> > the device. For simplicity the related property blobs for CSC matrix
> > and YCbCr preoffsets are also stored in the same place. The blob
> > contents are defined in the uapi/drm/drm_mode.h header.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_atomic.c        |  26 +++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
> >  drivers/gpu/drm/drm_color_mgmt.c    | 136 ++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/drm_plane.c         |  10 +++
> >  include/drm/drm_color_mgmt.h        |  23 ++++++
> >  include/drm/drm_plane.h             |  10 +++
> >  include/uapi/drm/drm_mode.h         |  12 ++++
> >  7 files changed, 223 insertions(+), 3 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -29,9 +29,14 @@
> >  /**
> >   * DOC: overview
> >   *
> > - * Color management or color space adjustments is supported through a set
> > of 5
> > - * properties on the &drm_crtc object. They are set up by calling
> > - * drm_crtc_enable_color_mgmt().
> > + * Color management or color space adjustments in CRTCs is supported
> > + * through a set of 5 properties on the &drm_crtc object. They are set
> > + * up by calling drm_crtc_enable_color_mgmt().
> > + *
> > + * Color space conversions from YCbCr to RGB color space in planes is
> > + * supporter trough 3 optional properties in &drm_plane object.
> > + *
> > + * The &drm_crtc object's properties are:
> >   *
> >   * "DEGAMMA_LUT”:
> >   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> > @@ -85,6 +90,28 @@
> >   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> >   with the
> >   * "GAMMA_LUT" property above.
> > 
> > + *
> > + * The &drm_plane object's properties are:
> > + *
> > + * "YCBCR_TO_RGB_MODE"
> > + * 	Optional plane enum property to configure YCbCr to RGB
> > + * 	conversion. The possible modes include a number of standard
> > + * 	conversions and a possibility to select custom conversion
> > + * 	matrix and preoffset vector. The driver should select the
> > + *		supported subset of of the modes.
> > + *
> > + * "YCBCR_TO_RGB_CSC"
> > + *	Optional plane property blob to set YCbCr to RGB conversion
> > + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > + *	defined in uapi/drm/drm_mode.h. Whether this property is
> > + *	active dependent on YCBCR_TO_RGB_MODE property.
> > + *
> > + * "YCBCR_TO_RGB_PREOFFSET"
> > + *	Optional plane property blob to configure YCbCr offset before
> > + *	YCbCr to RGB CSC is applied. The blob contains struct
> > + *	drm_ycbcr_to_rgb_preoffset which is defined in
> > + *	uapi/drm/drm_mode.h. Whether this property is active dependent
> > + *	on YCBCR_TO_RGB_MODE property.
> >   */
> >  
> >  /**
> > @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >  	drm_modeset_unlock_all(dev);
> >  	return ret;
> >  }
> > +
> > +static char *ycbcr_to_rgb_mode_name[] = {
> > +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> > +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> > +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> > +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> > +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> > +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> > +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> > +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> > +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> > +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> 
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
> 
> After staring at the v4l docs on this stuff I kinda like their
> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> little partial to calling the prop something like YCBCR_ENCODING.

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

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

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

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

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

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

[snip]

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

[snip]

> > @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> >  				 int gamma_size);
> > 
> > +enum drm_plane_ycbcr_to_rgb_mode {
> > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
> > +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
> > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
> > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
> > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
> > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
> > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
> > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
> > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
> > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
> > +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
> > +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
> > +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,

All those values should be documented.

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

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

[snip]

> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 8c67fc0..27e0bee 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -543,6 +543,18 @@ struct drm_color_lut {
> >  	__u16 reserved;
> >  };
> > 
> > +struct drm_ycbcr_to_rgb_csc {
> > +	/* Conversion matrix in 2-complement s32.32 format. */
> > +	__s64 ry, rcb, rcr;
> > +	__s64 gy, gcb, gcr;
> > +	__s64 by, bcb, bcr;
> > +};
> > +
> > +struct drm_ycbcr_to_rgb_preoffset {
> > +	/* Offset vector in 2-complement s.32 format. */
> > +	__s32 y, cb, cr;
> > +};
> > +
> > 
> >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 03:10:31PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> CC'ing Hans Verkuil for his knowledge on colorspace.
> 
> On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> > > Add standard properties to control YCbCr to RGB conversion in DRM
> > > planes. The created properties are stored to drm_plane object to allow
> > > different set of supported conversion modes for different planes on
> > > the device. For simplicity the related property blobs for CSC matrix
> > > and YCbCr preoffsets are also stored in the same place. The blob
> > > contents are defined in the uapi/drm/drm_mode.h header.
> > > 
> > > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic.c        |  26 +++++++
> > >  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
> > >  drivers/gpu/drm/drm_color_mgmt.c    | 136 ++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/drm_plane.c         |  10 +++
> > >  include/drm/drm_color_mgmt.h        |  23 ++++++
> > >  include/drm/drm_plane.h             |  10 +++
> > >  include/uapi/drm/drm_mode.h         |  12 ++++
> > >  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> [snip]
> 
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > > b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -29,9 +29,14 @@
> > >  /**
> > >   * DOC: overview
> > >   *
> > > - * Color management or color space adjustments is supported through a set
> > > of 5
> > > - * properties on the &drm_crtc object. They are set up by calling
> > > - * drm_crtc_enable_color_mgmt().
> > > + * Color management or color space adjustments in CRTCs is supported
> > > + * through a set of 5 properties on the &drm_crtc object. They are set
> > > + * up by calling drm_crtc_enable_color_mgmt().
> > > + *
> > > + * Color space conversions from YCbCr to RGB color space in planes is
> > > + * supporter trough 3 optional properties in &drm_plane object.
> > > + *
> > > + * The &drm_crtc object's properties are:
> > >   *
> > >   * "DEGAMMA_LUT”:
> > >   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> > > @@ -85,6 +90,28 @@
> > >   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> > >   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> > >   with the
> > >   * "GAMMA_LUT" property above.
> > > 
> > > + *
> > > + * The &drm_plane object's properties are:
> > > + *
> > > + * "YCBCR_TO_RGB_MODE"
> > > + * 	Optional plane enum property to configure YCbCr to RGB
> > > + * 	conversion. The possible modes include a number of standard
> > > + * 	conversions and a possibility to select custom conversion
> > > + * 	matrix and preoffset vector. The driver should select the
> > > + *		supported subset of of the modes.
> > > + *
> > > + * "YCBCR_TO_RGB_CSC"
> > > + *	Optional plane property blob to set YCbCr to RGB conversion
> > > + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > + *	defined in uapi/drm/drm_mode.h. Whether this property is
> > > + *	active dependent on YCBCR_TO_RGB_MODE property.
> > > + *
> > > + * "YCBCR_TO_RGB_PREOFFSET"
> > > + *	Optional plane property blob to configure YCbCr offset before
> > > + *	YCbCr to RGB CSC is applied. The blob contains struct
> > > + *	drm_ycbcr_to_rgb_preoffset which is defined in
> > > + *	uapi/drm/drm_mode.h. Whether this property is active dependent
> > > + *	on YCBCR_TO_RGB_MODE property.
> > >   */
> > >  
> > >  /**
> > > @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > >  	drm_modeset_unlock_all(dev);
> > >  	return ret;
> > >  }
> > > +
> > > +static char *ycbcr_to_rgb_mode_name[] = {
> > > +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> > > +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> > > +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> > > +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> > > +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> > > +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> > > +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> > > +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> > > +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> > > +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> > 
> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> > data and the colorspace conversion on linear data. So we need a degamma
> > stage in between. At least that seemed to be the general concencus after
> > the last round of mails on this topic.
> > 
> > After staring at the v4l docs on this stuff I kinda like their
> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> > little partial to calling the prop something like YCBCR_ENCODING.
> 
> For quite obvious reasons I agree with this partial reasoning :-) I would also 
> copy how V4L2 splits color space information into a transfer function, an 
> encoding and a quantization. If you group all three in a single enum you will 
> end up with lots of possible combinations.
> 
> > OTOH if we want to expose the raw matrix as a blob then maybe calling it a
> > CSC might be better. Not sure there's much point in exposing it though.
> > I don't think most people are in the habit if cooking up new ways to
> > encode their pixel data.
> 
> I believe it would be more about supporting weird color encodings that are in 
> the wild out there in various media sources. I'm not an expert in the field of 
> color spaces though.

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

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

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

> 
> > > +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
> > > +		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
> > > +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
> > > +		"YCbCr BT.709 limited range TO RGB BT.601 full range",
> > > +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
> > > +		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
> > > +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
> > > +		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
> > > +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
> > > +		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
> > > +	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
> > > +		"YCbCr TO RGB CSC limited range preoffset",
> > > +	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
> > > +		"YCbCr TO RGB CSC full range preoffset",
> > > +	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
> > > +		"YCBCR TO RGB CSC preoffset vector",
> > > +};
> > > +
> > > +/**
> > > + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related
> > > properties
> > > + * @enum_list: drm_prop_enum_list array of supported modes without names
> > > + * @enum_list_len: length of enum_list array
> > > + * @default_mode: default csc mode
> > > + *
> > > + * Create and attach plane specific YCbCr to RGB conversion related
> > > + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
> > > + * created and an the supported modes should be provided the enum_list
> > > + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
> > > + * created based on supported conversion modes. The enum_list parameter
> > > + * should not contain the enum names, because the standard names are
> > > + * added by this function.
> > > + */
> > > +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> > > +			struct drm_prop_enum_list *enum_list,
> > > +			unsigned int enum_list_len,
> > > +			enum drm_plane_ycbcr_to_rgb_mode default_mode)
> > > +{
> > > +	struct drm_device *dev = plane->dev;
> > > +	struct drm_property *prop;
> > > +	bool ycbcr_to_rgb_csc_create = false;
> > > +	bool ycbcr_to_rgb_preoffset_create = false;
> > > +	int i;
> 
> i takes positive values only, you can make it an unsigned int.
> 
> > > +
> > > +	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
> > > +		return -EEXIST;
> > > +
> > > +	for (i = 0; i < enum_list_len; i++) {
> > > +		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
> > > +
> > > +		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
> > > +
> > > +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
> > > +		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
> > > +			ycbcr_to_rgb_csc_create = true;
> > > +
> > > +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
> > > +			ycbcr_to_rgb_csc_create = true;
> > > +			ycbcr_to_rgb_preoffset_create = true;
> > > +		}
> > > +	}
> > > +
> > > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > > +					"YCBCR_TO_RGB_MODE",
> > > +					enum_list, enum_list_len);
> > > +	if (!prop)
> > > +		return -ENOMEM;
> > > +	plane->ycbcr_to_rgb_mode_property = prop;
> > > +	drm_object_attach_property(&plane->base, prop, default_mode);
> > > +
> > > +	if (ycbcr_to_rgb_csc_create) {
> > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > +					   DRM_MODE_PROP_BLOB,
> > > +					   "YCBCR_TO_RGB_CSC", 0);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		plane->ycbcr_to_rgb_csc_property = prop;
> > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > +	}
> > > +
> > > +	if (ycbcr_to_rgb_preoffset_create) {
> > > +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > +					   DRM_MODE_PROP_BLOB,
> > > +					   "YCBCR_TO_RGB_PREOFFSET", 0);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +		plane->ycbcr_to_rgb_preoffset_property = prop;
> > > +		drm_object_attach_property(&plane->base, prop, 0);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> 
> [snip]
> 
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index 03a59cb..a20b3ff 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> 
> [snip]
> 
> > > @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > >  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> > >  				 int gamma_size);
> > > 
> > > +enum drm_plane_ycbcr_to_rgb_mode {
> > > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
> > > +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
> > > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
> > > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
> > > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
> > > +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
> > > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
> > > +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
> > > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
> > > +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
> > > +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
> > > +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
> > > +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
> 
> All those values should be documented.
> 
> Should the CSC coefficient tables be centralized in core code ? Patch 5/6 adds 
> tables to the omapdrm driver, for standard conversions it would make sense to 
> share that data.
> 
> > > +};
> > > +
> > > +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> > > +			struct drm_prop_enum_list *enum_list,
> > > +			unsigned int enum_list_len,
> > > +			enum drm_plane_ycbcr_to_rgb_mode default_mode);
> > > +
> > >  #endif
> 
> [snip]
> 
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 8c67fc0..27e0bee 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -543,6 +543,18 @@ struct drm_color_lut {
> > >  	__u16 reserved;
> > >  };
> > > 
> > > +struct drm_ycbcr_to_rgb_csc {
> > > +	/* Conversion matrix in 2-complement s32.32 format. */
> > > +	__s64 ry, rcb, rcr;
> > > +	__s64 gy, gcb, gcr;
> > > +	__s64 by, bcb, bcr;
> > > +};
> > > +
> > > +struct drm_ycbcr_to_rgb_preoffset {
> > > +	/* Offset vector in 2-complement s.32 format. */
> > > +	__s32 y, cb, cr;
> > > +};
> > > +
> > > 
> > >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Sharma, Shashank]

Regards

Shashank


On 4/21/2017 4:47 PM, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
>> Add standard properties to control YCbCr to RGB conversion in DRM
>> planes. The created properties are stored to drm_plane object to allow
>> different set of supported conversion modes for different planes on
>> the device. For simplicity the related property blobs for CSC matrix
>> and YCbCr preoffsets are also stored in the same place. The blob
>> contents are defined in the uapi/drm/drm_mode.h header.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c        |  26 +++++++
>>   drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>>   drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/drm_plane.c         |  10 +++
>>   include/drm/drm_color_mgmt.h        |  23 ++++++
>>   include/drm/drm_plane.h             |  10 +++
>>   include/uapi/drm/drm_mode.h         |  12 ++++
>>   7 files changed, 223 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f881319..d1512aa 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   {
>>   	struct drm_device *dev = plane->dev;
>>   	struct drm_mode_config *config = &dev->mode_config;
>> +	int ret;
>> +	bool dummy;
>>   
>>   	if (property == config->prop_fb_id) {
>>   		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   		state->rotation = val;
>>   	} else if (property == plane->zpos_property) {
>>   		state->zpos = val;
>> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +		state->ycbcr_to_rgb_mode = val;
>> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->ycbcr_to_rgb_csc,
>> +				val,
>> +				sizeof(struct drm_ycbcr_to_rgb_csc),
>> +				&dummy);
>> +		return ret;
>> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->ycbcr_to_rgb_preoffset,
>> +				val,
>> +				sizeof(struct drm_ycbcr_to_rgb_preoffset),
>> +				&dummy);
>> +		return ret;
>>   	} else if (plane->funcs->atomic_set_property) {
>>   		return plane->funcs->atomic_set_property(plane, state,
>>   				property, val);
>> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   		*val = state->rotation;
>>   	} else if (property == plane->zpos_property) {
>>   		*val = state->zpos;
>> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +		*val = state->ycbcr_to_rgb_mode;
>> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +		*val = state->ycbcr_to_rgb_csc ?
>> +			state->ycbcr_to_rgb_csc->base.id : 0;
>> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +		*val = state->ycbcr_to_rgb_preoffset ?
>> +			state->ycbcr_to_rgb_preoffset->base.id : 0;
>>   	} else if (plane->funcs->atomic_get_property) {
>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>   	} else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c3994b4..89fd826 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>   {
>>   	memcpy(state, plane->state, sizeof(*state));
>>   
>> +	if (state->ycbcr_to_rgb_csc)
>> +		drm_property_blob_get(state->ycbcr_to_rgb_csc);
>> +
>> +	if (state->ycbcr_to_rgb_preoffset)
>> +		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
>> +
>>   	if (state->fb)
>>   		drm_framebuffer_get(state->fb);
>>   
>> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>>    */
>>   void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>   {
>> +	drm_property_blob_put(state->ycbcr_to_rgb_csc);
>> +	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
>> +
>>   	if (state->fb)
>>   		drm_framebuffer_put(state->fb);
>>   
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index cc23b9a..badaddd 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -29,9 +29,14 @@
>>   /**
>>    * DOC: overview
>>    *
>> - * Color management or color space adjustments is supported through a set of 5
>> - * properties on the &drm_crtc object. They are set up by calling
>> - * drm_crtc_enable_color_mgmt().
>> + * Color management or color space adjustments in CRTCs is supported
>> + * through a set of 5 properties on the &drm_crtc object. They are set
>> + * up by calling drm_crtc_enable_color_mgmt().
>> + *
>> + * Color space conversions from YCbCr to RGB color space in planes is
>> + * supporter trough 3 optional properties in &drm_plane object.
>> + *
>> + * The &drm_crtc object's properties are:
>>    *
>>    * "DEGAMMA_LUT”:
>>    *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>> @@ -85,6 +90,28 @@
>>    * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>    * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>>    * "GAMMA_LUT" property above.
>> + *
>> + * The &drm_plane object's properties are:
>> + *
>> + * "YCBCR_TO_RGB_MODE"
>> + * 	Optional plane enum property to configure YCbCr to RGB
>> + * 	conversion. The possible modes include a number of standard
>> + * 	conversions and a possibility to select custom conversion
>> + * 	matrix and preoffset vector. The driver should select the
>> + *	supported subset of of the modes.
>> + *
>> + * "YCBCR_TO_RGB_CSC"
>> + *	Optional plane property blob to set YCbCr to RGB conversion
>> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
>> + *	defined in uapi/drm/drm_mode.h. Whether this property is
>> + *	active dependent on YCBCR_TO_RGB_MODE property.
>> + *
>> + * "YCBCR_TO_RGB_PREOFFSET"
>> + *	Optional plane property blob to configure YCbCr offset before
>> + *	YCbCr to RGB CSC is applied. The blob contains struct
>> + *	drm_ycbcr_to_rgb_preoffset which is defined in
>> + *	uapi/drm/drm_mode.h. Whether this property is active dependent
>> + *	on YCBCR_TO_RGB_MODE property.
>>    */
>>   
>>   /**
>> @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>   	drm_modeset_unlock_all(dev);
>>   	return ret;
>>   }
>> +
>> +static char *ycbcr_to_rgb_mode_name[] = {
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
>
> After staring at the v4l docs on this stuff I kinda like their
> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> little partial to calling the prop something like YCBCR_ENCODING. OTOH
> if we want to expose the raw matrix as a blob then maybe calling it a
> CSC might be better. Not sure there's much point in exposing it though.
> I don't think most people are in the habit if cooking up new ways to
> encode their pixel data.
Ville has already conveyed the zest of this design. When we want to 
blend the data targeting
various framebuffers, we have to consider their color spaces too. So we 
should actually come up
with a set of properties, which in a combination (and in sequence) 
provide the capability to blend
a Rec 709 and Rec 2020 buffer.

I was supposed to publish a design RFC doc for the same, but it got 
delayed from my side.
Will publish that in parallel, to see if we can merge these two thoughts.

- Shashank
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
>> +		"YCbCr TO RGB CSC limited range preoffset",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
>> +		"YCbCr TO RGB CSC full range preoffset",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
>> +		"YCBCR TO RGB CSC preoffset vector",
>> +};
>> +
>> +/**
>> + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
>> + * @enum_list: drm_prop_enum_list array of supported modes without names
>> + * @enum_list_len: length of enum_list array
>> + * @default_mode: default csc mode
>> + *
>> + * Create and attach plane specific YCbCr to RGB conversion related
>> + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
>> + * created and an the supported modes should be provided the enum_list
>> + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
>> + * created based on supported conversion modes. The enum_list parameter
>> + * should not contain the enum names, because the standard names are
>> + * added by this function.
>> + */
>> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
>> +			struct drm_prop_enum_list *enum_list,
>> +			unsigned int enum_list_len,
>> +			enum drm_plane_ycbcr_to_rgb_mode default_mode)
>> +{
>> +	struct drm_device *dev = plane->dev;
>> +	struct drm_property *prop;
>> +	bool ycbcr_to_rgb_csc_create = false;
>> +	bool ycbcr_to_rgb_preoffset_create = false;
>> +	int i;
>> +
>> +	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
>> +		return -EEXIST;
>> +
>> +	for (i = 0; i < enum_list_len; i++) {
>> +		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
>> +
>> +		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
>> +
>> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
>> +		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
>> +			ycbcr_to_rgb_csc_create = true;
>> +
>> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
>> +			ycbcr_to_rgb_csc_create = true;
>> +			ycbcr_to_rgb_preoffset_create = true;
>> +		}
>> +	}
>> +
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +					"YCBCR_TO_RGB_MODE",
>> +					enum_list, enum_list_len);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	plane->ycbcr_to_rgb_mode_property = prop;
>> +	drm_object_attach_property(&plane->base, prop, default_mode);
>> +
>> +	if (ycbcr_to_rgb_csc_create) {
>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>> +					   DRM_MODE_PROP_BLOB,
>> +					   "YCBCR_TO_RGB_CSC", 0);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +		plane->ycbcr_to_rgb_csc_property = prop;
>> +		drm_object_attach_property(&plane->base, prop, 0);
>> +	}
>> +
>> +	if (ycbcr_to_rgb_preoffset_create) {
>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>> +					   DRM_MODE_PROP_BLOB,
>> +					   "YCBCR_TO_RGB_PREOFFSET", 0);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +		plane->ycbcr_to_rgb_preoffset_property = prop;
>> +		drm_object_attach_property(&plane->base, prop, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index bc71aa2..4c7e827 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
>>   
>>   	kfree(plane->name);
>>   
>> +	if (plane->ycbcr_to_rgb_mode_property)
>> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
>> +
>> +	if (plane->ycbcr_to_rgb_csc_property)
>> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
>> +
>> +	if (plane->ycbcr_to_rgb_preoffset_property)
>> +		drm_property_destroy(dev,
>> +				     plane->ycbcr_to_rgb_preoffset_property);
>> +
>>   	memset(plane, 0, sizeof(*plane));
>>   }
>>   EXPORT_SYMBOL(drm_plane_cleanup);
>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>> index 03a59cb..a20b3ff 100644
>> --- a/include/drm/drm_color_mgmt.h
>> +++ b/include/drm/drm_color_mgmt.h
>> @@ -26,6 +26,8 @@
>>   #include <linux/ctype.h>
>>   
>>   struct drm_crtc;
>> +struct drm_plane;
>> +struct drm_prop_enum_list;
>>   
>>   uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>   
>> @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>   int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>>   				 int gamma_size);
>>   
>> +enum drm_plane_ycbcr_to_rgb_mode {
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
>> +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
>> +};
>> +
>> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
>> +			struct drm_prop_enum_list *enum_list,
>> +			unsigned int enum_list_len,
>> +			enum drm_plane_ycbcr_to_rgb_mode default_mode);
>> +
>>   #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 9ab3e70..41dcde2 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -26,6 +26,7 @@
>>   #include <linux/list.h>
>>   #include <linux/ctype.h>
>>   #include <drm/drm_mode_object.h>
>> +#include <drm/drm_color_mgmt.h>
>>   
>>   struct drm_crtc;
>>   struct drm_printer;
>> @@ -112,6 +113,11 @@ struct drm_plane_state {
>>   	unsigned int zpos;
>>   	unsigned int normalized_zpos;
>>   
>> +	/* YCbCr to RGB conversion */
>> +	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
>> +	struct drm_property_blob *ycbcr_to_rgb_csc;
>> +	struct drm_property_blob *ycbcr_to_rgb_preoffset;
>> +
>>   	/* Clipped coordinates */
>>   	struct drm_rect src, dst;
>>   
>> @@ -523,6 +529,10 @@ struct drm_plane {
>>   
>>   	struct drm_property *zpos_property;
>>   	struct drm_property *rotation_property;
>> +
>> +	struct drm_property *ycbcr_to_rgb_mode_property;
>> +	struct drm_property *ycbcr_to_rgb_csc_property;
>> +	struct drm_property *ycbcr_to_rgb_preoffset_property;
>>   };
>>   
>>   #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 8c67fc0..27e0bee 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -543,6 +543,18 @@ struct drm_color_lut {
>>   	__u16 reserved;
>>   };
>>   
>> +struct drm_ycbcr_to_rgb_csc {
>> +	/* Conversion matrix in 2-complement s32.32 format. */
>> +	__s64 ry, rcb, rcr;
>> +	__s64 gy, gcb, gcr;
>> +	__s64 by, bcb, bcr;
>> +};
>> +
>> +struct drm_ycbcr_to_rgb_preoffset {
>> +	/* Offset vector in 2-complement s.32 format. */
>> +	__s32 y, cb, cr;
>> +};
>> +
>>   #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>>   #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>>   #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>> -- 
>> 1.9.1

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/21/17 14:17, Ville Syrjälä wrote:
>> +static char *ycbcr_to_rgb_mode_name[] = {
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
> 

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

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

I guess this property should be called YCBCR_DECODING.

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

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

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

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

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> > data and the colorspace conversion on linear data. So we need a degamma
> > stage in between. At least that seemed to be the general concencus after
> > the last round of mails on this topic.
> > 
> 
> I do not really have the expertise to argue with that. I merely copied
> the idea from the mail thread I referred to in the cover letter.
> However, there are several display HWs out there that do not have all
> bolts and knobs to make the color-space conversion in exactly the ideal
> order, omap DSS being one of them.

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

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

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

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

Only if you think of it as a verb.

> 
> > if we want to expose the raw matrix as a blob then maybe calling it a
> > CSC might be better. Not sure there's much point in exposing it though.
> 
> In my first version it was called just CSC, but then I wanted to make it
> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> of HW that can do only one or the other, e.g. the offset calculations
> are supported only to one direction.

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

> 
> > I don't think most people are in the habit if cooking up new ways to
> > encode their pixel data.
> > 
> 
> In the embedded side I can imagine there could be some custom appliances
> where one may want to do some custom thing with the CSC and not needing
> a custom kernel for that could make a life easier... but then again I am
> not really an expert in this area.

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

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

Hi,

Thanks for picking this up Jyri.

On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
>> > conversion because the YCbCr->RGB part should be performed on gamma encoded
>> > data and the colorspace conversion on linear data. So we need a degamma
>> > stage in between. At least that seemed to be the general concencus after
>> > the last round of mails on this topic.
>> >

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

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

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

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

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

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

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

>>
>> I do not really have the expertise to argue with that. I merely copied
>> the idea from the mail thread I referred to in the cover letter.
>> However, there are several display HWs out there that do not have all
>> bolts and knobs to make the color-space conversion in exactly the ideal
>> order, omap DSS being one of them.
>
>Yeah. Intel hardware is in the same boat for the time being. On current
>hw I think we can only really expose the YCbCr->RGB and degamma stages.
>
>On some limited set of platforms we could expose a blob as well, and I
>suppose it would then be possible to use it for color space conversion
>if you ignore gamma and/or only deal with linear RGB data. But it's such
>a limited subset of hardware for us that I don't think I'm interested
>in exposing it.
>

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

Thanks,
-Brian

>In the future we should be getting a more fully fleged pipeline.
>
>>
>> > After staring at the v4l docs on this stuff I kinda like their
>> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
>>
>> I guess this property should be called YCBCR_DECODING.
>
>Only if you think of it as a verb.
>
>>
>> > if we want to expose the raw matrix as a blob then maybe calling it a
>> > CSC might be better. Not sure there's much point in exposing it though.
>>
>> In my first version it was called just CSC, but then I wanted to make it
>> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> of HW that can do only one or the other, e.g. the offset calculations
>> are supported only to one direction.
>
>Are you planning to do RGB->YCbCr conversion in the plane as well? I
>think we'll be only doing that at crtc/connector level.
>
>>
>> > I don't think most people are in the habit if cooking up new ways to
>> > encode their pixel data.
>> >
>>
>> In the embedded side I can imagine there could be some custom appliances
>> where one may want to do some custom thing with the CSC and not needing
>> a custom kernel for that could make a life easier... but then again I am
>> not really an expert in this area.
>
>I would assume most customy things you'd do in the crtc (eg. color
>correction and whatnot). But could be that I just lack imagination.
>
>-- 
>Ville Syrjälä
>Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
> Hi,
> 
> Thanks for picking this up Jyri.
> 
> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> >> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> >> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> >> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> >> > data and the colorspace conversion on linear data. So we need a degamma
> >> > stage in between. At least that seemed to be the general concencus after
> >> > the last round of mails on this topic.
> >> >
> 
> I thought the conclusion of the last round was that on some hardware
> this would be conflated as a conscious choice. If the HW doesn't have
> the required degamma->csc->gamma stages, and the implementor/user
> doesn't care about being a little incorrect, then it can all be
> described in this single property.

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

> 
> If there is more capable hardware with the additional stages, then
> they should expose additional properties (in pipeline order):
> 
> YCBCR_TO_RGB_*:
>    Does YCbCr->RGB conversion on non-linear YCbCr data,
>    only the enum values which don't have a CSC are exposed.

Not sure what you mean but that last part.

> 
> DEGAMMA:
>    Does the non-linear to linear conversion on the RGB data output from
>    the YCBCR_TO_RGB stage.
> 
> RGB_TO_RGB_*:
>    Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
>    on the linear data.

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

> 
> GAMMA:
>    Convert the CSC'd RGB data back in to non-linear for blending (if
>    blending is to be done with non-linear data).
> 
> Drivers can expose as many or as few of the above properties as their
> hardware supports.
> 
> >>
> >> I do not really have the expertise to argue with that. I merely copied
> >> the idea from the mail thread I referred to in the cover letter.
> >> However, there are several display HWs out there that do not have all
> >> bolts and knobs to make the color-space conversion in exactly the ideal
> >> order, omap DSS being one of them.
> >
> >Yeah. Intel hardware is in the same boat for the time being. On current
> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
> >
> >On some limited set of platforms we could expose a blob as well, and I
> >suppose it would then be possible to use it for color space conversion
> >if you ignore gamma and/or only deal with linear RGB data. But it's such
> >a limited subset of hardware for us that I don't think I'm interested
> >in exposing it.
> >
> 
> I'm not sure the custom blob is worth having either. It can easily be
> added later if we decide we want it after all.
> 
> Thanks,
> -Brian
> 
> >In the future we should be getting a more fully fleged pipeline.
> >
> >>
> >> > After staring at the v4l docs on this stuff I kinda like their
> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
> >>
> >> I guess this property should be called YCBCR_DECODING.
> >
> >Only if you think of it as a verb.
> >
> >>
> >> > if we want to expose the raw matrix as a blob then maybe calling it a
> >> > CSC might be better. Not sure there's much point in exposing it though.
> >>
> >> In my first version it was called just CSC, but then I wanted to make it
> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> >> of HW that can do only one or the other, e.g. the offset calculations
> >> are supported only to one direction.
> >
> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
> >think we'll be only doing that at crtc/connector level.
> >
> >>
> >> > I don't think most people are in the habit if cooking up new ways to
> >> > encode their pixel data.
> >> >
> >>
> >> In the embedded side I can imagine there could be some custom appliances
> >> where one may want to do some custom thing with the CSC and not needing
> >> a custom kernel for that could make a life easier... but then again I am
> >> not really an expert in this area.
> >
> >I would assume most customy things you'd do in the crtc (eg. color
> >correction and whatnot). But could be that I just lack imagination.
> >
> >-- 
> >Ville Syrjälä
> >Intel OTC

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
>> Hi,
>>
>> Thanks for picking this up Jyri.
>>
>> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> >> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> >> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
>> >> > conversion because the YCbCr->RGB part should be performed on gamma encoded
>> >> > data and the colorspace conversion on linear data. So we need a degamma
>> >> > stage in between. At least that seemed to be the general concencus after
>> >> > the last round of mails on this topic.
>> >> >
>>
>> I thought the conclusion of the last round was that on some hardware
>> this would be conflated as a conscious choice. If the HW doesn't have
>> the required degamma->csc->gamma stages, and the implementor/user
>> doesn't care about being a little incorrect, then it can all be
>> described in this single property.
>
>I was proposing the single prop approach initially, but now I think it
>might just lead to more confusion. So a dedicated property for each stage
>is the clearer design I think. We do lose potentially a bit of
>discoverability when not all combinations are supported, but we have
>that problem in many other places as well, so not a big deal I think.
>

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

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

>>
>> If there is more capable hardware with the additional stages, then
>> they should expose additional properties (in pipeline order):
>>
>> YCBCR_TO_RGB_*:
>>    Does YCbCr->RGB conversion on non-linear YCbCr data,
>>    only the enum values which don't have a CSC are exposed.
>
>Not sure what you mean but that last part.
>

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

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

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

>>
>> DEGAMMA:
>>    Does the non-linear to linear conversion on the RGB data output from
>>    the YCBCR_TO_RGB stage.
>>
>> RGB_TO_RGB_*:
>>    Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
>>    on the linear data.
>
>We may want to call this just CTM to match the matching crtc prop.
>

Good call.

-Brian

>>
>> GAMMA:
>>    Convert the CSC'd RGB data back in to non-linear for blending (if
>>    blending is to be done with non-linear data).
>>
>> Drivers can expose as many or as few of the above properties as their
>> hardware supports.
>>
>> >>
>> >> I do not really have the expertise to argue with that. I merely copied
>> >> the idea from the mail thread I referred to in the cover letter.
>> >> However, there are several display HWs out there that do not have all
>> >> bolts and knobs to make the color-space conversion in exactly the ideal
>> >> order, omap DSS being one of them.
>> >
>> >Yeah. Intel hardware is in the same boat for the time being. On current
>> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
>> >
>> >On some limited set of platforms we could expose a blob as well, and I
>> >suppose it would then be possible to use it for color space conversion
>> >if you ignore gamma and/or only deal with linear RGB data. But it's such
>> >a limited subset of hardware for us that I don't think I'm interested
>> >in exposing it.
>> >
>>
>> I'm not sure the custom blob is worth having either. It can easily be
>> added later if we decide we want it after all.
>>
>> Thanks,
>> -Brian
>>
>> >In the future we should be getting a more fully fleged pipeline.
>> >
>> >>
>> >> > After staring at the v4l docs on this stuff I kinda like their
>> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
>> >>
>> >> I guess this property should be called YCBCR_DECODING.
>> >
>> >Only if you think of it as a verb.
>> >
>> >>
>> >> > if we want to expose the raw matrix as a blob then maybe calling it a
>> >> > CSC might be better. Not sure there's much point in exposing it though.
>> >>
>> >> In my first version it was called just CSC, but then I wanted to make it
>> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> >> of HW that can do only one or the other, e.g. the offset calculations
>> >> are supported only to one direction.
>> >
>> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
>> >think we'll be only doing that at crtc/connector level.
>> >
>> >>
>> >> > I don't think most people are in the habit if cooking up new ways to
>> >> > encode their pixel data.
>> >> >
>> >>
>> >> In the embedded side I can imagine there could be some custom appliances
>> >> where one may want to do some custom thing with the CSC and not needing
>> >> a custom kernel for that could make a life easier... but then again I am
>> >> not really an expert in this area.
>> >
>> >I would assume most customy things you'd do in the crtc (eg. color
>> >correction and whatnot). But could be that I just lack imagination.
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>-- 
>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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote:
> On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
> >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
> >> Hi,
> >>
> >> Thanks for picking this up Jyri.
> >>
> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
> >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> >> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> >> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> >> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> >> >> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> >> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> >> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> >> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> >> >> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> >> >> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> >> >> > data and the colorspace conversion on linear data. So we need a degamma
> >> >> > stage in between. At least that seemed to be the general concencus after
> >> >> > the last round of mails on this topic.
> >> >> >
> >>
> >> I thought the conclusion of the last round was that on some hardware
> >> this would be conflated as a conscious choice. If the HW doesn't have
> >> the required degamma->csc->gamma stages, and the implementor/user
> >> doesn't care about being a little incorrect, then it can all be
> >> described in this single property.
> >
> >I was proposing the single prop approach initially, but now I think it
> >might just lead to more confusion. So a dedicated property for each stage
> >is the clearer design I think. We do lose potentially a bit of
> >discoverability when not all combinations are supported, but we have
> >that problem in many other places as well, so not a big deal I think.
> >
> 
> Yeah you lose discoverability, but do you also lose the ability to do
> the non-perfect, single-stage conversions?

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

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

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

> 
> >>
> >> If there is more capable hardware with the additional stages, then
> >> they should expose additional properties (in pipeline order):
> >>
> >> YCBCR_TO_RGB_*:
> >>    Does YCbCr->RGB conversion on non-linear YCbCr data,
> >>    only the enum values which don't have a CSC are exposed.
> >
> >Not sure what you mean but that last part.
> >
> 
> Just that the enum list should only contain things that are:
> YCbCr BT.601  -> RGB BT.601
> YCbCr BT.709  -> RGB BT.709
> YcbCr BT.2020 -> RGB BT.2020
> 
> and not a those including a color space change, e.g.:
> YCbCr BT.601  -> RGB BT.709

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

> 
> because an RGB BT.601 -> RGB BT.709 conversion can be performed
> later with the RGB_TO_RGB/CTM/whatever property.
> 
> >>
> >> DEGAMMA:
> >>    Does the non-linear to linear conversion on the RGB data output from
> >>    the YCBCR_TO_RGB stage.
> >>
> >> RGB_TO_RGB_*:
> >>    Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
> >>    on the linear data.
> >
> >We may want to call this just CTM to match the matching crtc prop.
> >
> 
> Good call.
> 
> -Brian
> 
> >>
> >> GAMMA:
> >>    Convert the CSC'd RGB data back in to non-linear for blending (if
> >>    blending is to be done with non-linear data).
> >>
> >> Drivers can expose as many or as few of the above properties as their
> >> hardware supports.
> >>
> >> >>
> >> >> I do not really have the expertise to argue with that. I merely copied
> >> >> the idea from the mail thread I referred to in the cover letter.
> >> >> However, there are several display HWs out there that do not have all
> >> >> bolts and knobs to make the color-space conversion in exactly the ideal
> >> >> order, omap DSS being one of them.
> >> >
> >> >Yeah. Intel hardware is in the same boat for the time being. On current
> >> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
> >> >
> >> >On some limited set of platforms we could expose a blob as well, and I
> >> >suppose it would then be possible to use it for color space conversion
> >> >if you ignore gamma and/or only deal with linear RGB data. But it's such
> >> >a limited subset of hardware for us that I don't think I'm interested
> >> >in exposing it.
> >> >
> >>
> >> I'm not sure the custom blob is worth having either. It can easily be
> >> added later if we decide we want it after all.
> >>
> >> Thanks,
> >> -Brian
> >>
> >> >In the future we should be getting a more fully fleged pipeline.
> >> >
> >> >>
> >> >> > After staring at the v4l docs on this stuff I kinda like their
> >> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> >> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
> >> >>
> >> >> I guess this property should be called YCBCR_DECODING.
> >> >
> >> >Only if you think of it as a verb.
> >> >
> >> >>
> >> >> > if we want to expose the raw matrix as a blob then maybe calling it a
> >> >> > CSC might be better. Not sure there's much point in exposing it though.
> >> >>
> >> >> In my first version it was called just CSC, but then I wanted to make it
> >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> >> >> of HW that can do only one or the other, e.g. the offset calculations
> >> >> are supported only to one direction.
> >> >
> >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
> >> >think we'll be only doing that at crtc/connector level.
> >> >
> >> >>
> >> >> > I don't think most people are in the habit if cooking up new ways to
> >> >> > encode their pixel data.
> >> >> >
> >> >>
> >> >> In the embedded side I can imagine there could be some custom appliances
> >> >> where one may want to do some custom thing with the CSC and not needing
> >> >> a custom kernel for that could make a life easier... but then again I am
> >> >> not really an expert in this area.
> >> >
> >> >I would assume most customy things you'd do in the crtc (eg. color
> >> >correction and whatnot). But could be that I just lack imagination.
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel OTC
> >
> >-- 
> >Ville Syrjälä
> >Intel OTC

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

On Fri, Apr 21, 2017 at 07:49:04PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote:
>> On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
>> >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
>> >> Hi,
>> >>
>> >> Thanks for picking this up Jyri.
>> >>
>> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>> >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> >> >> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> >> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> >> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> >> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> >> >> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
>> >> >> > conversion because the YCbCr->RGB part should be performed on gamma encoded
>> >> >> > data and the colorspace conversion on linear data. So we need a degamma
>> >> >> > stage in between. At least that seemed to be the general concencus after
>> >> >> > the last round of mails on this topic.
>> >> >> >
>> >>
>> >> I thought the conclusion of the last round was that on some hardware
>> >> this would be conflated as a conscious choice. If the HW doesn't have
>> >> the required degamma->csc->gamma stages, and the implementor/user
>> >> doesn't care about being a little incorrect, then it can all be
>> >> described in this single property.
>> >
>> >I was proposing the single prop approach initially, but now I think it
>> >might just lead to more confusion. So a dedicated property for each stage
>> >is the clearer design I think. We do lose potentially a bit of
>> >discoverability when not all combinations are supported, but we have
>> >that problem in many other places as well, so not a big deal I think.
>> >
>>
>> Yeah you lose discoverability, but do you also lose the ability to do
>> the non-perfect, single-stage conversions?
>
>Not sure why one vs. multiple props should matter here. Either you
>accept the less than perfect pipeline or you don't. Whether you ask it
>via one of multiple props doesn't seem all that different to me.
>
>>
>> For HW that only has a matrix, is the driver expected to combine all
>> of the separated stages down into a single matrix? Or it wouldn't
>> expose the other properties, only a matrix, and userspace has to come
>> up with a blob that does the (approximate) right thing?
>
>If you're not happy with exposing just one or the other, then I guess
>you would expose both but indicate that there's no degamma in between
>and userspace can then choose whether it's happy with that solution or
>or not.
>

I might understand/explain better if we take a concrete example.

Let's assume I have a hardware pipeline with the following bits:

Input -> 3x3 + 3 matrix -> degamma LUT -> CRTC

My input is non-linear YCbCr BT.601 full-range, and I want linear RGB
BT.709, full-range to reach the CRTC.


Which properties would my driver expose, and which values should be
set on them?

My assumption /was/ that:

3x3 + 3 matrix:
    Exposed as
    YCBCR_TO_RGB_MODE = "YCbCr BT.601 full-range to RGB BT.709 full-range"

degamma LUT:
    Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
    curve


Are you suggesting instead that:

3x3 + 3 matrix:
    Exposed as
    YCBCR_TO_RGB_MODE = "YCbCr TO RGB CSC full range preoffset"
    with YCBCR_TO_RGB_CSC = <userspace-derived-matrix>

degamma LUT:
    Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
    curve

Thanks,
Brian

>>
>> >>
>> >> If there is more capable hardware with the additional stages, then
>> >> they should expose additional properties (in pipeline order):
>> >>
>> >> YCBCR_TO_RGB_*:
>> >>    Does YCbCr->RGB conversion on non-linear YCbCr data,
>> >>    only the enum values which don't have a CSC are exposed.
>> >
>> >Not sure what you mean but that last part.
>> >
>>
>> Just that the enum list should only contain things that are:
>> YCbCr BT.601  -> RGB BT.601
>> YCbCr BT.709  -> RGB BT.709
>> YcbCr BT.2020 -> RGB BT.2020
>>
>> and not a those including a color space change, e.g.:
>> YCbCr BT.601  -> RGB BT.709
>
>Right. We probably shouldn't even have the "BT.whatever" on both sides
>since it's not really a colorspace, just the encoding, and once you
>decoded the YCbCr into RGB it's just RGB. We could actually just define
>the enum values as "BT.601","BT.709" etc.
>
>>
>> because an RGB BT.601 -> RGB BT.709 conversion can be performed
>> later with the RGB_TO_RGB/CTM/whatever property.
>>
>> >>
>> >> DEGAMMA:
>> >>    Does the non-linear to linear conversion on the RGB data output from
>> >>    the YCBCR_TO_RGB stage.
>> >>
>> >> RGB_TO_RGB_*:
>> >>    Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
>> >>    on the linear data.
>> >
>> >We may want to call this just CTM to match the matching crtc prop.
>> >
>>
>> Good call.
>>
>> -Brian
>>
>> >>
>> >> GAMMA:
>> >>    Convert the CSC'd RGB data back in to non-linear for blending (if
>> >>    blending is to be done with non-linear data).
>> >>
>> >> Drivers can expose as many or as few of the above properties as their
>> >> hardware supports.
>> >>
>> >> >>
>> >> >> I do not really have the expertise to argue with that. I merely copied
>> >> >> the idea from the mail thread I referred to in the cover letter.
>> >> >> However, there are several display HWs out there that do not have all
>> >> >> bolts and knobs to make the color-space conversion in exactly the ideal
>> >> >> order, omap DSS being one of them.
>> >> >
>> >> >Yeah. Intel hardware is in the same boat for the time being. On current
>> >> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
>> >> >
>> >> >On some limited set of platforms we could expose a blob as well, and I
>> >> >suppose it would then be possible to use it for color space conversion
>> >> >if you ignore gamma and/or only deal with linear RGB data. But it's such
>> >> >a limited subset of hardware for us that I don't think I'm interested
>> >> >in exposing it.
>> >> >
>> >>
>> >> I'm not sure the custom blob is worth having either. It can easily be
>> >> added later if we decide we want it after all.
>> >>
>> >> Thanks,
>> >> -Brian
>> >>
>> >> >In the future we should be getting a more fully fleged pipeline.
>> >> >
>> >> >>
>> >> >> > After staring at the v4l docs on this stuff I kinda like their
>> >> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>> >> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
>> >> >>
>> >> >> I guess this property should be called YCBCR_DECODING.
>> >> >
>> >> >Only if you think of it as a verb.
>> >> >
>> >> >>
>> >> >> > if we want to expose the raw matrix as a blob then maybe calling it a
>> >> >> > CSC might be better. Not sure there's much point in exposing it though.
>> >> >>
>> >> >> In my first version it was called just CSC, but then I wanted to make it
>> >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> >> >> of HW that can do only one or the other, e.g. the offset calculations
>> >> >> are supported only to one direction.
>> >> >
>> >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
>> >> >think we'll be only doing that at crtc/connector level.
>> >> >
>> >> >>
>> >> >> > I don't think most people are in the habit if cooking up new ways to
>> >> >> > encode their pixel data.
>> >> >> >
>> >> >>
>> >> >> In the embedded side I can imagine there could be some custom appliances
>> >> >> where one may want to do some custom thing with the CSC and not needing
>> >> >> a custom kernel for that could make a life easier... but then again I am
>> >> >> not really an expert in this area.
>> >> >
>> >> >I would assume most customy things you'd do in the crtc (eg. color
>> >> >correction and whatnot). But could be that I just lack imagination.
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel OTC
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>-- 
>Ville Syrjälä
>Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] RFC: Design: DRM: Blending pipeline using DRM plane properties

[Shashank Sharma]

This patch proposes a RFC design to handle blending of various
framebuffers with different color spaces, using the DRM color
properties.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/rfc-design-blending.txt | 52 +++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 drivers/gpu/drm/rfc-design-blending.txt

diff --git a/drivers/gpu/drm/rfc-design-blending.txt b/drivers/gpu/drm/rfc-design-blending.txt
new file mode 100644
index 0000000..55d96e9
--- /dev/null
+++ b/drivers/gpu/drm/rfc-design-blending.txt
@@ -0,0 +1,52 @@
+Hi all,
+
+I wanted to send this design in a separate thread, but then I realized we can use this thread itself as many of the
+stakeholders are already active here.
+
+As you all know, we will be dealing with the complex situations of blending of two(or more) different buffers
+in the pipeline, which are in different format and different color space.
+(PS: This ascii block design is best visible in a widescreen monitor, or in HTML page)
+
+=====================================================================================================================================================
+
+	   property 1 = CSC	       property 2 = Degamma	 property 3 = Gamut	     property 4 = palette
+          +------------------+        +-------------------+      +------------------+       +-------------------+  RGB REC 2020 buffer
+YUV       |color space       |        |Linearizion        |      |Gamut mapping     |       |Non-Linearizion    +-------------------+
+REC709--> |conversion        +------->+(Degamma)          +----->+(REC709->REC2020) +-------+(Gamma)            |                   |
+          |(YUV->RGB)        |        |                   |      |                  |       |                   |            +------v---------------+
+          +------------------+        +-------------------+      +------------------+       +-------------------+            |                      |
+                                                                                                                             |                      |
+                                                                                                                             | Blending unit        |
+                                                                                                                             |                      |------> blended output
+RGB REC 2020 buffer (Bypass everything)                                                                                      |                      |
+  +--------------------------------------------------------------------------------------------------------------------->    |                      |
+                                                                                                                             |                      |
+                                                                                                                             +----------------------+
+=====================================================================================================================================================
+
+This is a design proposal of a blending pipeline, using a sequence of plane level DRM properties.
+The description of the block is:
+- Input buffers are two different streams for example
+	- YCBCR buffer in REC709 colorspace
+	- RGB buffer in BT2020 colorspace
+- Aim is to make bending accurate by providing similar input to the blending unit (same in format as well as color space).
+- Every block here represents a plane level DRM property, with specific task to do.
+	- first block is CSC property, which is for conversion from YCBCR->RGB only (This doesnt do gamut mapping)
+	- second block is the property which will linearize the input, a degamma kind of property
+	- third block is a Gamut mapping proprty, which will do a gamut conversion (ex 709->2020)
+	- forth block is a Non-Linearizion block, which will apply back the curve (like Gamma) required
+	- The output of this pipeline is a RGB buffer in REC2020 color space
+	- Any driver can map its HW's plane level color correction capabilities to these properties.
+	- Once blending is done, driver can apply any post blending CRTC level color property, to:
+		- Change output type (ex. changing RGB->YUV using CRTC level CSC property)
+		- Apply a curve on the blended output (using CRTC level gamma/LUT property)
+
+- Important points:
+	- The sequence of the properties has to be fixed (almost in this order), considering the linear data requirement of Gamut mapping
+	- The color space of blending needs to be decided in the userspace, by UI manager, with some policies like (examples):
+		- If any of the buffer is REC2020, all buffers should be converted into 2020, before blending.
+		- Always blend in Higher/Wider color space.
+		- Always blend in RGB.
+
+- Opens:
+	- Is there a need to communicate HW's capabilities to UI manager/userspace ?
-- 
2.7.4

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/21/17 16:52, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> On 04/21/17 14:17, Ville Syrjälä wrote:
>>>> +static char *ycbcr_to_rgb_mode_name[] = {
>>>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>>>> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>>>> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>>>> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>>>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>>>> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>>>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>>>> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>>>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>>>> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>>> We probably don't want to conflate the YCbCr->RGB part with the colorspace
>>> conversion because the YCbCr->RGB part should be performed on gamma encoded
>>> data and the colorspace conversion on linear data. So we need a degamma
>>> stage in between. At least that seemed to be the general concencus after
>>> the last round of mails on this topic.
>>>
>>
>> I do not really have the expertise to argue with that. I merely copied
>> the idea from the mail thread I referred to in the cover letter.
>> However, there are several display HWs out there that do not have all
>> bolts and knobs to make the color-space conversion in exactly the ideal
>> order, omap DSS being one of them.
> 
> Yeah. Intel hardware is in the same boat for the time being. On current
> hw I think we can only really expose the YCbCr->RGB and degamma stages.
> 
> On some limited set of platforms we could expose a blob as well, and I
> suppose it would then be possible to use it for color space conversion
> if you ignore gamma and/or only deal with linear RGB data. But it's such
> a limited subset of hardware for us that I don't think I'm interested
> in exposing it.
> 
> In the future we should be getting a more fully fleged pipeline.
> 

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

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

>>
>>> After staring at the v4l docs on this stuff I kinda like their
>>> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>>> little partial to calling the prop something like YCBCR_ENCODING. OTOH
>>
>> I guess this property should be called YCBCR_DECODING.
> 
> Only if you think of it as a verb.
> 

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

>>
>>> if we want to expose the raw matrix as a blob then maybe calling it a
>>> CSC might be better. Not sure there's much point in exposing it though.
>>
>> In my first version it was called just CSC, but then I wanted to make it
>> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> of HW that can do only one or the other, e.g. the offset calculations
>> are supported only to one direction.
> 
> Are you planning to do RGB->YCbCr conversion in the plane as well? I
> think we'll be only doing that at crtc/connector level.
> 

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

>>
>>> I don't think most people are in the habit if cooking up new ways to
>>> encode their pixel data.
>>>
>>
>> In the embedded side I can imagine there could be some custom appliances
>> where one may want to do some custom thing with the CSC and not needing
>> a custom kernel for that could make a life easier... but then again I am
>> not really an expert in this area.
> 
> I would assume most customy things you'd do in the crtc (eg. color
> correction and whatnot). But could be that I just lack imagination.
> 

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

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

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 04:21:39PM +0300, Jyri Sarha wrote:
> On 04/21/17 16:52, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> On 04/21/17 14:17, Ville Syrjälä wrote:
> >>>> +static char *ycbcr_to_rgb_mode_name[] = {
> >>>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >>>> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> >>>> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >>>> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> >>>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >>>> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> >>>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >>>> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >>>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >>>> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> >>> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> >>> conversion because the YCbCr->RGB part should be performed on gamma encoded
> >>> data and the colorspace conversion on linear data. So we need a degamma
> >>> stage in between. At least that seemed to be the general concencus after
> >>> the last round of mails on this topic.
> >>>
> >>
> >> I do not really have the expertise to argue with that. I merely copied
> >> the idea from the mail thread I referred to in the cover letter.
> >> However, there are several display HWs out there that do not have all
> >> bolts and knobs to make the color-space conversion in exactly the ideal
> >> order, omap DSS being one of them.
> > 
> > Yeah. Intel hardware is in the same boat for the time being. On current
> > hw I think we can only really expose the YCbCr->RGB and degamma stages.
> > 
> > On some limited set of platforms we could expose a blob as well, and I
> > suppose it would then be possible to use it for color space conversion
> > if you ignore gamma and/or only deal with linear RGB data. But it's such
> > a limited subset of hardware for us that I don't think I'm interested
> > in exposing it.
> > 
> > In the future we should be getting a more fully fleged pipeline.
> > 
> 
> If going forward with these patches, then maybe it is better to stick
> with the enum options that we are sure of, e.g. BT.601, BT.709, and
> BT.2020 to full range RGB. It is easy enough to add more enum values in
> the future.
> 
> What is more important is the naming approach, whether we keep the
> conversion mode naming explicit about the output format, or just specify
> the output format implicitly to be always full range (non linear) RGB?
> 
> >>
> >>> After staring at the v4l docs on this stuff I kinda like their
> >>> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> >>> little partial to calling the prop something like YCBCR_ENCODING. OTOH
> >>
> >> I guess this property should be called YCBCR_DECODING.
> > 
> > Only if you think of it as a verb.
> > 
> 
> No strong opinions here. I am fine with any of the names that have been
> suggested so far.
> 
> >>
> >>> if we want to expose the raw matrix as a blob then maybe calling it a
> >>> CSC might be better. Not sure there's much point in exposing it though.
> >>
> >> In my first version it was called just CSC, but then I wanted to make it
> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> >> of HW that can do only one or the other, e.g. the offset calculations
> >> are supported only to one direction.
> > 
> > Are you planning to do RGB->YCbCr conversion in the plane as well? I
> > think we'll be only doing that at crtc/connector level.
> > 
> 
> No, but we need such a property to CRTC output, and there we certainly
> need to expose the CSC, because we do not have CTM (before gamma)
> matrix, and may have to use the output CSC in its place.

We're sort of in the same boat. We have just one matrix currently, but
the gamma can be either before, after, or both. So mixing CTM and YCbCr
output is a bit of a challenge, especially when gamma is involved.
Shashank has recently posted a patch set to do YCbCr output with i915.

So what we're going to be doing is using our matrix as the output CSC
or CTM as needed. And if the user asks to use both, well, then we get
to either multiply the matrices together (assuming user didn't want
some gamma in between) or we just refuse the entire thing. So I think
better to just expose the standard properties and map the hardware
resources to those dynamically. Otherwise there's going to be too many
ways to do things, and that just leads to confusion for userspace.

> 
> >>
> >>> I don't think most people are in the habit if cooking up new ways to
> >>> encode their pixel data.
> >>>
> >>
> >> In the embedded side I can imagine there could be some custom appliances
> >> where one may want to do some custom thing with the CSC and not needing
> >> a custom kernel for that could make a life easier... but then again I am
> >> not really an expert in this area.
> > 
> > I would assume most customy things you'd do in the crtc (eg. color
> > correction and whatnot). But could be that I just lack imagination.
> > 
> 
> I am on thin ice here :), but surely there is no harm in specifying an
> optional property for exposing the CSC as many display HWs provide a way
> to set an arbitrary CSC.

Well, if we don't have a clear use case we're more likely to specify it
wrong. And when the real use case appears we might discover that what we
specified isn't good enough. Hence I would avoid leaning forward too
heavily.

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

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

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

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/24/17 18:13, Ville Syrjälä wrote:
>> What about the uapi structs? In the patch there is an explicit struct
>> naming each coefficient for what they are for in YCbCr to RGB
>> conversion. Is this Ok, or should we go with a generic (CTM style)
>> matrix, that could be used for RGB to YCbCr conversion too?
> Not sure what we're talking about here, but like I said I think we
> should stick to a fairly limited set of standard props and just have
> each driver map the hardware resources to them as best they can.
> 

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

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

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

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 06:49:04PM +0300, Jyri Sarha wrote:
> On 04/24/17 18:13, Ville Syrjälä wrote:
> >> What about the uapi structs? In the patch there is an explicit struct
> >> naming each coefficient for what they are for in YCbCr to RGB
> >> conversion. Is this Ok, or should we go with a generic (CTM style)
> >> matrix, that could be used for RGB to YCbCr conversion too?
> > Not sure what we're talking about here, but like I said I think we
> > should stick to a fairly limited set of standard props and just have
> > each driver map the hardware resources to them as best they can.
> > 
> 
> Just about the implementation detail, if we should have a separate uapi
> struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use
> the same struct, then we could as well use the already existing CTM struct.
> 
> > If you just have csc+(de)gamma then I guess it might make sense
> > to just expose the YCbCr->RGB and degamma. If you have
> > degamma+csc+gamma then it might make sense to expose both
> > YCbCr->RGB, degamma, CTM, and gamma, and just refuse any
> > combination that can't be done. Eg. can't do YCbCr->RGB if
> > degamma is used, and YCbCr->RGB + CTM would require multiplying
> > the matrices together which you may or may not want to bother
> > with, I guess we could try to put some matrix math helpers into
> > the core to make such things less painful for drivers?
> 
> In fact we have plane specific YCbCr to RGB CSC (only preoffset
> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
> YCbCr CSC with optional post offset (so it can be used either as CSC or
> CTM).

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

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

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/24/17 19:55, Ville Syrjälä wrote:
>> In fact we have plane specific YCbCr to RGB CSC (only preoffset
>> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
>> YCbCr CSC with optional post offset (so it can be used either as CSC or
>> CTM).
> So with that plane hw you could perhaps do:
> - YCbCr->RGB if you input is not linear, but then you must
>   blend using non-linear data
> - colorspace conversion if your input is alredy linear
> 
> And with your crtc hw you could do:
> - degamma + CTM
> - gamma + RGB->YCbCr


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

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

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

... or have I misunderstood something?

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Sharma, Shashank]

Regards

Shashank


On 4/26/2017 6:26 PM, Jyri Sarha wrote:
> On 04/24/17 19:55, Ville Syrjälä wrote:
>>> In fact we have plane specific YCbCr to RGB CSC (only preoffset
>>> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
>>> YCbCr CSC with optional post offset (so it can be used either as CSC or
>>> CTM).
>> So with that plane hw you could perhaps do:
>> - YCbCr->RGB if you input is not linear, but then you must
>>    blend using non-linear data
>> - colorspace conversion if your input is alredy linear
>>
>> And with your crtc hw you could do:
>> - degamma + CTM
>> - gamma + RGB->YCbCr
>
> Just a generic question. Shouldn't - in an ideal HW - the degamma phase
> and the CTM be a plane specific property?
>
> I mean, isn't the purpose of normalizing the non linear RGB to linear
> (and possibly converting the color space) to have same format for all
> plane data before blending and composing them together?
I think Ville's point is something else.
There are two types of color operations possible:
- color space matching so that you can blend the plane data accurately.
- color correction or transformation to enhance display, or change 
output from one format to other format.

Now, when you want to blend, you have to make sure that, you blend among 
the same, that means:
- all the framebuffers should be in one state, either linear or non-linear
- all the framebuffers should be in one color space (REC 709 or 601 or 
2020), else you have to do gamut mapping
- Gamut mapping is possible on Linear data only.

Else, if we try to blend one REC2020 buffer and one REC709 buffer, its 
like adding 2CM + 3MM and making = 5CM

So, we can use the plane level properties in such a way that, you should 
have similar data to the input of the blender (linear Or non-linear)
And then, once blending is done, you can use CRTC level operations for 
color correction and transformation
(Like RGB->YCBCR conversion using CRTC level CSC, for YCBCR420 kind of 
outputs)

Does it helps in explanation ?

- Shashank
>
> Of course it does not matter if all the planes use the same color space,
> which then should be converted to something else for the output.
>
> ... or have I misunderstood something?
>
> Cheers,
> Jyri

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

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 03:56:54PM +0300, Jyri Sarha wrote:
> On 04/24/17 19:55, Ville Syrjälä wrote:
> >> In fact we have plane specific YCbCr to RGB CSC (only preoffset
> >> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
> >> YCbCr CSC with optional post offset (so it can be used either as CSC or
> >> CTM).
> > So with that plane hw you could perhaps do:
> > - YCbCr->RGB if you input is not linear, but then you must
> >   blend using non-linear data
> > - colorspace conversion if your input is alredy linear
> > 
> > And with your crtc hw you could do:
> > - degamma + CTM
> > - gamma + RGB->YCbCr
> 
> 
> Just a generic question. Shouldn't - in an ideal HW - the degamma phase
> and the CTM be a plane specific property?
> 
> I mean, isn't the purpose of normalizing the non linear RGB to linear
> (and possibly converting the color space) to have same format for all
> plane data before blending and composing them together?
> 
> Of course it does not matter if all the planes use the same color space,
> which then should be converted to something else for the output.
> 
> ... or have I misunderstood something?

I think the pipeline we want to expose is

 planes:                                    crtc:
 YCbCr->RGB -> degamma -> CTM -> (gamma) \
 YCbCr->RGB -> degamma -> CTM -> (gamma) -> (degamma) -> CTM -> gamma -> RGB->YCbCr
 ...                                     /

I put the plane gamma and crtc degamma in parentheses because ideally
you perhaps wouldn't need them (since you want to blend with linear
data). But we have hardware which has them, and might be lacking some
of the other stages so they can actually be useful. Eg. if you don't
have plane gamma/degamma and you want to use the crtc CTM to change
the colorspace you would also use the crtc degamma.

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Daniel Vetter]

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> Add standard properties to control YCbCr to RGB conversion in DRM
> planes. The created properties are stored to drm_plane object to allow
> different set of supported conversion modes for different planes on
> the device. For simplicity the related property blobs for CSC matrix
> and YCbCr preoffsets are also stored in the same place. The blob
> contents are defined in the uapi/drm/drm_mode.h header.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

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

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

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic.c        |  26 +++++++
>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>  drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_plane.c         |  10 +++
>  include/drm/drm_color_mgmt.h        |  23 ++++++
>  include/drm/drm_plane.h             |  10 +++
>  include/uapi/drm/drm_mode.h         |  12 ++++
>  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f881319..d1512aa 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	int ret;
> +	bool dummy;
>  
>  	if (property == config->prop_fb_id) {
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->rotation = val;
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
> +		state->ycbcr_to_rgb_mode = val;
> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_to_rgb_csc,
> +				val,
> +				sizeof(struct drm_ycbcr_to_rgb_csc),
> +				&dummy);
> +		return ret;
> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +				&state->ycbcr_to_rgb_preoffset,
> +				val,
> +				sizeof(struct drm_ycbcr_to_rgb_preoffset),
> +				&dummy);
> +		return ret;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
>  		*val = state->zpos;
> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
> +		*val = state->ycbcr_to_rgb_mode;
> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
> +		*val = state->ycbcr_to_rgb_csc ?
> +			state->ycbcr_to_rgb_csc->base.id : 0;
> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> +		*val = state->ycbcr_to_rgb_preoffset ?
> +			state->ycbcr_to_rgb_preoffset->base.id : 0;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3994b4..89fd826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  {
>  	memcpy(state, plane->state, sizeof(*state));
>  
> +	if (state->ycbcr_to_rgb_csc)
> +		drm_property_blob_get(state->ycbcr_to_rgb_csc);
> +
> +	if (state->ycbcr_to_rgb_preoffset)
> +		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_get(state->fb);
>  
> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>   */
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
> +	drm_property_blob_put(state->ycbcr_to_rgb_csc);
> +	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
> +
>  	if (state->fb)
>  		drm_framebuffer_put(state->fb);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index cc23b9a..badaddd 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -29,9 +29,14 @@
>  /**
>   * DOC: overview
>   *
> - * Color management or color space adjustments is supported through a set of 5
> - * properties on the &drm_crtc object. They are set up by calling
> - * drm_crtc_enable_color_mgmt().
> + * Color management or color space adjustments in CRTCs is supported
> + * through a set of 5 properties on the &drm_crtc object. They are set
> + * up by calling drm_crtc_enable_color_mgmt().
> + *
> + * Color space conversions from YCbCr to RGB color space in planes is
> + * supporter trough 3 optional properties in &drm_plane object.
> + *
> + * The &drm_crtc object's properties are:
>   *
>   * "DEGAMMA_LUT”:
>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> @@ -85,6 +90,28 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>   * "GAMMA_LUT" property above.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "YCBCR_TO_RGB_MODE"
> + * 	Optional plane enum property to configure YCbCr to RGB
> + * 	conversion. The possible modes include a number of standard
> + * 	conversions and a possibility to select custom conversion
> + * 	matrix and preoffset vector. The driver should select the
> + *	supported subset of of the modes.
> + *
> + * "YCBCR_TO_RGB_CSC"
> + *	Optional plane property blob to set YCbCr to RGB conversion
> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> + *	defined in uapi/drm/drm_mode.h. Whether this property is
> + *	active dependent on YCBCR_TO_RGB_MODE property.
> + *
> + * "YCBCR_TO_RGB_PREOFFSET"
> + *	Optional plane property blob to configure YCbCr offset before
> + *	YCbCr to RGB CSC is applied. The blob contains struct
> + *	drm_ycbcr_to_rgb_preoffset which is defined in
> + *	uapi/drm/drm_mode.h. Whether this property is active dependent
> + *	on YCBCR_TO_RGB_MODE property.
>   */
>  
>  /**
> @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock_all(dev);
>  	return ret;
>  }
> +
> +static char *ycbcr_to_rgb_mode_name[] = {
> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
> +		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
> +		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
> +		"YCbCr TO RGB CSC limited range preoffset",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
> +		"YCbCr TO RGB CSC full range preoffset",
> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
> +		"YCBCR TO RGB CSC preoffset vector",
> +};
> +
> +/**
> + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
> + * @enum_list: drm_prop_enum_list array of supported modes without names
> + * @enum_list_len: length of enum_list array
> + * @default_mode: default csc mode
> + *
> + * Create and attach plane specific YCbCr to RGB conversion related
> + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
> + * created and an the supported modes should be provided the enum_list
> + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
> + * created based on supported conversion modes. The enum_list parameter
> + * should not contain the enum names, because the standard names are
> + * added by this function.
> + */
> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_to_rgb_mode default_mode)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	bool ycbcr_to_rgb_csc_create = false;
> +	bool ycbcr_to_rgb_preoffset_create = false;
> +	int i;
> +
> +	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
> +		return -EEXIST;
> +
> +	for (i = 0; i < enum_list_len; i++) {
> +		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
> +
> +		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
> +
> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
> +		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
> +			ycbcr_to_rgb_csc_create = true;
> +
> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
> +			ycbcr_to_rgb_csc_create = true;
> +			ycbcr_to_rgb_preoffset_create = true;
> +		}
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"YCBCR_TO_RGB_MODE",
> +					enum_list, enum_list_len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->ycbcr_to_rgb_mode_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_mode);
> +
> +	if (ycbcr_to_rgb_csc_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_TO_RGB_CSC", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_to_rgb_csc_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
> +	if (ycbcr_to_rgb_preoffset_create) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> +					   DRM_MODE_PROP_BLOB,
> +					   "YCBCR_TO_RGB_PREOFFSET", 0);
> +		if (!prop)
> +			return -ENOMEM;
> +		plane->ycbcr_to_rgb_preoffset_property = prop;
> +		drm_object_attach_property(&plane->base, prop, 0);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index bc71aa2..4c7e827 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	kfree(plane->name);
>  
> +	if (plane->ycbcr_to_rgb_mode_property)
> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
> +
> +	if (plane->ycbcr_to_rgb_csc_property)
> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
> +
> +	if (plane->ycbcr_to_rgb_preoffset_property)
> +		drm_property_destroy(dev,
> +				     plane->ycbcr_to_rgb_preoffset_property);
> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cb..a20b3ff 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,8 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
> +struct drm_prop_enum_list;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  				 int gamma_size);
>  
> +enum drm_plane_ycbcr_to_rgb_mode {
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
> +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
> +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
> +};
> +
> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_to_rgb_mode default_mode);
> +
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e70..41dcde2 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/ctype.h>
>  #include <drm/drm_mode_object.h>
> +#include <drm/drm_color_mgmt.h>
>  
>  struct drm_crtc;
>  struct drm_printer;
> @@ -112,6 +113,11 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* YCbCr to RGB conversion */
> +	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
> +	struct drm_property_blob *ycbcr_to_rgb_csc;
> +	struct drm_property_blob *ycbcr_to_rgb_preoffset;
> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -523,6 +529,10 @@ struct drm_plane {
>  
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> +
> +	struct drm_property *ycbcr_to_rgb_mode_property;
> +	struct drm_property *ycbcr_to_rgb_csc_property;
> +	struct drm_property *ycbcr_to_rgb_preoffset_property;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc0..27e0bee 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -543,6 +543,18 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +struct drm_ycbcr_to_rgb_csc {
> +	/* Conversion matrix in 2-complement s32.32 format. */
> +	__s64 ry, rcb, rcr;
> +	__s64 gy, gcb, gcr;
> +	__s64 by, bcb, bcr;
> +};
> +
> +struct drm_ycbcr_to_rgb_preoffset {
> +	/* Offset vector in 2-complement s.32 format. */
> +	__s32 y, cb, cr;
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> -- 
> 1.9.1
> 

-- 
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 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 05/02/17 11:33, Daniel Vetter wrote:
> On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
>> Add standard properties to control YCbCr to RGB conversion in DRM
>> planes. The created properties are stored to drm_plane object to allow
>> different set of supported conversion modes for different planes on
>> the device. For simplicity the related property blobs for CSC matrix
>> and YCbCr preoffsets are also stored in the same place. The blob
>> contents are defined in the uapi/drm/drm_mode.h header.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> 
> Just to make sure there's no surprises: We need the userspace for this
> too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone,
> whatever you feel like) or drm_hwcomposer.
> 
> But yeah might be good to bikeshed the uabi first a bit more and get at
> least some agreement on that.
> 

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

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

Cheer,
Jyri

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

> Thanks, Daniel
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  26 +++++++
>>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>>  drivers/gpu/drm/drm_color_mgmt.c    | 136 +++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/drm_plane.c         |  10 +++
>>  include/drm/drm_color_mgmt.h        |  23 ++++++
>>  include/drm/drm_plane.h             |  10 +++
>>  include/uapi/drm/drm_mode.h         |  12 ++++
>>  7 files changed, 223 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f881319..d1512aa 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_mode_config *config = &dev->mode_config;
>> +	int ret;
>> +	bool dummy;
>>  
>>  	if (property == config->prop_fb_id) {
>>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>  		state->rotation = val;
>>  	} else if (property == plane->zpos_property) {
>>  		state->zpos = val;
>> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +		state->ycbcr_to_rgb_mode = val;
>> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->ycbcr_to_rgb_csc,
>> +				val,
>> +				sizeof(struct drm_ycbcr_to_rgb_csc),
>> +				&dummy);
>> +		return ret;
>> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->ycbcr_to_rgb_preoffset,
>> +				val,
>> +				sizeof(struct drm_ycbcr_to_rgb_preoffset),
>> +				&dummy);
>> +		return ret;
>>  	} else if (plane->funcs->atomic_set_property) {
>>  		return plane->funcs->atomic_set_property(plane, state,
>>  				property, val);
>> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>  		*val = state->rotation;
>>  	} else if (property == plane->zpos_property) {
>>  		*val = state->zpos;
>> +	} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +		*val = state->ycbcr_to_rgb_mode;
>> +	} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +		*val = state->ycbcr_to_rgb_csc ?
>> +			state->ycbcr_to_rgb_csc->base.id : 0;
>> +	} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +		*val = state->ycbcr_to_rgb_preoffset ?
>> +			state->ycbcr_to_rgb_preoffset->base.id : 0;
>>  	} else if (plane->funcs->atomic_get_property) {
>>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>>  	} else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c3994b4..89fd826 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>  {
>>  	memcpy(state, plane->state, sizeof(*state));
>>  
>> +	if (state->ycbcr_to_rgb_csc)
>> +		drm_property_blob_get(state->ycbcr_to_rgb_csc);
>> +
>> +	if (state->ycbcr_to_rgb_preoffset)
>> +		drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
>> +
>>  	if (state->fb)
>>  		drm_framebuffer_get(state->fb);
>>  
>> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>>   */
>>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>  {
>> +	drm_property_blob_put(state->ycbcr_to_rgb_csc);
>> +	drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
>> +
>>  	if (state->fb)
>>  		drm_framebuffer_put(state->fb);
>>  
>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>> index cc23b9a..badaddd 100644
>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>> @@ -29,9 +29,14 @@
>>  /**
>>   * DOC: overview
>>   *
>> - * Color management or color space adjustments is supported through a set of 5
>> - * properties on the &drm_crtc object. They are set up by calling
>> - * drm_crtc_enable_color_mgmt().
>> + * Color management or color space adjustments in CRTCs is supported
>> + * through a set of 5 properties on the &drm_crtc object. They are set
>> + * up by calling drm_crtc_enable_color_mgmt().
>> + *
>> + * Color space conversions from YCbCr to RGB color space in planes is
>> + * supporter trough 3 optional properties in &drm_plane object.
>> + *
>> + * The &drm_crtc object's properties are:
>>   *
>>   * "DEGAMMA_LUT”:
>>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
>> @@ -85,6 +90,28 @@
>>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
>>   * "GAMMA_LUT" property above.
>> + *
>> + * The &drm_plane object's properties are:
>> + *
>> + * "YCBCR_TO_RGB_MODE"
>> + * 	Optional plane enum property to configure YCbCr to RGB
>> + * 	conversion. The possible modes include a number of standard
>> + * 	conversions and a possibility to select custom conversion
>> + * 	matrix and preoffset vector. The driver should select the
>> + *	supported subset of of the modes.
>> + *
>> + * "YCBCR_TO_RGB_CSC"
>> + *	Optional plane property blob to set YCbCr to RGB conversion
>> + *	matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
>> + *	defined in uapi/drm/drm_mode.h. Whether this property is
>> + *	active dependent on YCBCR_TO_RGB_MODE property.
>> + *
>> + * "YCBCR_TO_RGB_PREOFFSET"
>> + *	Optional plane property blob to configure YCbCr offset before
>> + *	YCbCr to RGB CSC is applied. The blob contains struct
>> + *	drm_ycbcr_to_rgb_preoffset which is defined in
>> + *	uapi/drm/drm_mode.h. Whether this property is active dependent
>> + *	on YCBCR_TO_RGB_MODE property.
>>   */
>>  
>>  /**
>> @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>>  	drm_modeset_unlock_all(dev);
>>  	return ret;
>>  }
>> +
>> +static char *ycbcr_to_rgb_mode_name[] = {
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.601 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
>> +		"YCbCr BT.709 limited range TO RGB BT.2020 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.601 full range",
>> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
>> +		"YCbCr BT.2020 limited range TO RGB BT.709 full range",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
>> +		"YCbCr TO RGB CSC limited range preoffset",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
>> +		"YCbCr TO RGB CSC full range preoffset",
>> +	[DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
>> +		"YCBCR TO RGB CSC preoffset vector",
>> +};
>> +
>> +/**
>> + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related properties
>> + * @enum_list: drm_prop_enum_list array of supported modes without names
>> + * @enum_list_len: length of enum_list array
>> + * @default_mode: default csc mode
>> + *
>> + * Create and attach plane specific YCbCr to RGB conversion related
>> + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
>> + * created and an the supported modes should be provided the enum_list
>> + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
>> + * created based on supported conversion modes. The enum_list parameter
>> + * should not contain the enum names, because the standard names are
>> + * added by this function.
>> + */
>> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
>> +			struct drm_prop_enum_list *enum_list,
>> +			unsigned int enum_list_len,
>> +			enum drm_plane_ycbcr_to_rgb_mode default_mode)
>> +{
>> +	struct drm_device *dev = plane->dev;
>> +	struct drm_property *prop;
>> +	bool ycbcr_to_rgb_csc_create = false;
>> +	bool ycbcr_to_rgb_preoffset_create = false;
>> +	int i;
>> +
>> +	if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
>> +		return -EEXIST;
>> +
>> +	for (i = 0; i < enum_list_len; i++) {
>> +		enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
>> +
>> +		enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
>> +
>> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
>> +		    mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
>> +			ycbcr_to_rgb_csc_create = true;
>> +
>> +		if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
>> +			ycbcr_to_rgb_csc_create = true;
>> +			ycbcr_to_rgb_preoffset_create = true;
>> +		}
>> +	}
>> +
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +					"YCBCR_TO_RGB_MODE",
>> +					enum_list, enum_list_len);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	plane->ycbcr_to_rgb_mode_property = prop;
>> +	drm_object_attach_property(&plane->base, prop, default_mode);
>> +
>> +	if (ycbcr_to_rgb_csc_create) {
>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>> +					   DRM_MODE_PROP_BLOB,
>> +					   "YCBCR_TO_RGB_CSC", 0);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +		plane->ycbcr_to_rgb_csc_property = prop;
>> +		drm_object_attach_property(&plane->base, prop, 0);
>> +	}
>> +
>> +	if (ycbcr_to_rgb_preoffset_create) {
>> +		prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
>> +					   DRM_MODE_PROP_BLOB,
>> +					   "YCBCR_TO_RGB_PREOFFSET", 0);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +		plane->ycbcr_to_rgb_preoffset_property = prop;
>> +		drm_object_attach_property(&plane->base, prop, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index bc71aa2..4c7e827 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane)
>>  
>>  	kfree(plane->name);
>>  
>> +	if (plane->ycbcr_to_rgb_mode_property)
>> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property);
>> +
>> +	if (plane->ycbcr_to_rgb_csc_property)
>> +		drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property);
>> +
>> +	if (plane->ycbcr_to_rgb_preoffset_property)
>> +		drm_property_destroy(dev,
>> +				     plane->ycbcr_to_rgb_preoffset_property);
>> +
>>  	memset(plane, 0, sizeof(*plane));
>>  }
>>  EXPORT_SYMBOL(drm_plane_cleanup);
>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>> index 03a59cb..a20b3ff 100644
>> --- a/include/drm/drm_color_mgmt.h
>> +++ b/include/drm/drm_color_mgmt.h
>> @@ -26,6 +26,8 @@
>>  #include <linux/ctype.h>
>>  
>>  struct drm_crtc;
>> +struct drm_plane;
>> +struct drm_prop_enum_list;
>>  
>>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>>  
>> @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>>  				 int gamma_size);
>>  
>> +enum drm_plane_ycbcr_to_rgb_mode {
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
>> +	DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
>> +	DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
>> +	DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
>> +};
>> +
>> +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
>> +			struct drm_prop_enum_list *enum_list,
>> +			unsigned int enum_list_len,
>> +			enum drm_plane_ycbcr_to_rgb_mode default_mode);
>> +
>>  #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 9ab3e70..41dcde2 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/list.h>
>>  #include <linux/ctype.h>
>>  #include <drm/drm_mode_object.h>
>> +#include <drm/drm_color_mgmt.h>
>>  
>>  struct drm_crtc;
>>  struct drm_printer;
>> @@ -112,6 +113,11 @@ struct drm_plane_state {
>>  	unsigned int zpos;
>>  	unsigned int normalized_zpos;
>>  
>> +	/* YCbCr to RGB conversion */
>> +	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
>> +	struct drm_property_blob *ycbcr_to_rgb_csc;
>> +	struct drm_property_blob *ycbcr_to_rgb_preoffset;
>> +
>>  	/* Clipped coordinates */
>>  	struct drm_rect src, dst;
>>  
>> @@ -523,6 +529,10 @@ struct drm_plane {
>>  
>>  	struct drm_property *zpos_property;
>>  	struct drm_property *rotation_property;
>> +
>> +	struct drm_property *ycbcr_to_rgb_mode_property;
>> +	struct drm_property *ycbcr_to_rgb_csc_property;
>> +	struct drm_property *ycbcr_to_rgb_preoffset_property;
>>  };
>>  
>>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 8c67fc0..27e0bee 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -543,6 +543,18 @@ struct drm_color_lut {
>>  	__u16 reserved;
>>  };
>>  
>> +struct drm_ycbcr_to_rgb_csc {
>> +	/* Conversion matrix in 2-complement s32.32 format. */
>> +	__s64 ry, rcb, rcr;
>> +	__s64 gy, gcb, gcr;
>> +	__s64 by, bcb, bcr;
>> +};
>> +
>> +struct drm_ycbcr_to_rgb_preoffset {
>> +	/* Offset vector in 2-complement s.32 format. */
>> +	__s32 y, cb, cr;
>> +};
>> +
>>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>> -- 
>> 1.9.1
>>
> 

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

[PATCH RFC 4/6] drm/omap: cleanup color space conversion

[Jyri Sarha]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

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

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 59 +++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 5ac0145..b53e63d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -293,11 +293,6 @@ struct dispc_gamma_desc {
 	},
 };
 
-struct color_conv_coef {
-	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
-	int full_range;
-};
-
 static unsigned long dispc_fclk_rate(void);
 static unsigned long dispc_core_clk_rate(void);
 static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
@@ -757,9 +752,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id plane, int fir_hinc,
 	}
 }
 
+struct csc_coef_yuv2rgb {
+	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
+	bool full_range;
+};
+
+struct csc_coef_rgb2yuv {
+	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
+	bool full_range;
+};
 
 static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
-		const struct color_conv_coef *ct)
+		const struct csc_coef_yuv2rgb *ct)
 {
 #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
 
@@ -769,7 +773,24 @@ static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
 	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->bcr, ct->by));
 	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->bcb));
 
-	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
+
+#undef CVAL
+}
+
+static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv *ct)
+{
+	const enum omap_plane_id plane = OMAP_DSS_WB;
+
+#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
+
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
+	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
+
+	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
 
 #undef CVAL
 }
@@ -778,20 +799,28 @@ static void dispc_setup_color_conv_coef(void)
 {
 	int i;
 	int num_ovl = dss_feat_get_num_ovls();
-	const struct color_conv_coef ctbl_bt601_5_ovl = {
-		/* YUV -> RGB */
-		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
+
+	/* YUV -> RGB, ITU-R BT.601, limited range */
+	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
+		298,    0,  409,	/* ry, rcb, rcr */
+		298, -100, -208,	/* gy, gcb, gcr */
+		298,  516,    0,	/* by, bcb, bcr */
+		false,			/* limited range */
 	};
-	const struct color_conv_coef ctbl_bt601_5_wb = {
-		/* RGB -> YUV */
-		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
+
+	/* RGB -> YUV, ITU-R BT.601, limited range */
+	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
+		 66, 129,  25,		/* yr,   yg,  yb */
+		-38, -74, 112,		/* cbr, cbg, cbb */
+		112, -94, -18,		/* crr, crg, crb */
+		false,			/* limited range */
 	};
 
 	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
+		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
 
 	if (dispc.feat->has_writeback)
-		dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, &ctbl_bt601_5_wb);
+		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
 }
 
 static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)
-- 
1.9.1

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

Re: [PATCH RFC 4/6] drm/omap: cleanup color space conversion

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:15 Jyri Sarha wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> The setup code for color space conversion is a bit messy. This patch
> cleans it up.
> 
> For some reason the TRM uses values in YCrCb order, which is also used
> in the current driver, whereas everywhere else it's YCbCr (which also
> matches YUV order). This patch changes the tables to use the common
> order to avoid confusion.
> 
> The tables are split into separate lines, and comments added for
> clarity.
> 
> WB color conversion registers are similar but different than non-WB, but
> the same function was used to write both. It worked fine because the
> coef table was adjusted accordingly, but that was rather confusing. This
> patch adds a separate function to write the WB values so that the coef
> table can be written in an understandable way.

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

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 +++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 5ac0145..b53e63d 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -293,11 +293,6 @@ struct dispc_gamma_desc {
>  	},
>  };
> 
> -struct color_conv_coef {
> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
> -	int full_range;
> -};
> -
>  static unsigned long dispc_fclk_rate(void);
>  static unsigned long dispc_core_clk_rate(void);
>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
> @@ -757,9 +752,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id
> plane, int fir_hinc, }
>  }
> 
> +struct csc_coef_yuv2rgb {
> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
> +	bool full_range;
> +};
> +
> +struct csc_coef_rgb2yuv {
> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
> +	bool full_range;
> +};
> 
>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
> -		const struct color_conv_coef *ct)
> +		const struct csc_coef_yuv2rgb *ct)
>  {
>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> 
> @@ -769,7 +773,24 @@ static void dispc_ovl_write_color_conv_coef(enum
> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
> CVAL(0, ct->bcb));
> 
> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> +
> +#undef CVAL
> +}
> +
> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
> *ct) +{
> +	const enum omap_plane_id plane = OMAP_DSS_WB;
> +
> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> +
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct-
>crg));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct-
>cbr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> +
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> 
>  #undef CVAL
>  }
> @@ -778,20 +799,28 @@ static void dispc_setup_color_conv_coef(void)
>  {
>  	int i;
>  	int num_ovl = dss_feat_get_num_ovls();
> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
> -		/* YUV -> RGB */
> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
> +
> +	/* YUV -> RGB, ITU-R BT.601, limited range */
> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> +		298,    0,  409,	/* ry, rcb, rcr */
> +		298, -100, -208,	/* gy, gcb, gcr */
> +		298,  516,    0,	/* by, bcb, bcr */

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

> +		false,			/* limited range */
>  	};
> -	const struct color_conv_coef ctbl_bt601_5_wb = {
> -		/* RGB -> YUV */
> -		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
> +
> +	/* RGB -> YUV, ITU-R BT.601, limited range */
> +	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> +		 66, 129,  25,		/* yr,   yg,  yb */
> +		-38, -74, 112,		/* cbr, cbg, cbb */
> +		112, -94, -18,		/* crr, crg, crb */
> +		false,			/* limited range */
>  	};
> 
>  	for (i = 1; i < num_ovl; i++)
> -		dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
> +		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
> 
>  	if (dispc.feat->has_writeback)
> -		dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, 
&ctbl_bt601_5_wb);
> +		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
>  }
> 
>  static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

-- 
Regards,

Laurent Pinchart

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

[PATCH RFC 5/6] drm/omap: csc full range support

[Jyri Sarha]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index b53e63d..f2a2d08 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -799,6 +799,8 @@ static void dispc_setup_color_conv_coef(void)
 {
 	int i;
 	int num_ovl = dss_feat_get_num_ovls();
+	/* always use full range for now */
+	bool use_full_range = true;
 
 	/* YUV -> RGB, ITU-R BT.601, limited range */
 	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
@@ -808,6 +810,14 @@ static void dispc_setup_color_conv_coef(void)
 		false,			/* limited range */
 	};
 
+	/* YUV -> RGB, ITU-R BT.601, full range */
+	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
+		256,   0,  358,		/* ry, rcb, rcr */
+		256, -88, -182,		/* gy, gcb, gcr */
+		256, 452,    0,		/* by, bcb, bcr */
+		true,			/* full range */
+	};
+
 	/* RGB -> YUV, ITU-R BT.601, limited range */
 	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
 		 66, 129,  25,		/* yr,   yg,  yb */
@@ -816,11 +826,30 @@ static void dispc_setup_color_conv_coef(void)
 		false,			/* limited range */
 	};
 
+	/* RGB -> YUV, ITU-R BT.601, full range */
+	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {
+		 77,  150,  29,		/* yr,   yg,  yb */
+		-43,  -85, 128,		/* cbr, cbg, cbb */
+		128, -107, -21,		/* crr, crg, crb */
+		true,			/* full range */
+	};
+
+	const struct csc_coef_yuv2rgb *yuv2rgb;
+	const struct csc_coef_rgb2yuv *rgb2yuv;
+
+	if (use_full_range) {
+		yuv2rgb = &coefs_yuv2rgb_bt601_full;
+		rgb2yuv = &coefs_rgb2yuv_bt601_full;
+	} else {
+		yuv2rgb = &coefs_yuv2rgb_bt601_lim;
+		rgb2yuv = &coefs_rgb2yuv_bt601_lim;
+	}
+
 	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
+		dispc_ovl_write_color_conv_coef(i, yuv2rgb);
 
 	if (dispc.feat->has_writeback)
-		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
+		dispc_wb_write_color_conv_coef(rgb2yuv);
 }
 
 static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)
-- 
1.9.1

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

Re: [PATCH RFC 5/6] drm/omap: csc full range support

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:16 Jyri Sarha wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> At the moment the driver always uses limited range when doing YUV-RGB
> conversions. This patch adds full-range tables, and makes the code to
> always use full-range tables.
> 
> In the future we should allow the user to select the color range instead
> of hardcoding it.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index b53e63d..f2a2d08 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -799,6 +799,8 @@ static void dispc_setup_color_conv_coef(void)
>  {
>  	int i;
>  	int num_ovl = dss_feat_get_num_ovls();
> +	/* always use full range for now */
> +	bool use_full_range = true;
> 
>  	/* YUV -> RGB, ITU-R BT.601, limited range */
>  	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> @@ -808,6 +810,14 @@ static void dispc_setup_color_conv_coef(void)
>  		false,			/* limited range */
>  	};
> 
> +	/* YUV -> RGB, ITU-R BT.601, full range */
> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> +		256,   0,  358,		/* ry, rcb, rcr */
> +		256, -88, -182,		/* gy, gcb, gcr */
> +		256, 452,    0,		/* by, bcb, bcr */
> +		true,			/* full range */
> +	};

Shouldn't all those tables be static const ?

With that fixed,

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

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

-- 
Regards,

Laurent Pinchart

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

[PATCH RFC 6/6] drm/omap: Enable ycbcr_to_rgb_properties for omapdrm planes REVISIT

[Jyri Sarha]

Adds support for YCBCR_TO_RGB_MODE and YCBCR_TO_RGB_CSC properties to
omap_plane.c and dispc.c. The supported CSC presets are:

- YCbCt BT.601 limited range to RGB BT.601 full range
- YCbCt BT.601 full range to RGB BT.601 full range
- YCbCt BT.709 limited range to RGB BT.709 full range

Custom CSC with YCbCr limited and full range preoffsets are
also supported.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 131 +++++++++++++++++++++++-----------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  14 ++++
 drivers/gpu/drm/omapdrm/omap_plane.c  |  41 +++++++++++
 3 files changed, 144 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index f2a2d08..48dfb9c 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -752,16 +752,6 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id plane, int fir_hinc,
 	}
 }
 
-struct csc_coef_yuv2rgb {
-	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
-	bool full_range;
-};
-
-struct csc_coef_rgb2yuv {
-	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
-	bool full_range;
-};
-
 static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
 		const struct csc_coef_yuv2rgb *ct)
 {
@@ -795,6 +785,54 @@ static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv *ct)
 #undef CVAL
 }
 
+/* YUV -> RGB, ITU-R BT.601, full range */
+const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
+	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
+	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
+	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
+	true,			/* full range */
+};
+
+/* YUV -> RGB, ITU-R BT.601, limited range */
+const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
+	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
+	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
+	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
+	false,			/* limited range */
+};
+
+/* YUV -> RGB, ITU-R BT.709, limited range */
+const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
+	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
+	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
+	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
+	false,			/* limited range */
+};
+
+/* RGB -> YUV, ITU-R BT.601, limited range */
+const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
+	 66, 129,  25,		/* yr,   yg,  yb | 0.257  0.504  0.098|*/
+	-38, -74, 112,		/* cbr, cbg, cbb |-0.148 -0.291  0.439|*/
+	112, -94, -18,		/* crr, crg, crb | 0.439 -0.368 -0.071|*/
+	false,			/* limited range */
+};
+
+/* RGB -> YUV, ITU-R BT.601, full range */
+const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {
+	 77,  150,  29,		/* yr,   yg,  yb | 0.299  0.587  0.114|*/
+	-43,  -85, 128,		/* cbr, cbg, cbb |-0.173 -0.339  0.511|*/
+	128, -107, -21,		/* crr, crg, crb | 0.511 -0.428 -0.083|*/
+	true,			/* full range */
+};
+
+/* RGB -> YUV, ITU-R BT.709, limited range */
+const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {
+	 47,  157,   16,	/* yr,   yg,  yb | 0.1826  0.6142  0.0620|*/
+	-26,  -87,  112,	/* cbr, cbg, cbb |-0.1006 -0.3386  0.4392|*/
+	112, -102,  -10,	/* crr, crg, crb | 0.4392 -0.3989 -0.0403|*/
+	false,			/* limited range */
+};
+
 static void dispc_setup_color_conv_coef(void)
 {
 	int i;
@@ -802,38 +840,6 @@ static void dispc_setup_color_conv_coef(void)
 	/* always use full range for now */
 	bool use_full_range = true;
 
-	/* YUV -> RGB, ITU-R BT.601, limited range */
-	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
-		298,    0,  409,	/* ry, rcb, rcr */
-		298, -100, -208,	/* gy, gcb, gcr */
-		298,  516,    0,	/* by, bcb, bcr */
-		false,			/* limited range */
-	};
-
-	/* YUV -> RGB, ITU-R BT.601, full range */
-	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
-		256,   0,  358,		/* ry, rcb, rcr */
-		256, -88, -182,		/* gy, gcb, gcr */
-		256, 452,    0,		/* by, bcb, bcr */
-		true,			/* full range */
-	};
-
-	/* RGB -> YUV, ITU-R BT.601, limited range */
-	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
-		 66, 129,  25,		/* yr,   yg,  yb */
-		-38, -74, 112,		/* cbr, cbg, cbb */
-		112, -94, -18,		/* crr, crg, crb */
-		false,			/* limited range */
-	};
-
-	/* RGB -> YUV, ITU-R BT.601, full range */
-	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {
-		 77,  150,  29,		/* yr,   yg,  yb */
-		-43,  -85, 128,		/* cbr, cbg, cbb */
-		128, -107, -21,		/* crr, crg, crb */
-		true,			/* full range */
-	};
-
 	const struct csc_coef_yuv2rgb *yuv2rgb;
 	const struct csc_coef_rgb2yuv *rgb2yuv;
 
@@ -2890,6 +2896,42 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 	return 0;
 }
 
+
+static int dispc_ovl_set_csc(enum omap_plane_id plane,
+			     const struct omap_overlay_info *oi)
+{
+	struct csc_coef_yuv2rgb csc;
+	const struct csc_coef_yuv2rgb *cscp = &csc;
+
+	switch (oi->ycbcr_to_rgb_mode) {
+	case DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET:
+		csc = oi->ycbcr_to_rgb_csc;
+		csc.full_range = false;
+		break;
+	case DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET:
+		csc = oi->ycbcr_to_rgb_csc;
+		csc.full_range = true;
+		break;
+	case DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL:
+		cscp = &coefs_yuv2rgb_bt601_lim;
+		break;
+	case DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL:
+		cscp = &coefs_yuv2rgb_bt601_full;
+		break;
+	case DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL:
+		cscp = &coefs_yuv2rgb_bt709_lim;
+		break;
+	default:
+		DSSERR("Unsupported CSC mode %d for plane %d\n",
+		       oi->ycbcr_to_rgb_mode, plane);
+		return -EINVAL;
+	}
+
+	dispc_ovl_write_color_conv_coef(plane, cscp);
+
+	return 0;
+}
+
 static int dispc_ovl_setup(enum omap_plane_id plane,
 		const struct omap_overlay_info *oi,
 		const struct videomode *vm, bool mem_to_mem)
@@ -2912,6 +2954,11 @@ static int dispc_ovl_setup(enum omap_plane_id plane,
 		oi->out_width, oi->out_height, oi->color_mode, oi->rotation,
 		oi->mirror, oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
 		oi->rotation_type, replication, vm, mem_to_mem);
+	if (r)
+		return r;
+
+	if (dss_feat_color_mode_supported(plane, OMAP_DSS_COLOR_UYVY))
+		r = dispc_ovl_set_csc(plane, oi);
 
 	return r;
 }
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 63c2684..f4aab99 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -25,6 +25,7 @@
 #include <video/videomode.h>
 #include <linux/platform_data/omapdss.h>
 #include <uapi/drm/drm_mode.h>
+#include <drm/drm_color_mgmt.h>
 
 #define DISPC_IRQ_FRAMEDONE		(1 << 0)
 #define DISPC_IRQ_VSYNC			(1 << 1)
@@ -312,6 +313,16 @@ struct omap_dss_cpr_coefs {
 	s16 br, bg, bb;
 };
 
+struct csc_coef_yuv2rgb {
+	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
+	bool full_range;
+};
+
+struct csc_coef_rgb2yuv {
+	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
+	bool full_range;
+};
+
 struct omap_overlay_info {
 	dma_addr_t paddr;
 	dma_addr_t p_uv_addr;  /* for NV12 format */
@@ -330,6 +341,9 @@ struct omap_overlay_info {
 	u8 global_alpha;
 	u8 pre_mult_alpha;
 	u8 zorder;
+
+	enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode;
+	struct csc_coef_yuv2rgb ycbcr_to_rgb_csc;
 };
 
 struct omap_overlay {
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 9168154..ec38f9c 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -67,6 +67,27 @@ static void omap_plane_cleanup_fb(struct drm_plane *plane,
 		omap_framebuffer_unpin(old_state->fb);
 }
 
+static int omap_plane_s32_32_to_s2_8(s64 coef)
+{
+	return clamp_val((int)(coef >> 24), -0x1FF - 1, 0x1FF);
+}
+
+static void omap_plane_csc_coefs_from_blob(const void *blob,
+					   struct csc_coef_yuv2rgb *csc)
+{
+	const struct drm_ycbcr_to_rgb_csc *cscbp = blob;
+
+	csc->ry = omap_plane_s32_32_to_s2_8(cscbp->ry);
+	csc->rcb = omap_plane_s32_32_to_s2_8(cscbp->rcb);
+	csc->rcr = omap_plane_s32_32_to_s2_8(cscbp->rcr);
+	csc->gy = omap_plane_s32_32_to_s2_8(cscbp->gy);
+	csc->gcb = omap_plane_s32_32_to_s2_8(cscbp->gcb);
+	csc->gcr = omap_plane_s32_32_to_s2_8(cscbp->gcr);
+	csc->by = omap_plane_s32_32_to_s2_8(cscbp->by);
+	csc->bcb = omap_plane_s32_32_to_s2_8(cscbp->bcb);
+	csc->bcr = omap_plane_s32_32_to_s2_8(cscbp->bcr);
+}
+
 static void omap_plane_atomic_update(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
@@ -86,6 +107,10 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.global_alpha = 0xff;
 	info.mirror = 0;
 	info.zorder = omap_state->zorder;
+	info.ycbcr_to_rgb_mode = state->ycbcr_to_rgb_mode;
+	if (state->ycbcr_to_rgb_csc)
+		omap_plane_csc_coefs_from_blob(state->ycbcr_to_rgb_csc->data,
+					       &info.ycbcr_to_rgb_csc);
 
 	memset(&win, 0, sizeof(win));
 	win.rotation = state->rotation;
@@ -324,6 +349,15 @@ static int omap_plane_atomic_get_property(struct drm_plane *plane,
 	.atomic_get_property = omap_plane_atomic_get_property,
 };
 
+/* The enum names are filled by drm_plane_create_ycbcr_to_rgb_properties() */
+static struct drm_prop_enum_list omap_ycbcr_to_rgb_enum_list[] = {
+	{ DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL, NULL },
+	{ DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL, NULL },
+	{ DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL, NULL },
+	{ DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET, NULL },
+	{ DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET, NULL },
+};
+
 static const char *plane_id_to_name[] = {
 	[OMAP_DSS_GFX] = "gfx",
 	[OMAP_DSS_VIDEO1] = "vid1",
@@ -378,6 +412,13 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 
 	omap_plane_install_properties(plane, &plane->base);
 
+	if ((priv->dispc_ops->ovl_get_color_modes(omap_plane->id) &
+	     OMAP_DSS_COLOR_UYVY) != 0)
+		drm_plane_create_ycbcr_to_rgb_properties(plane,
+				omap_ycbcr_to_rgb_enum_list,
+				ARRAY_SIZE(omap_ycbcr_to_rgb_enum_list),
+				DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL);
+
 	return plane;
 
 error:
-- 
1.9.1

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

Re: [PATCH RFC 0/6] drm: Add properties to control YCbCr to RGB conversion

[Sharma, Shashank]

Regards

Shashank


On 4/21/2017 3:21 PM, Jyri Sarha wrote:
> The series adds plane specific atomic properties to control YCbCr to
> RGB conversions. My intention was to try to implement the plane
> specific (before DEGAMMA) part of the suggestion in this dri-devel
> post:
I would probably extend this series to have a bigger view. Instead of 
addressing RGB->YCBCR conversion, the actual target here should be 
blending of various framebuffers considering:
- their color spaces (Rec 601/709/2020)
- their format (YCBCR->RGB or RGB->YCBCR)
- their linear state (Linear/Non-linear) or if they are ready for Gamut 
mapping/color conversion or not.

Also, we need to have the sequence of properties, so that, it would 
match all(most of the) HWs.
I will add my comments in the upcoming patches accordingly.

- Shashank
> https://lists.freedesktop.org/archives/dri-devel/2017-March/135870.html
>
> This series may not be ready as such. At least the kernel doc parts
> should be more detailed and carefully written. The purpose is merely
> to move the discussion to a more concrete level.
>
> The series also includes drm/omap patches that implement the standard
> properties for OMAP DSS in omapdrm driver.
>
> Best regards,
> Jyri
>
> Jyri Sarha (4):
>    drm: drm_color_mgmt.h needs struct drm_crtc declaration
>    drm: Make drm_atomic_replace_property_blob_from_id() more generic
>    drm: Plane YCbCr to RGB conversion related properties
>    drm/omap: Enable ycbcr_to_rgb_properties for omapdrm planes REVISIT
>
> Tomi Valkeinen (2):
>    drm/omap: cleanup color space conversion
>    drm/omap: csc full range support
>
>   drivers/gpu/drm/drm_atomic.c          |  36 +++++++--
>   drivers/gpu/drm/drm_atomic_helper.c   |   9 +++
>   drivers/gpu/drm/drm_color_mgmt.c      | 136 +++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/drm_plane.c           |  10 +++
>   drivers/gpu/drm/omapdrm/dss/dispc.c   | 141 +++++++++++++++++++++++++++++-----
>   drivers/gpu/drm/omapdrm/dss/omapdss.h |  14 ++++
>   drivers/gpu/drm/omapdrm/omap_plane.c  |  41 ++++++++++
>   include/drm/drm_color_mgmt.h          |  25 ++++++
>   include/drm/drm_plane.h               |  10 +++
>   include/uapi/drm/drm_mode.h           |  12 +++
>   10 files changed, 408 insertions(+), 26 deletions(-)
>

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

Re: [PATCH RFC 0/6] drm: Add properties to control YCbCr to RGB conversion

[Liviu Dudau]

Hi Jyri,

On Fri, Apr 21, 2017 at 12:51:11PM +0300, Jyri Sarha wrote:
> The series adds plane specific atomic properties to control YCbCr to
> RGB conversions. My intention was to try to implement the plane
> specific (before DEGAMMA) part of the suggestion in this dri-devel
> post:
> 
> https://lists.freedesktop.org/archives/dri-devel/2017-March/135870.html
> 
> This series may not be ready as such. At least the kernel doc parts
> should be more detailed and carefully written. The purpose is merely
> to move the discussion to a more concrete level.
> 
> The series also includes drm/omap patches that implement the standard
> properties for OMAP DSS in omapdrm driver.

Thanks for re-starting this. The first two patches sort of matches what
I've had internally from Mihail when we starting the discussion in the
thread you mentioned. I will also try to port the mali-dp specific patches
on top of your series and provide feedback.

Best regards,
Liviu

> 
> Best regards,
> Jyri
> 
> Jyri Sarha (4):
>   drm: drm_color_mgmt.h needs struct drm_crtc declaration
>   drm: Make drm_atomic_replace_property_blob_from_id() more generic
>   drm: Plane YCbCr to RGB conversion related properties
>   drm/omap: Enable ycbcr_to_rgb_properties for omapdrm planes REVISIT
> 
> Tomi Valkeinen (2):
>   drm/omap: cleanup color space conversion
>   drm/omap: csc full range support
> 
>  drivers/gpu/drm/drm_atomic.c          |  36 +++++++--
>  drivers/gpu/drm/drm_atomic_helper.c   |   9 +++
>  drivers/gpu/drm/drm_color_mgmt.c      | 136 +++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_plane.c           |  10 +++
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 141 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  14 ++++
>  drivers/gpu/drm/omapdrm/omap_plane.c  |  41 ++++++++++
>  include/drm/drm_color_mgmt.h          |  25 ++++++
>  include/drm/drm_plane.h               |  10 +++
>  include/uapi/drm/drm_mode.h           |  12 +++
>  10 files changed, 408 insertions(+), 26 deletions(-)
> 
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for RFC: Design: DRM: Blending pipeline using DRM plane properties

[Patchwork]

== Series Details ==

Series: RFC: Design: DRM: Blending pipeline using DRM plane properties
URL   : https://patchwork.freedesktop.org/series/23443/
State : success

== Summary ==

Series 23443v1 RFC: Design: DRM: Blending pipeline using DRM plane properties
https://patchwork.freedesktop.org/api/1.0/series/23443/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:429s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:428s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:509s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:553s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:483s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:407s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:401s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:485s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:461s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:457s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:448s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:576s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:462s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:495s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:431s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:399s

bc781a3cf9a80e4c5ed0d47fd0c67923bcfcdf2c drm-tip: 2017y-04m-22d-08h-49m-55s UTC integration manifest
cc35e8d RFC: Design: DRM: Blending pipeline using DRM plane properties

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4534/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx