[PATCH 0/7] drm/omap: misc improvements

[PATCH 0/7] drm/omap: misc improvements

[Tomi Valkeinen]

Hi,

Misc improvements to omapdrm which have been lying around for a long
time, and I've missed upstreaming them.

And one new one, which makes the DSS5 HDMI pick up the color range
automatically.

 Tomi

Alejandro Hernandez (1):
  drm/omap: tweak HDMI DDC timings

Jyri Sarha (3):
  drm/omap: Implement CTM property for CRTC using OVL managers CPR
    matrix
  drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes
  drm/omap: dss: platform_register_drivers() to dss.c and remove core.c

Tomi Valkeinen (3):
  drm/omap: drop unneeded locking from mgr_fld_write()
  drm/omap: fix missing scaler pixel fmt limitations
  drm/omap: hdmi5: automatically choose limited/full range output

 drivers/gpu/drm/omapdrm/dss/Makefile     |   2 +-
 drivers/gpu/drm/omapdrm/dss/core.c       |  55 --------
 drivers/gpu/drm/omapdrm/dss/dispc.c      | 160 +++++++++++++++++------
 drivers/gpu/drm/omapdrm/dss/dss.c        |  37 ++++++
 drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 125 ++++++++++--------
 drivers/gpu/drm/omapdrm/dss/omapdss.h    |   4 +
 drivers/gpu/drm/omapdrm/omap_crtc.c      |  39 +++++-
 drivers/gpu/drm/omapdrm/omap_plane.c     |  30 +++++
 8 files changed, 295 insertions(+), 157 deletions(-)
 delete mode 100644 drivers/gpu/drm/omapdrm/dss/core.c

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 1/7] drm/omap: drop unneeded locking from mgr_fld_write()

[Tomi Valkeinen]

Commit d49cd15550d9d4495f6187425318c245d58cb63f ("OMAPDSS: DISPC: lock
access to DISPC_CONTROL & DISPC_CONFIG") added locking to
mgr_fld_write(). This was needed in omapfb times due to lack of good
locking, especially in the case of both V4L2 and fbdev layers using the
DSS driver.

This is not needed for omapdrm, so we can remove the locking.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 785c5546067a..c6da33e7014f 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -184,9 +184,6 @@ struct dispc_device {
 
 	struct regmap *syscon_pol;
 	u32 syscon_pol_offset;
-
-	/* DISPC_CONTROL & DISPC_CONFIG lock*/
-	spinlock_t control_lock;
 };
 
 enum omap_color_component {
@@ -377,16 +374,7 @@ static void mgr_fld_write(struct dispc_device *dispc, enum omap_channel channel,
 			  enum mgr_reg_fields regfld, int val)
 {
 	const struct dispc_reg_field rfld = mgr_desc[channel].reg_desc[regfld];
-	const bool need_lock = rfld.reg == DISPC_CONTROL || rfld.reg == DISPC_CONFIG;
-	unsigned long flags;
-
-	if (need_lock) {
-		spin_lock_irqsave(&dispc->control_lock, flags);
-		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
-		spin_unlock_irqrestore(&dispc->control_lock, flags);
-	} else {
-		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
-	}
+	REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
 }
 
 static int dispc_get_num_ovls(struct dispc_device *dispc)
@@ -4769,8 +4757,6 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	platform_set_drvdata(pdev, dispc);
 	dispc->dss = dss;
 
-	spin_lock_init(&dispc->control_lock);
-
 	/*
 	 * The OMAP3-based models can't be told apart using the compatible
 	 * string, use SoC device matching.
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 2/7] drm/omap: tweak HDMI DDC timings

[Tomi Valkeinen]

From: Alejandro Hernandez <ajhernandez@ti.com>

A "HDMI I2C Master Error" is sometimes reported with the current DDC SCL
timings. The current settings for a 10us SCL period (100 KHz) causes the
error with some displays.  This patch increases the SCL signal period
from 10us to 10.2us, with the new settings the error is not observed

Signed-off-by: Alejandro Hernandez <ajhernandez@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
index 7400fb99d453..4c588ec7634a 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
@@ -39,8 +39,8 @@ static void hdmi_core_ddc_init(struct hdmi_core_data *core)
 {
 	void __iomem *base = core->base;
 	const unsigned long long iclk = 266000000;	/* DSS L3 ICLK */
-	const unsigned int ss_scl_high = 4600;		/* ns */
-	const unsigned int ss_scl_low = 5400;		/* ns */
+	const unsigned int ss_scl_high = 4700;		/* ns */
+	const unsigned int ss_scl_low = 5500;		/* ns */
 	const unsigned int fs_scl_high = 600;		/* ns */
 	const unsigned int fs_scl_low = 1300;		/* ns */
 	const unsigned int sda_hold = 1000;		/* ns */
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 3/7] drm/omap: fix missing scaler pixel fmt limitations

[Tomi Valkeinen]

OMAP2 and OMAP3/AM4 have limitations with the scaler:
- OMAP2 can only scale XRGB8888
- OMAP3/AM4 can only scale XRGB8888, RGB565, YUYV and UYVY

The driver doesn't check these limitations, which leads to sync-lost
floods.

This patch adds a check for the pixel formats when scaling.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index c6da33e7014f..7d87d60edb80 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -114,6 +114,7 @@ struct dispc_features {
 	const unsigned int num_reg_fields;
 	const enum omap_overlay_caps *overlay_caps;
 	const u32 **supported_color_modes;
+	const u32 *supported_scaler_color_modes;
 	unsigned int num_mgrs;
 	unsigned int num_ovls;
 	unsigned int buffer_size_unit;
@@ -2498,6 +2499,19 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc,
 	if (width == out_width && height == out_height)
 		return 0;
 
+	if (dispc->feat->supported_scaler_color_modes) {
+		const u32 *modes = dispc->feat->supported_scaler_color_modes;
+		int i;
+
+		for (i = 0; modes[i]; ++i) {
+			if (modes[i] == fourcc)
+				break;
+		}
+
+		if (modes[i] == 0)
+			return -EINVAL;
+	}
+
 	if (plane == OMAP_DSS_WB) {
 		switch (fourcc) {
 		case DRM_FORMAT_NV12:
@@ -4213,6 +4227,12 @@ static const u32 *omap4_dispc_supported_color_modes[] = {
 	DRM_FORMAT_RGBX8888),
 };
 
+static const u32 omap3_dispc_supported_scaler_color_modes[] = {
+	DRM_FORMAT_XRGB8888, DRM_FORMAT_RGB565, DRM_FORMAT_YUYV,
+	DRM_FORMAT_UYVY,
+	0,
+};
+
 static const struct dispc_features omap24xx_dispc_feats = {
 	.sw_start		=	5,
 	.fp_start		=	15,
@@ -4241,6 +4261,7 @@ static const struct dispc_features omap24xx_dispc_feats = {
 	.num_reg_fields		=	ARRAY_SIZE(omap2_dispc_reg_fields),
 	.overlay_caps		=	omap2_dispc_overlay_caps,
 	.supported_color_modes	=	omap2_dispc_supported_color_modes,
+	.supported_scaler_color_modes = COLOR_ARRAY(DRM_FORMAT_XRGB8888),
 	.num_mgrs		=	2,
 	.num_ovls		=	3,
 	.buffer_size_unit	=	1,
@@ -4275,6 +4296,7 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = {
 	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
 	.overlay_caps		=	omap3430_dispc_overlay_caps,
 	.supported_color_modes	=	omap3_dispc_supported_color_modes,
+	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
 	.num_mgrs		=	2,
 	.num_ovls		=	3,
 	.buffer_size_unit	=	1,
@@ -4309,6 +4331,7 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = {
 	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
 	.overlay_caps		=	omap3430_dispc_overlay_caps,
 	.supported_color_modes	=	omap3_dispc_supported_color_modes,
+	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
 	.num_mgrs		=	2,
 	.num_ovls		=	3,
 	.buffer_size_unit	=	1,
@@ -4343,6 +4366,7 @@ static const struct dispc_features omap36xx_dispc_feats = {
 	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
 	.overlay_caps		=	omap3630_dispc_overlay_caps,
 	.supported_color_modes	=	omap3_dispc_supported_color_modes,
+	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
 	.num_mgrs		=	2,
 	.num_ovls		=	3,
 	.buffer_size_unit	=	1,
@@ -4377,6 +4401,7 @@ static const struct dispc_features am43xx_dispc_feats = {
 	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
 	.overlay_caps		=	omap3430_dispc_overlay_caps,
 	.supported_color_modes	=	omap3_dispc_supported_color_modes,
+	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
 	.num_mgrs		=	1,
 	.num_ovls		=	3,
 	.buffer_size_unit	=	1,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Implement CTM color management property for OMAP CRTC using DSS
overlay manager's Color Phase Rotation matrix. The CPR matrix does not
exactly match the CTM property documentation. On DSS the CPR matrix is
applied after gamma table look up. However, it seems stupid to add a
custom property just for that.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 3c5ddbf30e97..d63213dd7d83 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
 	}
 }
 
+static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
+{
+	uint64_t sign_bit = 1ULL << 63;
+	uint64_t cbits = (uint64_t) coef;
+	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
+
+	if (cbits & sign_bit)
+		ret = -ret;
+
+	return ret;
+}
+
+static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
+					 struct omap_dss_cpr_coefs *cpr)
+{
+	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
+	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
+	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
+	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
+	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
+	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
+	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
+	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
+	cpr->bb = omap_crtc_S31_32_to_s2_8(ctm->matrix[8]);
+}
+
 static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
 {
 	struct omap_drm_private *priv = crtc->dev->dev_private;
@@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
 	info.default_color = 0x000000;
 	info.trans_enabled = false;
 	info.partial_alpha_enabled = false;
-	info.cpr_enable = false;
+
+	if (crtc->state->ctm) {
+		struct drm_color_ctm *ctm =
+			(struct drm_color_ctm *) crtc->state->ctm->data;
+
+		info.cpr_enable = true;
+		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
+	} else {
+		info.cpr_enable = false;
+	}
 
 	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
 }
@@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
 		unsigned int gamma_lut_size = 256;
 
-		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
+		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
 		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 5/7] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Adds support for COLOR_ENCODING and COLOR_RANGE properties to
omap_plane.c and dispc.c. The supported encodings and ranges are:

For COLOR_ENCODING:
- YCbCt BT.601 (default)
- YCbCt BT.709

For COLOR_RANGE:
- YCbCt limited range
- YCbCt full range (default)

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 119 ++++++++++++++++++++------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
 drivers/gpu/drm/omapdrm/omap_plane.c  |  30 +++++++
 3 files changed, 127 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 7d87d60edb80..40ddb28ee7aa 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
 #undef CVAL
 }
 
-static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
+/* 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, full range */
+const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
+	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
+	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
+	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
+	true,                   /* full 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 static 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 static 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 int dispc_ovl_set_csc(struct dispc_device *dispc,
+			     enum omap_plane_id plane,
+			     enum drm_color_encoding color_encoding,
+			     enum drm_color_range color_range)
 {
-	int i;
-	int num_ovl = dispc_get_num_ovls(dispc);
-
-	/* 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 csc_coef_yuv2rgb *csc;
 
-	/* 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 */
-	};
+	switch (color_encoding) {
+	case DRM_COLOR_YCBCR_BT601:
+		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+			csc = &coefs_yuv2rgb_bt601_full;
+		else
+			csc = &coefs_yuv2rgb_bt601_lim;
+		break;
+	case DRM_COLOR_YCBCR_BT709:
+		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+			csc = &coefs_yuv2rgb_bt709_full;
+		else
+			csc = &coefs_yuv2rgb_bt709_lim;
+		break;
+	default:
+		DSSERR("Unsupported CSC mode %d for plane %d\n",
+		       color_encoding, plane);
+		return -EINVAL;
+	}
 
-	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
+	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
 
-	if (dispc->feat->has_writeback)
-		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
+	return 0;
 }
 
 static void dispc_ovl_set_ba0(struct dispc_device *dispc,
@@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
 				  u8 pre_mult_alpha, u8 global_alpha,
 				  enum omap_dss_rotation_type rotation_type,
 				  bool replication, const struct videomode *vm,
-				  bool mem_to_mem)
+				  bool mem_to_mem,
+				  enum drm_color_encoding color_encoding,
+				  enum drm_color_range color_range)
 {
 	bool five_taps = true;
 	bool fieldmode = false;
@@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
 				      fieldmode, fourcc, rotation);
 		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
 		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
+
+		if (plane != OMAP_DSS_WB)
+			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
 	}
 
 	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
@@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
 		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
 		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
 		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
-		oi->rotation_type, replication, vm, mem_to_mem);
+		oi->rotation_type, replication, vm, mem_to_mem,
+		oi->color_encoding, oi->color_range);
 
 	return r;
 }
@@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
 		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
 		wi->height, wi->fourcc, wi->rotation, zorder,
 		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
-		replication, vm, mem_to_mem);
+		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
+		DRM_COLOR_YCBCR_LIMITED_RANGE);
 	if (r)
 		return r;
 
@@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
 	    dispc->feat->has_gamma_table)
 		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
 
-	dispc_setup_color_conv_coef(dispc);
+	if (dispc->feat->has_writeback)
+		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
 
 	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 79f6b195c7cf..1430cab6b877 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -14,6 +14,7 @@
 #include <linux/platform_data/omapdss.h>
 #include <uapi/drm/drm_mode.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_color_mgmt.h>
 
 #define DISPC_IRQ_FRAMEDONE		(1 << 0)
 #define DISPC_IRQ_VSYNC			(1 << 1)
@@ -243,6 +244,9 @@ struct omap_overlay_info {
 	u8 global_alpha;
 	u8 pre_mult_alpha;
 	u8 zorder;
+
+	enum drm_color_encoding color_encoding;
+	enum drm_color_range color_range;
 };
 
 struct omap_overlay_manager_info {
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 73ec99819a3d..db8e917260df 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 		info.pre_mult_alpha = 1;
 	else
 		info.pre_mult_alpha = 0;
+	info.color_encoding = state->color_encoding;
+	info.color_range = state->color_range;
 
 	/* update scanout: */
 	omap_framebuffer_update_scanout(state->fb, state, &info);
@@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
 	 */
 	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
 			   ? 0 : omap_plane->id;
+	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
+	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
 }
 
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
@@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
 	.atomic_get_property = omap_plane_atomic_get_property,
 };
 
