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

[Marek Vasut]

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