[PATCH 0/6] drm/exynos: mixer: Add color range property

[PATCH 0/6] drm/exynos: mixer: Add color range property

[Christoph Manszewski]

Hi,

This patch series adds rgb input buffer range property to exynos_mixer.
It applies on top of drm-exynos/exynos-drm-next with patches:
- [PATCH 1/2] drm/edid: Add colorimetry helper function,
- [PATCH 2/2] drm/exynos: mixer: Use colorimetry helper function

Patch 1 Add apropriate labels
Patch 2 Split color properties function
Patch 3 Add default range field to exynos_drm_plane config
Patch 4 Minor code cleanup
Patch 5 Add optional range property to exynos plane
Patch 6 Make input rgb range configurable in exynos_mixer

Regards,
Chris

Christoph Manszewski (6):
  include/drm: color_mgmt: Add enum labels
  drm: color_mgmt: Split create_color_properties function
  drm/exynos: drm_drv: Extend exynos_drm_plane_config
  drm/exynos: plane: Minor cleanup
  drm/exynos: plane: Add range property to exynos plane
  drm/exynos: mixer: Make input buffer color range configurable

 drivers/gpu/drm/drm_color_mgmt.c          | 93 ++++++++++++++++++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  2 +
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 15 ++++-
 drivers/gpu/drm/exynos/exynos_mixer.c     | 57 +++++++++++++++++--
 drivers/gpu/drm/exynos/regs-mixer.h       |  2 +
 include/drm/drm_color_mgmt.h              | 10 ++++
 6 files changed, 152 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 1/6] include/drm: color_mgmt: Add enum labels

[Christoph Manszewski]

Range setting makes sense for YCbCr and RGB buffers. Current
drm_color_range enum labels suggest use with YCbCr buffers.
Create enum labels without colorspace specification.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.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 90ef9996d9a4..52f6d5221a0d 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -62,6 +62,8 @@ enum drm_color_range {
 	DRM_COLOR_YCBCR_LIMITED_RANGE,
 	DRM_COLOR_YCBCR_FULL_RANGE,
 	DRM_COLOR_RANGE_MAX,
+	DRM_COLOR_LIMITED_RANGE = DRM_COLOR_YCBCR_LIMITED_RANGE,
+	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,
 };
 
 int drm_plane_create_color_properties(struct drm_plane *plane,
-- 
2.7.4

[PATCH 2/6] drm: color_mgmt: Split create_color_properties function

[Christoph Manszewski]

Split drm_plane_create_color_properties into two separate functions.
This allows to create range and encoding property independently.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 93 +++++++++++++++++++++++++++++++---------
 include/drm/drm_color_mgmt.h     |  8 ++++
 2 files changed, 81 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 581cc3788223..416cc9c8560b 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -396,29 +396,24 @@ const char *drm_get_color_range_name(enum drm_color_range range)
 }
 
 /**
- * drm_plane_create_color_properties - color encoding related plane properties
+ * drm_plane_create_encoding_property - color encoding related plane property
  * @plane: plane object
  * @supported_encodings: bitfield indicating supported color encodings
- * @supported_ranges: bitfileld indicating supported color ranges
  * @default_encoding: default color encoding
- * @default_range: default color range
  *
- * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
- * properties to @plane. The supported encodings and ranges should
- * be provided in supported_encodings and supported_ranges bitmasks.
- * Each bit set in the bitmask indicates that its number as enum
- * value is supported.
+ * Create and attach plane specific COLOR_ENCODING property to @plane.
+ * The supported encodings be provided in supported_encodings bitmask.
+ * Each bit set in the bitmask indicates that its number as enum value
+ * is supported.
  */