+static bool omap_plane_supports_yuv(struct drm_plane *plane)
+{
+	struct omap_drm_private *priv = plane->dev->dev_private;
+	struct omap_plane *omap_plane = to_omap_plane(plane);
+	const u32 *formats =
+		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
+	int i;
+
+	for (i = 0; formats[i]; i++)
+		if (formats[i] == DRM_FORMAT_YUYV ||
+		    formats[i] == DRM_FORMAT_UYVY ||
+		    formats[i] == DRM_FORMAT_NV12)
+			return true;
+
+	return false;
+}
+
 static const char *plane_id_to_name[] = {
 	[OMAP_DSS_GFX] = "gfx",
 	[OMAP_DSS_VIDEO1] = "vid1",
@@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
 					     BIT(DRM_MODE_BLEND_COVERAGE));
 
+	if (omap_plane_supports_yuv(plane))
+		drm_plane_create_color_properties(plane,
+					BIT(DRM_COLOR_YCBCR_BT601) |
+					BIT(DRM_COLOR_YCBCR_BT709),
+					BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
+					BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+					DRM_COLOR_YCBCR_BT601,
+					DRM_COLOR_YCBCR_FULL_RANGE);
+
 	return plane;
 
 error:
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 6/7] drm/omap: dss: platform_register_drivers() to dss.c and remove core.c

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

The core.c just for registering the drivers is kind of useless. Let's
get rid of it and register the dss drivers in dss.c.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/Makefile |  2 +-
 drivers/gpu/drm/omapdrm/dss/core.c   | 55 ----------------------------
 drivers/gpu/drm/omapdrm/dss/dss.c    | 37 +++++++++++++++++++
 3 files changed, 38 insertions(+), 56 deletions(-)
 delete mode 100644 drivers/gpu/drm/omapdrm/dss/core.c

diff --git a/drivers/gpu/drm/omapdrm/dss/Makefile b/drivers/gpu/drm/omapdrm/dss/Makefile
index 904101c5e79d..5950c3f52c2e 100644
--- a/drivers/gpu/drm/omapdrm/dss/Makefile
+++ b/drivers/gpu/drm/omapdrm/dss/Makefile
@@ -6,7 +6,7 @@ omapdss-base-y := base.o display.o dss-of.o output.o
 
 obj-$(CONFIG_OMAP2_DSS) += omapdss.o
 # Core DSS files
-omapdss-y := core.o dss.o dispc.o dispc_coefs.o \
+omapdss-y := dss.o dispc.o dispc_coefs.o \
 	pll.o video-pll.o
 omapdss-$(CONFIG_OMAP2_DSS_DPI) += dpi.o
 omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o
diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c
deleted file mode 100644
index 6ac497b63711..000000000000
--- a/drivers/gpu/drm/omapdrm/dss/core.c
+++ /dev/null
@@ -1,55 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (C) 2009 Nokia Corporation
- * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
- *
- * Some code and ideas taken from drivers/video/omap/ driver
- * by Imre Deak.
- */
-
-#define DSS_SUBSYS_NAME "CORE"
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-
-#include "omapdss.h"
-#include "dss.h"
-
-/* INIT */
-static struct platform_driver * const omap_dss_drivers[] = {
-	&omap_dsshw_driver,
-	&omap_dispchw_driver,
-#ifdef CONFIG_OMAP2_DSS_DSI
-	&omap_dsihw_driver,
-#endif
-#ifdef CONFIG_OMAP2_DSS_VENC
-	&omap_venchw_driver,
-#endif
-#ifdef CONFIG_OMAP4_DSS_HDMI
-	&omapdss_hdmi4hw_driver,
-#endif
-#ifdef CONFIG_OMAP5_DSS_HDMI
-	&omapdss_hdmi5hw_driver,
-#endif
-};
-
-static int __init omap_dss_init(void)
-{
-	return platform_register_drivers(omap_dss_drivers,
-					 ARRAY_SIZE(omap_dss_drivers));
-}
-
-static void __exit omap_dss_exit(void)
-{
-	platform_unregister_drivers(omap_dss_drivers,
-				    ARRAY_SIZE(omap_dss_drivers));
-}
-
-module_init(omap_dss_init);
-module_exit(omap_dss_exit);
-
-MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
-MODULE_DESCRIPTION("OMAP2/3 Display Subsystem");
-MODULE_LICENSE("GPL v2");
-
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
index e226324adb69..41d495a360d8 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1598,3 +1598,40 @@ struct platform_driver omap_dsshw_driver = {
 		.suppress_bind_attrs = true,
 	},
 };
+
+/* INIT */
+static struct platform_driver * const omap_dss_drivers[] = {
+	&omap_dsshw_driver,
+	&omap_dispchw_driver,
+#ifdef CONFIG_OMAP2_DSS_DSI
+	&omap_dsihw_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_VENC
+	&omap_venchw_driver,
+#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+	&omapdss_hdmi4hw_driver,
+#endif
+#ifdef CONFIG_OMAP5_DSS_HDMI
+	&omapdss_hdmi5hw_driver,
+#endif
+};
+
+static int __init omap_dss_init(void)
+{
+	return platform_register_drivers(omap_dss_drivers,
+					 ARRAY_SIZE(omap_dss_drivers));
+}
+
+static void __exit omap_dss_exit(void)
+{
+	platform_unregister_drivers(omap_dss_drivers,
+				    ARRAY_SIZE(omap_dss_drivers));
+}
+
+module_init(omap_dss_init);
+module_exit(omap_dss_exit);
+
+MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
+MODULE_DESCRIPTION("OMAP2/3/4/5 Display Subsystem");
+MODULE_LICENSE("GPL v2");
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH 7/7] drm/omap: hdmi5: automatically choose limited/full range output

[Tomi Valkeinen]

Currently the HDMI driver uses always limited range RGB output. This
patch improves the behavior by using limited range only if the output is
identified as a HDMI display, and VIC > 1.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 121 ++++++++++++-----------
 1 file changed, 65 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
index 4c588ec7634a..96f5cd17768c 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
@@ -23,18 +23,6 @@
 
 #include "hdmi5_core.h"
 
-/* only 24 bit color depth used for now */
-static const struct csc_table csc_table_deepcolor[] = {
-	/* HDMI_DEEP_COLOR_24BIT */
-	[0] = { 7036, 0, 0, 32, 0, 7036, 0, 32, 0, 0, 7036, 32, },
-	/* HDMI_DEEP_COLOR_30BIT */
-	[1] = { 7015, 0, 0, 128, 0, 7015, 0, 128, 0, 0, 7015, 128, },
-	/* HDMI_DEEP_COLOR_36BIT */
-	[2] = { 7010, 0, 0, 512, 0, 7010, 0, 512, 0, 0, 7010, 512, },
-	/* FULL RANGE */
-	[3] = { 8192, 0, 0, 0, 0, 8192, 0, 0, 0, 0, 8192, 0, },
-};
-
 static void hdmi_core_ddc_init(struct hdmi_core_data *core)
 {
 	void __iomem *base = core->base;
@@ -397,14 +385,6 @@ static void hdmi_core_config_video_packetizer(struct hdmi_core_data *core)
 	REG_FLD_MOD(base, HDMI_CORE_VP_CONF, clr_depth ? 0 : 2, 1, 0);
 }
 
-static void hdmi_core_config_csc(struct hdmi_core_data *core)
-{
-	int clr_depth = 0;	/* 24 bit color depth */
-
-	/* CSC_COLORDEPTH */
-	REG_FLD_MOD(core->base, HDMI_CORE_CSC_SCALE, clr_depth, 7, 4);
-}
-
 static void hdmi_core_config_video_sampler(struct hdmi_core_data *core)
 {
 	int video_mapping = 1;	/* for 24 bit color depth */
@@ -469,47 +449,67 @@ static void hdmi_core_write_avi_infoframe(struct hdmi_core_data *core,
 	REG_FLD_MOD(base, HDMI_CORE_FC_PRCONF, pr, 3, 0);
 }
 
-static void hdmi_core_csc_config(struct hdmi_core_data *core,
-		struct csc_table csc_coeff)
+static void hdmi_core_write_csc(struct hdmi_core_data *core,
+		const struct csc_table *csc_coeff)
 {
 	void __iomem *base = core->base;
 
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_MSB, csc_coeff.a1 >> 8 , 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_LSB, csc_coeff.a1, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_MSB, csc_coeff.a2 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_LSB, csc_coeff.a2, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_MSB, csc_coeff.a3 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_LSB, csc_coeff.a3, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_MSB, csc_coeff.a4 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_LSB, csc_coeff.a4, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_MSB, csc_coeff.b1 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_LSB, csc_coeff.b1, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_MSB, csc_coeff.b2 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_LSB, csc_coeff.b2, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_MSB, csc_coeff.b3 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_LSB, csc_coeff.b3, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_MSB, csc_coeff.b4 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_LSB, csc_coeff.b4, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_MSB, csc_coeff.c1 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_LSB, csc_coeff.c1, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_MSB, csc_coeff.c2 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_LSB, csc_coeff.c2, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_MSB, csc_coeff.c3 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_LSB, csc_coeff.c3, 7, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_MSB, csc_coeff.c4 >> 8, 6, 0);
-	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_LSB, csc_coeff.c4, 7, 0);
-
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_MSB, csc_coeff->a1 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_LSB, csc_coeff->a1, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_MSB, csc_coeff->a2 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_LSB, csc_coeff->a2, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_MSB, csc_coeff->a3 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_LSB, csc_coeff->a3, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_MSB, csc_coeff->a4 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_LSB, csc_coeff->a4, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_MSB, csc_coeff->b1 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_LSB, csc_coeff->b1, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_MSB, csc_coeff->b2 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_LSB, csc_coeff->b2, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_MSB, csc_coeff->b3 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_LSB, csc_coeff->b3, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_MSB, csc_coeff->b4 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_LSB, csc_coeff->b4, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_MSB, csc_coeff->c1 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_LSB, csc_coeff->c1, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_MSB, csc_coeff->c2 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_LSB, csc_coeff->c2, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_MSB, csc_coeff->c3 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_LSB, csc_coeff->c3, 7, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_MSB, csc_coeff->c4 >> 8, 6, 0);
+	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_LSB, csc_coeff->c4, 7, 0);
+
+	/* enable CSC */
 	REG_FLD_MOD(base, HDMI_CORE_MC_FLOWCTRL, 0x1, 0, 0);
 }
 
-static void hdmi_core_configure_range(struct hdmi_core_data *core)
+static void hdmi_core_configure_range(struct hdmi_core_data *core,
+				      enum hdmi_quantization_range range)
 {
-	struct csc_table csc_coeff = { 0 };
+	static const struct csc_table csc_limited_range = {
+		7036, 0, 0, 32, 0, 7036, 0, 32, 0, 0, 7036, 32
+	};
+	static const struct csc_table csc_full_range = {
+		8192, 0, 0, 0, 0, 8192, 0, 0, 0, 0, 8192, 0
+	};
+	const struct csc_table *csc_coeff;
+
+	/* CSC_COLORDEPTH  = 24 bits*/
+	REG_FLD_MOD(core->base, HDMI_CORE_CSC_SCALE, 0, 7, 4);
+
+	switch (range) {
+	case HDMI_QUANTIZATION_RANGE_FULL:
+		csc_coeff = &csc_full_range;
+		break;
 
-	/* support limited range with 24 bit color depth for now */
-	csc_coeff = csc_table_deepcolor[0];
+	case HDMI_QUANTIZATION_RANGE_DEFAULT:
+	case HDMI_QUANTIZATION_RANGE_LIMITED:
+	default:
+		csc_coeff = &csc_limited_range;
+		break;
+	}
 
-	hdmi_core_csc_config(core, csc_coeff);
+	hdmi_core_write_csc(core, csc_coeff);
 }
 
 static void hdmi_core_enable_video_path(struct hdmi_core_data *core)
@@ -600,9 +600,20 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
 	struct videomode vm;
 	struct hdmi_video_format video_format;
 	struct hdmi_core_vid_config v_core_cfg;
+	enum hdmi_quantization_range range;
 
 	hdmi_core_mask_interrupts(core);
 
+	if (cfg->hdmi_dvi_mode == HDMI_HDMI) {
+		char vic = cfg->infoframe.video_code;
+
+		/* All CEA modes other than VIC 1 use limited quantization range. */
+		range = vic > 1 ? HDMI_QUANTIZATION_RANGE_LIMITED :
+			HDMI_QUANTIZATION_RANGE_FULL;
+	} else {
+		range = HDMI_QUANTIZATION_RANGE_FULL;
+	}
+
 	hdmi_core_init(&v_core_cfg, cfg);
 
 	hdmi_wp_init_vid_fmt_timings(&video_format, &vm, cfg);
@@ -616,9 +627,8 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
 
 	hdmi_wp_video_config_interface(wp, &vm);
 
