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

[Laurent Pinchart]

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 at denx.de>

-- 
Regards,

Laurent Pinchart