-int drm_plane_create_color_properties(struct drm_plane *plane,
-				      u32 supported_encodings,
-				      u32 supported_ranges,
-				      enum drm_color_encoding default_encoding,
-				      enum drm_color_range default_range)
+
+int drm_plane_create_encoding_property(struct drm_plane *plane,
+				       u32 supported_encodings,
+				       enum drm_color_encoding default_encoding)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_property *prop;
-	struct drm_prop_enum_list enum_list[max_t(int, DRM_COLOR_ENCODING_MAX,
-						       DRM_COLOR_RANGE_MAX)];
+	struct drm_prop_enum_list enum_list[DRM_COLOR_ENCODING_MAX];
 	int i, len;
 
 	if (WARN_ON(supported_encodings == 0 ||
@@ -426,11 +421,6 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 		    (supported_encodings & BIT(default_encoding)) == 0))
 		return -EINVAL;
 
-	if (WARN_ON(supported_ranges == 0 ||
-		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
-		    (supported_ranges & BIT(default_range)) == 0))
-		return -EINVAL;
-
 	len = 0;
 	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
 		if ((supported_encodings & BIT(i)) == 0)
@@ -450,6 +440,36 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 	if (plane->state)
 		plane->state->color_encoding = default_encoding;
 
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_encoding_property);
+
+/**
+ * drm_plane_create_range_property - color range related plane property
+ * @plane: plane object
+ * @supported_ranges: bitfileld indicating supported color ranges
+ * @default_range: default color range
+ *
+ * Create and attach plane specific COLOR_RANGE property to @plane.
+ * The supported ranges should be provided in supported_ranges bitmask.
+ * Each bit set in the bitmask indicates that its number as enum value
+ * is supported.
+ */
+
+int drm_plane_create_range_property(struct drm_plane *plane,
+				    u32 supported_ranges,
+				    enum drm_color_range default_range)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_property *prop;
+	struct drm_prop_enum_list enum_list[DRM_COLOR_RANGE_MAX];
+	int i, len;
+
+	if (WARN_ON(supported_ranges == 0 ||
+		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
+		    (supported_ranges & BIT(default_range)) == 0))
+		return -EINVAL;
+
 	len = 0;
 	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
 		if ((supported_ranges & BIT(i)) == 0)
@@ -471,4 +491,37 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_plane_create_range_property);
+
+/**
+ * drm_plane_create_color_properties - color encoding related plane properties
+ * @plane: plane object
+ * @supported_encodings: bitfield indicating supported color encodings
+ * @supported_ranges: bitfileld indicating supported color ranges
+ * @default_encoding: default color encoding
+ * @default_range: default color range
+ *
+ * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
+ * properties to @plane. The supported encodings and ranges should
+ * be provided in supported_encodings and supported_ranges bitmasks.
+ * Each bit set in the bitmask indicates that its number as enum
+ * value is supported.
+ */
+
+int drm_plane_create_color_properties(struct drm_plane *plane,
+				      u32 supported_encodings,
+				      u32 supported_ranges,
+				      enum drm_color_encoding default_encoding,
+				      enum drm_color_range default_range)
+{
+	int ret;
+
+	ret = drm_plane_create_encoding_property(plane, supported_encodings,
+						 default_encoding);
+	if (ret)
+		return ret;
+
+	return drm_plane_create_range_property(plane, supported_ranges,
+					       default_range);
+}
 EXPORT_SYMBOL(drm_plane_create_color_properties);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 52f6d5221a0d..fca8d3fabcd9 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -66,6 +66,14 @@ enum drm_color_range {
 	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,
 };
 