-	/* support limited range with 24 bit color depth for now */
-	hdmi_core_configure_range(core);
-	cfg->infoframe.quantization_range = HDMI_QUANTIZATION_RANGE_LIMITED;
+	hdmi_core_configure_range(core, range);
+	cfg->infoframe.quantization_range = range;
 
 	/*
 	 * configure core video part, set software reset in the core
@@ -628,7 +638,6 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
 	hdmi_core_video_config(core, &v_core_cfg);
 
 	hdmi_core_config_video_packetizer(core);
-	hdmi_core_config_csc(core);
 	hdmi_core_config_video_sampler(core);
 
 	if (cfg->hdmi_dvi_mode == HDMI_HDMI)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH 1/7] drm/omap: drop unneeded locking from mgr_fld_write()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:53PM +0300, Tomi Valkeinen wrote:
> Commit d49cd15550d9d4495f6187425318c245d58cb63f ("OMAPDSS: DISPC: lock
> access to DISPC_CONTROL & DISPC_CONFIG") added locking to
> mgr_fld_write(). This was needed in omapfb times due to lack of good
> locking, especially in the case of both V4L2 and fbdev layers using the
> DSS driver.
> 
> This is not needed for omapdrm, so we can remove the locking.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I've followed all code paths, and they end up either in the bridge
enable operations or the CRTC atomic flush (disregarding
suspend/resume). Those can't race each other, so this should be safe.

> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 785c5546067a..c6da33e7014f 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -184,9 +184,6 @@ struct dispc_device {
>  
>  	struct regmap *syscon_pol;
>  	u32 syscon_pol_offset;
> -
> -	/* DISPC_CONTROL & DISPC_CONFIG lock*/
> -	spinlock_t control_lock;
>  };
>  
>  enum omap_color_component {
> @@ -377,16 +374,7 @@ static void mgr_fld_write(struct dispc_device *dispc, enum omap_channel channel,
>  			  enum mgr_reg_fields regfld, int val)
>  {
>  	const struct dispc_reg_field rfld = mgr_desc[channel].reg_desc[regfld];

While at it, should you turn this into a pointer to avoid an unnecessary
copy ?

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

> -	const bool need_lock = rfld.reg == DISPC_CONTROL || rfld.reg == DISPC_CONFIG;
> -	unsigned long flags;
> -
> -	if (need_lock) {
> -		spin_lock_irqsave(&dispc->control_lock, flags);
> -		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
> -		spin_unlock_irqrestore(&dispc->control_lock, flags);
> -	} else {
> -		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
> -	}
> +	REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
>  }
>  
>  static int dispc_get_num_ovls(struct dispc_device *dispc)
> @@ -4769,8 +4757,6 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  	platform_set_drvdata(pdev, dispc);
>  	dispc->dss = dss;
>  
> -	spin_lock_init(&dispc->control_lock);
> -
>  	/*
>  	 * The OMAP3-based models can't be told apart using the compatible
>  	 * string, use SoC device matching.

-- 
Regards,

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

Re: [PATCH 2/7] drm/omap: tweak HDMI DDC timings

[Laurent Pinchart]

Hi Tomi,

Thank you for the path.

On Mon, Sep 02, 2019 at 03:53:54PM +0300, Tomi Valkeinen wrote:
> From: Alejandro Hernandez <ajhernandez@ti.com>
> 
> A "HDMI I2C Master Error" is sometimes reported with the current DDC SCL
> timings. The current settings for a 10us SCL period (100 KHz) causes the
> error with some displays.  This patch increases the SCL signal period
> from 10us to 10.2us, with the new settings the error is not observed
> 

It would be useful to document what those "some displays" are if you
can.

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

> Signed-off-by: Alejandro Hernandez <ajhernandez@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> index 7400fb99d453..4c588ec7634a 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> @@ -39,8 +39,8 @@ static void hdmi_core_ddc_init(struct hdmi_core_data *core)
>  {
>  	void __iomem *base = core->base;
>  	const unsigned long long iclk = 266000000;	/* DSS L3 ICLK */
> -	const unsigned int ss_scl_high = 4600;		/* ns */
> -	const unsigned int ss_scl_low = 5400;		/* ns */
> +	const unsigned int ss_scl_high = 4700;		/* ns */
> +	const unsigned int ss_scl_low = 5500;		/* ns */
>  	const unsigned int fs_scl_high = 600;		/* ns */
>  	const unsigned int fs_scl_low = 1300;		/* ns */
>  	const unsigned int sda_hold = 1000;		/* ns */

-- 
Regards,

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

Re: [PATCH 3/7] drm/omap: fix missing scaler pixel fmt limitations

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:55PM +0300, Tomi Valkeinen wrote:
> OMAP2 and OMAP3/AM4 have limitations with the scaler:
> - OMAP2 can only scale XRGB8888
> - OMAP3/AM4 can only scale XRGB8888, RGB565, YUYV and UYVY
> 
> The driver doesn't check these limitations, which leads to sync-lost
> floods.
> 
> This patch adds a check for the pixel formats when scaling.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index c6da33e7014f..7d87d60edb80 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -114,6 +114,7 @@ struct dispc_features {
>  	const unsigned int num_reg_fields;
>  	const enum omap_overlay_caps *overlay_caps;
>  	const u32 **supported_color_modes;
> +	const u32 *supported_scaler_color_modes;
>  	unsigned int num_mgrs;
>  	unsigned int num_ovls;
>  	unsigned int buffer_size_unit;
> @@ -2498,6 +2499,19 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc,
>  	if (width == out_width && height == out_height)
>  		return 0;
>  
> +	if (dispc->feat->supported_scaler_color_modes) {
> +		const u32 *modes = dispc->feat->supported_scaler_color_modes;
> +		int i;

i is never negative and can thus be an unsigned int. Apart from that,

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

> +
> +		for (i = 0; modes[i]; ++i) {
> +			if (modes[i] == fourcc)
> +				break;
> +		}
> +
> +		if (modes[i] == 0)
> +			return -EINVAL;
> +	}
> +
>  	if (plane == OMAP_DSS_WB) {
>  		switch (fourcc) {
>  		case DRM_FORMAT_NV12:
> @@ -4213,6 +4227,12 @@ static const u32 *omap4_dispc_supported_color_modes[] = {
>  	DRM_FORMAT_RGBX8888),
>  };
>  
> +static const u32 omap3_dispc_supported_scaler_color_modes[] = {
> +	DRM_FORMAT_XRGB8888, DRM_FORMAT_RGB565, DRM_FORMAT_YUYV,
> +	DRM_FORMAT_UYVY,
> +	0,
> +};
> +
>  static const struct dispc_features omap24xx_dispc_feats = {
>  	.sw_start		=	5,
>  	.fp_start		=	15,
> @@ -4241,6 +4261,7 @@ static const struct dispc_features omap24xx_dispc_feats = {
>  	.num_reg_fields		=	ARRAY_SIZE(omap2_dispc_reg_fields),
>  	.overlay_caps		=	omap2_dispc_overlay_caps,
>  	.supported_color_modes	=	omap2_dispc_supported_color_modes,
> +	.supported_scaler_color_modes = COLOR_ARRAY(DRM_FORMAT_XRGB8888),
>  	.num_mgrs		=	2,
>  	.num_ovls		=	3,
>  	.buffer_size_unit	=	1,
> @@ -4275,6 +4296,7 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = {
>  	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
>  	.overlay_caps		=	omap3430_dispc_overlay_caps,
>  	.supported_color_modes	=	omap3_dispc_supported_color_modes,
> +	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
>  	.num_mgrs		=	2,
>  	.num_ovls		=	3,
>  	.buffer_size_unit	=	1,
> @@ -4309,6 +4331,7 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = {
>  	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
>  	.overlay_caps		=	omap3430_dispc_overlay_caps,
>  	.supported_color_modes	=	omap3_dispc_supported_color_modes,
> +	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
>  	.num_mgrs		=	2,
>  	.num_ovls		=	3,
>  	.buffer_size_unit	=	1,
> @@ -4343,6 +4366,7 @@ static const struct dispc_features omap36xx_dispc_feats = {
>  	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
>  	.overlay_caps		=	omap3630_dispc_overlay_caps,
>  	.supported_color_modes	=	omap3_dispc_supported_color_modes,
> +	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
>  	.num_mgrs		=	2,
>  	.num_ovls		=	3,
>  	.buffer_size_unit	=	1,
> @@ -4377,6 +4401,7 @@ static const struct dispc_features am43xx_dispc_feats = {
>  	.num_reg_fields		=	ARRAY_SIZE(omap3_dispc_reg_fields),
>  	.overlay_caps		=	omap3430_dispc_overlay_caps,
>  	.supported_color_modes	=	omap3_dispc_supported_color_modes,
> +	.supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes,
>  	.num_mgrs		=	1,
>  	.num_ovls		=	3,
>  	.buffer_size_unit	=	1,

-- 
Regards,

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Implement CTM color management property for OMAP CRTC using DSS
> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> exactly match the CTM property documentation. On DSS the CPR matrix is
> applied after gamma table look up. However, it seems stupid to add a
> custom property just for that.

In that case the DRM documentation should be updated to mention that
both options are allowed.

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 3c5ddbf30e97..d63213dd7d83 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
>  	}
>  }
>  
> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
> +{
> +	uint64_t sign_bit = 1ULL << 63;
> +	uint64_t cbits = (uint64_t) coef;

s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
there's no need for a space before coef.

> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
> +
> +	if (cbits & sign_bit)
> +		ret = -ret;
> +
> +	return ret;

Can't this be simplified to 

	s16 ret = (coef >> 24) & 0x1ff;

	return coef < 0 ? -ret : ret;

> +}
> +
> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> +					 struct omap_dss_cpr_coefs *cpr)
> +{
> +	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
> +	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
> +	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
> +	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
> +	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
> +	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
> +	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
> +	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
> +	cpr->bb = omap_crtc_S31_32_to_s2_8(ctm->matrix[8]);
> +}
> +
>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>  {
>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>  	info.default_color = 0x000000;
>  	info.trans_enabled = false;
>  	info.partial_alpha_enabled = false;
> -	info.cpr_enable = false;
> +
> +	if (crtc->state->ctm) {
> +		struct drm_color_ctm *ctm =
> +			(struct drm_color_ctm *) crtc->state->ctm->data;
> +
> +		info.cpr_enable = true;
> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);

As an optimisation it would be nice to only write the coefficients when
they actually change. That could be implemented on top of this series.

> +	} else {
> +		info.cpr_enable = false;
> +	}
>  
>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
>  }
> @@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
>  		unsigned int gamma_lut_size = 256;
>  
> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
> +		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
>  	}
>  

-- 
Regards,

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

Re: [PATCH 5/7] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> omap_plane.c and dispc.c. The supported encodings and ranges are:
> 
> For COLOR_ENCODING:
> - YCbCt BT.601 (default)

Did you mean YCbCr ?

> - YCbCt BT.709
> 
> For COLOR_RANGE:
> - YCbCt limited range
> - YCbCt full range (default)
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 119 ++++++++++++++++++++------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 +++++++
>  3 files changed, 127 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 7d87d60edb80..40ddb28ee7aa 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
>  #undef CVAL
>  }
>  
> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> +/* YUV -> RGB, ITU-R BT.601, full range */
> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {

static const is usually preferred over const static, here and below.

> +	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, full range */
> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
> +	true,                   /* full 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 static 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 static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {

That should be coefs_rgb2yuv_bt709_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 */
> +};

Why is coefs_rgb2yuv_bt709_full not supported ? Actually
coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.

> +
> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> +			     enum omap_plane_id plane,
> +			     enum drm_color_encoding color_encoding,
> +			     enum drm_color_range color_range)
>  {
> -	int i;
> -	int num_ovl = dispc_get_num_ovls(dispc);
> -
> -	/* 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 csc_coef_yuv2rgb *csc;
>  
> -	/* 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 */
> -	};
> +	switch (color_encoding) {
> +	case DRM_COLOR_YCBCR_BT601:
> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> +			csc = &coefs_yuv2rgb_bt601_full;
> +		else
> +			csc = &coefs_yuv2rgb_bt601_lim;
> +		break;
> +	case DRM_COLOR_YCBCR_BT709:
> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> +			csc = &coefs_yuv2rgb_bt709_full;
> +		else
> +			csc = &coefs_yuv2rgb_bt709_lim;
> +		break;
> +	default:
> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
> +		       color_encoding, plane);
> +		return -EINVAL;
> +	}
>  
> -	for (i = 1; i < num_ovl; i++)
> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
>  
> -	if (dispc->feat->has_writeback)
> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> +	return 0;
>  }
>  
>  static void dispc_ovl_set_ba0(struct dispc_device *dispc,
> @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>  				  u8 pre_mult_alpha, u8 global_alpha,
>  				  enum omap_dss_rotation_type rotation_type,
>  				  bool replication, const struct videomode *vm,
> -				  bool mem_to_mem)
> +				  bool mem_to_mem,
> +				  enum drm_color_encoding color_encoding,
> +				  enum drm_color_range color_range)
>  {
>  	bool five_taps = true;
>  	bool fieldmode = false;
> @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>  				      fieldmode, fourcc, rotation);
>  		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
>  		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
> +
> +		if (plane != OMAP_DSS_WB)
> +			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
>  	}
>  
>  	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
> @@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
>  		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
>  		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
>  		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
> -		oi->rotation_type, replication, vm, mem_to_mem);
> +		oi->rotation_type, replication, vm, mem_to_mem,
> +		oi->color_encoding, oi->color_range);
>  
>  	return r;
>  }
> @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
>  		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
>  		wi->height, wi->fourcc, wi->rotation, zorder,
>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
> -		replication, vm, mem_to_mem);
> +		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
> +		DRM_COLOR_YCBCR_LIMITED_RANGE);
>  	if (r)
>  		return r;
>  
> @@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
>  	    dispc->feat->has_gamma_table)
>  		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
>  
> -	dispc_setup_color_conv_coef(dispc);
> +	if (dispc->feat->has_writeback)
> +		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
>  
>  	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
>  
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 79f6b195c7cf..1430cab6b877 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -14,6 +14,7 @@
>  #include <linux/platform_data/omapdss.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_color_mgmt.h>

Alphabetical order ? While at is you coulde remove uapi/

