[PATCH 0/4] drm: lcdif: Improve YUV support

[PATCH 0/4] drm: lcdif: Improve YUV support

[Laurent Pinchart]

Hello,

This small patch series improves YUV support in the i.MX8MP LCDIF
driver. After patches 1/4 and 2/4 that fix tiny cosmetic issues, patch
3/4 fixes YUV quantization range for the RGB to YUV conversion. Patch
4/4 addresses the other direction and adds support for YUV planes.

Please see individual patches for details.

The series has been tested on a Polyhex Debix board with the currently
out-of-tree HDMI encoder support patches developed by Lucas Stach.

Kieran Bingham (1):
  drm: lcdif: Add support for YUV planes

Laurent Pinchart (3):
  drm: lcdif: Fix indentation in lcdif_regs.h
  drm: lcdif: Don't use BIT() for multi-bit register fields
  drm: lcdif: Switch to limited range for RGB to YUV conversion

 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 117 ++++++++++++++++++++++++-----
 drivers/gpu/drm/mxsfb/lcdif_regs.h |  37 ++++-----
 2 files changed, 117 insertions(+), 37 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/4] drm: lcdif: Fix indentation in lcdif_regs.h

[Laurent Pinchart]

A couple of the register macro values are incorrectly indented. Fix
them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/mxsfb/lcdif_regs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
index 8e8bef175bf2..013f2cace2a0 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
@@ -130,7 +130,7 @@
 #define CTRL_FETCH_START_OPTION_BPV	BIT(9)
 #define CTRL_FETCH_START_OPTION_RESV	GENMASK(9, 8)
 #define CTRL_FETCH_START_OPTION_MASK	GENMASK(9, 8)
-#define CTRL_NEG				BIT(4)
+#define CTRL_NEG			BIT(4)
 #define CTRL_INV_PXCK			BIT(3)
 #define CTRL_INV_DE			BIT(2)
 #define CTRL_INV_VS			BIT(1)
@@ -186,7 +186,7 @@
 #define INT_ENABLE_D1_PLANE_PANIC_EN	BIT(0)
 
 #define CTRLDESCL0_1_HEIGHT(n)		(((n) & 0xffff) << 16)
-#define CTRLDESCL0_1_HEIGHT_MASK		GENMASK(31, 16)
+#define CTRLDESCL0_1_HEIGHT_MASK	GENMASK(31, 16)
 #define CTRLDESCL0_1_WIDTH(n)		((n) & 0xffff)
 #define CTRLDESCL0_1_WIDTH_MASK		GENMASK(15, 0)
 
-- 
Regards,

Laurent Pinchart