+int drm_plane_create_encoding_property(struct drm_plane *plane,
+				       u32 supported_encodings,
+				       enum drm_color_encoding default_encoding);
+
+int drm_plane_create_range_property(struct drm_plane *plane,
+				    u32 supported_ranges,
+				    enum drm_color_range default_range);
+
 int drm_plane_create_color_properties(struct drm_plane *plane,
 				      u32 supported_encodings,
 				      u32 supported_ranges,
-- 
2.7.4

[PATCH 3/6] drm/exynos: drm_drv: Extend exynos_drm_plane_config

[Christoph Manszewski]

Define new range capability and add range value to
exynos_drm_plane_config.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 5e61e707f955..1bc334d846c5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -94,6 +94,7 @@ struct exynos_drm_plane {
 #define EXYNOS_DRM_PLANE_CAP_TILE	(1 << 3)
 #define EXYNOS_DRM_PLANE_CAP_PIX_BLEND	(1 << 4)
 #define EXYNOS_DRM_PLANE_CAP_WIN_BLEND	(1 << 5)
+#define EXYNOS_DRM_PLANE_CAP_RANGE	(1 << 6)
 
 /*
  * Exynos DRM plane configuration structure.
@@ -111,6 +112,7 @@ struct exynos_drm_plane_config {
 	const uint32_t *pixel_formats;
 	unsigned int num_pixel_formats;
 	unsigned int capabilities;
+	enum drm_color_range range;
 };
 
 /*
-- 
2.7.4

[PATCH 4/6] drm/exynos: plane: Minor cleanup

[Christoph Manszewski]

Use available drm_plane pointer, for consistency.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index df0508e0e49e..cf352d980eee 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -303,7 +303,7 @@ int exynos_plane_init(struct drm_device *dev,
 				       BIT(DRM_MODE_BLEND_COVERAGE);
 	struct drm_plane *plane = &exynos_plane->base;
 
-	err = drm_universal_plane_init(dev, &exynos_plane->base,
+	err = drm_universal_plane_init(dev, plane,
 				       1 << dev->mode_config.num_crtc,
 				       &exynos_plane_funcs,
 				       config->pixel_formats,
@@ -314,12 +314,12 @@ int exynos_plane_init(struct drm_device *dev,
 		return err;
 	}
 
-	drm_plane_helper_add(&exynos_plane->base, &plane_helper_funcs);
+	drm_plane_helper_add(plane, &plane_helper_funcs);
 
 	exynos_plane->index = index;
 	exynos_plane->config = config;
 
-	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos,
+	exynos_plane_attach_zpos_property(plane, config->zpos,
 			   !(config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS));
 
 	if (config->capabilities & EXYNOS_DRM_PLANE_CAP_PIX_BLEND)
-- 
2.7.4

[PATCH 5/6] drm/exynos: plane: Add range property to exynos plane

[Christoph Manszewski]

Create range property within exynos_drm_plane_init and set to default value
in exynos_drm_plane_reset.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index cf352d980eee..a6ab0e7277f0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -140,6 +140,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
 	if (exynos_state) {
 		__drm_atomic_helper_plane_reset(plane, &exynos_state->base);
 		plane->state->zpos = exynos_plane->config->zpos;
+		plane->state->color_range = exynos_plane->config->range;
 	}
 }
 
@@ -301,6 +302,10 @@ int exynos_plane_init(struct drm_device *dev,
 	unsigned int supported_modes = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
 				       BIT(DRM_MODE_BLEND_PREMULTI) |
 				       BIT(DRM_MODE_BLEND_COVERAGE);
+
+	u32 supported_ranges = BIT(DRM_COLOR_LIMITED_RANGE) |
+			       BIT(DRM_COLOR_FULL_RANGE);
+
 	struct drm_plane *plane = &exynos_plane->base;
 
 	err = drm_universal_plane_init(dev, plane,
@@ -328,5 +333,9 @@ int exynos_plane_init(struct drm_device *dev,
 	if (config->capabilities & EXYNOS_DRM_PLANE_CAP_WIN_BLEND)
 		drm_plane_create_alpha_property(plane);
 
+	if (config->capabilities & EXYNOS_DRM_PLANE_CAP_RANGE)
+		drm_plane_create_range_property(plane, supported_ranges,
+						config->range);
+
 	return 0;
 }
-- 
2.7.4

[PATCH 6/6] drm/exynos: mixer: Make input buffer color range configurable

[Christoph Manszewski]

Mixer hardware supports RGB input buffers with full and limited range.
Set the csc matrix accordingly to chosen range. Note that range setting
has to be equal for both graphic layers.

Signed-off-by: Christoph Manszewski <c.manszewski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 57 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/exynos/regs-mixer.h   |  2 ++
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 895c6268025d..035d635188d7 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -35,6 +35,8 @@
 #include <linux/component.h>
 
 #include <drm/exynos_drm.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_crtc.h"
@@ -133,7 +135,9 @@ static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
 		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
 				EXYNOS_DRM_PLANE_CAP_ZPOS |
 				EXYNOS_DRM_PLANE_CAP_PIX_BLEND |
-				EXYNOS_DRM_PLANE_CAP_WIN_BLEND,
+				EXYNOS_DRM_PLANE_CAP_WIN_BLEND |
+				EXYNOS_DRM_PLANE_CAP_RANGE,
+		.range = DRM_COLOR_FULL_RANGE,
 	}, {
 		.zpos = 1,
 		.type = DRM_PLANE_TYPE_CURSOR,
@@ -142,7 +146,9 @@ static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
 		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
 				EXYNOS_DRM_PLANE_CAP_ZPOS |
 				EXYNOS_DRM_PLANE_CAP_PIX_BLEND |
-				EXYNOS_DRM_PLANE_CAP_WIN_BLEND,
+				EXYNOS_DRM_PLANE_CAP_WIN_BLEND |
+				EXYNOS_DRM_PLANE_CAP_RANGE,
+		.range = DRM_COLOR_FULL_RANGE,
 	}, {
 		.zpos = 2,
 		.type = DRM_PLANE_TYPE_OVERLAY,
@@ -389,13 +395,19 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, struct drm_display_mode
 
 	if (format == HDMI_COLORIMETRY_ITU_601) {
 		val = MXR_CFG_RGB601;
+
+		mixer_reg_write(ctx, MXR_CM_COEFF_Y,
+			MXR_CSC_CT( 0.257,  0.504,  0.098));
+		mixer_reg_write(ctx, MXR_CM_COEFF_CB,
+			MXR_CSC_CT(-0.148, -0.291,  0.439));
+		mixer_reg_write(ctx, MXR_CM_COEFF_CR,
+			MXR_CSC_CT( 0.439, -0.368, -0.071));
 	} else {
 		val = MXR_CFG_RGB709;
 
 		/* Configure the BT.709 CSC matrix for full range RGB. */
 		mixer_reg_write(ctx, MXR_CM_COEFF_Y,
-			MXR_CSC_CT( 0.184,  0.614,  0.063) |
-			MXR_CM_COEFF_RGB_FULL);
+			MXR_CSC_CT( 0.184,  0.614,  0.063));
 		mixer_reg_write(ctx, MXR_CM_COEFF_CB,
 			MXR_CSC_CT(-0.102, -0.338,  0.440));
 		mixer_reg_write(ctx, MXR_CM_COEFF_CR,
@@ -560,6 +572,20 @@ static void mixer_layer_update(struct mixer_context *ctx)
 	mixer_reg_writemask(ctx, MXR_CFG, ~0, MXR_CFG_LAYER_UPDATE);
 }
 
+static void mixer_set_input_buffer_range(struct mixer_context *ctx,
+					 struct exynos_drm_plane *plane)
+{
+	u32 cm_coeff;
+
+	if (plane->base.state->color_range == DRM_COLOR_LIMITED_RANGE)
+		cm_coeff = MXR_CM_COEFF_RGB_LIMITED;
+	else
+		cm_coeff = MXR_CM_COEFF_RGB_FULL;
+
+	mixer_reg_writemask(ctx, MXR_CM_COEFF_Y, cm_coeff,
+			    MXR_CM_COEFF_RGB_RANGE_MASK);
+}
+
 static void mixer_graph_buffer(struct mixer_context *ctx,
 			       struct exynos_drm_plane *plane)
 {
@@ -647,6 +673,9 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 		ctx->mxr_ver == MXR_VER_128_0_0_184)
 		mixer_layer_update(ctx);
 
+	/* set the input buffer rgb range */
+	mixer_set_input_buffer_range(ctx, plane);
+
 	spin_unlock_irqrestore(&ctx->reg_slock, flags);
 
 	mixer_regs_dump(ctx);
@@ -917,6 +946,25 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
 }
 
