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

[Marek Vasut]

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.