>  
>  #define DISPC_IRQ_FRAMEDONE		(1 << 0)
>  #define DISPC_IRQ_VSYNC			(1 << 1)
> @@ -243,6 +244,9 @@ struct omap_overlay_info {
>  	u8 global_alpha;
>  	u8 pre_mult_alpha;
>  	u8 zorder;
> +
> +	enum drm_color_encoding color_encoding;
> +	enum drm_color_range color_range;
>  };
>  
>  struct omap_overlay_manager_info {
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 73ec99819a3d..db8e917260df 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>  		info.pre_mult_alpha = 1;
>  	else
>  		info.pre_mult_alpha = 0;
> +	info.color_encoding = state->color_encoding;
> +	info.color_range = state->color_range;
>  
>  	/* update scanout: */
>  	omap_framebuffer_update_scanout(state->fb, state, &info);
> @@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
>  	 */
>  	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>  			   ? 0 : omap_plane->id;
> +	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
> +	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
>  }
>  
>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
> @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
>  	.atomic_get_property = omap_plane_atomic_get_property,
>  };
>  
> +static bool omap_plane_supports_yuv(struct drm_plane *plane)
> +{
> +	struct omap_drm_private *priv = plane->dev->dev_private;
> +	struct omap_plane *omap_plane = to_omap_plane(plane);
> +	const u32 *formats =
> +		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
> +	int i;

unsigned int i ?

> +
> +	for (i = 0; formats[i]; i++)
> +		if (formats[i] == DRM_FORMAT_YUYV ||
> +		    formats[i] == DRM_FORMAT_UYVY ||
> +		    formats[i] == DRM_FORMAT_NV12)
> +			return true;
> +
> +	return false;
> +}
> +
>  static const char *plane_id_to_name[] = {
>  	[OMAP_DSS_GFX] = "gfx",
>  	[OMAP_DSS_VIDEO1] = "vid1",
> @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>  	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
>  					     BIT(DRM_MODE_BLEND_COVERAGE));
>  
> +	if (omap_plane_supports_yuv(plane))
> +		drm_plane_create_color_properties(plane,
> +					BIT(DRM_COLOR_YCBCR_BT601) |
> +					BIT(DRM_COLOR_YCBCR_BT709),
> +					BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
> +					BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> +					DRM_COLOR_YCBCR_BT601,
> +					DRM_COLOR_YCBCR_FULL_RANGE);
> +
>  	return plane;
>  
>  error:

-- 
Regards,

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

Re: [PATCH 6/7] drm/omap: dss: platform_register_drivers() to dss.c and remove core.c

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

Missing "Move" in the subject after "dss: " ?

On Mon, Sep 02, 2019 at 03:53:58PM +0300, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> The core.c just for registering the drivers is kind of useless. Let's
> get rid of it and register the dss drivers in dss.c.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> ---
>  drivers/gpu/drm/omapdrm/dss/Makefile |  2 +-
>  drivers/gpu/drm/omapdrm/dss/core.c   | 55 ----------------------------
>  drivers/gpu/drm/omapdrm/dss/dss.c    | 37 +++++++++++++++++++
>  3 files changed, 38 insertions(+), 56 deletions(-)
>  delete mode 100644 drivers/gpu/drm/omapdrm/dss/core.c
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/Makefile b/drivers/gpu/drm/omapdrm/dss/Makefile
> index 904101c5e79d..5950c3f52c2e 100644
> --- a/drivers/gpu/drm/omapdrm/dss/Makefile
> +++ b/drivers/gpu/drm/omapdrm/dss/Makefile
> @@ -6,7 +6,7 @@ omapdss-base-y := base.o display.o dss-of.o output.o
>  
>  obj-$(CONFIG_OMAP2_DSS) += omapdss.o
>  # Core DSS files
> -omapdss-y := core.o dss.o dispc.o dispc_coefs.o \
> +omapdss-y := dss.o dispc.o dispc_coefs.o \
>  	pll.o video-pll.o
>  omapdss-$(CONFIG_OMAP2_DSS_DPI) += dpi.o
>  omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o
> diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c
> deleted file mode 100644
> index 6ac497b63711..000000000000
> --- a/drivers/gpu/drm/omapdrm/dss/core.c
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (C) 2009 Nokia Corporation
> - * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> - *
> - * Some code and ideas taken from drivers/video/omap/ driver
> - * by Imre Deak.
> - */
> -
> -#define DSS_SUBSYS_NAME "CORE"
> -
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -
> -#include "omapdss.h"
> -#include "dss.h"
> -
> -/* INIT */
> -static struct platform_driver * const omap_dss_drivers[] = {
> -	&omap_dsshw_driver,
> -	&omap_dispchw_driver,
> -#ifdef CONFIG_OMAP2_DSS_DSI
> -	&omap_dsihw_driver,
> -#endif
> -#ifdef CONFIG_OMAP2_DSS_VENC
> -	&omap_venchw_driver,
> -#endif
> -#ifdef CONFIG_OMAP4_DSS_HDMI
> -	&omapdss_hdmi4hw_driver,
> -#endif
> -#ifdef CONFIG_OMAP5_DSS_HDMI
> -	&omapdss_hdmi5hw_driver,
> -#endif
> -};
> -
> -static int __init omap_dss_init(void)
> -{
> -	return platform_register_drivers(omap_dss_drivers,
> -					 ARRAY_SIZE(omap_dss_drivers));
> -}
> -
> -static void __exit omap_dss_exit(void)
> -{
> -	platform_unregister_drivers(omap_dss_drivers,
> -				    ARRAY_SIZE(omap_dss_drivers));
> -}
> -
> -module_init(omap_dss_init);
> -module_exit(omap_dss_exit);
> -
> -MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
> -MODULE_DESCRIPTION("OMAP2/3 Display Subsystem");
> -MODULE_LICENSE("GPL v2");
> -
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
> index e226324adb69..41d495a360d8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
> @@ -1598,3 +1598,40 @@ struct platform_driver omap_dsshw_driver = {
>  		.suppress_bind_attrs = true,
>  	},
>  };
> +
> +/* INIT */
> +static struct platform_driver * const omap_dss_drivers[] = {
> +	&omap_dsshw_driver,
> +	&omap_dispchw_driver,
> +#ifdef CONFIG_OMAP2_DSS_DSI
> +	&omap_dsihw_driver,
> +#endif
> +#ifdef CONFIG_OMAP2_DSS_VENC
> +	&omap_venchw_driver,
> +#endif
> +#ifdef CONFIG_OMAP4_DSS_HDMI
> +	&omapdss_hdmi4hw_driver,
> +#endif
> +#ifdef CONFIG_OMAP5_DSS_HDMI
> +	&omapdss_hdmi5hw_driver,
> +#endif
> +};
> +
> +static int __init omap_dss_init(void)
> +{
> +	return platform_register_drivers(omap_dss_drivers,
> +					 ARRAY_SIZE(omap_dss_drivers));
> +}
> +
> +static void __exit omap_dss_exit(void)
> +{
> +	platform_unregister_drivers(omap_dss_drivers,
> +				    ARRAY_SIZE(omap_dss_drivers));
> +}
> +
> +module_init(omap_dss_init);
> +module_exit(omap_dss_exit);
> +
> +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
> +MODULE_DESCRIPTION("OMAP2/3/4/5 Display Subsystem");
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

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

Re: [PATCH 7/7] drm/omap: hdmi5: automatically choose limited/full range output

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:59PM +0300, Tomi Valkeinen wrote:
> Currently the HDMI driver uses always limited range RGB output. This
> patch improves the behavior by using limited range only if the output is
> identified as a HDMI display, and VIC > 1.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 121 ++++++++++++-----------
>  1 file changed, 65 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> index 4c588ec7634a..96f5cd17768c 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
> @@ -23,18 +23,6 @@
>  
>  #include "hdmi5_core.h"
>  
> -/* only 24 bit color depth used for now */
> -static const struct csc_table csc_table_deepcolor[] = {
> -	/* HDMI_DEEP_COLOR_24BIT */
> -	[0] = { 7036, 0, 0, 32, 0, 7036, 0, 32, 0, 0, 7036, 32, },
> -	/* HDMI_DEEP_COLOR_30BIT */
> -	[1] = { 7015, 0, 0, 128, 0, 7015, 0, 128, 0, 0, 7015, 128, },
> -	/* HDMI_DEEP_COLOR_36BIT */
> -	[2] = { 7010, 0, 0, 512, 0, 7010, 0, 512, 0, 0, 7010, 512, },
> -	/* FULL RANGE */
> -	[3] = { 8192, 0, 0, 0, 0, 8192, 0, 0, 0, 0, 8192, 0, },
> -};
> -
>  static void hdmi_core_ddc_init(struct hdmi_core_data *core)
>  {
>  	void __iomem *base = core->base;
> @@ -397,14 +385,6 @@ static void hdmi_core_config_video_packetizer(struct hdmi_core_data *core)
>  	REG_FLD_MOD(base, HDMI_CORE_VP_CONF, clr_depth ? 0 : 2, 1, 0);
>  }
>  
> -static void hdmi_core_config_csc(struct hdmi_core_data *core)
> -{
> -	int clr_depth = 0;	/* 24 bit color depth */
> -
> -	/* CSC_COLORDEPTH */
> -	REG_FLD_MOD(core->base, HDMI_CORE_CSC_SCALE, clr_depth, 7, 4);
> -}
> -
>  static void hdmi_core_config_video_sampler(struct hdmi_core_data *core)
>  {
>  	int video_mapping = 1;	/* for 24 bit color depth */
> @@ -469,47 +449,67 @@ static void hdmi_core_write_avi_infoframe(struct hdmi_core_data *core,
>  	REG_FLD_MOD(base, HDMI_CORE_FC_PRCONF, pr, 3, 0);
>  }
>  
> -static void hdmi_core_csc_config(struct hdmi_core_data *core,
> -		struct csc_table csc_coeff)
> +static void hdmi_core_write_csc(struct hdmi_core_data *core,
> +		const struct csc_table *csc_coeff)
>  {
>  	void __iomem *base = core->base;
>  
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_MSB, csc_coeff.a1 >> 8 , 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_LSB, csc_coeff.a1, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_MSB, csc_coeff.a2 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_LSB, csc_coeff.a2, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_MSB, csc_coeff.a3 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_LSB, csc_coeff.a3, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_MSB, csc_coeff.a4 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_LSB, csc_coeff.a4, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_MSB, csc_coeff.b1 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_LSB, csc_coeff.b1, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_MSB, csc_coeff.b2 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_LSB, csc_coeff.b2, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_MSB, csc_coeff.b3 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_LSB, csc_coeff.b3, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_MSB, csc_coeff.b4 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_LSB, csc_coeff.b4, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_MSB, csc_coeff.c1 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_LSB, csc_coeff.c1, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_MSB, csc_coeff.c2 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_LSB, csc_coeff.c2, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_MSB, csc_coeff.c3 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_LSB, csc_coeff.c3, 7, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_MSB, csc_coeff.c4 >> 8, 6, 0);
> -	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_LSB, csc_coeff.c4, 7, 0);
> -
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_MSB, csc_coeff->a1 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_LSB, csc_coeff->a1, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_MSB, csc_coeff->a2 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_LSB, csc_coeff->a2, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_MSB, csc_coeff->a3 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_LSB, csc_coeff->a3, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_MSB, csc_coeff->a4 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_LSB, csc_coeff->a4, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_MSB, csc_coeff->b1 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_LSB, csc_coeff->b1, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_MSB, csc_coeff->b2 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_LSB, csc_coeff->b2, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_MSB, csc_coeff->b3 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_LSB, csc_coeff->b3, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_MSB, csc_coeff->b4 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_LSB, csc_coeff->b4, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_MSB, csc_coeff->c1 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_LSB, csc_coeff->c1, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_MSB, csc_coeff->c2 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_LSB, csc_coeff->c2, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_MSB, csc_coeff->c3 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_LSB, csc_coeff->c3, 7, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_MSB, csc_coeff->c4 >> 8, 6, 0);
> +	REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_LSB, csc_coeff->c4, 7, 0);
> +
> +	/* enable CSC */
>  	REG_FLD_MOD(base, HDMI_CORE_MC_FLOWCTRL, 0x1, 0, 0);
>  }
>  
> -static void hdmi_core_configure_range(struct hdmi_core_data *core)
> +static void hdmi_core_configure_range(struct hdmi_core_data *core,
> +				      enum hdmi_quantization_range range)
>  {
> -	struct csc_table csc_coeff = { 0 };
> +	static const struct csc_table csc_limited_range = {
> +		7036, 0, 0, 32, 0, 7036, 0, 32, 0, 0, 7036, 32
> +	};
> +	static const struct csc_table csc_full_range = {
> +		8192, 0, 0, 0, 0, 8192, 0, 0, 0, 0, 8192, 0
> +	};
> +	const struct csc_table *csc_coeff;
> +
> +	/* CSC_COLORDEPTH  = 24 bits*/
> +	REG_FLD_MOD(core->base, HDMI_CORE_CSC_SCALE, 0, 7, 4);
> +
> +	switch (range) {
> +	case HDMI_QUANTIZATION_RANGE_FULL:
> +		csc_coeff = &csc_full_range;
> +		break;
>  
> -	/* support limited range with 24 bit color depth for now */
> -	csc_coeff = csc_table_deepcolor[0];
> +	case HDMI_QUANTIZATION_RANGE_DEFAULT:
> +	case HDMI_QUANTIZATION_RANGE_LIMITED:
> +	default:
> +		csc_coeff = &csc_limited_range;
> +		break;
> +	}
>  
> -	hdmi_core_csc_config(core, csc_coeff);
> +	hdmi_core_write_csc(core, csc_coeff);
>  }
>  
>  static void hdmi_core_enable_video_path(struct hdmi_core_data *core)
> @@ -600,9 +600,20 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
>  	struct videomode vm;
>  	struct hdmi_video_format video_format;
>  	struct hdmi_core_vid_config v_core_cfg;
> +	enum hdmi_quantization_range range;
>  
>  	hdmi_core_mask_interrupts(core);
>  
> +	if (cfg->hdmi_dvi_mode == HDMI_HDMI) {
> +		char vic = cfg->infoframe.video_code;
> +
> +		/* All CEA modes other than VIC 1 use limited quantization range. */
> +		range = vic > 1 ? HDMI_QUANTIZATION_RANGE_LIMITED :
> +			HDMI_QUANTIZATION_RANGE_FULL;
> +	} else {
> +		range = HDMI_QUANTIZATION_RANGE_FULL;
> +	}
> +
>  	hdmi_core_init(&v_core_cfg, cfg);
>  
>  	hdmi_wp_init_vid_fmt_timings(&video_format, &vm, cfg);
> @@ -616,9 +627,8 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
>  
>  	hdmi_wp_video_config_interface(wp, &vm);
>  
> -	/* support limited range with 24 bit color depth for now */
> -	hdmi_core_configure_range(core);
> -	cfg->infoframe.quantization_range = HDMI_QUANTIZATION_RANGE_LIMITED;
> +	hdmi_core_configure_range(core, range);
> +	cfg->infoframe.quantization_range = range;
>  
>  	/*
>  	 * configure core video part, set software reset in the core
> @@ -628,7 +638,6 @@ void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
>  	hdmi_core_video_config(core, &v_core_cfg);
>  
>  	hdmi_core_config_video_packetizer(core);
> -	hdmi_core_config_csc(core);
>  	hdmi_core_config_video_sampler(core);
>  
>  	if (cfg->hdmi_dvi_mode == HDMI_HDMI)

-- 
Regards,

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

Re: [PATCH 6/7] drm/omap: dss: platform_register_drivers() to dss.c and remove core.c

[Jyri Sarha]

On 03/09/2019 18:34, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> Missing "Move" in the subject after "dss: " ?
> 

That was intentional to keep the subject short enough. But it looks like
it is just bellow 76 chars (80 - 4 char indent) even with "Move" added
to it.

BR,
Jyri

> On Mon, Sep 02, 2019 at 03:53:58PM +0300, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@ti.com>
>>
>> The core.c just for registering the drivers is kind of useless. Let's
>> get rid of it and register the dss drivers in dss.c.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>>  drivers/gpu/drm/omapdrm/dss/Makefile |  2 +-
>>  drivers/gpu/drm/omapdrm/dss/core.c   | 55 ----------------------------
>>  drivers/gpu/drm/omapdrm/dss/dss.c    | 37 +++++++++++++++++++
>>  3 files changed, 38 insertions(+), 56 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/omapdrm/dss/core.c
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/Makefile b/drivers/gpu/drm/omapdrm/dss/Makefile
>> index 904101c5e79d..5950c3f52c2e 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/Makefile
>> +++ b/drivers/gpu/drm/omapdrm/dss/Makefile
>> @@ -6,7 +6,7 @@ omapdss-base-y := base.o display.o dss-of.o output.o
>>  
>>  obj-$(CONFIG_OMAP2_DSS) += omapdss.o
>>  # Core DSS files
>> -omapdss-y := core.o dss.o dispc.o dispc_coefs.o \
>> +omapdss-y := dss.o dispc.o dispc_coefs.o \
>>  	pll.o video-pll.o
>>  omapdss-$(CONFIG_OMAP2_DSS_DPI) += dpi.o
>>  omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o
>> diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c
>> deleted file mode 100644
>> index 6ac497b63711..000000000000
>> --- a/drivers/gpu/drm/omapdrm/dss/core.c
>> +++ /dev/null
>> @@ -1,55 +0,0 @@
>> -// SPDX-License-Identifier: GPL-2.0-only
>> -/*
>> - * Copyright (C) 2009 Nokia Corporation
>> - * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> - *
>> - * Some code and ideas taken from drivers/video/omap/ driver
>> - * by Imre Deak.
>> - */
>> -
>> -#define DSS_SUBSYS_NAME "CORE"
>> -
>> -#include <linux/kernel.h>
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> -
>> -#include "omapdss.h"
>> -#include "dss.h"
>> -
>> -/* INIT */
>> -static struct platform_driver * const omap_dss_drivers[] = {
>> -	&omap_dsshw_driver,
>> -	&omap_dispchw_driver,
>> -#ifdef CONFIG_OMAP2_DSS_DSI
>> -	&omap_dsihw_driver,
>> -#endif
>> -#ifdef CONFIG_OMAP2_DSS_VENC
>> -	&omap_venchw_driver,
>> -#endif
>> -#ifdef CONFIG_OMAP4_DSS_HDMI
>> -	&omapdss_hdmi4hw_driver,
>> -#endif
>> -#ifdef CONFIG_OMAP5_DSS_HDMI
>> -	&omapdss_hdmi5hw_driver,
>> -#endif
>> -};
>> -
>> -static int __init omap_dss_init(void)
>> -{
>> -	return platform_register_drivers(omap_dss_drivers,
>> -					 ARRAY_SIZE(omap_dss_drivers));
>> -}
>> -
>> -static void __exit omap_dss_exit(void)
>> -{
>> -	platform_unregister_drivers(omap_dss_drivers,
>> -				    ARRAY_SIZE(omap_dss_drivers));
>> -}
>> -
>> -module_init(omap_dss_init);
>> -module_exit(omap_dss_exit);
>> -
>> -MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
>> -MODULE_DESCRIPTION("OMAP2/3 Display Subsystem");
>> -MODULE_LICENSE("GPL v2");
>> -
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
>> index e226324adb69..41d495a360d8 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
>> @@ -1598,3 +1598,40 @@ struct platform_driver omap_dsshw_driver = {
>>  		.suppress_bind_attrs = true,
>>  	},
>>  };
>> +
>> +/* INIT */
>> +static struct platform_driver * const omap_dss_drivers[] = {
>> +	&omap_dsshw_driver,
>> +	&omap_dispchw_driver,
>> +#ifdef CONFIG_OMAP2_DSS_DSI
>> +	&omap_dsihw_driver,
>> +#endif
>> +#ifdef CONFIG_OMAP2_DSS_VENC
>> +	&omap_venchw_driver,
>> +#endif
>> +#ifdef CONFIG_OMAP4_DSS_HDMI
>> +	&omapdss_hdmi4hw_driver,
>> +#endif
>> +#ifdef CONFIG_OMAP5_DSS_HDMI
>> +	&omapdss_hdmi5hw_driver,
>> +#endif
>> +};
>> +
>> +static int __init omap_dss_init(void)
>> +{
>> +	return platform_register_drivers(omap_dss_drivers,
>> +					 ARRAY_SIZE(omap_dss_drivers));
>> +}
>> +
>> +static void __exit omap_dss_exit(void)
>> +{
>> +	platform_unregister_drivers(omap_dss_drivers,
>> +				    ARRAY_SIZE(omap_dss_drivers));
>> +}
>> +
>> +module_init(omap_dss_init);
>> +module_exit(omap_dss_exit);
>> +
>> +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
>> +MODULE_DESCRIPTION("OMAP2/3/4/5 Display Subsystem");
>> +MODULE_LICENSE("GPL v2");
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Jyri,