[PATCH 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields

[Laurent Pinchart]

The BIT() macro is meant to represent a single bit. Don't use it for
values of register fields that span multiple bits.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
index 013f2cace2a0..bc4d020aaa7c 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
@@ -138,9 +138,9 @@
 
 #define DISP_PARA_DISP_ON		BIT(31)
 #define DISP_PARA_SWAP_EN		BIT(30)
-#define DISP_PARA_LINE_PATTERN_UYVY_H	(GENMASK(29, 28) | BIT(26))
-#define DISP_PARA_LINE_PATTERN_RGB565	GENMASK(28, 26)
-#define DISP_PARA_LINE_PATTERN_RGB888	0
+#define DISP_PARA_LINE_PATTERN_UYVY_H	(13 << 26)
+#define DISP_PARA_LINE_PATTERN_RGB565	(7 << 26)
+#define DISP_PARA_LINE_PATTERN_RGB888	(0 << 26)
 #define DISP_PARA_LINE_PATTERN_MASK	GENMASK(29, 26)
 #define DISP_PARA_DISP_MODE_MASK	GENMASK(25, 24)
 #define DISP_PARA_BGND_R_MASK		GENMASK(23, 16)
@@ -202,18 +202,18 @@
 
 #define CTRLDESCL0_5_EN			BIT(31)
 #define CTRLDESCL0_5_SHADOW_LOAD_EN	BIT(30)
-#define CTRLDESCL0_5_BPP_16_RGB565	BIT(26)
-#define CTRLDESCL0_5_BPP_16_ARGB1555	(BIT(26) | BIT(24))
-#define CTRLDESCL0_5_BPP_16_ARGB4444	(BIT(26) | BIT(25))
-#define CTRLDESCL0_5_BPP_YCbCr422	(BIT(26) | BIT(25) | BIT(24))
-#define CTRLDESCL0_5_BPP_24_RGB888	BIT(27)
-#define CTRLDESCL0_5_BPP_32_ARGB8888	(BIT(27) | BIT(24))
-#define CTRLDESCL0_5_BPP_32_ABGR8888	(BIT(27) | BIT(25))
+#define CTRLDESCL0_5_BPP_16_RGB565	(4 << 24)
+#define CTRLDESCL0_5_BPP_16_ARGB1555	(5 << 24)
+#define CTRLDESCL0_5_BPP_16_ARGB4444	(6 << 24)
+#define CTRLDESCL0_5_BPP_YCbCr422	(7 << 24)
+#define CTRLDESCL0_5_BPP_24_RGB888	(8 << 24)
+#define CTRLDESCL0_5_BPP_32_ARGB8888	(9 << 24)
+#define CTRLDESCL0_5_BPP_32_ABGR8888	(10 << 24)
 #define CTRLDESCL0_5_BPP_MASK		GENMASK(27, 24)
-#define CTRLDESCL0_5_YUV_FORMAT_Y2VY1U	0
-#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V	BIT(14)
-#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1	BIT(15)
-#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1	(BIT(15) | BIT(14))
+#define CTRLDESCL0_5_YUV_FORMAT_Y2VY1U	(0 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V	(1 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1	(2 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1	(3 << 14)
 #define CTRLDESCL0_5_YUV_FORMAT_MASK	GENMASK(15, 14)
 
 #define CSC0_CTRL_CSC_MODE_RGB2YCbCr	GENMASK(2, 1)
-- 
Regards,

Laurent Pinchart

[PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Laurent Pinchart]

Up to and including v1.3, HDMI supported limited quantization range only
for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this
features isn't supported in the dw-hdmi driver that is used in
conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus
always advertised in the AVI infoframe as limited range.

The LCDIF driver, on the other hand, configures the CSC to produce full
range YCbCr. This mismatch results in loss of details and incorrect
colours. Fix it by switching to limited range YCbCr.

Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 1f22ea5896d5..ba84b51598b3 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 		writel(DISP_PARA_LINE_PATTERN_UYVY_H,
 		       lcdif->base + LCDC_V8_DISP_PARA);
 
-		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
-		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
+		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
+		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
 		       lcdif->base + LCDC_V8_CSC0_COEF0);
-		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
+		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
 		       lcdif->base + LCDC_V8_CSC0_COEF1);
-		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
+		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
 		       lcdif->base + LCDC_V8_CSC0_COEF2);
-		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
+		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
 		       lcdif->base + LCDC_V8_CSC0_COEF3);
-		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
+		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
 		       lcdif->base + LCDC_V8_CSC0_COEF4);
 		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
 		       lcdif->base + LCDC_V8_CSC0_COEF5);
-- 
Regards,

Laurent Pinchart

[PATCH 4/4] drm: lcdif: Add support for YUV planes

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

The LCDIF includes a color space converter that supports YUV input. Use
it to support YUV planes, either through the converter if the output
format is RGB, or in conversion bypass mode otherwise. For now only
BT.601 YCbCr encoding in limited quantization range is supported,
additional encodings and ranges may be added later.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 117 ++++++++++++++++++++++++-----
 drivers/gpu/drm/mxsfb/lcdif_regs.h |   5 +-
 2 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index ba84b51598b3..a97a5f512aae 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -15,6 +15,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_color_mgmt.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_framebuffer.h>
