Message ID | 20220928005812.21060-5-laurent.pinchart@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm: lcdif: Improve YUV support | expand |
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>
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>
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.
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 >
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) > >
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
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) > > > >
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, > };
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, > > };
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
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) ?
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)