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

[Liu Ying]

Hi Laurent,

On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham at 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 at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at 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,
>  };