@@ -37,9 +38,10 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 {
 	struct drm_device *drm = lcdif->drm;
 	const u32 format = lcdif->crtc.primary->state->fb->format->format;
+	bool in_yuv = false;
+	bool out_yuv = false;
 
-	writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
-
+	/* HDMI output */
 	switch (bus_format) {
 	case MEDIA_BUS_FMT_RGB565_1X16:
 		writel(DISP_PARA_LINE_PATTERN_RGB565,
@@ -52,24 +54,7 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 	case MEDIA_BUS_FMT_UYVY8_1X16:
 		writel(DISP_PARA_LINE_PATTERN_UYVY_H,
 		       lcdif->base + LCDC_V8_DISP_PARA);
-
-		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
-		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
-		       lcdif->base + LCDC_V8_CSC0_COEF0);
-		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
-		       lcdif->base + LCDC_V8_CSC0_COEF1);
-		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
-		       lcdif->base + LCDC_V8_CSC0_COEF2);
-		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
-		       lcdif->base + LCDC_V8_CSC0_COEF3);
-		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
-		       lcdif->base + LCDC_V8_CSC0_COEF4);
-		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
-		       lcdif->base + LCDC_V8_CSC0_COEF5);
-
-		writel(CSC0_CTRL_CSC_MODE_RGB2YCbCr,
-		       lcdif->base + LCDC_V8_CSC0_CTRL);
-
+		out_yuv = true;
 		break;
 	default:
 		dev_err(drm->dev, "Unknown media bus format 0x%x\n", bus_format);
@@ -77,6 +62,7 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 	}
 
 	switch (format) {
+	/* RGB Formats */
 	case DRM_FORMAT_RGB565:
 		writel(CTRLDESCL0_5_BPP_16_RGB565,
 		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
@@ -101,10 +87,87 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 		writel(CTRLDESCL0_5_BPP_32_ARGB8888,
 		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
 		break;
+
+	/* YUYV Formats */
+	case DRM_FORMAT_YUYV:
+		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_VY2UY1,
+		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		in_yuv = true;
+		break;
+	case DRM_FORMAT_YVYU:
+		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_UY2VY1,
+		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		in_yuv = true;
+		break;
+	case DRM_FORMAT_UYVY:
+		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2VY1U,
+		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		in_yuv = true;
+		break;
+	case DRM_FORMAT_VYUY:
+		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2UY1V,
+		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		in_yuv = true;
+		break;
+
 	default:
 		dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
 		break;
 	}
+
+	/*
+	 * The CSC differentiates between "YCbCr" and "YUV", but the reference
+	 * manual doesn't detail how they differ. Experiments showed that the
+	 * luminance value is unaffected, only the calculations involving chroma
+	 * values differ. The YCbCr mode behaves as expected, with chroma values
+	 * being offset by 128. The YUV mode isn't fully understood.
+	 */
+	if (!in_yuv && out_yuv) {
+		/* RGB -> YCbCr */
+		writel(CSC0_CTRL_CSC_MODE_RGB2YCbCr,
+		       lcdif->base + LCDC_V8_CSC0_CTRL);
+
+		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
+		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
+		       lcdif->base + LCDC_V8_CSC0_COEF0);
+		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
+		       lcdif->base + LCDC_V8_CSC0_COEF1);
+		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
+		       lcdif->base + LCDC_V8_CSC0_COEF2);
+		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
+		       lcdif->base + LCDC_V8_CSC0_COEF3);
+		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
+		       lcdif->base + LCDC_V8_CSC0_COEF4);
+		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
+		       lcdif->base + LCDC_V8_CSC0_COEF5);
+	} else if (in_yuv && !out_yuv) {
+		/* YCbCr -> RGB */
+		writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
+		       lcdif->base + LCDC_V8_CSC0_CTRL);
+
+		/*
+		 * CSC: BT.601 Limited Range YCbCr to RGB coefficients.
+		 *
+		 * Despite the reference manual stating the opposite, the D1,
+		 * D2 and D3 values are added to Y, U and V, not subtracted.
+		 * They must thus be programmed with negative values.
+		 */
+		writel(CSC0_COEF0_A2(0x000) | CSC0_COEF0_A1(0x12a),
+		       lcdif->base + LCDC_V8_CSC0_COEF0);
+		writel(CSC0_COEF1_B1(0x123) | CSC0_COEF1_A3(0x1a2),
+		       lcdif->base + LCDC_V8_CSC0_COEF1);
+		writel(CSC0_COEF2_B3(0x730) | CSC0_COEF2_B2(0x79c),
+		       lcdif->base + LCDC_V8_CSC0_COEF2);
+		writel(CSC0_COEF3_C2(0x204) | CSC0_COEF3_C1(0x124),
+		       lcdif->base + LCDC_V8_CSC0_COEF3);
+		writel(CSC0_COEF4_D1(0x1f0) | CSC0_COEF4_C3(0x000),
+		       lcdif->base + LCDC_V8_CSC0_COEF4);
+		writel(CSC0_COEF5_D3(0x180) | CSC0_COEF5_D2(0x180),
+		       lcdif->base + LCDC_V8_CSC0_COEF5);
+	} else {
+		/* RGB -> RGB, YCbCr -> YCbCr: bypass colorspace converter. */
+		writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
+	}
 }
 
 static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