On Wed, Sep 04, 2019 at 10:17:00AM +0300, Jyri Sarha wrote:
> On 03/09/2019 18:24, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha@ti.com>
> >>
> >> Implement CTM color management property for OMAP CRTC using DSS
> >> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> >> exactly match the CTM property documentation. On DSS the CPR matrix is
> >> applied after gamma table look up. However, it seems stupid to add a
> >> custom property just for that.
> > 
> > In that case the DRM documentation should be updated to mention that
> > both options are allowed.
> 
> Ok, if that is alright. But if we do that, then I guess all the drivers
> implementing CTM should document the point where it is applied in the
> pipeline.

Whatever solution we end up picking, I think it should at least be
discussed with a broader upstream audience and not just swept under the
omapdrm carpet :-)

> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
> >>  1 file changed, 37 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> index 3c5ddbf30e97..d63213dd7d83 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
> >>  	}
> >>  }
> >>  
> >> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
> >> +{
> >> +	uint64_t sign_bit = 1ULL << 63;
> >> +	uint64_t cbits = (uint64_t) coef;
> > 
> > s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
> > there's no need for a space before coef.
> > 
> >> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
> >> +
> >> +	if (cbits & sign_bit)
> >> +		ret = -ret;
> >> +
> >> +	return ret;
> > 
> > Can't this be simplified to 
> > 
> > 	s16 ret = (coef >> 24) & 0x1ff;
> > 
> > 	return coef < 0 ? -ret : ret;
> > 
> 
> No. Clamping is different thing. If the original value is greater than
> what we can present with our 2 magnitude bit, we want to use the maximum
> value, not something that we may have in the LSB end of bits. e.g if
> user-space tries to set the value to 2.0 (= 0x200) we rather present it
> as 1.996 (= 0x1FF) than 0.0 (= 0x000).

Of course, my bad.

Perhaps a stupid question, should we reject out of range values at
atomic check time ?

> >> +}
> >> +
> >> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> >> +					 struct omap_dss_cpr_coefs *cpr)
> >> +{
> >> +	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
> >> +	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
> >> +	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
> >> +	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
> >> +	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
> >> +	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
> >> +	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
> >> +	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
> >> +	cpr->bb = omap_crtc_S31_32_to_s2_8(ctm->matrix[8]);
> >> +}
> >> +
> >>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>  {
> >>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> >> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>  	info.default_color = 0x000000;
> >>  	info.trans_enabled = false;
> >>  	info.partial_alpha_enabled = false;
> >> -	info.cpr_enable = false;
> >> +
> >> +	if (crtc->state->ctm) {
> >> +		struct drm_color_ctm *ctm =
> >> +			(struct drm_color_ctm *) crtc->state->ctm->data;
> >> +
> >> +		info.cpr_enable = true;
> >> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
> > 
> > As an optimisation it would be nice to only write the coefficients when
> > they actually change. That could be implemented on top of this series.
> 
> E.g. apply this ?
> 
> - if (crtc->state->ctm)
> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)

Something like that, but .mgr_setup() should then be taught not to write
unchanged CTM tables to registers. Do you think it would be worth it ?

> >> +	} else {
> >> +		info.cpr_enable = false;
> >> +	}
> >>  
> >>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
> >>  }
> >> @@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
> >>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
> >>  		unsigned int gamma_lut_size = 256;
> >>  
> >> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
> >> +		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
> >>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
> >>  	}
> >>  

-- 
Regards,

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Jyri Sarha]

On 04/09/2019 14:11, Laurent Pinchart wrote:
> Hi Jyri,
> 
> On Wed, Sep 04, 2019 at 10:17:00AM +0300, Jyri Sarha wrote:
>> On 03/09/2019 18:24, Laurent Pinchart wrote:
>>> On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
>>>> From: Jyri Sarha <jsarha@ti.com>
>>>>
>>>> Implement CTM color management property for OMAP CRTC using DSS
>>>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
>>>> exactly match the CTM property documentation. On DSS the CPR matrix is
>>>> applied after gamma table look up. However, it seems stupid to add a
>>>> custom property just for that.
>>>
>>> In that case the DRM documentation should be updated to mention that
>>> both options are allowed.
>>
>> Ok, if that is alright. But if we do that, then I guess all the drivers
>> implementing CTM should document the point where it is applied in the
>> pipeline.
> 
> Whatever solution we end up picking, I think it should at least be
> discussed with a broader upstream audience and not just swept under the
> omapdrm carpet :-)
> 

I'll try to write something and send the next series to wider audience.
Let's see what jury says.

>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
>>>>  1 file changed, 37 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>>> index 3c5ddbf30e97..d63213dd7d83 100644
>>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>>> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
>>>>  	}
>>>>  }
>>>>  
>>>> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
>>>> +{
>>>> +	uint64_t sign_bit = 1ULL << 63;
>>>> +	uint64_t cbits = (uint64_t) coef;
>>>
>>> s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
>>> there's no need for a space before coef.
>>>
>>>> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
>>>> +
>>>> +	if (cbits & sign_bit)
>>>> +		ret = -ret;
>>>> +
>>>> +	return ret;
>>>
>>> Can't this be simplified to 
>>>
>>> 	s16 ret = (coef >> 24) & 0x1ff;
>>>
>>> 	return coef < 0 ? -ret : ret;
>>>
>>
>> No. Clamping is different thing. If the original value is greater than
>> what we can present with our 2 magnitude bit, we want to use the maximum
>> value, not something that we may have in the LSB end of bits. e.g if
>> user-space tries to set the value to 2.0 (= 0x200) we rather present it
>> as 1.996 (= 0x1FF) than 0.0 (= 0x000).
> 
> Of course, my bad.
> 
> Perhaps a stupid question, should we reject out of range values at
> atomic check time ?
> 

I've at least seen CSC matrices with 2.0 values, so I think we should
accept those and use clamping, but maybe we should reject CTMs with
values far bigger than what we can represent. Such matrices would hardly
work the way they were intended.

>>>> +}
>>>> +
>>>> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
>>>> +					 struct omap_dss_cpr_coefs *cpr)
>>>> +{
>>>> +	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
>>>> +	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
>>>> +	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
>>>> +	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
>>>> +	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
>>>> +	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
>>>> +	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
>>>> +	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
>>>> +	cpr->bb = omap_crtc_S31_32_to_s2_8(ctm->matrix[8]);
>>>> +}
>>>> +
>>>>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>  {
>>>>  	struct omap_drm_private *priv = crtc->dev->dev_private;
>>>> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>  	info.default_color = 0x000000;
>>>>  	info.trans_enabled = false;
>>>>  	info.partial_alpha_enabled = false;
>>>> -	info.cpr_enable = false;
>>>> +
>>>> +	if (crtc->state->ctm) {
>>>> +		struct drm_color_ctm *ctm =
>>>> +			(struct drm_color_ctm *) crtc->state->ctm->data;
>>>> +
>>>> +		info.cpr_enable = true;
>>>> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
>>>
>>> As an optimisation it would be nice to only write the coefficients when
>>> they actually change. That could be implemented on top of this series.
>>
>> E.g. apply this ?
>>
>> - if (crtc->state->ctm)
>> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)
> 
> Something like that, but .mgr_setup() should then be taught not to write
> unchanged CTM tables to registers. Do you think it would be worth it ?
> 

Hmmm, jess I should do it like this:
if (crtc->state->color_mgmt_changed) {
	if (crtc->state->ctm) {
...
>>>> +	} else {
>>>> +		info.cpr_enable = false;
>>>> +	}
}

This way the whole CPR functionality is turned off, if the there is no
CTM in the crtc state.

>>>>  
>>>>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
>>>>  }
>>>> @@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>>>>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
>>>>  		unsigned int gamma_lut_size = 256;
>>>>  
>>>> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
>>>> +		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
>>>>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
>>>>  	}
>>>>  
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Ilia Mirkin]

On Wed, Sep 4, 2019 at 4:08 PM Jyri Sarha <jsarha@ti.com> wrote:
>
> On 04/09/2019 14:11, Laurent Pinchart wrote:
> > Hi Jyri,
> >
> > On Wed, Sep 04, 2019 at 10:17:00AM +0300, Jyri Sarha wrote:
> >> On 03/09/2019 18:24, Laurent Pinchart wrote:
> >>> On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
> >>>> From: Jyri Sarha <jsarha@ti.com>
> >>>>
> >>>> Implement CTM color management property for OMAP CRTC using DSS
> >>>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> >>>> exactly match the CTM property documentation. On DSS the CPR matrix is
> >>>> applied after gamma table look up. However, it seems stupid to add a
> >>>> custom property just for that.
> >>>
> >>> In that case the DRM documentation should be updated to mention that
> >>> both options are allowed.
> >>
> >> Ok, if that is alright. But if we do that, then I guess all the drivers
> >> implementing CTM should document the point where it is applied in the
> >> pipeline.
> >
> > Whatever solution we end up picking, I think it should at least be
> > discussed with a broader upstream audience and not just swept under the
> > omapdrm carpet :-)
> >
>
> I'll try to write something and send the next series to wider audience.
> Let's see what jury says.

