* [PATCH v2 0/4] drm: lcdif: Improve YUV support
@ 2022-09-28 0:58 Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 0:58 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
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 | 183 +++++++++++++++++++++++++----
drivers/gpu/drm/mxsfb/lcdif_regs.h | 37 +++---
2 files changed, 180 insertions(+), 40 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h
2022-09-28 0:58 [PATCH v2 0/4] drm: lcdif: Improve YUV support Laurent Pinchart
@ 2022-09-28 0:58 ` Laurent Pinchart
2022-09-28 9:36 ` Kieran Bingham
2022-09-29 7:55 ` Liu Ying
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
` (2 subsequent siblings)
3 siblings, 2 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 0:58 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
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>
---
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields
2022-09-28 0:58 [PATCH v2 0/4] drm: lcdif: Improve YUV support Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
@ 2022-09-28 0:58 ` Laurent Pinchart
2022-09-28 1:00 ` Marek Vasut
` (2 more replies)
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
3 siblings, 3 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 0:58 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
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>
---
Changes since v1:
- Use hex for field values
---
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..0d5d9bedd94a 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 (0xd << 26)
+#define DISP_PARA_LINE_PATTERN_RGB565 (0x7 << 26)
+#define DISP_PARA_LINE_PATTERN_RGB888 (0x0 << 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 (0x4 << 24)
+#define CTRLDESCL0_5_BPP_16_ARGB1555 (0x5 << 24)
+#define CTRLDESCL0_5_BPP_16_ARGB4444 (0x6 << 24)
+#define CTRLDESCL0_5_BPP_YCbCr422 (0x7 << 24)
+#define CTRLDESCL0_5_BPP_24_RGB888 (0x8 << 24)
+#define CTRLDESCL0_5_BPP_32_ARGB8888 (0x9 << 24)
+#define CTRLDESCL0_5_BPP_32_ABGR8888 (0xa << 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 (0x0 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V (0x1 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 (0x2 << 14)
+#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (0x3 << 14)
#define CTRLDESCL0_5_YUV_FORMAT_MASK GENMASK(15, 14)
#define CSC0_CTRL_CSC_MODE_RGB2YCbCr GENMASK(2, 1)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion
2022-09-28 0:58 [PATCH v2 0/4] drm: lcdif: Improve YUV support Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
@ 2022-09-28 0:58 ` Laurent Pinchart
2022-09-28 1:01 ` Marek Vasut
` (2 more replies)
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
3 siblings, 3 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 0:58 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
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.
The coefficients are copied from drivers/media/platforms/nxp/imx-pxp.c
for coherency, as the hardware is most likely identical.
Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Use coefficients from imx-pxp.c
---
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..c3622be0c587 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(0x041),
lcdif->base + LCDC_V8_CSC0_COEF0);
- writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
+ writel(CSC0_COEF1_B1(0x7db) | 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 0:58 [PATCH v2 0/4] drm: lcdif: Improve YUV support Laurent Pinchart
` (2 preceding siblings ...)
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
@ 2022-09-28 0:58 ` Laurent Pinchart
2022-09-28 1:09 ` Marek Vasut
` (3 more replies)
3 siblings, 4 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 0:58 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
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.
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Support all YCbCr encodings and quantization ranges
- Drop incorrect comment
---
drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
2 files changed, 164 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index c3622be0c587..b469a90fd50f 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>
@@ -32,13 +33,77 @@
/* -----------------------------------------------------------------------------
* CRTC
*/
+
+/*
+ * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
+ * values are added to Y, U and V, not subtracted. They must thus be programmed
+ * with negative values.
+ */
+static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
+ [DRM_COLOR_YCBCR_BT601] = {
+ [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
+ CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
+ CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
+ CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ [DRM_COLOR_YCBCR_FULL_RANGE] = {
+ CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
+ CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
+ CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ },
+ [DRM_COLOR_YCBCR_BT709] = {
+ [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
+ CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
+ CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
+ CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ [DRM_COLOR_YCBCR_FULL_RANGE] = {
+ CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
+ CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
+ CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ },
+ [DRM_COLOR_YCBCR_BT2020] = {
+ [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
+ CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
+ CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
+ CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ [DRM_COLOR_YCBCR_FULL_RANGE] = {
+ CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
+ CSC0_COEF1_A3(0x0179) | CSC0_COEF1_B1(0x0100),
+ CSC0_COEF2_B2(0x07d6) | CSC0_COEF2_B3(0x076e),
+ CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01e2),
+ CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
+ CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
+ },
+ },
+};
+
static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
+ struct drm_plane_state *plane_state,
const u32 bus_format)
{
struct drm_device *drm = lcdif->drm;
- const u32 format = lcdif->crtc.primary->state->fb->format->format;
-
- writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
+ const u32 format = plane_state->fb->format->format;
+ bool in_yuv = false;
+ bool out_yuv = false;
switch (bus_format) {
case MEDIA_BUS_FMT_RGB565_1X16:
@@ -52,24 +117,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(0x041),
- lcdif->base + LCDC_V8_CSC0_COEF0);
- writel(CSC0_COEF1_B1(0x7db) | 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 +125,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 +150,78 @@ 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(0x041),
+ lcdif->base + LCDC_V8_CSC0_COEF0);
+ writel(CSC0_COEF1_B1(0x7db) | 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 */
+ const u32 *coeffs =
+ lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
+ [plane_state->color_range];
+
+ writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
+ lcdif->base + LCDC_V8_CSC0_CTRL);
+
+ writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
+ writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
+ writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
+ writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
+ writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
+ writel(coeffs[5], 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)
@@ -201,6 +318,7 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
}
static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
+ struct drm_plane_state *plane_state,
struct drm_bridge_state *bridge_state,
const u32 bus_format)
{
@@ -223,7 +341,7 @@ static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
/* Mandatory eLCDIF reset as per the Reference Manual */
lcdif_reset_block(lcdif);
- lcdif_set_formats(lcdif, bus_format);
+ lcdif_set_formats(lcdif, plane_state, bus_format);
lcdif_set_mode(lcdif, bus_flags);
}
@@ -306,7 +424,7 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
pm_runtime_get_sync(drm->dev);
- lcdif_crtc_mode_set_nofb(lcdif, bridge_state, bus_format);
+ lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
/* Write cur_buf as well to avoid an initial corrupt frame */
paddr = drm_fb_cma_get_gem_addr(new_pstate->fb, new_pstate, 0);
@@ -456,6 +574,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[] = {
@@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
int lcdif_kms_init(struct lcdif_drm_private *lcdif)
{
+ const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
+ | BIT(DRM_COLOR_YCBCR_BT709)
+ | BIT(DRM_COLOR_YCBCR_BT2020);
+ const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
+ | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
struct drm_encoder *encoder = &lcdif->encoder;
struct drm_crtc *crtc = &lcdif->crtc;
int ret;
@@ -484,6 +613,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
if (ret)
return ret;
+ ret = drm_plane_create_color_properties(&lcdif->planes.primary,
+ supported_encodings,
+ supported_ranges,
+ 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 0d5d9bedd94a..fb74eb5ccbf1 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 (0x3 << 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 (0x0 << 1)
+#define CSC0_CTRL_CSC_MODE_YCbCr2RGB (0x1 << 1)
+#define CSC0_CTRL_CSC_MODE_RGB2YUV (0x2 << 1)
+#define CSC0_CTRL_CSC_MODE_RGB2YCbCr (0x3 << 1)
#define CSC0_CTRL_CSC_MODE_MASK GENMASK(2, 1)
#define CSC0_CTRL_BYPASS BIT(0)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
@ 2022-09-28 1:00 ` Marek Vasut
2022-09-28 9:38 ` Kieran Bingham
2022-09-29 7:59 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Marek Vasut @ 2022-09-28 1:00 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Peng Fan, Alexander Stein, Kieran Bingham, Daniel Scally, Robby Cai
On 9/28/22 02:58, 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>
Reviewed-by: Marek Vasut <marex@denx.de>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
@ 2022-09-28 1:01 ` Marek Vasut
2022-09-28 9:48 ` Kieran Bingham
2022-09-29 8:06 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Marek Vasut @ 2022-09-28 1:01 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Peng Fan, Alexander Stein, Kieran Bingham, Daniel Scally, Robby Cai
On 9/28/22 02:58, 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.
>
> The coefficients are copied from drivers/media/platforms/nxp/imx-pxp.c
> for coherency, as the hardware is most likely identical.
>
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Marek Vasut <marex@denx.de>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
@ 2022-09-28 1:09 ` Marek Vasut
2022-09-28 1:54 ` Laurent Pinchart
2022-09-28 9:59 ` Kieran Bingham
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Marek Vasut @ 2022-09-28 1:09 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Peng Fan, Alexander Stein, Kieran Bingham, Daniel Scally, Robby Cai
On 9/28/22 02:58, Laurent Pinchart wrote:
[...]
> + /*
> + * 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(0x041),
> + lcdif->base + LCDC_V8_CSC0_COEF0);
> + writel(CSC0_COEF1_B1(0x7db) | 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);
Would it make sense to turn the above ^ into a nice coeffs table like
the lcdif_yuv2rgb_coeffs table used in the else if branch below ?
> + } else if (in_yuv && !out_yuv) {
> + /* YCbCr -> RGB */
> + const u32 *coeffs =
> + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> + [plane_state->color_range];
> +
> + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> + lcdif->base + LCDC_V8_CSC0_CTRL);
> +
> + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> + writel(coeffs[5], 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)
[...]
> @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
>
> int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> {
> + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> + | BIT(DRM_COLOR_YCBCR_BT709)
> + | BIT(DRM_COLOR_YCBCR_BT2020);
> + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
Nitpick, is the | operator in front OK, or should it be at the end of
the line ?
Besides those nitpicks:
Reviewed-by: Marek Vasut <marex@denx.de>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 1:09 ` Marek Vasut
@ 2022-09-28 1:54 ` Laurent Pinchart
2022-09-28 4:44 ` Marek Vasut
0 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 1:54 UTC (permalink / raw)
To: Marek Vasut
Cc: Peng Fan, Alexander Stein, Kieran Bingham, Daniel Scally,
dri-devel, Robby Cai
Hi Marek,
On Wed, Sep 28, 2022 at 03:09:11AM +0200, Marek Vasut wrote:
> On 9/28/22 02:58, Laurent Pinchart wrote:
>
> [...]
>
> > + /*
> > + * 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(0x041),
> > + lcdif->base + LCDC_V8_CSC0_COEF0);
> > + writel(CSC0_COEF1_B1(0x7db) | 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);
>
> Would it make sense to turn the above ^ into a nice coeffs table like
> the lcdif_yuv2rgb_coeffs table used in the else if branch below ?
I thought about that, and decided to leave it as-is given that only one
encoding and quantization range is supported. It could be nice to fix
this, but it would then involve work in the dw-hdmi driver to select the
quantization through the AVI infoframe, as well as more work here to
pick a default based on the device capabilites reported through EDID.
I've decided not to explore that rabbit hole :-)
This being said, the coefficients could be moved to a table already even
without support for multiple encodings or ranges. Feel free to add a
patch on top, I'll review it :-)
> > + } else if (in_yuv && !out_yuv) {
> > + /* YCbCr -> RGB */
> > + const u32 *coeffs =
> > + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> > + [plane_state->color_range];
> > +
> > + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> > + lcdif->base + LCDC_V8_CSC0_CTRL);
> > +
> > + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> > + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> > + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> > + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> > + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> > + writel(coeffs[5], 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)
>
> [...]
>
> > @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
> >
> > int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > {
> > + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> > + | BIT(DRM_COLOR_YCBCR_BT709)
> > + | BIT(DRM_COLOR_YCBCR_BT2020);
> > + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> > + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
>
> Nitpick, is the | operator in front OK, or should it be at the end of
> the line ?
I'll move it to the end of the line.
> Besides those nitpicks:
>
> Reviewed-by: Marek Vasut <marex@denx.de>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 1:54 ` Laurent Pinchart
@ 2022-09-28 4:44 ` Marek Vasut
0 siblings, 0 replies; 24+ messages in thread
From: Marek Vasut @ 2022-09-28 4:44 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Peng Fan, Alexander Stein, Kieran Bingham, Daniel Scally,
dri-devel, Robby Cai
On 9/28/22 03:54, Laurent Pinchart wrote:
> Hi Marek,
Hi,
>>> + /*
>>> + * 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(0x041),
>>> + lcdif->base + LCDC_V8_CSC0_COEF0);
>>> + writel(CSC0_COEF1_B1(0x7db) | 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);
>>
>> Would it make sense to turn the above ^ into a nice coeffs table like
>> the lcdif_yuv2rgb_coeffs table used in the else if branch below ?
>
> I thought about that, and decided to leave it as-is given that only one
> encoding and quantization range is supported. It could be nice to fix
> this, but it would then involve work in the dw-hdmi driver to select the
> quantization through the AVI infoframe, as well as more work here to
> pick a default based on the device capabilites reported through EDID.
> I've decided not to explore that rabbit hole :-)
>
> This being said, the coefficients could be moved to a table already even
> without support for multiple encodings or ranges. Feel free to add a
> patch on top, I'll review it :-)
I'll just add it to the end of my todo list ...
>>> + } else if (in_yuv && !out_yuv) {
>>> + /* YCbCr -> RGB */
>>> + const u32 *coeffs =
>>> + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
>>> + [plane_state->color_range];
>>> +
>>> + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
>>> + lcdif->base + LCDC_V8_CSC0_CTRL);
>>> +
>>> + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
>>> + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
>>> + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
>>> + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
>>> + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
>>> + writel(coeffs[5], 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)
>>
>> [...]
>>
>>> @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
>>>
>>> int lcdif_kms_init(struct lcdif_drm_private *lcdif)
>>> {
>>> + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
>>> + | BIT(DRM_COLOR_YCBCR_BT709)
>>> + | BIT(DRM_COLOR_YCBCR_BT2020);
>>> + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
>>> + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
>>
>> Nitpick, is the | operator in front OK, or should it be at the end of
>> the line ?
>
> I'll move it to the end of the line.
Thanks. Let's wait a bit for the other reviews and then let's apply this
all.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
@ 2022-09-28 9:36 ` Kieran Bingham
2022-09-29 7:55 ` Liu Ying
1 sibling, 0 replies; 24+ messages in thread
From: Kieran Bingham @ 2022-09-28 9:36 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, Robby Cai
Quoting Laurent Pinchart (2022-09-28 01:58:09)
> A couple of the register macro values are incorrectly indented. Fix
> them.
>
Argh, there was already a v2 posted. Sometimes (more often than I like)
I really hate email...
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Marek Vasut <marex@denx.de>
> ---
> 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
2022-09-28 1:00 ` Marek Vasut
@ 2022-09-28 9:38 ` Kieran Bingham
2022-09-29 7:59 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Kieran Bingham @ 2022-09-28 9:38 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, Robby Cai
Quoting Laurent Pinchart (2022-09-28 01:58:10)
> 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>
> ---
> Changes since v1:
>
> - Use hex for field values
> ---
> 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..0d5d9bedd94a 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))
I mean, I like BIT (and sometimes GENMASK) but ... What was going on
there!
Defintely better this way.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> -#define DISP_PARA_LINE_PATTERN_RGB565 GENMASK(28, 26)
> -#define DISP_PARA_LINE_PATTERN_RGB888 0
> +#define DISP_PARA_LINE_PATTERN_UYVY_H (0xd << 26)
> +#define DISP_PARA_LINE_PATTERN_RGB565 (0x7 << 26)
> +#define DISP_PARA_LINE_PATTERN_RGB888 (0x0 << 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 (0x4 << 24)
> +#define CTRLDESCL0_5_BPP_16_ARGB1555 (0x5 << 24)
> +#define CTRLDESCL0_5_BPP_16_ARGB4444 (0x6 << 24)
> +#define CTRLDESCL0_5_BPP_YCbCr422 (0x7 << 24)
> +#define CTRLDESCL0_5_BPP_24_RGB888 (0x8 << 24)
> +#define CTRLDESCL0_5_BPP_32_ARGB8888 (0x9 << 24)
> +#define CTRLDESCL0_5_BPP_32_ABGR8888 (0xa << 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 (0x0 << 14)
> +#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V (0x1 << 14)
> +#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 (0x2 << 14)
> +#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (0x3 << 14)
> #define CTRLDESCL0_5_YUV_FORMAT_MASK GENMASK(15, 14)
>
> #define CSC0_CTRL_CSC_MODE_RGB2YCbCr GENMASK(2, 1)
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
2022-09-28 1:01 ` Marek Vasut
@ 2022-09-28 9:48 ` Kieran Bingham
2022-09-29 8:06 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Kieran Bingham @ 2022-09-28 9:48 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, Robby Cai
Quoting Laurent Pinchart (2022-09-28 01:58:11)
> 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.
>
> The coefficients are copied from drivers/media/platforms/nxp/imx-pxp.c
> for coherency, as the hardware is most likely identical.
Perhaps we need one or two of these somewhere:
https://colorconfidence.com/products/calibrite-colorchecker-display
Or does anyone have one that could test this patch?
Anyway:
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Use coefficients from imx-pxp.c
> ---
> 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..c3622be0c587 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(0x041),
> lcdif->base + LCDC_V8_CSC0_COEF0);
> - writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
> + writel(CSC0_COEF1_B1(0x7db) | 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
2022-09-28 1:09 ` Marek Vasut
@ 2022-09-28 9:59 ` Kieran Bingham
2022-09-28 10:05 ` Laurent Pinchart
2022-09-29 7:47 ` Liu Ying
2022-09-29 9:53 ` Liu Ying
3 siblings, 1 reply; 24+ messages in thread
From: Kieran Bingham @ 2022-09-28 9:59 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, Robby Cai
Quoting Laurent Pinchart (2022-09-28 01:58:12)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
It looks like this has progressed a bit since it left my computer ;-)
> 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.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Support all YCbCr encodings and quantization ranges
> - Drop incorrect comment
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> 2 files changed, 164 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index c3622be0c587..b469a90fd50f 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>
> @@ -32,13 +33,77 @@
> /* -----------------------------------------------------------------------------
> * CRTC
> */
> +
> +/*
> + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> + * values are added to Y, U and V, not subtracted. They must thus be programmed
> + * with negative values.
> + */
> +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
Ick ... I sort of dislike this. It's fine here at the moment, and I like
the table ... but here we're definining the size of the table based on
external enum values. (Are those ABI stable, perhaps they are already?)
If someone were to put
enum drm_color_encoding {
+ DRM_COLOR_LEGACY,
DRM_COLOR_YCBCR_BT601,
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_BT2020,
DRM_COLOR_ENCODING_MAX,
};
enum drm_color_range {
DRM_COLOR_YCBCR_LIMITED_RANGE,
+ DRM_COLOR_YCBCR_MID_RANGE,
DRM_COLOR_YCBCR_FULL_RANGE,
DRM_COLOR_RANGE_MAX,
};
Then this table allocation would be wrong.
Perhaps swapping for
> +static const u32 lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
Would be safer ... but longer :-( ?
Anyway, I think the rest of it looks fine, and perhaps these enums are
in the UAPI which would make them stable anyway:
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> + [DRM_COLOR_YCBCR_BT601] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + },
> + [DRM_COLOR_YCBCR_BT709] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + },
> + [DRM_COLOR_YCBCR_BT2020] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
> + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> + CSC0_COEF1_A3(0x0179) | CSC0_COEF1_B1(0x0100),
> + CSC0_COEF2_B2(0x07d6) | CSC0_COEF2_B3(0x076e),
> + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01e2),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + },
> +};
> +
> static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> + struct drm_plane_state *plane_state,
> const u32 bus_format)
> {
> struct drm_device *drm = lcdif->drm;
> - const u32 format = lcdif->crtc.primary->state->fb->format->format;
> -
> - writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> + const u32 format = plane_state->fb->format->format;
> + bool in_yuv = false;
> + bool out_yuv = false;
>
> switch (bus_format) {
> case MEDIA_BUS_FMT_RGB565_1X16:
> @@ -52,24 +117,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(0x041),
> - lcdif->base + LCDC_V8_CSC0_COEF0);
> - writel(CSC0_COEF1_B1(0x7db) | 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 +125,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 +150,78 @@ 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(0x041),
> + lcdif->base + LCDC_V8_CSC0_COEF0);
> + writel(CSC0_COEF1_B1(0x7db) | 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 */
> + const u32 *coeffs =
> + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> + [plane_state->color_range];
> +
> + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> + lcdif->base + LCDC_V8_CSC0_CTRL);
> +
> + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> + writel(coeffs[5], 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)
> @@ -201,6 +318,7 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
> }
>
> static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> + struct drm_plane_state *plane_state,
> struct drm_bridge_state *bridge_state,
> const u32 bus_format)
> {
> @@ -223,7 +341,7 @@ static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> /* Mandatory eLCDIF reset as per the Reference Manual */
> lcdif_reset_block(lcdif);
>
> - lcdif_set_formats(lcdif, bus_format);
> + lcdif_set_formats(lcdif, plane_state, bus_format);
>
> lcdif_set_mode(lcdif, bus_flags);
> }
> @@ -306,7 +424,7 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
>
> pm_runtime_get_sync(drm->dev);
>
> - lcdif_crtc_mode_set_nofb(lcdif, bridge_state, bus_format);
> + lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
>
> /* Write cur_buf as well to avoid an initial corrupt frame */
> paddr = drm_fb_cma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> @@ -456,6 +574,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[] = {
> @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
>
> int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> {
> + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> + | BIT(DRM_COLOR_YCBCR_BT709)
> + | BIT(DRM_COLOR_YCBCR_BT2020);
> + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
> struct drm_encoder *encoder = &lcdif->encoder;
> struct drm_crtc *crtc = &lcdif->crtc;
> int ret;
> @@ -484,6 +613,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> if (ret)
> return ret;
>
> + ret = drm_plane_create_color_properties(&lcdif->planes.primary,
> + supported_encodings,
> + supported_ranges,
> + 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 0d5d9bedd94a..fb74eb5ccbf1 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 (0x3 << 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 (0x0 << 1)
> +#define CSC0_CTRL_CSC_MODE_YCbCr2RGB (0x1 << 1)
> +#define CSC0_CTRL_CSC_MODE_RGB2YUV (0x2 << 1)
> +#define CSC0_CTRL_CSC_MODE_RGB2YCbCr (0x3 << 1)
> #define CSC0_CTRL_CSC_MODE_MASK GENMASK(2, 1)
> #define CSC0_CTRL_BYPASS BIT(0)
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 9:59 ` Kieran Bingham
@ 2022-09-28 10:05 ` Laurent Pinchart
2022-09-28 11:05 ` Kieran Bingham
0 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 10:05 UTC (permalink / raw)
To: Kieran Bingham
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, dri-devel,
Robby Cai
Hi Kieran,
On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> It looks like this has progressed a bit since it left my computer ;-)
I wish the same would be universally true for all patches :-)
> > 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.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Support all YCbCr encodings and quantization ranges
> > - Drop incorrect comment
> > ---
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> > drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> > 2 files changed, 164 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index c3622be0c587..b469a90fd50f 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>
> > @@ -32,13 +33,77 @@
> > /* -----------------------------------------------------------------------------
> > * CRTC
> > */
> > +
> > +/*
> > + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> > + * values are added to Y, U and V, not subtracted. They must thus be programmed
> > + * with negative values.
> > + */
> > +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
>
> Ick ... I sort of dislike this. It's fine here at the moment, and I like
> the table ... but here we're definining the size of the table based on
> external enum values. (Are those ABI stable, perhaps they are already?)
>
> If someone were to put
>
> enum drm_color_encoding {
> + DRM_COLOR_LEGACY,
> DRM_COLOR_YCBCR_BT601,
> DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_BT2020,
> DRM_COLOR_ENCODING_MAX,
> };
>
> enum drm_color_range {
> DRM_COLOR_YCBCR_LIMITED_RANGE,
> + DRM_COLOR_YCBCR_MID_RANGE,
> DRM_COLOR_YCBCR_FULL_RANGE,
> DRM_COLOR_RANGE_MAX,
> };
>
> Then this table allocation would be wrong.
>
> Perhaps swapping for
>
> > +static const u32 lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
>
> Would be safer ... but longer :-( ?
>
> Anyway, I think the rest of it looks fine, and perhaps these enums are
> in the UAPI which would make them stable anyway:
The enums themselves are not exposed in UAPI headers, but userspace
depends on the values, which thus have to remain stable.
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > + [DRM_COLOR_YCBCR_BT601] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> > + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + },
> > + [DRM_COLOR_YCBCR_BT709] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + },
> > + [DRM_COLOR_YCBCR_BT2020] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
> > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
> > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > + CSC0_COEF1_A3(0x0179) | CSC0_COEF1_B1(0x0100),
> > + CSC0_COEF2_B2(0x07d6) | CSC0_COEF2_B3(0x076e),
> > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01e2),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + },
> > +};
> > +
> > static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> > + struct drm_plane_state *plane_state,
> > const u32 bus_format)
> > {
> > struct drm_device *drm = lcdif->drm;
> > - const u32 format = lcdif->crtc.primary->state->fb->format->format;
> > -
> > - writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> > + const u32 format = plane_state->fb->format->format;
> > + bool in_yuv = false;
> > + bool out_yuv = false;
> >
> > switch (bus_format) {
> > case MEDIA_BUS_FMT_RGB565_1X16:
> > @@ -52,24 +117,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(0x041),
> > - lcdif->base + LCDC_V8_CSC0_COEF0);
> > - writel(CSC0_COEF1_B1(0x7db) | 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 +125,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 +150,78 @@ 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(0x041),
> > + lcdif->base + LCDC_V8_CSC0_COEF0);
> > + writel(CSC0_COEF1_B1(0x7db) | 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 */
> > + const u32 *coeffs =
> > + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> > + [plane_state->color_range];
> > +
> > + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> > + lcdif->base + LCDC_V8_CSC0_CTRL);
> > +
> > + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> > + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> > + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> > + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> > + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> > + writel(coeffs[5], 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)
> > @@ -201,6 +318,7 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
> > }
> >
> > static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > + struct drm_plane_state *plane_state,
> > struct drm_bridge_state *bridge_state,
> > const u32 bus_format)
> > {
> > @@ -223,7 +341,7 @@ static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > /* Mandatory eLCDIF reset as per the Reference Manual */
> > lcdif_reset_block(lcdif);
> >
> > - lcdif_set_formats(lcdif, bus_format);
> > + lcdif_set_formats(lcdif, plane_state, bus_format);
> >
> > lcdif_set_mode(lcdif, bus_flags);
> > }
> > @@ -306,7 +424,7 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
> >
> > pm_runtime_get_sync(drm->dev);
> >
> > - lcdif_crtc_mode_set_nofb(lcdif, bridge_state, bus_format);
> > + lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
> >
> > /* Write cur_buf as well to avoid an initial corrupt frame */
> > paddr = drm_fb_cma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> > @@ -456,6 +574,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[] = {
> > @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
> >
> > int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > {
> > + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> > + | BIT(DRM_COLOR_YCBCR_BT709)
> > + | BIT(DRM_COLOR_YCBCR_BT2020);
> > + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> > + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
> > struct drm_encoder *encoder = &lcdif->encoder;
> > struct drm_crtc *crtc = &lcdif->crtc;
> > int ret;
> > @@ -484,6 +613,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > if (ret)
> > return ret;
> >
> > + ret = drm_plane_create_color_properties(&lcdif->planes.primary,
> > + supported_encodings,
> > + supported_ranges,
> > + 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 0d5d9bedd94a..fb74eb5ccbf1 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 (0x3 << 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 (0x0 << 1)
> > +#define CSC0_CTRL_CSC_MODE_YCbCr2RGB (0x1 << 1)
> > +#define CSC0_CTRL_CSC_MODE_RGB2YUV (0x2 << 1)
> > +#define CSC0_CTRL_CSC_MODE_RGB2YCbCr (0x3 << 1)
> > #define CSC0_CTRL_CSC_MODE_MASK GENMASK(2, 1)
> > #define CSC0_CTRL_BYPASS BIT(0)
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 10:05 ` Laurent Pinchart
@ 2022-09-28 11:05 ` Kieran Bingham
2022-09-28 11:13 ` Laurent Pinchart
0 siblings, 1 reply; 24+ messages in thread
From: Kieran Bingham @ 2022-09-28 11:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, dri-devel,
Robby Cai
Quoting Laurent Pinchart (2022-09-28 11:05:33)
> Hi Kieran,
>
> On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > It looks like this has progressed a bit since it left my computer ;-)
>
> I wish the same would be universally true for all patches :-)
>
> > > 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.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Support all YCbCr encodings and quantization ranges
> > > - Drop incorrect comment
> > > ---
> > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> > > drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> > > 2 files changed, 164 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > index c3622be0c587..b469a90fd50f 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>
> > > @@ -32,13 +33,77 @@
> > > /* -----------------------------------------------------------------------------
> > > * CRTC
> > > */
> > > +
> > > +/*
> > > + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> > > + * values are added to Y, U and V, not subtracted. They must thus be programmed
> > > + * with negative values.
> > > + */
> > > +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
> >
> > Ick ... I sort of dislike this. It's fine here at the moment, and I like
> > the table ... but here we're definining the size of the table based on
> > external enum values. (Are those ABI stable, perhaps they are already?)
> >
> > If someone were to put
> >
> > enum drm_color_encoding {
> > + DRM_COLOR_LEGACY,
> > DRM_COLOR_YCBCR_BT601,
> > DRM_COLOR_YCBCR_BT709,
> > DRM_COLOR_YCBCR_BT2020,
> > DRM_COLOR_ENCODING_MAX,
> > };
> >
> > enum drm_color_range {
> > DRM_COLOR_YCBCR_LIMITED_RANGE,
> > + DRM_COLOR_YCBCR_MID_RANGE,
> > DRM_COLOR_YCBCR_FULL_RANGE,
> > DRM_COLOR_RANGE_MAX,
> > };
> >
> > Then this table allocation would be wrong.
> >
> > Perhaps swapping for
> >
> > > +static const u32 lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
> >
> > Would be safer ... but longer :-( ?
> >
> > Anyway, I think the rest of it looks fine, and perhaps these enums are
> > in the UAPI which would make them stable anyway:
>
> The enums themselves are not exposed in UAPI headers, but userspace
> depends on the values, which thus have to remain stable.
And I saw you had to redefine them to use them in libcamera. Perhaps
they should be in a UAPI header then...
--
Kieran
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > + [DRM_COLOR_YCBCR_BT601] = {
> > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> > > + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> > > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + },
> > > + [DRM_COLOR_YCBCR_BT709] = {
> > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > > + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> > > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + },
> > > + [DRM_COLOR_YCBCR_BT2020] = {
> > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
> > > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
> > > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > + CSC0_COEF1_A3(0x0179) | CSC0_COEF1_B1(0x0100),
> > > + CSC0_COEF2_B2(0x07d6) | CSC0_COEF2_B3(0x076e),
> > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01e2),
> > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + },
> > > +};
> > > +
> > > static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> > > + struct drm_plane_state *plane_state,
> > > const u32 bus_format)
> > > {
> > > struct drm_device *drm = lcdif->drm;
> > > - const u32 format = lcdif->crtc.primary->state->fb->format->format;
> > > -
> > > - writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> > > + const u32 format = plane_state->fb->format->format;
> > > + bool in_yuv = false;
> > > + bool out_yuv = false;
> > >
> > > switch (bus_format) {
> > > case MEDIA_BUS_FMT_RGB565_1X16:
> > > @@ -52,24 +117,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(0x041),
> > > - lcdif->base + LCDC_V8_CSC0_COEF0);
> > > - writel(CSC0_COEF1_B1(0x7db) | 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 +125,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 +150,78 @@ 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(0x041),
> > > + lcdif->base + LCDC_V8_CSC0_COEF0);
> > > + writel(CSC0_COEF1_B1(0x7db) | 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 */
> > > + const u32 *coeffs =
> > > + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> > > + [plane_state->color_range];
> > > +
> > > + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> > > + lcdif->base + LCDC_V8_CSC0_CTRL);
> > > +
> > > + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> > > + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> > > + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> > > + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> > > + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> > > + writel(coeffs[5], 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)
> > > @@ -201,6 +318,7 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
> > > }
> > >
> > > static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > > + struct drm_plane_state *plane_state,
> > > struct drm_bridge_state *bridge_state,
> > > const u32 bus_format)
> > > {
> > > @@ -223,7 +341,7 @@ static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > > /* Mandatory eLCDIF reset as per the Reference Manual */
> > > lcdif_reset_block(lcdif);
> > >
> > > - lcdif_set_formats(lcdif, bus_format);
> > > + lcdif_set_formats(lcdif, plane_state, bus_format);
> > >
> > > lcdif_set_mode(lcdif, bus_flags);
> > > }
> > > @@ -306,7 +424,7 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
> > >
> > > pm_runtime_get_sync(drm->dev);
> > >
> > > - lcdif_crtc_mode_set_nofb(lcdif, bridge_state, bus_format);
> > > + lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
> > >
> > > /* Write cur_buf as well to avoid an initial corrupt frame */
> > > paddr = drm_fb_cma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> > > @@ -456,6 +574,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[] = {
> > > @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
> > >
> > > int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > > {
> > > + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> > > + | BIT(DRM_COLOR_YCBCR_BT709)
> > > + | BIT(DRM_COLOR_YCBCR_BT2020);
> > > + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> > > + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
> > > struct drm_encoder *encoder = &lcdif->encoder;
> > > struct drm_crtc *crtc = &lcdif->crtc;
> > > int ret;
> > > @@ -484,6 +613,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > > if (ret)
> > > return ret;
> > >
> > > + ret = drm_plane_create_color_properties(&lcdif->planes.primary,
> > > + supported_encodings,
> > > + supported_ranges,
> > > + 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 0d5d9bedd94a..fb74eb5ccbf1 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 (0x3 << 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 (0x0 << 1)
> > > +#define CSC0_CTRL_CSC_MODE_YCbCr2RGB (0x1 << 1)
> > > +#define CSC0_CTRL_CSC_MODE_RGB2YUV (0x2 << 1)
> > > +#define CSC0_CTRL_CSC_MODE_RGB2YCbCr (0x3 << 1)
> > > #define CSC0_CTRL_CSC_MODE_MASK GENMASK(2, 1)
> > > #define CSC0_CTRL_BYPASS BIT(0)
> > >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 11:05 ` Kieran Bingham
@ 2022-09-28 11:13 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-28 11:13 UTC (permalink / raw)
To: Kieran Bingham
Cc: Marek Vasut, Peng Fan, Alexander Stein, Daniel Scally, dri-devel,
Robby Cai
Hi Kieran,
On Wed, Sep 28, 2022 at 12:05:03PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-28 11:05:33)
> > On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > It looks like this has progressed a bit since it left my computer ;-)
> >
> > I wish the same would be universally true for all patches :-)
> >
> > > > 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.
> > > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Support all YCbCr encodings and quantization ranges
> > > > - Drop incorrect comment
> > > > ---
> > > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> > > > drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> > > > 2 files changed, 164 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > index c3622be0c587..b469a90fd50f 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>
> > > > @@ -32,13 +33,77 @@
> > > > /* -----------------------------------------------------------------------------
> > > > * CRTC
> > > > */
> > > > +
> > > > +/*
> > > > + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> > > > + * values are added to Y, U and V, not subtracted. They must thus be programmed
> > > > + * with negative values.
> > > > + */
> > > > +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
> > >
> > > Ick ... I sort of dislike this. It's fine here at the moment, and I like
> > > the table ... but here we're definining the size of the table based on
> > > external enum values. (Are those ABI stable, perhaps they are already?)
> > >
> > > If someone were to put
> > >
> > > enum drm_color_encoding {
> > > + DRM_COLOR_LEGACY,
> > > DRM_COLOR_YCBCR_BT601,
> > > DRM_COLOR_YCBCR_BT709,
> > > DRM_COLOR_YCBCR_BT2020,
> > > DRM_COLOR_ENCODING_MAX,
> > > };
> > >
> > > enum drm_color_range {
> > > DRM_COLOR_YCBCR_LIMITED_RANGE,
> > > + DRM_COLOR_YCBCR_MID_RANGE,
> > > DRM_COLOR_YCBCR_FULL_RANGE,
> > > DRM_COLOR_RANGE_MAX,
> > > };
> > >
> > > Then this table allocation would be wrong.
> > >
> > > Perhaps swapping for
> > >
> > > > +static const u32 lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
> > >
> > > Would be safer ... but longer :-( ?
> > >
> > > Anyway, I think the rest of it looks fine, and perhaps these enums are
> > > in the UAPI which would make them stable anyway:
> >
> > The enums themselves are not exposed in UAPI headers, but userspace
> > depends on the values, which thus have to remain stable.
>
> And I saw you had to redefine them to use them in libcamera. Perhaps
> they should be in a UAPI header then...
I think that would make sense. Patches are welcome :-)
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > > + [DRM_COLOR_YCBCR_BT601] = {
> > > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> > > > + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> > > > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > > > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + },
> > > > + [DRM_COLOR_YCBCR_BT709] = {
> > > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > > > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > > > + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> > > > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> > > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + },
> > > > + [DRM_COLOR_YCBCR_BT2020] = {
> > > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
> > > > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
> > > > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x0000),
> > > > + CSC0_COEF1_A3(0x0179) | CSC0_COEF1_B1(0x0100),
> > > > + CSC0_COEF2_B2(0x07d6) | CSC0_COEF2_B3(0x076e),
> > > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01e2),
> > > > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x0000),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + },
> > > > +};
> > > > +
> > > > static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> > > > + struct drm_plane_state *plane_state,
> > > > const u32 bus_format)
> > > > {
> > > > struct drm_device *drm = lcdif->drm;
> > > > - const u32 format = lcdif->crtc.primary->state->fb->format->format;
> > > > -
> > > > - writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> > > > + const u32 format = plane_state->fb->format->format;
> > > > + bool in_yuv = false;
> > > > + bool out_yuv = false;
> > > >
> > > > switch (bus_format) {
> > > > case MEDIA_BUS_FMT_RGB565_1X16:
> > > > @@ -52,24 +117,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(0x041),
> > > > - lcdif->base + LCDC_V8_CSC0_COEF0);
> > > > - writel(CSC0_COEF1_B1(0x7db) | 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 +125,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 +150,78 @@ 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(0x041),
> > > > + lcdif->base + LCDC_V8_CSC0_COEF0);
> > > > + writel(CSC0_COEF1_B1(0x7db) | 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 */
> > > > + const u32 *coeffs =
> > > > + lcdif_yuv2rgb_coeffs[plane_state->color_encoding]
> > > > + [plane_state->color_range];
> > > > +
> > > > + writel(CSC0_CTRL_CSC_MODE_YCbCr2RGB,
> > > > + lcdif->base + LCDC_V8_CSC0_CTRL);
> > > > +
> > > > + writel(coeffs[0], lcdif->base + LCDC_V8_CSC0_COEF0);
> > > > + writel(coeffs[1], lcdif->base + LCDC_V8_CSC0_COEF1);
> > > > + writel(coeffs[2], lcdif->base + LCDC_V8_CSC0_COEF2);
> > > > + writel(coeffs[3], lcdif->base + LCDC_V8_CSC0_COEF3);
> > > > + writel(coeffs[4], lcdif->base + LCDC_V8_CSC0_COEF4);
> > > > + writel(coeffs[5], 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)
> > > > @@ -201,6 +318,7 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
> > > > }
> > > >
> > > > static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > > > + struct drm_plane_state *plane_state,
> > > > struct drm_bridge_state *bridge_state,
> > > > const u32 bus_format)
> > > > {
> > > > @@ -223,7 +341,7 @@ static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> > > > /* Mandatory eLCDIF reset as per the Reference Manual */
> > > > lcdif_reset_block(lcdif);
> > > >
> > > > - lcdif_set_formats(lcdif, bus_format);
> > > > + lcdif_set_formats(lcdif, plane_state, bus_format);
> > > >
> > > > lcdif_set_mode(lcdif, bus_flags);
> > > > }
> > > > @@ -306,7 +424,7 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
> > > >
> > > > pm_runtime_get_sync(drm->dev);
> > > >
> > > > - lcdif_crtc_mode_set_nofb(lcdif, bridge_state, bus_format);
> > > > + lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
> > > >
> > > > /* Write cur_buf as well to avoid an initial corrupt frame */
> > > > paddr = drm_fb_cma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> > > > @@ -456,6 +574,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[] = {
> > > > @@ -469,6 +593,11 @@ static const u64 lcdif_modifiers[] = {
> > > >
> > > > int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > > > {
> > > > + const u32 supported_encodings = BIT(DRM_COLOR_YCBCR_BT601)
> > > > + | BIT(DRM_COLOR_YCBCR_BT709)
> > > > + | BIT(DRM_COLOR_YCBCR_BT2020);
> > > > + const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE)
> > > > + | BIT(DRM_COLOR_YCBCR_FULL_RANGE);
> > > > struct drm_encoder *encoder = &lcdif->encoder;
> > > > struct drm_crtc *crtc = &lcdif->crtc;
> > > > int ret;
> > > > @@ -484,6 +613,14 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + ret = drm_plane_create_color_properties(&lcdif->planes.primary,
> > > > + supported_encodings,
> > > > + supported_ranges,
> > > > + 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 0d5d9bedd94a..fb74eb5ccbf1 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 (0x3 << 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 (0x0 << 1)
> > > > +#define CSC0_CTRL_CSC_MODE_YCbCr2RGB (0x1 << 1)
> > > > +#define CSC0_CTRL_CSC_MODE_RGB2YUV (0x2 << 1)
> > > > +#define CSC0_CTRL_CSC_MODE_RGB2YCbCr (0x3 << 1)
> > > > #define CSC0_CTRL_CSC_MODE_MASK GENMASK(2, 1)
> > > > #define CSC0_CTRL_BYPASS BIT(0)
> > > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
2022-09-28 1:09 ` Marek Vasut
2022-09-28 9:59 ` Kieran Bingham
@ 2022-09-29 7:47 ` Liu Ying
2022-09-29 8:01 ` Laurent Pinchart
2022-09-29 9:53 ` Liu Ying
3 siblings, 1 reply; 24+ messages in thread
From: Liu Ying @ 2022-09-29 7:47 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
Hi Laurent,
On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> 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.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Support all YCbCr encodings and quantization ranges
> - Drop incorrect comment
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> 2 files changed, 164 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index c3622be0c587..b469a90fd50f 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>
> @@ -32,13 +33,77 @@
> /* -----------------------------------------------------------------------------
> * CRTC
> */
> +
> +/*
> + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> + * values are added to Y, U and V, not subtracted. They must thus be programmed
> + * with negative values.
> + */
> +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
> + [DRM_COLOR_YCBCR_BT601] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
Can you add conversion equations as comments in code for each encoding
and range, like imx-pxp.c does? Also for RGB to YCbCr conversion.
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
Looks like hex with 3 numbers is enough for each coefficient. Use
'0x12a' and '0x000'?
> + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
Shouldn't D1 be 0x110 since it's -16 and you set D2/D3 to 0x180 as they
are -128? The same for D1s with other encodings and limited ranges.
I didn't check other coefficients closely though.
[...]
> @@ -456,6 +574,12 @@ static const u32 lcdif_primary_plane_formats[] = {
> DRM_FORMAT_XRGB1555,
> DRM_FORMAT_XRGB4444,
> DRM_FORMAT_XRGB8888,
> +
> + /* packed YCbCr */
Nitpick: Add a similar comment for above RGB pixel formats?
Regards,
Liu Ying
> + DRM_FORMAT_YUYV,
> + DRM_FORMAT_YVYU,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_VYUY,
> };
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
2022-09-28 9:36 ` Kieran Bingham
@ 2022-09-29 7:55 ` Liu Ying
1 sibling, 0 replies; 24+ messages in thread
From: Liu Ying @ 2022-09-29 7:55 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
On Wed, 2022-09-28 at 03:58 +0300, 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>
> ---
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Liu Ying <victor.liu@nxp.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
2022-09-28 1:00 ` Marek Vasut
2022-09-28 9:38 ` Kieran Bingham
@ 2022-09-29 7:59 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Liu Ying @ 2022-09-29 7:59 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
On Wed, 2022-09-28 at 03:58 +0300, 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>
> ---
> Changes since v1:
>
> - Use hex for field values
> ---
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
One option is to use FIELD_RPEP() macro for register field values.
But I think this patch is ok without using it, so:
Reviewed-by: Liu Ying <victor.liu@nxp.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-29 7:47 ` Liu Ying
@ 2022-09-29 8:01 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-29 8:01 UTC (permalink / raw)
To: Liu Ying
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
dri-devel, Daniel Scally, Robby Cai
Hi Liu,
On Thu, Sep 29, 2022 at 03:47:33PM +0800, Liu Ying wrote:
> On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> > 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.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Support all YCbCr encodings and quantization ranges
> > - Drop incorrect comment
> > ---
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> > drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> > 2 files changed, 164 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index c3622be0c587..b469a90fd50f 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>
> > @@ -32,13 +33,77 @@
> > /* -----------------------------------------------------------------------------
> > * CRTC
> > */
> > +
> > +/*
> > + * Despite the reference manual stating the opposite, the D1, D2 and D3 offset
> > + * values are added to Y, U and V, not subtracted. They must thus be programmed
> > + * with negative values.
> > + */
> > +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {
> > + [DRM_COLOR_YCBCR_BT601] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
>
> Can you add conversion equations as comments in code for each encoding
> and range, like imx-pxp.c does? Also for RGB to YCbCr conversion.
Sure.
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
>
> Looks like hex with 3 numbers is enough for each coefficient. Use
> '0x12a' and '0x000'?
OK.
> > + CSC0_COEF1_A3(0x01a2) | CSC0_COEF1_B1(0x0123),
> > + CSC0_COEF2_B2(0x079c) | CSC0_COEF2_B3(0x0730),
> > + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0204),
> > + CSC0_COEF4_C3(0x0000) | CSC0_COEF4_D1(0x01f0),
>
> Shouldn't D1 be 0x110 since it's -16 and you set D2/D3 to 0x180 as they
> are -128? The same for D1s with other encodings and limited ranges.
The D values are stored in two-complement format, so 0x1f0 is -16.
> I didn't check other coefficients closely though.
>
> [...]
>
> > @@ -456,6 +574,12 @@ static const u32 lcdif_primary_plane_formats[] = {
> > DRM_FORMAT_XRGB1555,
> > DRM_FORMAT_XRGB4444,
> > DRM_FORMAT_XRGB8888,
> > +
> > + /* packed YCbCr */
>
> Nitpick: Add a similar comment for above RGB pixel formats?
Sure.
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
> > };
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
2022-09-28 1:01 ` Marek Vasut
2022-09-28 9:48 ` Kieran Bingham
@ 2022-09-29 8:06 ` Liu Ying
2 siblings, 0 replies; 24+ messages in thread
From: Liu Ying @ 2022-09-29 8:06 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
On Wed, 2022-09-28 at 03:58 +0300, 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
s/features/feature/
> 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.
>
> The coefficients are copied from drivers/media/platforms/nxp/imx-pxp.c
> for coherency, as the hardware is most likely identical.
>
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Use coefficients from imx-pxp.c
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
With the typo in commit message fixed:
Reviewed-by: Liu Ying <victor.liu@nxp.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
` (2 preceding siblings ...)
2022-09-29 7:47 ` Liu Ying
@ 2022-09-29 9:53 ` Liu Ying
2022-09-29 20:33 ` Laurent Pinchart
3 siblings, 1 reply; 24+ messages in thread
From: Liu Ying @ 2022-09-29 9:53 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
Daniel Scally, Robby Cai
On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> 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.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Support all YCbCr encodings and quantization ranges
> - Drop incorrect comment
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> 2 files changed, 164 insertions(+), 24 deletions(-)
I have a chance to test this series and find that my
'koe,tx26d202vm0bwa' 1920x1200 LVDS panel connected with i.MX8mp EVK
can only show the test pattern at the top half of the display with YUV
fb. Looks like something with wrong stride. XR24 fb is ok, but RG16 fb
has similar issue. Anything I missed?
The command I'm using to test YUV fb:
modetest -M imx-lcdif -P 31@35:1920x1200@YUYV
Liu Ying
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
2022-09-29 9:53 ` Liu Ying
@ 2022-09-29 20:33 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2022-09-29 20:33 UTC (permalink / raw)
To: Liu Ying
Cc: Marek Vasut, Peng Fan, Alexander Stein, Kieran Bingham,
dri-devel, Daniel Scally, Robby Cai
Hi Liu,
On Thu, Sep 29, 2022 at 05:53:37PM +0800, Liu Ying wrote:
> On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> > 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.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Support all YCbCr encodings and quantization ranges
> > - Drop incorrect comment
> > ---
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 183 +++++++++++++++++++++++++----
> > drivers/gpu/drm/mxsfb/lcdif_regs.h | 5 +-
> > 2 files changed, 164 insertions(+), 24 deletions(-)
>
> I have a chance to test this series and find that my
> 'koe,tx26d202vm0bwa' 1920x1200 LVDS panel connected with i.MX8mp EVK
> can only show the test pattern at the top half of the display with YUV
> fb. Looks like something with wrong stride. XR24 fb is ok, but RG16 fb
> has similar issue. Anything I missed?
>
> The command I'm using to test YUV fb:
> modetest -M imx-lcdif -P 31@35:1920x1200@YUYV
Thanks for the bug report. The problem didn't occur with kmstest nor the
libcamera 'cam' test application, but I have been able to reproduce it
with modetest. The modetest application uses the legacy mode setting API
by default, so it exercises code paths in the driver in different ways,
uncovering a few preexisting issues.
The problem is caused by the fact that the functions called from the
.atomic_enable() handler access the frame buffer from the plane state
and configure the hardware using that information. Depending on how the
device is configured, the display can be enabled with one frame buffer,
and then immediately switch to a different frame buffer that has a
different format and/or pitch.
Properties of the frame buffer should only be accessed from the plane
.atomic_update() operation. Fixing this requires quite a bit of
refactoring of the driver, which I won't have time to work on at the
moment. Marek, would you have some time to look at this ? As the issue
isn't introduced by this series but preexists (you should be able to
reproduce it by selecting the XR15 format instead of XR24 for instance),
can YUV support be merged already (after I send v3) ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-09-29 20:34 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-28 0:58 [PATCH v2 0/4] drm: lcdif: Improve YUV support Laurent Pinchart
2022-09-28 0:58 ` [PATCH v2 1/4] drm: lcdif: Fix indentation in lcdif_regs.h Laurent Pinchart
2022-09-28 9:36 ` Kieran Bingham
2022-09-29 7:55 ` Liu Ying
2022-09-28 0:58 ` [PATCH v2 2/4] drm: lcdif: Don't use BIT() for multi-bit register fields Laurent Pinchart
2022-09-28 1:00 ` Marek Vasut
2022-09-28 9:38 ` Kieran Bingham
2022-09-29 7:59 ` Liu Ying
2022-09-28 0:58 ` [PATCH v2 3/4] drm: lcdif: Switch to limited range for RGB to YUV conversion Laurent Pinchart
2022-09-28 1:01 ` Marek Vasut
2022-09-28 9:48 ` Kieran Bingham
2022-09-29 8:06 ` Liu Ying
2022-09-28 0:58 ` [PATCH v2 4/4] drm: lcdif: Add support for YUV planes Laurent Pinchart
2022-09-28 1:09 ` Marek Vasut
2022-09-28 1:54 ` Laurent Pinchart
2022-09-28 4:44 ` Marek Vasut
2022-09-28 9:59 ` Kieran Bingham
2022-09-28 10:05 ` Laurent Pinchart
2022-09-28 11:05 ` Kieran Bingham
2022-09-28 11:13 ` Laurent Pinchart
2022-09-29 7:47 ` Liu Ying
2022-09-29 8:01 ` Laurent Pinchart
2022-09-29 9:53 ` Liu Ying
2022-09-29 20:33 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).