@@ -456,6 +519,12 @@ static const u32 lcdif_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB1555,
 	DRM_FORMAT_XRGB4444,
 	DRM_FORMAT_XRGB8888,
+
+	/* packed YCbCr */
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_YVYU,
+	DRM_FORMAT_UYVY,
+	DRM_FORMAT_VYUY,
 };
 
 static const u64 lcdif_modifiers[] = {
@@ -484,6 +553,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
 	if (ret)
 		return ret;
 
+	ret = drm_plane_create_color_properties(&lcdif->planes.primary,
+						BIT(DRM_COLOR_YCBCR_BT601),
+						BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+						DRM_COLOR_YCBCR_BT601,
+						DRM_COLOR_YCBCR_LIMITED_RANGE);
+	if (ret)
+		return ret;
+
 	drm_crtc_helper_add(crtc, &lcdif_crtc_helper_funcs);
 	ret = drm_crtc_init_with_planes(lcdif->drm, crtc,
 					&lcdif->planes.primary, NULL,
diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
index bc4d020aaa7c..c53f2eb6818a 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
@@ -216,7 +216,10 @@
 #define CTRLDESCL0_5_YUV_FORMAT_UY2VY1	(3 << 14)
 #define CTRLDESCL0_5_YUV_FORMAT_MASK	GENMASK(15, 14)
 
-#define CSC0_CTRL_CSC_MODE_RGB2YCbCr	GENMASK(2, 1)
+#define CSC0_CTRL_CSC_MODE_YUV2RGB	(0 << 1)
+#define CSC0_CTRL_CSC_MODE_YCbCr2RGB	(1 << 1)
+#define CSC0_CTRL_CSC_MODE_RGB2YUV	(2 << 1)
+#define CSC0_CTRL_CSC_MODE_RGB2YCbCr	(3 << 1)
 #define CSC0_CTRL_CSC_MODE_MASK		GENMASK(2, 1)
 #define CSC0_CTRL_BYPASS		BIT(0)
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] drm: lcdif: Fix indentation in lcdif_regs.h

[Marek Vasut]

On 9/28/22 01:38, Laurent Pinchart wrote:
> A couple of the register macro values are incorrectly indented. Fix
> them.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields

[Marek Vasut]

On 9/28/22 01:38, Laurent Pinchart wrote:
> The BIT() macro is meant to represent a single bit. Don't use it for
> values of register fields that span multiple bits.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> index 013f2cace2a0..bc4d020aaa7c 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> @@ -138,9 +138,9 @@
>   
>   #define DISP_PARA_DISP_ON		BIT(31)
>   #define DISP_PARA_SWAP_EN		BIT(30)
> -#define DISP_PARA_LINE_PATTERN_UYVY_H	(GENMASK(29, 28) | BIT(26))
> -#define DISP_PARA_LINE_PATTERN_RGB565	GENMASK(28, 26)
> -#define DISP_PARA_LINE_PATTERN_RGB888	0
> +#define DISP_PARA_LINE_PATTERN_UYVY_H	(13 << 26)
> +#define DISP_PARA_LINE_PATTERN_RGB565	(7 << 26)
> +#define DISP_PARA_LINE_PATTERN_RGB888	(0 << 26)

Can we use hex here for the left size of the shift operation, so it's 
subjectively easier to read ? DTTO for the other values ?

That is:
-#define DISP_PARA_LINE_PATTERN_UYVY_H	(13 << 26)
+#define DISP_PARA_LINE_PATTERN_UYVY_H	(0xd << 26)

[...]

Re: [PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Marek Vasut]

On 9/28/22 01:38, Laurent Pinchart wrote:
> Up to and including v1.3, HDMI supported limited quantization range only
> for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this
> features isn't supported in the dw-hdmi driver that is used in
> conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus
> always advertised in the AVI infoframe as limited range.
> 
> The LCDIF driver, on the other hand, configures the CSC to produce full
> range YCbCr. This mismatch results in loss of details and incorrect
> colours. Fix it by switching to limited range YCbCr.
> 
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 1f22ea5896d5..ba84b51598b3 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
>   		writel(DISP_PARA_LINE_PATTERN_UYVY_H,
>   		       lcdif->base + LCDC_V8_DISP_PARA);
>   
> -		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
> -		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
> +		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
> +		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
>   		       lcdif->base + LCDC_V8_CSC0_COEF0);
> -		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
> +		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
>   		       lcdif->base + LCDC_V8_CSC0_COEF1);
> -		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
> +		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
>   		       lcdif->base + LCDC_V8_CSC0_COEF2);
> -		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
> +		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
>   		       lcdif->base + LCDC_V8_CSC0_COEF3);
> -		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
> +		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
>   		       lcdif->base + LCDC_V8_CSC0_COEF4);
>   		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
>   		       lcdif->base + LCDC_V8_CSC0_COEF5);