In case it's useful ... the pipeline normally goes degamma -> ctm ->
gamma. If your ctm is applied after gamma, perhaps you can just rename
"gamma" to "degamma" and be done? (There's the unfortunate case of
legacy gamma which does end up in "GAMMA" when using atomic helpers.
But in such a case, you won't have a CTM.)

>
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>> ---
> >>>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
> >>>>  1 file changed, 37 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> index 3c5ddbf30e97..d63213dd7d83 100644
> >>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
> >>>>    }
> >>>>  }
> >>>>
> >>>> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
> >>>> +{
> >>>> +  uint64_t sign_bit = 1ULL << 63;
> >>>> +  uint64_t cbits = (uint64_t) coef;
> >>>
> >>> s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
> >>> there's no need for a space before coef.
> >>>
> >>>> +  s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
> >>>> +
> >>>> +  if (cbits & sign_bit)
> >>>> +          ret = -ret;
> >>>> +
> >>>> +  return ret;
> >>>
> >>> Can't this be simplified to
> >>>
> >>>     s16 ret = (coef >> 24) & 0x1ff;
> >>>
> >>>     return coef < 0 ? -ret : ret;
> >>>
> >>
> >> No. Clamping is different thing. If the original value is greater than
> >> what we can present with our 2 magnitude bit, we want to use the maximum
> >> value, not something that we may have in the LSB end of bits. e.g if
> >> user-space tries to set the value to 2.0 (= 0x200) we rather present it
> >> as 1.996 (= 0x1FF) than 0.0 (= 0x000).
> >
> > Of course, my bad.
> >
> > Perhaps a stupid question, should we reject out of range values at
> > atomic check time ?
> >
>
> I've at least seen CSC matrices with 2.0 values, so I think we should
> accept those and use clamping, but maybe we should reject CTMs with
> values far bigger than what we can represent. Such matrices would hardly
> work the way they were intended.

I clamped in nouveau. Not 100% sure it's the right policy. Having
something consistent across drivers is probably good. I don't think I
came up with clamping all by myself, so someone else must have been
doing it. (https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/nouveau?id=88b703527ba70659365d989f29579f1292ebf9c3
-- see csc_drm_to_base.)

Cheers,

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Jyri,

On Wed, Sep 04, 2019 at 11:08:20PM +0300, Jyri Sarha wrote:
> On 04/09/2019 14:11, Laurent Pinchart wrote:
> > On Wed, Sep 04, 2019 at 10:17:00AM +0300, Jyri Sarha wrote:
> >> On 03/09/2019 18:24, Laurent Pinchart wrote:
> >>> On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
> >>>> From: Jyri Sarha <jsarha@ti.com>
> >>>>
> >>>> Implement CTM color management property for OMAP CRTC using DSS
> >>>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> >>>> exactly match the CTM property documentation. On DSS the CPR matrix is
> >>>> applied after gamma table look up. However, it seems stupid to add a
> >>>> custom property just for that.
> >>>
> >>> In that case the DRM documentation should be updated to mention that
> >>> both options are allowed.
> >>
> >> Ok, if that is alright. But if we do that, then I guess all the drivers
> >> implementing CTM should document the point where it is applied in the
> >> pipeline.
> > 
> > Whatever solution we end up picking, I think it should at least be
> > discussed with a broader upstream audience and not just swept under the
> > omapdrm carpet :-)
> 
> I'll try to write something and send the next series to wider audience.
> Let's see what jury says.
> 
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>> ---
> >>>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
> >>>>  1 file changed, 37 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> index 3c5ddbf30e97..d63213dd7d83 100644
> >>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>>> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
> >>>> +{
> >>>> +	uint64_t sign_bit = 1ULL << 63;
> >>>> +	uint64_t cbits = (uint64_t) coef;
> >>>
> >>> s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
> >>> there's no need for a space before coef.
> >>>
> >>>> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
> >>>> +
> >>>> +	if (cbits & sign_bit)
> >>>> +		ret = -ret;
> >>>> +
> >>>> +	return ret;
> >>>
> >>> Can't this be simplified to 
> >>>
> >>> 	s16 ret = (coef >> 24) & 0x1ff;
> >>>
> >>> 	return coef < 0 ? -ret : ret;
> >>>
> >>
> >> No. Clamping is different thing. If the original value is greater than
> >> what we can present with our 2 magnitude bit, we want to use the maximum
> >> value, not something that we may have in the LSB end of bits. e.g if
> >> user-space tries to set the value to 2.0 (= 0x200) we rather present it
> >> as 1.996 (= 0x1FF) than 0.0 (= 0x000).
> > 
> > Of course, my bad.
> > 
> > Perhaps a stupid question, should we reject out of range values at
> > atomic check time ?
> 
> I've at least seen CSC matrices with 2.0 values, so I think we should
> accept those and use clamping, but maybe we should reject CTMs with
> values far bigger than what we can represent. Such matrices would hardly
> work the way they were intended.

I tend to be conservative in such cases and reject invalid values, but
if you think there are users in the wild that would break, then clamping
is fine with me too. If we want to reject values higher than 2.0 and
clamp 2.0 to 0x1ff then that should be done in atomic check, and here we
could convert the values blindly.

> >>>> +}
> >>>> +
> >>>> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> >>>> +					 struct omap_dss_cpr_coefs *cpr)
> >>>> +{
> >>>> +	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
> >>>> +	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
> >>>> +	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
> >>>> +	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
> >>>> +	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
> >>>> +	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
> >>>> +	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
> >>>> +	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
> >>>> +	cpr->bb = omap_crtc_S31_32_to_s2_8(ctm->matrix[8]);
> >>>> +}
> >>>> +
> >>>>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>>>  {
> >>>>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> >>>> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>>>  	info.default_color = 0x000000;
> >>>>  	info.trans_enabled = false;
> >>>>  	info.partial_alpha_enabled = false;
> >>>> -	info.cpr_enable = false;
> >>>> +
> >>>> +	if (crtc->state->ctm) {
> >>>> +		struct drm_color_ctm *ctm =
> >>>> +			(struct drm_color_ctm *) crtc->state->ctm->data;
> >>>> +
> >>>> +		info.cpr_enable = true;
> >>>> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
> >>>
> >>> As an optimisation it would be nice to only write the coefficients when
> >>> they actually change. That could be implemented on top of this series.
> >>
> >> E.g. apply this ?
> >>
> >> - if (crtc->state->ctm)
> >> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)
> > 
> > Something like that, but .mgr_setup() should then be taught not to write
> > unchanged CTM tables to registers. Do you think it would be worth it ?
> 
> Hmmm, jess I should do it like this:
> if (crtc->state->color_mgmt_changed) {
> 	if (crtc->state->ctm) {
> ...
> >>>> +	} else {
> >>>> +		info.cpr_enable = false;
> >>>> +	}
> }
> 
> This way the whole CPR functionality is turned off, if the there is no
> CTM in the crtc state.

Yes, but you would also need to update .mgr_setup() :-) A new
color_mgmt_changed flag would be needed in the info structure too.

> >>>>  
> >>>>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
> >>>>  }
> >>>> @@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
> >>>>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
> >>>>  		unsigned int gamma_lut_size = 256;
> >>>>  
> >>>> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
> >>>> +		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
> >>>>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
> >>>>  	}
> >>>>  

-- 
Regards,

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

Re: [PATCH 5/7] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Jyri Sarha]

On 03/09/2019 18:32, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@ti.com>
>>
>> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
>> omap_plane.c and dispc.c. The supported encodings and ranges are:
>>
>> For COLOR_ENCODING:
>> - YCbCt BT.601 (default)
> 
> Did you mean YCbCr ?
> >> - YCbCt BT.709
>>
>> For COLOR_RANGE:
>> - YCbCt limited range
>> - YCbCt full range (default)
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 119 ++++++++++++++++++++------
>>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
>>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 +++++++
>>  3 files changed, 127 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> index 7d87d60edb80..40ddb28ee7aa 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
>>  #undef CVAL
>>  }
>>  
>> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
>> +/* YUV -> RGB, ITU-R BT.601, full range */
>> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> 
> static const is usually preferred over const static, here and below.
> 
>> +	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, full range */
>> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
>> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
>> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
>> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
>> +	true,                   /* full 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 static 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 static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {
> 
> That should be coefs_rgb2yuv_bt709_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 */
>> +};
> 
> Why is coefs_rgb2yuv_bt709_full not supported ? Actually
> coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.
> 

I must have simply forgotten. This is an old patch and I can not
remember exactly. But I remember that I wanted to add all CSCs at one
time so that I do not need start from the beginning when ever a new
conversion is asked. For the moment rgb to yuv conversions are only used
for write back and it currently only uses the default BT.601 Full range.

I'll add rgb2yuv_bt709_full. Or should I remove all the unused CSCs? Or
maybe put them behind some ifdef so that they do not bloat the kernel?

>> +
>> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
>> +			     enum omap_plane_id plane,
>> +			     enum drm_color_encoding color_encoding,
>> +			     enum drm_color_range color_range)
>>  {
>> -	int i;
>> -	int num_ovl = dispc_get_num_ovls(dispc);
>> -
>> -	/* 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 csc_coef_yuv2rgb *csc;
>>  
>> -	/* 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 */
>> -	};
>> +	switch (color_encoding) {
>> +	case DRM_COLOR_YCBCR_BT601:
>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>> +			csc = &coefs_yuv2rgb_bt601_full;
>> +		else
>> +			csc = &coefs_yuv2rgb_bt601_lim;
>> +		break;
>> +	case DRM_COLOR_YCBCR_BT709:
>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>> +			csc = &coefs_yuv2rgb_bt709_full;
>> +		else
>> +			csc = &coefs_yuv2rgb_bt709_lim;
>> +		break;
>> +	default:
>> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
>> +		       color_encoding, plane);
>> +		return -EINVAL;
>> +	}
>>  
>> -	for (i = 1; i < num_ovl; i++)
>> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
>> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
>>  
>> -	if (dispc->feat->has_writeback)
>> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
>> +	return 0;
>>  }
>>  
>>  static void dispc_ovl_set_ba0(struct dispc_device *dispc,
>> @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>>  				  u8 pre_mult_alpha, u8 global_alpha,
>>  				  enum omap_dss_rotation_type rotation_type,
>>  				  bool replication, const struct videomode *vm,
>> -				  bool mem_to_mem)
>> +				  bool mem_to_mem,
>> +				  enum drm_color_encoding color_encoding,
>> +				  enum drm_color_range color_range)
>>  {
>>  	bool five_taps = true;
>>  	bool fieldmode = false;
>> @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>>  				      fieldmode, fourcc, rotation);
>>  		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
>>  		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
>> +
>> +		if (plane != OMAP_DSS_WB)
>> +			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
>>  	}
>>  
>>  	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
>> @@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
>>  		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
>>  		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
>>  		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
>> -		oi->rotation_type, replication, vm, mem_to_mem);
>> +		oi->rotation_type, replication, vm, mem_to_mem,
>> +		oi->color_encoding, oi->color_range);
>>  
>>  	return r;
>>  }
>> @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
>>  		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
>>  		wi->height, wi->fourcc, wi->rotation, zorder,
>>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
>> -		replication, vm, mem_to_mem);
>> +		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
>> +		DRM_COLOR_YCBCR_LIMITED_RANGE);
>>  	if (r)
>>  		return r;
>>  
>> @@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
>>  	    dispc->feat->has_gamma_table)
>>  		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
>>  
>> -	dispc_setup_color_conv_coef(dispc);
>> +	if (dispc->feat->has_writeback)
>> +		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
>>  
>>  	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
>>  
>> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> index 79f6b195c7cf..1430cab6b877 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> @@ -14,6 +14,7 @@
>>  #include <linux/platform_data/omapdss.h>
>>  #include <uapi/drm/drm_mode.h>
>>  #include <drm/drm_crtc.h>
>> +#include <drm/drm_color_mgmt.h>
> 
> Alphabetical order ? While at is you coulde remove uapi/
> 
>>  
>>  #define DISPC_IRQ_FRAMEDONE		(1 << 0)
>>  #define DISPC_IRQ_VSYNC			(1 << 1)
>> @@ -243,6 +244,9 @@ struct omap_overlay_info {
>>  	u8 global_alpha;
>>  	u8 pre_mult_alpha;
>>  	u8 zorder;
>> +
>> +	enum drm_color_encoding color_encoding;
>> +	enum drm_color_range color_range;
>>  };
>>  
>>  struct omap_overlay_manager_info {
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 73ec99819a3d..db8e917260df 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>>  		info.pre_mult_alpha = 1;
>>  	else
>>  		info.pre_mult_alpha = 0;
>> +	info.color_encoding = state->color_encoding;
>> +	info.color_range = state->color_range;
>>  
>>  	/* update scanout: */
>>  	omap_framebuffer_update_scanout(state->fb, state, &info);
>> @@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
>>  	 */
>>  	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>>  			   ? 0 : omap_plane->id;
>> +	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
>> +	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
>>  }
>>  
>>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
>> @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
>>  	.atomic_get_property = omap_plane_atomic_get_property,
>>  };
>>  
>> +static bool omap_plane_supports_yuv(struct drm_plane *plane)
>> +{
>> +	struct omap_drm_private *priv = plane->dev->dev_private;
>> +	struct omap_plane *omap_plane = to_omap_plane(plane);
>> +	const u32 *formats =
>> +		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
>> +	int i;
> 
> unsigned int i ?
> 
>> +
>> +	for (i = 0; formats[i]; i++)
>> +		if (formats[i] == DRM_FORMAT_YUYV ||
>> +		    formats[i] == DRM_FORMAT_UYVY ||
>> +		    formats[i] == DRM_FORMAT_NV12)
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>>  static const char *plane_id_to_name[] = {
>>  	[OMAP_DSS_GFX] = "gfx",
>>  	[OMAP_DSS_VIDEO1] = "vid1",
>> @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>>  	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
>>  					     BIT(DRM_MODE_BLEND_COVERAGE));
>>  
>> +	if (omap_plane_supports_yuv(plane))
>> +		drm_plane_create_color_properties(plane,
>> +					BIT(DRM_COLOR_YCBCR_BT601) |
>> +					BIT(DRM_COLOR_YCBCR_BT709),
>> +					BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
>> +					BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
>> +					DRM_COLOR_YCBCR_BT601,
>> +					DRM_COLOR_YCBCR_FULL_RANGE);
>> +
>>  	return plane;
>>  
>>  error:
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Laurent Pinchart]

