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

[Liu Ying]

Hi Laurent,

On Thu, 2022-09-29 at 23:42 +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>
> Reviewed-by: Marek Vasut <marex at denx.de>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> Changes since v2:
> 
> - Fix YUV to RGB coefficients
> - List floating point coefficient values in comment
> - Express CSC coefficients with three hex digits
> - Move | to the end of the line
> - Add comment header before RGB formats
> 
> Changes since v1:
> 
> - Support all YCbCr encodings and quantization ranges
> - Drop incorrect comment
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c  | 260 ++++++++++++++++++++++++-----
>  drivers/gpu/drm/mxsfb/lcdif_regs.h |   5 +-
>  2 files changed, 224 insertions(+), 41 deletions(-)

Hmmm, 'git am' fails to apply this one to drm-misc-next but it's ok
with v6.0-rc1.  I should have tried drm-misc-next with v2.

> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 08e4880ec6cf..81454b53936f 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,119 @@
>  /* -----------------------------------------------------------------------------
>   * 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.

Can you add the generic conversion equations with [A-D][1-3]
coefficient symbols as comments in code, like imx-pxp.c does?  It will
improve code readability better and tell which coefficient is which.
Also for RGB to YCbCr conversion if you want to do that with this
series.  Sorry for not pointing this out in v2 review cycle.

> + */
> +static const u32 lcdif_yuv2rgb_coeffs[3][2][6] = {

[...]

> +	/* YUYV Formats */

'YUV Formats' would be better?  Again, failed to catch this in v2...

Regards,
Liu Ying

> +	case DRM_FORMAT_YUYV:
> +		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_VY2UY1,
> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +		in_yuv = true;
> +		break;
> +	case DRM_FORMAT_YVYU:
> +		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_UY2VY1,
> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +		in_yuv = true;
> +		break;
> +	case DRM_FORMAT_UYVY:
> +		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2VY1U,
> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +		in_yuv = true;
> +		break;
> +	case DRM_FORMAT_VYUY:
> +		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2UY1V,
> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +		in_yuv = true;
> +		break;
> +
> +	default:
> +		dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
> +		break;
> +	}