Would it make sense to use the same coeffs as csc2_coef_bt601_lim in 
drivers/media/platform/nxp/imx-pxp.c , since the block is most likely 
identical ?

Re: [PATCH 4/4] drm: lcdif: Add support for YUV planes

[Marek Vasut]

On 9/28/22 01:38, Laurent Pinchart wrote:

Hi,

[...]

> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index ba84b51598b3..a97a5f512aae 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c

[...]

> @@ -37,9 +38,10 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
>   {
>   	struct drm_device *drm = lcdif->drm;
>   	const u32 format = lcdif->crtc.primary->state->fb->format->format;
> +	bool in_yuv = false;
> +	bool out_yuv = false;
>   
> -	writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> -
> +	/* HDMI output */

Should this comment really be here ? The IP can be connected to either 
LVDS serializer/DSI bridge/HDMI , it is not just HDMI output IP.

>   	switch (bus_format) {
>   	case MEDIA_BUS_FMT_RGB565_1X16:
>   		writel(DISP_PARA_LINE_PATTERN_RGB565,

[...]

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields

[Laurent Pinchart]

Hi Marek,

On Wed, Sep 28, 2022 at 02:10:26AM +0200, Marek Vasut wrote:
> On 9/28/22 01:38, Laurent Pinchart wrote:
> > The BIT() macro is meant to represent a single bit. Don't use it for
> > values of register fields that span multiple bits.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++--------------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> > index 013f2cace2a0..bc4d020aaa7c 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> > @@ -138,9 +138,9 @@
> >   
> >   #define DISP_PARA_DISP_ON		BIT(31)
> >   #define DISP_PARA_SWAP_EN		BIT(30)
> > -#define DISP_PARA_LINE_PATTERN_UYVY_H	(GENMASK(29, 28) | BIT(26))
> > -#define DISP_PARA_LINE_PATTERN_RGB565	GENMASK(28, 26)
> > -#define DISP_PARA_LINE_PATTERN_RGB888	0
> > +#define DISP_PARA_LINE_PATTERN_UYVY_H	(13 << 26)
> > +#define DISP_PARA_LINE_PATTERN_RGB565	(7 << 26)
> > +#define DISP_PARA_LINE_PATTERN_RGB888	(0 << 26)
> 
> Can we use hex here for the left size of the shift operation, so it's 
> subjectively easier to read ? DTTO for the other values ?
> 
> That is:
> -#define DISP_PARA_LINE_PATTERN_UYVY_H	(13 << 26)
> +#define DISP_PARA_LINE_PATTERN_UYVY_H	(0xd << 26)
> 
> [...]

Sure, I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Laurent Pinchart]

Hi Marek,

On Wed, Sep 28, 2022 at 02:12:19AM +0200, Marek Vasut wrote:
> On 9/28/22 01:38, Laurent Pinchart wrote:
> > Up to and including v1.3, HDMI supported limited quantization range only
> > for YCbCr. HDMI v1.4 introduced selectable quantization ranges, but this
> > features isn't supported in the dw-hdmi driver that is used in
> > conjunction with the LCDIF in the i.MX8MP. The HDMI YCbCr output is thus
> > always advertised in the AVI infoframe as limited range.
> > 
> > The LCDIF driver, on the other hand, configures the CSC to produce full
> > range YCbCr. This mismatch results in loss of details and incorrect
> > colours. Fix it by switching to limited range YCbCr.
> > 
> > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 1f22ea5896d5..ba84b51598b3 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -53,16 +53,16 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> >   		writel(DISP_PARA_LINE_PATTERN_UYVY_H,
> >   		       lcdif->base + LCDC_V8_DISP_PARA);
> >   
> > -		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
> > -		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
> > +		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
> > +		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF0);
> > -		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
> > +		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF1);
> > -		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
> > +		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF2);
> > -		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
> > +		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF3);
> > -		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
> > +		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF4);
> >   		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
> >   		       lcdif->base + LCDC_V8_CSC0_COEF5);
> 
> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in 
> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely 
> identical ?