Hi Jyri,

On Thu, Sep 05, 2019 at 12:24:37PM +0300, Jyri Sarha wrote:
> On 03/09/2019 18:32, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha@ti.com>
> >>
> >> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> >> omap_plane.c and dispc.c. The supported encodings and ranges are:
> >>
> >> For COLOR_ENCODING:
> >> - YCbCt BT.601 (default)
> > 
> > Did you mean YCbCr ?
> > >> - YCbCt BT.709
> >>
> >> For COLOR_RANGE:
> >> - YCbCt limited range
> >> - YCbCt full range (default)
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 119 ++++++++++++++++++++------
> >>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
> >>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 +++++++
> >>  3 files changed, 127 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> index 7d87d60edb80..40ddb28ee7aa 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
> >>  #undef CVAL
> >>  }
> >>  
> >> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> >> +/* YUV -> RGB, ITU-R BT.601, full range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> > 
> > static const is usually preferred over const static, here and below.
> > 
> >> +	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, full range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> >> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
> >> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> >> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
> >> +	true,                   /* full 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 static 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 static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {
> > 
> > That should be coefs_rgb2yuv_bt709_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 */
> >> +};
> > 
> > Why is coefs_rgb2yuv_bt709_full not supported ? Actually
> > coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.
> > 
> 
> I must have simply forgotten. This is an old patch and I can not
> remember exactly. But I remember that I wanted to add all CSCs at one
> time so that I do not need start from the beginning when ever a new
> conversion is asked. For the moment rgb to yuv conversions are only used
> for write back and it currently only uses the default BT.601 Full range.
> 
> I'll add rgb2yuv_bt709_full. Or should I remove all the unused CSCs? Or
> maybe put them behind some ifdef so that they do not bloat the kernel?

Commented-out code is frowned upon. I would just drop the unused data.

> >> +
> >> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> >> +			     enum omap_plane_id plane,
> >> +			     enum drm_color_encoding color_encoding,
> >> +			     enum drm_color_range color_range)
> >>  {
> >> -	int i;
> >> -	int num_ovl = dispc_get_num_ovls(dispc);
> >> -
> >> -	/* 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 csc_coef_yuv2rgb *csc;
> >>  
> >> -	/* 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 */
> >> -	};
> >> +	switch (color_encoding) {
> >> +	case DRM_COLOR_YCBCR_BT601:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt601_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt601_lim;
> >> +		break;
> >> +	case DRM_COLOR_YCBCR_BT709:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt709_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt709_lim;
> >> +		break;
> >> +	default:
> >> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
> >> +		       color_encoding, plane);
> >> +		return -EINVAL;
> >> +	}
> >>  
> >> -	for (i = 1; i < num_ovl; i++)
> >> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> >> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
> >>  
> >> -	if (dispc->feat->has_writeback)
> >> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> >> +	return 0;
> >>  }
> >>  
> >>  static void dispc_ovl_set_ba0(struct dispc_device *dispc,
> >> @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
> >>  				  u8 pre_mult_alpha, u8 global_alpha,
> >>  				  enum omap_dss_rotation_type rotation_type,
> >>  				  bool replication, const struct videomode *vm,
> >> -				  bool mem_to_mem)
> >> +				  bool mem_to_mem,
> >> +				  enum drm_color_encoding color_encoding,
> >> +				  enum drm_color_range color_range)
> >>  {
> >>  	bool five_taps = true;
> >>  	bool fieldmode = false;
> >> @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
> >>  				      fieldmode, fourcc, rotation);
> >>  		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
> >>  		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
> >> +
> >> +		if (plane != OMAP_DSS_WB)
> >> +			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
> >>  	}
> >>  
> >>  	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
> >> @@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
> >>  		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
> >>  		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
> >>  		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
> >> -		oi->rotation_type, replication, vm, mem_to_mem);
> >> +		oi->rotation_type, replication, vm, mem_to_mem,
> >> +		oi->color_encoding, oi->color_range);
> >>  
> >>  	return r;
> >>  }
> >> @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
> >>  		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
> >>  		wi->height, wi->fourcc, wi->rotation, zorder,
> >>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
> >> -		replication, vm, mem_to_mem);
> >> +		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
> >> +		DRM_COLOR_YCBCR_LIMITED_RANGE);
> >>  	if (r)
> >>  		return r;
> >>  
> >> @@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
> >>  	    dispc->feat->has_gamma_table)
> >>  		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
> >>  
> >> -	dispc_setup_color_conv_coef(dispc);
> >> +	if (dispc->feat->has_writeback)
> >> +		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
> >>  
> >>  	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
> >>  
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> index 79f6b195c7cf..1430cab6b877 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> @@ -14,6 +14,7 @@
> >>  #include <linux/platform_data/omapdss.h>
> >>  #include <uapi/drm/drm_mode.h>
> >>  #include <drm/drm_crtc.h>
> >> +#include <drm/drm_color_mgmt.h>
> > 
> > Alphabetical order ? While at is you coulde remove uapi/
> > 
> >>  
> >>  #define DISPC_IRQ_FRAMEDONE		(1 << 0)
> >>  #define DISPC_IRQ_VSYNC			(1 << 1)
> >> @@ -243,6 +244,9 @@ struct omap_overlay_info {
> >>  	u8 global_alpha;
> >>  	u8 pre_mult_alpha;
> >>  	u8 zorder;
> >> +
> >> +	enum drm_color_encoding color_encoding;
> >> +	enum drm_color_range color_range;
> >>  };
> >>  
> >>  struct omap_overlay_manager_info {
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> index 73ec99819a3d..db8e917260df 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
> >>  		info.pre_mult_alpha = 1;
> >>  	else
> >>  		info.pre_mult_alpha = 0;
> >> +	info.color_encoding = state->color_encoding;
> >> +	info.color_range = state->color_range;
> >>  
> >>  	/* update scanout: */
> >>  	omap_framebuffer_update_scanout(state->fb, state, &info);
> >> @@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
> >>  	 */
> >>  	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> >>  			   ? 0 : omap_plane->id;
> >> +	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
> >> +	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
> >>  }
> >>  
> >>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
> >> @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
> >>  	.atomic_get_property = omap_plane_atomic_get_property,
> >>  };
> >>  
> >> +static bool omap_plane_supports_yuv(struct drm_plane *plane)
> >> +{
> >> +	struct omap_drm_private *priv = plane->dev->dev_private;
> >> +	struct omap_plane *omap_plane = to_omap_plane(plane);
> >> +	const u32 *formats =
> >> +		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
> >> +	int i;
> > 
> > unsigned int i ?
> > 
> >> +
> >> +	for (i = 0; formats[i]; i++)
> >> +		if (formats[i] == DRM_FORMAT_YUYV ||
> >> +		    formats[i] == DRM_FORMAT_UYVY ||
> >> +		    formats[i] == DRM_FORMAT_NV12)
> >> +			return true;
> >> +
> >> +	return false;
> >> +}
> >> +
> >>  static const char *plane_id_to_name[] = {
> >>  	[OMAP_DSS_GFX] = "gfx",
> >>  	[OMAP_DSS_VIDEO1] = "vid1",
> >> @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
> >>  	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
> >>  					     BIT(DRM_MODE_BLEND_COVERAGE));
> >>  
> >> +	if (omap_plane_supports_yuv(plane))
> >> +		drm_plane_create_color_properties(plane,
> >> +					BIT(DRM_COLOR_YCBCR_BT601) |
> >> +					BIT(DRM_COLOR_YCBCR_BT709),
> >> +					BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
> >> +					BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> >> +					DRM_COLOR_YCBCR_BT601,
> >> +					DRM_COLOR_YCBCR_FULL_RANGE);
> >> +
> >>  	return plane;
> >>  
> >>  error:

-- 
Regards,

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Jyri Sarha]

On 05/09/2019 00:52, Laurent Pinchart wrote:
>>>>>>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>>>  {
>>>>>>  	struct omap_drm_private *priv = crtc->dev->dev_private;
>>>>>> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>>>  	info.default_color = 0x000000;
>>>>>>  	info.trans_enabled = false;
>>>>>>  	info.partial_alpha_enabled = false;
>>>>>> -	info.cpr_enable = false;
>>>>>> +
>>>>>> +	if (crtc->state->ctm) {
>>>>>> +		struct drm_color_ctm *ctm =
>>>>>> +			(struct drm_color_ctm *) crtc->state->ctm->data;
>>>>>> +
>>>>>> +		info.cpr_enable = true;
>>>>>> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
>>>>> As an optimisation it would be nice to only write the coefficients when
>>>>> they actually change. That could be implemented on top of this series.
>>>> E.g. apply this ?
>>>>
>>>> - if (crtc->state->ctm)
>>>> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)
>>> Something like that, but .mgr_setup() should then be taught not to write
>>> unchanged CTM tables to registers. Do you think it would be worth it ?
>> Hmmm, jess I should do it like this:
>> if (crtc->state->color_mgmt_changed) {
>> 	if (crtc->state->ctm) {
>> ...
>>>>>> +	} else {
>>>>>> +		info.cpr_enable = false;
>>>>>> +	}
>> }
>>
>> This way the whole CPR functionality is turned off, if the there is no
>> CTM in the crtc state.
> Yes, but you would also need to update .mgr_setup() :-) A new
> color_mgmt_changed flag would be needed in the info structure too.
> 

I am starting to thing that such an "optimization" may not be worth the
added complexity. The arithmetic and writing three registers is not that
costly and we do not commit a new crtc state that often.

If we later consider otherwise we can add the optimization as a separate
patch.

BR,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Jyri,

On Thu, Sep 05, 2019 at 01:00:51PM +0300, Jyri Sarha wrote:
> On 05/09/2019 00:52, Laurent Pinchart wrote:
> >>>>>>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>>>>>  {
> >>>>>>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> >>>>>> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>>>>>  	info.default_color = 0x000000;
> >>>>>>  	info.trans_enabled = false;
> >>>>>>  	info.partial_alpha_enabled = false;
> >>>>>> -	info.cpr_enable = false;
> >>>>>> +
> >>>>>> +	if (crtc->state->ctm) {
> >>>>>> +		struct drm_color_ctm *ctm =
> >>>>>> +			(struct drm_color_ctm *) crtc->state->ctm->data;
> >>>>>> +
> >>>>>> +		info.cpr_enable = true;
> >>>>>> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
> >>>>>
> >>>>> As an optimisation it would be nice to only write the coefficients when
> >>>>> they actually change. That could be implemented on top of this series.
> >>>>
> >>>> E.g. apply this ?
> >>>>
> >>>> - if (crtc->state->ctm)
> >>>> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)
> >>>
> >>> Something like that, but .mgr_setup() should then be taught not to write
> >>> unchanged CTM tables to registers. Do you think it would be worth it ?
> >>
> >> Hmmm, jess I should do it like this:
> >> if (crtc->state->color_mgmt_changed) {
> >> 	if (crtc->state->ctm) {
> >> ...
> >>>>>> +	} else {
> >>>>>> +		info.cpr_enable = false;
> >>>>>> +	}
> >> }
> >>
> >> This way the whole CPR functionality is turned off, if the there is no
> >> CTM in the crtc state.
> >
> > Yes, but you would also need to update .mgr_setup() :-) A new
> > color_mgmt_changed flag would be needed in the info structure too.
> 
> I am starting to thing that such an "optimization" may not be worth the
> added complexity. The arithmetic and writing three registers is not that
> costly and we do not commit a new crtc state that often.

We call omap_crtc_write_crtc_properties() in omap_crtc_atomic_flush(),
so that's at every page flip...

> If we later consider otherwise we can add the optimization as a separate
> patch.

-- 
Regards,

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Jyri Sarha]

On 05/09/2019 13:05, Laurent Pinchart wrote:
> Hi Jyri,
> 
> On Thu, Sep 05, 2019 at 01:00:51PM +0300, Jyri Sarha wrote:
>> On 05/09/2019 00:52, Laurent Pinchart wrote:
>>>>>>>>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>>>>>  {
>>>>>>>>  	struct omap_drm_private *priv = crtc->dev->dev_private;
>>>>>>>> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>>>>>>>>  	info.default_color = 0x000000;
>>>>>>>>  	info.trans_enabled = false;
>>>>>>>>  	info.partial_alpha_enabled = false;
>>>>>>>> -	info.cpr_enable = false;
>>>>>>>> +
>>>>>>>> +	if (crtc->state->ctm) {
>>>>>>>> +		struct drm_color_ctm *ctm =
>>>>>>>> +			(struct drm_color_ctm *) crtc->state->ctm->data;
>>>>>>>> +
>>>>>>>> +		info.cpr_enable = true;
>>>>>>>> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
>>>>>>>
>>>>>>> As an optimisation it would be nice to only write the coefficients when
>>>>>>> they actually change. That could be implemented on top of this series.
>>>>>>
>>>>>> E.g. apply this ?
>>>>>>
>>>>>> - if (crtc->state->ctm)
>>>>>> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)
>>>>>
>>>>> Something like that, but .mgr_setup() should then be taught not to write
>>>>> unchanged CTM tables to registers. Do you think it would be worth it ?
>>>>
>>>> Hmmm, jess I should do it like this:
>>>> if (crtc->state->color_mgmt_changed) {
>>>> 	if (crtc->state->ctm) {
>>>> ...
>>>>>>>> +	} else {
>>>>>>>> +		info.cpr_enable = false;
>>>>>>>> +	}
>>>> }
>>>>
>>>> This way the whole CPR functionality is turned off, if the there is no
>>>> CTM in the crtc state.
>>>
>>> Yes, but you would also need to update .mgr_setup() :-) A new
>>> color_mgmt_changed flag would be needed in the info structure too.
>>
>> I am starting to thing that such an "optimization" may not be worth the
>> added complexity. The arithmetic and writing three registers is not that
>> costly and we do not commit a new crtc state that often.
> 
> We call omap_crtc_write_crtc_properties() in omap_crtc_atomic_flush(),
> so that's at every page flip...
> 