+static int mixer_atomic_check(struct exynos_drm_crtc *crtc,
+			      struct drm_crtc_state *state)
+{
+	const struct drm_plane_state *pstate;
+	enum drm_color_range range_val[2];
+	struct drm_plane *plane;
+	unsigned int cnt = 0;
+
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
+		if (plane->color_range_property && cnt < 2) {
+			range_val[cnt] = pstate->color_range;
+			++cnt;
+		}
+	}
+	if (cnt == 2 && range_val[0] != range_val[1])
+		return -EINVAL;
+	return 0;
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *mixer_ctx = crtc->ctx;
@@ -1096,6 +1144,7 @@ static const struct exynos_drm_crtc_ops mixer_crtc_ops = {
 	.disable		= mixer_disable,
 	.enable_vblank		= mixer_enable_vblank,
 	.disable_vblank		= mixer_disable_vblank,
+	.atomic_check		= mixer_atomic_check,
 	.atomic_begin		= mixer_atomic_begin,
 	.update_plane		= mixer_update_plane,
 	.disable_plane		= mixer_disable_plane,
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index 5ff095b0c1b3..7d558e3dd46c 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -158,7 +158,9 @@
 #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
 
 /* bits for MXR_CM_COEFF_Y */
+#define MXR_CM_COEFF_RGB_LIMITED	(0 << 30)
 #define MXR_CM_COEFF_RGB_FULL		(1 << 30)
+#define MXR_CM_COEFF_RGB_RANGE_MASK	(1 << 30)
 
 #endif /* SAMSUNG_REGS_MIXER_H */
 
-- 
2.7.4

Re: [PATCH 1/6] include/drm: color_mgmt: Add enum labels

[Inki Dae]

Hi,

18. 12. 14. 오후 9:10에 Christoph Manszewski 이(가) 쓴 글:
> Range setting makes sense for YCbCr and RGB buffers. Current
> drm_color_range enum labels suggest use with YCbCr buffers.
> Create enum labels without colorspace specification.
> 
> Signed-off-by: Christoph Manszewski <c.manszewski@samsung.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 90ef9996d9a4..52f6d5221a0d 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -62,6 +62,8 @@ enum drm_color_range {
>  	DRM_COLOR_YCBCR_LIMITED_RANGE,
>  	DRM_COLOR_YCBCR_FULL_RANGE,
>  	DRM_COLOR_RANGE_MAX,
> +	DRM_COLOR_LIMITED_RANGE = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,

I don't see why above two types are needed. Is there any case that the range checking behavior should be different according to the color space?
And even if there is that case, I think you have to put DRM_COLOR_RANG_MAX under DRM_COLOR_FULL_RANGE.

Thanks,
Inki Dae

>  };
>  
>  int drm_plane_create_color_properties(struct drm_plane *plane,
> 

Re: [PATCH 1/6] include/drm: color_mgmt: Add enum labels

[Ville Syrjälä]

On Fri, Dec 14, 2018 at 01:10:16PM +0100, Christoph Manszewski wrote:
> Range setting makes sense for YCbCr and RGB buffers. Current
> drm_color_range enum labels suggest use with YCbCr buffers.
> Create enum labels without colorspace specification.
> 
> Signed-off-by: Christoph Manszewski <c.manszewski@samsung.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 90ef9996d9a4..52f6d5221a0d 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -62,6 +62,8 @@ enum drm_color_range {
>  	DRM_COLOR_YCBCR_LIMITED_RANGE,
>  	DRM_COLOR_YCBCR_FULL_RANGE,
>  	DRM_COLOR_RANGE_MAX,
> +	DRM_COLOR_LIMITED_RANGE = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,

These enum values don't mean anything really. The strings are what
matter for the uapi.

The default for YCbCr needs to be limited range, the default for RGB
needs to be full range. So I think this would really require a separate
prop for each.

But is there an actual usecase for this stuff?

>  };
>  
>  int drm_plane_create_color_properties(struct drm_plane *plane,
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] include/drm: color_mgmt: Add enum labels

[Andrzej Hajda]

+CC: Hans

On 17.01.2019 20:47, Ville Syrjälä wrote:
> On Fri, Dec 14, 2018 at 01:10:16PM +0100, Christoph Manszewski wrote:
>> Range setting makes sense for YCbCr and RGB buffers. Current
>> drm_color_range enum labels suggest use with YCbCr buffers.
>> Create enum labels without colorspace specification.
>>
>> Signed-off-by: Christoph Manszewski <c.manszewski@samsung.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 90ef9996d9a4..52f6d5221a0d 100644
>> --- a/include/drm/drm_color_mgmt.h
>> +++ b/include/drm/drm_color_mgmt.h
>> @@ -62,6 +62,8 @@ enum drm_color_range {
>>  	DRM_COLOR_YCBCR_LIMITED_RANGE,
>>  	DRM_COLOR_YCBCR_FULL_RANGE,
>>  	DRM_COLOR_RANGE_MAX,
>> +	DRM_COLOR_LIMITED_RANGE = DRM_COLOR_YCBCR_LIMITED_RANGE,
>> +	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,
> These enum values don't mean anything really. The strings are what
> matter for the uapi.
>
> The default for YCbCr needs to be limited range, the default for RGB
> needs to be full range. So I think this would really require a separate
> prop for each.
>
> But is there an actual usecase for this stuff?


Yes, for example HDMI path (Display Controller and HDMI encoder) in
Exynos5433 works on RGB buffers and do not perform any conversion if the
output is also RGB, on the other side CEA861 specs says that in case of
CEA modes output RGB should be in limited range, in case of non CEA
modes it should be full range.

The idea was to allow userspace to check (atomic_check) which ranges are
acceptable in given mode and to set desired one if possible.

Any better solution is welcome?


Regards

Andrzej



>
>>  };
>>  
>>  int drm_plane_create_color_properties(struct drm_plane *plane,
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] include/drm: color_mgmt: Add enum labels

[Ville Syrjälä]

On Fri, Jan 18, 2019 at 03:34:18PM +0100, Andrzej Hajda wrote:
> +CC: Hans
> 
> On 17.01.2019 20:47, Ville Syrjälä wrote:
> > On Fri, Dec 14, 2018 at 01:10:16PM +0100, Christoph Manszewski wrote:
> >> Range setting makes sense for YCbCr and RGB buffers. Current
> >> drm_color_range enum labels suggest use with YCbCr buffers.
> >> Create enum labels without colorspace specification.
> >>
> >> Signed-off-by: Christoph Manszewski <c.manszewski@samsung.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 90ef9996d9a4..52f6d5221a0d 100644
> >> --- a/include/drm/drm_color_mgmt.h
> >> +++ b/include/drm/drm_color_mgmt.h
> >> @@ -62,6 +62,8 @@ enum drm_color_range {
> >>  	DRM_COLOR_YCBCR_LIMITED_RANGE,
> >>  	DRM_COLOR_YCBCR_FULL_RANGE,
> >>  	DRM_COLOR_RANGE_MAX,
> >> +	DRM_COLOR_LIMITED_RANGE = DRM_COLOR_YCBCR_LIMITED_RANGE,
> >> +	DRM_COLOR_FULL_RANGE = DRM_COLOR_YCBCR_FULL_RANGE,
> > These enum values don't mean anything really. The strings are what
> > matter for the uapi.
> >
> > The default for YCbCr needs to be limited range, the default for RGB
> > needs to be full range. So I think this would really require a separate
> > prop for each.
> >
> > But is there an actual usecase for this stuff?
> 
> 
> Yes, for example HDMI path (Display Controller and HDMI encoder) in
> Exynos5433 works on RGB buffers and do not perform any conversion if the
> output is also RGB, on the other side CEA861 specs says that in case of
> CEA modes output RGB should be in limited range, in case of non CEA
> modes it should be full range.
> 
> The idea was to allow userspace to check (atomic_check) which ranges are
> acceptable in given mode and to set desired one if possible.
> 
> Any better solution is welcome?

i915 has the "Broadcast RGB" property for the CEA-861 stuff.
You don't have a way to implement that?

-- 
Ville Syrjälä
Intel