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@xxxxxxx>
--
Regards,
Laurent Pinchart