Still, the mgr_setup() accesses five different registers even if we do
not touch CPR settings (another 4 registers). All of those have static
values in the mainline omapdrm (there are custom properties to control
those in ti-linux).

I would rather keep this patch as it is and implement another one with a
cached struct omap_overlay_manager_info, that calls mgr_setup() only if
the info values have changed.

With the cached values in place the unneeded conversion arithmetic can
also be skipped based on color_mgmt_changed.

BR,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/omap: tweak HDMI DDC timings

[Tomi Valkeinen]

On 03/09/2019 17:23, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the path.
> 
> On Mon, Sep 02, 2019 at 03:53:54PM +0300, Tomi Valkeinen wrote:
>> From: Alejandro Hernandez <ajhernandez@ti.com>
>>
>> A "HDMI I2C Master Error" is sometimes reported with the current DDC SCL
>> timings. The current settings for a 10us SCL period (100 KHz) causes the
>> error with some displays.  This patch increases the SCL signal period
>> from 10us to 10.2us, with the new settings the error is not observed
>>
> 
> It would be useful to document what those "some displays" are if you
> can.

Unfortunately I have no idea. This was quite a while ago.

Alejandro, do you recall?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/omap: fix missing scaler pixel fmt limitations

[Tomi Valkeinen]

On 03/09/2019 18:12, Laurent Pinchart wrote:

>> @@ -2498,6 +2499,19 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc,
>>   	if (width == out_width && height == out_height)
>>   		return 0;
>>   
>> +	if (dispc->feat->supported_scaler_color_modes) {
>> +		const u32 *modes = dispc->feat->supported_scaler_color_modes;
>> +		int i;
> 
> i is never negative and can thus be an unsigned int. Apart from that,

Thanks, fixed that.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/omap: tweak HDMI DDC timings

[Alejandro Hernandez]

On 9/26/19 8:54 AM, Tomi Valkeinen wrote:
> On 03/09/2019 17:23, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> Thank you for the path.
>>
>> On Mon, Sep 02, 2019 at 03:53:54PM +0300, Tomi Valkeinen wrote:
>>> From: Alejandro Hernandez <ajhernandez@ti.com>
>>>
>>> A "HDMI I2C Master Error" is sometimes reported with the current DDC 
>>> SCL
>>> timings. The current settings for a 10us SCL period (100 KHz) causes 
>>> the
>>> error with some displays.  This patch increases the SCL signal period
>>> from 10us to 10.2us, with the new settings the error is not observed
>>>
>>
>> It would be useful to document what those "some displays" are if you
>> can.
>
> Unfortunately I have no idea. This was quite a while ago.
>
> Alejandro, do you recall?

Not at this point, we threw out a bunch of equipment out during the move 
to the new office last year. I could only find one and it has no 
identifying information on it.

Alejandro

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Tomi Valkeinen]

Hi,

On 04/09/2019 23:20, Ilia Mirkin wrote:

>>>>>> Implement CTM color management property for OMAP CRTC using DSS
>>>>>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
>>>>>> exactly match the CTM property documentation. On DSS the CPR matrix is
>>>>>> applied after gamma table look up. However, it seems stupid to add a
>>>>>> custom property just for that.
>>>>>
>>>>> In that case the DRM documentation should be updated to mention that
>>>>> both options are allowed.
>>>>
>>>> Ok, if that is alright. But if we do that, then I guess all the drivers
>>>> implementing CTM should document the point where it is applied in the
>>>> pipeline.
>>>
>>> Whatever solution we end up picking, I think it should at least be
>>> discussed with a broader upstream audience and not just swept under the
>>> omapdrm carpet :-)
>>>
>>
>> I'll try to write something and send the next series to wider audience.
>> Let's see what jury says.
> 
> In case it's useful ... the pipeline normally goes degamma -> ctm ->
> gamma. If your ctm is applied after gamma, perhaps you can just rename
> "gamma" to "degamma" and be done? (There's the unfortunate case of
> legacy gamma which does end up in "GAMMA" when using atomic helpers.
> But in such a case, you won't have a CTM.)

Waking up old thread, as I started looking at these patches again. So the problem was that DRM
defines the output color transformations as:

degamma -> ctm -> gamma -> out

whereas OMAP DSS has

gamma -> ctm -> out

The omapdrm driver could declare the gamma table as degamma, as suggested by Ilia, and for the
legacy drmModeCrtcSetGamma, the omapdrm driver could implement its own version, and use the degamma
table internally (with no ctm).

For legacy, that would work fine and as expected, but I think the atomic version would be a bit odd,
with only degamma, and no gamma.

Quick grep for drm_crtc_enable_color_mgmt shows that there are other drivers that have CTM and
gamma, but no degamma. I wonder if all those have ctm -> gamma -> out, or are they similar to OMAP DSS.

Any thoughts on how to proceed with this?

Should we have a property that describes the order? Or new property name for gamma before ctm
(PREGAMMA)? Or just have it as degamma, and let the userspace deal with it (i.e. figure out there's
no gamma, but there's degamma, so use degamma)?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3524 bytes --]

On Mon, 21 Sep 2020 14:08:48 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> Hi,
> 
> On 04/09/2019 23:20, Ilia Mirkin wrote:
> 
> >>>>>> Implement CTM color management property for OMAP CRTC using DSS
> >>>>>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> >>>>>> exactly match the CTM property documentation. On DSS the CPR matrix is
> >>>>>> applied after gamma table look up. However, it seems stupid to add a
> >>>>>> custom property just for that.  
> >>>>>
> >>>>> In that case the DRM documentation should be updated to mention that
> >>>>> both options are allowed.  
> >>>>
> >>>> Ok, if that is alright. But if we do that, then I guess all the drivers
> >>>> implementing CTM should document the point where it is applied in the
> >>>> pipeline.  
> >>>
> >>> Whatever solution we end up picking, I think it should at least be
> >>> discussed with a broader upstream audience and not just swept under the
> >>> omapdrm carpet :-)
> >>>  
> >>
> >> I'll try to write something and send the next series to wider audience.
> >> Let's see what jury says.  
> > 
> > In case it's useful ... the pipeline normally goes degamma -> ctm ->
> > gamma. If your ctm is applied after gamma, perhaps you can just rename
> > "gamma" to "degamma" and be done? (There's the unfortunate case of
> > legacy gamma which does end up in "GAMMA" when using atomic helpers.
> > But in such a case, you won't have a CTM.)  
> 
> Waking up old thread, as I started looking at these patches again. So the problem was that DRM
> defines the output color transformations as:
> 
> degamma -> ctm -> gamma -> out
> 
> whereas OMAP DSS has
> 
> gamma -> ctm -> out
> 
> The omapdrm driver could declare the gamma table as degamma, as suggested by Ilia, and for the
> legacy drmModeCrtcSetGamma, the omapdrm driver could implement its own version, and use the degamma
> table internally (with no ctm).
> 
> For legacy, that would work fine and as expected, but I think the atomic version would be a bit odd,
> with only degamma, and no gamma.
> 
> Quick grep for drm_crtc_enable_color_mgmt shows that there are other drivers that have CTM and
> gamma, but no degamma. I wonder if all those have ctm -> gamma -> out, or are they similar to OMAP DSS.
> 
> Any thoughts on how to proceed with this?
> 
> Should we have a property that describes the order? Or new property name for gamma before ctm
> (PREGAMMA)? Or just have it as degamma, and let the userspace deal with it (i.e. figure out there's
> no gamma, but there's degamma, so use degamma)?

Hi,

would it not be simplest if KMS UAPI specification defined the abstract
color pipeline with all the blocks that may be exposed with
driver-agnostic UAPI, and then just say that if a block is not present,
it behaves as pass-through, a no-op?

Each block would be represented by standardised KMS properties, that
either exist or don't.

I think that would be fairly easy for userspace to grasp, but I don't
know if the abstract model itself would be feasible considering all the
hardware out there.

If we happened to be limited to

FB -> plane-degamma -> plane-CTM -> plane-gamma -> (blending) ->
degamma -> CTM -> gamma -> encoder -> wire

it would still be tractable.

No funny business with new KMS properties changing how old KMS
properties behave. Old userspace understands and uses old KMS
properties but not new KMS properties, so it wouldn't even work.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Tomi Valkeinen]

On 21/09/2020 14:49, Pekka Paalanen wrote:

> would it not be simplest if KMS UAPI specification defined the abstract
> color pipeline with all the blocks that may be exposed with
> driver-agnostic UAPI, and then just say that if a block is not present,
> it behaves as pass-through, a no-op?
> 
> Each block would be represented by standardised KMS properties, that
> either exist or don't.
> 
> I think that would be fairly easy for userspace to grasp, but I don't
> know if the abstract model itself would be feasible considering all the
> hardware out there.
> 
> If we happened to be limited to
> 
> FB -> plane-degamma -> plane-CTM -> plane-gamma -> (blending) ->
> degamma -> CTM -> gamma -> encoder -> wire
> 
> it would still be tractable.
> 
> No funny business with new KMS properties changing how old KMS
> properties behave. Old userspace understands and uses old KMS
> properties but not new KMS properties, so it wouldn't even work.

Isn't this how it's currently defined for the output side? So if I understand right, your suggestion
means that a HW that has:

gamma -> CTM -> out

would map those to DRM's degamma and CTM, and the userspace should use degamma to do gamma? I'm ok
with that, and it's probably more manageable than having properties which would describe the order
of the blocks.

While using degamma for gamma sounds a bit illogical, but thinking of it as:

pregamma -> ctm -> postgamma

sounds fine.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2600 bytes --]

On Tue, 22 Sep 2020 10:44:38 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 21/09/2020 14:49, Pekka Paalanen wrote:
> 
> > would it not be simplest if KMS UAPI specification defined the abstract
> > color pipeline with all the blocks that may be exposed with
> > driver-agnostic UAPI, and then just say that if a block is not present,
> > it behaves as pass-through, a no-op?
> > 
> > Each block would be represented by standardised KMS properties, that
> > either exist or don't.
> > 
> > I think that would be fairly easy for userspace to grasp, but I don't
> > know if the abstract model itself would be feasible considering all the
> > hardware out there.
> > 
> > If we happened to be limited to
> > 
> > FB -> plane-degamma -> plane-CTM -> plane-gamma -> (blending) ->
> > degamma -> CTM -> gamma -> encoder -> wire
> > 
> > it would still be tractable.
> > 
> > No funny business with new KMS properties changing how old KMS
> > properties behave. Old userspace understands and uses old KMS
> > properties but not new KMS properties, so it wouldn't even work.  
> 
> Isn't this how it's currently defined for the output side? So if I
> understand right, your suggestion means that a HW that has:
> 
> gamma -> CTM -> out
> 
> would map those to DRM's degamma and CTM, and the userspace should
> use degamma to do gamma? I'm ok with that, and it's probably more
> manageable than having properties which would describe the order of
> the blocks.

Hi,

yes.

When I have been thinking about using the KMS pipeline elements for
Weston, I didn't take "degamma" or "gamma" literally. I just think of
them as arbitrary LUTs at specific points in the pipeline.

Legacy KMS UAPI implementation for drmModeSetGamma() ioctl or whatever
could use the same heuristic: look at all the pipeline blocks after the
blending step, set everything to identity except for the last (or
first? or largest?) LUT which is used as "the gamma LUT".

> While using degamma for gamma sounds a bit illogical, but thinking of it as:
> 
> pregamma -> ctm -> postgamma
> 
> sounds fine.

Indeed. Better naming for new blocks in the future, I hope. :-)

I think even "gamma" is a little too much meaning, they're just LUTs.
Not sure if 3x 1D vs. 3D LUTs should be different blocks in the
pipeline, depends if the UAPI can handle both kinds.

Having blending -> degamma -> CTM even implies incorrect pipeline,
because blending should happen in linear space while degamma is about
converting from non-linear space into linear space.



Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Daniel Stone]

Hi,

On Tue, 22 Sep 2020 at 08:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 21/09/2020 14:49, Pekka Paalanen wrote:
> > would it not be simplest if KMS UAPI specification defined the abstract
> > color pipeline with all the blocks that may be exposed with
> > driver-agnostic UAPI, and then just say that if a block is not present,
> > it behaves as pass-through, a no-op?

Correct, that's the intention and also the result you get today. If
the documentation doesn't make that clear then it should be fixed.

> > Each block would be represented by standardised KMS properties, that
> > either exist or don't.
> >
> > I think that would be fairly easy for userspace to grasp, but I don't
> > know if the abstract model itself would be feasible considering all the
> > hardware out there.
> >
> > If we happened to be limited to
> >
> > FB -> plane-degamma -> plane-CTM -> plane-gamma -> (blending) ->
> > degamma -> CTM -> gamma -> encoder -> wire
> >
> > it would still be tractable.
> >
> > No funny business with new KMS properties changing how old KMS
> > properties behave. Old userspace understands and uses old KMS
> > properties but not new KMS properties, so it wouldn't even work.
>
> Isn't this how it's currently defined for the output side? So if I understand right, your suggestion
> means that a HW that has:
>
> gamma -> CTM -> out
>
> would map those to DRM's degamma and CTM, and the userspace should use degamma to do gamma? I'm ok
> with that, and it's probably more manageable than having properties which would describe the order
> of the blocks.
>
> While using degamma for gamma sounds a bit illogical, but thinking of it as:
>
> pregamma -> ctm -> postgamma
>
> sounds fine.

Totally. 'degamma' and 'gamma' are just normative from most uses,
they're not prescriptive, e.g. userspace can use the 'degamma' LUT to
do gamma whilst leaving the CTM and 'gamma' LUT disabled if it wants.

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