The coefficients in this patch have been computed to distribute the
error in such a way that the sum of all lines stays the same. This
avoids biases and overflow, but it likely makes very little difference
in practice. I'm thus fine with the coefficients from imx-pxp.c.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] drm: lcdif: Add support for YUV planes

[Laurent Pinchart]

On Wed, Sep 28, 2022 at 02:16:54AM +0200, Marek Vasut wrote:
> On 9/28/22 01:38, Laurent Pinchart wrote:
> 
> Hi,
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index ba84b51598b3..a97a5f512aae 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> 
> [...]
> 
> > @@ -37,9 +38,10 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> >   {
> >   	struct drm_device *drm = lcdif->drm;
> >   	const u32 format = lcdif->crtc.primary->state->fb->format->format;
> > +	bool in_yuv = false;
> > +	bool out_yuv = false;
> >   
> > -	writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> > -
> > +	/* HDMI output */
> 
> Should this comment really be here ? The IP can be connected to either 
> LVDS serializer/DSI bridge/HDMI , it is not just HDMI output IP.

You're right, I'll drop the comment.

> >   	switch (bus_format) {
> >   	case MEDIA_BUS_FMT_RGB565_1X16:
> >   		writel(DISP_PARA_LINE_PATTERN_RGB565,
> 
> [...]
> 
> Reviewed-by: Marek Vasut <marex@denx.de>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Marek Vasut]

On 9/28/22 02:21, Laurent Pinchart wrote:

Hi,

[...]

>>> -		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
>>> -		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
>>> +		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
>>> +		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF0);
>>> -		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
>>> +		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF1);
>>> -		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
>>> +		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF2);
>>> -		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
>>> +		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF3);
>>> -		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
>>> +		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF4);
>>>    		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
>>>    		       lcdif->base + LCDC_V8_CSC0_COEF5);
>>
>> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in
>> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely
>> identical ?
> 
> The coefficients in this patch have been computed to distribute the
> error in such a way that the sum of all lines stays the same. This
> avoids biases and overflow, but it likely makes very little difference
> in practice. I'm thus fine with the coefficients from imx-pxp.c.

Would it then make sense to update the coeffs in the pxp driver instead?

Either option works for me.

Re: [PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Laurent Pinchart]

On Wed, Sep 28, 2022 at 02:37:04AM +0200, Marek Vasut wrote:
> On 9/28/22 02:21, Laurent Pinchart wrote:
> 
> Hi,
> 
> [...]
> 
> >>> -		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
> >>> -		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
> >>> +		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
> >>> +		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF0);
> >>> -		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
> >>> +		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF1);
> >>> -		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
> >>> +		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF2);
> >>> -		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
> >>> +		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF3);
> >>> -		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
> >>> +		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF4);
> >>>    		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
> >>>    		       lcdif->base + LCDC_V8_CSC0_COEF5);
> >>
> >> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in
> >> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely
> >> identical ?
> > 
> > The coefficients in this patch have been computed to distribute the
> > error in such a way that the sum of all lines stays the same. This
> > avoids biases and overflow, but it likely makes very little difference
> > in practice. I'm thus fine with the coefficients from imx-pxp.c.
> 
> Would it then make sense to update the coeffs in the pxp driver instead?
> 
> Either option works for me.

