Hi Liu,
On Thu, Sep 29, 2022 at 03:47:33PM +0800, Liu Ying wrote:
> On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > 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.
Sure.
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x0000),
>
> Looks like hex with 3 numbers is enough for each coefficient. Use
> '0x12a' and '0x000'?
OK.
> > + 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.
The D values are stored in two-complement format, so 0x1f0 is -16.
> 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?
Sure.
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_VYUY,
> > };
--
Regards,
Laurent Pinchart