It could, but I won't be able to easily test it. As the hardware clamps
the calculated value, there's no risk of wraparound, and the difference
in the +/-1 error distribution will not be noticeable, so I'll just copy
the coefficients from imx-pxp.c to ensure coherency.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion

[Marek Vasut]

On 9/28/22 02:40, Laurent Pinchart wrote:
> On Wed, Sep 28, 2022 at 02:37:04AM +0200, Marek Vasut wrote:
>> On 9/28/22 02:21, Laurent Pinchart wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>>> -		/* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
>>>>> -		writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
>>>>> +		/* CSC: BT.601 Limited Range RGB to YCbCr coefficients. */
>>>>> +		writel(CSC0_COEF0_A2(0x081) | CSC0_COEF0_A1(0x042),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF0);
>>>>> -		writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
>>>>> +		writel(CSC0_COEF1_B1(0x7da) | CSC0_COEF1_A3(0x019),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF1);
>>>>> -		writel(CSC0_COEF2_B3(0x080) | CSC0_COEF2_B2(0x7ac),
>>>>> +		writel(CSC0_COEF2_B3(0x070) | CSC0_COEF2_B2(0x7b6),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF2);
>>>>> -		writel(CSC0_COEF3_C2(0x795) | CSC0_COEF3_C1(0x080),
>>>>> +		writel(CSC0_COEF3_C2(0x7a2) | CSC0_COEF3_C1(0x070),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF3);
>>>>> -		writel(CSC0_COEF4_D1(0x000) | CSC0_COEF4_C3(0x7ec),
>>>>> +		writel(CSC0_COEF4_D1(0x010) | CSC0_COEF4_C3(0x7ee),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF4);
>>>>>     		writel(CSC0_COEF5_D3(0x080) | CSC0_COEF5_D2(0x080),
>>>>>     		       lcdif->base + LCDC_V8_CSC0_COEF5);
>>>>
>>>> Would it make sense to use the same coeffs as csc2_coef_bt601_lim in
>>>> drivers/media/platform/nxp/imx-pxp.c , since the block is most likely
>>>> identical ?
>>>
>>> The coefficients in this patch have been computed to distribute the
>>> error in such a way that the sum of all lines stays the same. This
>>> avoids biases and overflow, but it likely makes very little difference
>>> in practice. I'm thus fine with the coefficients from imx-pxp.c.
>>
>> Would it then make sense to update the coeffs in the pxp driver instead?
>>
>> Either option works for me.
> 
> It could, but I won't be able to easily test it. As the hardware clamps
> the calculated value, there's no risk of wraparound, and the difference
> in the +/-1 error distribution will not be noticeable, so I'll just copy
> the coefficients from imx-pxp.c to ensure coherency.

That works too, thanks !

Re: [PATCH 1/4] drm: lcdif: Fix indentation in lcdif_regs.h

[Kieran Bingham]

Quoting Laurent Pinchart (2022-09-28 00:38:18)
> A couple of the register macro values are incorrectly indented. Fix
> them.
> 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> index 8e8bef175bf2..013f2cace2a0 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> @@ -130,7 +130,7 @@
>  #define CTRL_FETCH_START_OPTION_BPV    BIT(9)
>  #define CTRL_FETCH_START_OPTION_RESV   GENMASK(9, 8)
>  #define CTRL_FETCH_START_OPTION_MASK   GENMASK(9, 8)
> -#define CTRL_NEG                               BIT(4)
> +#define CTRL_NEG                       BIT(4)
>  #define CTRL_INV_PXCK                  BIT(3)
>  #define CTRL_INV_DE                    BIT(2)
>  #define CTRL_INV_VS                    BIT(1)
> @@ -186,7 +186,7 @@
>  #define INT_ENABLE_D1_PLANE_PANIC_EN   BIT(0)
>  
>  #define CTRLDESCL0_1_HEIGHT(n)         (((n) & 0xffff) << 16)
> -#define CTRLDESCL0_1_HEIGHT_MASK               GENMASK(31, 16)
> +#define CTRLDESCL0_1_HEIGHT_MASK       GENMASK(31, 16)
>  #define CTRLDESCL0_1_WIDTH(n)          ((n) & 0xffff)
>  #define CTRLDESCL0_1_WIDTH_MASK                GENMASK(15, 0)
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>