Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Hi Liu, On Thu, Sep 29, 2022 at 05:53:37PM +0800, Liu Ying wrote: > On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote: > > From: Kieran Bingham > > > > 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 > > Signed-off-by: Laurent Pinchart > > --- > > 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(-) > > I have a chance to test this series and find that my > 'koe,tx26d202vm0bwa' 1920x1200 LVDS panel connected with i.MX8mp EVK > can only show the test pattern at the top half of the display with YUV > fb. Looks like something with wrong stride. XR24 fb is ok, but RG16 fb > has similar issue. Anything I missed? > > The command I'm using to test YUV fb: > modetest -M imx-lcdif -P 31@35:1920x1200@YUYV Thanks for the bug report. The problem didn't occur with kmstest nor the libcamera 'cam' test application, but I have been able to reproduce it with modetest. The modetest application uses the legacy mode setting API by default, so it exercises code paths in the driver in different ways, uncovering a few preexisting issues. The problem is caused by the fact that the functions called from the .atomic_enable() handler access the frame buffer from the plane state and configure the hardware using that information. Depending on how the device is configured, the display can be enabled with one frame buffer, and then immediately switch to a different frame buffer that has a different format and/or pitch. Properties of the frame buffer should only be accessed from the plane .atomic_update() operation. Fixing this requires quite a bit of refactoring of the driver, which I won't have time to work on at the moment. Marek, would you have some time to look at this ? As the issue isn't introduced by this series but preexists (you should be able to reproduce it by selecting the XR15 format instead of XR24 for instance), can YUV support be merged already (after I send v3) ? -- Regards, Laurent Pinchart
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote: > From: Kieran Bingham > > 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 > Signed-off-by: Laurent Pinchart > --- > 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(-) I have a chance to test this series and find that my 'koe,tx26d202vm0bwa' 1920x1200 LVDS panel connected with i.MX8mp EVK can only show the test pattern at the top half of the display with YUV fb. Looks like something with wrong stride. XR24 fb is ok, but RG16 fb has similar issue. Anything I missed? The command I'm using to test YUV fb: modetest -M imx-lcdif -P 31@35:1920x1200@YUYV Liu Ying
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
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
> >
> > 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
> > Signed-off-by: Laurent Pinchart
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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(0x),
>
> 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(0x) | 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_XRGB,
> > DRM_FORMAT_XRGB,
> > +
> > + /* 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
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Hi Laurent,
On Wed, 2022-09-28 at 03:58 +0300, Laurent Pinchart wrote:
> From: Kieran Bingham
>
> 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
> Signed-off-by: Laurent Pinchart
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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(0x),
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(0x) | 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_XRGB,
> DRM_FORMAT_XRGB,
> +
> + /* 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,
> };
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Hi Kieran,
On Wed, Sep 28, 2022 at 12:05:03PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-28 11:05:33)
> > On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > > > From: Kieran Bingham
> > >
> > > It looks like this has progressed a bit since it left my computer ;-)
> >
> > I wish the same would be universally true for all patches :-)
> >
> > > > 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
> > > > Signed-off-by: Laurent Pinchart
> > > > ---
> > > > 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -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] = {
> > >
> > > Ick ... I sort of dislike this. It's fine here at the moment, and I like
> > > the table ... but here we're definining the size of the table based on
> > > external enum values. (Are those ABI stable, perhaps they are already?)
> > >
> > > If someone were to put
> > >
> > > enum drm_color_encoding {
> > > +DRM_COLOR_LEGACY,
> > > DRM_COLOR_YCBCR_BT601,
> > > DRM_COLOR_YCBCR_BT709,
> > > DRM_COLOR_YCBCR_BT2020,
> > > DRM_COLOR_ENCODING_MAX,
> > > };
> > >
> > > enum drm_color_range {
> > > DRM_COLOR_YCBCR_LIMITED_RANGE,
> > > + DRM_COLOR_YCBCR_MID_RANGE,
> > > DRM_COLOR_YCBCR_FULL_RANGE,
> > > DRM_COLOR_RANGE_MAX,
> > > };
> > >
> > > Then this table allocation would be wrong.
> > >
> > > Perhaps swapping for
> > >
> > > > +static const u32
> > > > lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
> > >
> > > Would be safer ... but longer :-( ?
> > >
> > > Anyway, I think the rest of it looks fine, and perhaps these enums are
> > > in the UAPI which would make them stable anyway:
> >
> > The enums themselves are not exposed in UAPI headers, but userspace
> > depends on the values, which thus have to remain stable.
>
> And I saw you had to redefine them to use them in libcamera. Perhaps
> they should be in a UAPI header then...
I think that would make sense. Patches are welcome :-)
> > > Reviewed-by: Kieran Bingham
> > >
> > > > + [DRM_COLOR_YCBCR_BT601] = {
> > > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > > > + 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(0x) | CSC0_COEF4_D1(0x01f0),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> > > > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > > > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > > > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> > > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > > + },
> > > > + },
> > > > + [DRM_COLOR_YCBCR_BT709] = {
> > > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > > > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > > > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > > > +
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Quoting Laurent Pinchart (2022-09-28 11:05:33)
> Hi Kieran,
>
> On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > > From: Kieran Bingham
> >
> > It looks like this has progressed a bit since it left my computer ;-)
>
> I wish the same would be universally true for all patches :-)
>
> > > 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
> > > Signed-off-by: Laurent Pinchart
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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] = {
> >
> > Ick ... I sort of dislike this. It's fine here at the moment, and I like
> > the table ... but here we're definining the size of the table based on
> > external enum values. (Are those ABI stable, perhaps they are already?)
> >
> > If someone were to put
> >
> > enum drm_color_encoding {
> > +DRM_COLOR_LEGACY,
> > DRM_COLOR_YCBCR_BT601,
> > DRM_COLOR_YCBCR_BT709,
> > DRM_COLOR_YCBCR_BT2020,
> > DRM_COLOR_ENCODING_MAX,
> > };
> >
> > enum drm_color_range {
> > DRM_COLOR_YCBCR_LIMITED_RANGE,
> > + DRM_COLOR_YCBCR_MID_RANGE,
> > DRM_COLOR_YCBCR_FULL_RANGE,
> > DRM_COLOR_RANGE_MAX,
> > };
> >
> > Then this table allocation would be wrong.
> >
> > Perhaps swapping for
> >
> > > +static const u32
> > > lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
> >
> > Would be safer ... but longer :-( ?
> >
> > Anyway, I think the rest of it looks fine, and perhaps these enums are
> > in the UAPI which would make them stable anyway:
>
> The enums themselves are not exposed in UAPI headers, but userspace
> depends on the values, which thus have to remain stable.
And I saw you had to redefine them to use them in libcamera. Perhaps
they should be in a UAPI header then...
--
Kieran
>
> > Reviewed-by: Kieran Bingham
> >
> > > + [DRM_COLOR_YCBCR_BT601] = {
> > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > > + 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(0x) | CSC0_COEF4_D1(0x01f0),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> > > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + },
> > > + [DRM_COLOR_YCBCR_BT709] = {
> > > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > > + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> > > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x01f0),
> > > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > > + },
> > > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > > + CSC0_COEF
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Hi Kieran,
On Wed, Sep 28, 2022 at 10:59:36AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-28 01:58:12)
> > From: Kieran Bingham
>
> It looks like this has progressed a bit since it left my computer ;-)
I wish the same would be universally true for all patches :-)
> > 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
> > Signed-off-by: Laurent Pinchart
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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] = {
>
> Ick ... I sort of dislike this. It's fine here at the moment, and I like
> the table ... but here we're definining the size of the table based on
> external enum values. (Are those ABI stable, perhaps they are already?)
>
> If someone were to put
>
> enum drm_color_encoding {
> +DRM_COLOR_LEGACY,
> DRM_COLOR_YCBCR_BT601,
> DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_BT2020,
> DRM_COLOR_ENCODING_MAX,
> };
>
> enum drm_color_range {
> DRM_COLOR_YCBCR_LIMITED_RANGE,
> + DRM_COLOR_YCBCR_MID_RANGE,
> DRM_COLOR_YCBCR_FULL_RANGE,
> DRM_COLOR_RANGE_MAX,
> };
>
> Then this table allocation would be wrong.
>
> Perhaps swapping for
>
> > +static const u32
> > lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
>
> Would be safer ... but longer :-( ?
>
> Anyway, I think the rest of it looks fine, and perhaps these enums are
> in the UAPI which would make them stable anyway:
The enums themselves are not exposed in UAPI headers, but userspace
depends on the values, which thus have to remain stable.
> Reviewed-by: Kieran Bingham
>
> > + [DRM_COLOR_YCBCR_BT601] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > + 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(0x) | CSC0_COEF4_D1(0x01f0),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> > + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> > + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + },
> > + [DRM_COLOR_YCBCR_BT709] = {
> > + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> > + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> > + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> > + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> > + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x01f0),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> > + },
> > + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> > + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> > + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> > + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> > + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> > + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> > + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180)
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Quoting Laurent Pinchart (2022-09-28 01:58:12)
> From: Kieran Bingham
>
It looks like this has progressed a bit since it left my computer ;-)
> 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
> Signed-off-by: Laurent Pinchart
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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] = {
Ick ... I sort of dislike this. It's fine here at the moment, and I like
the table ... but here we're definining the size of the table based on
external enum values. (Are those ABI stable, perhaps they are already?)
If someone were to put
enum drm_color_encoding {
+DRM_COLOR_LEGACY,
DRM_COLOR_YCBCR_BT601,
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_BT2020,
DRM_COLOR_ENCODING_MAX,
};
enum drm_color_range {
DRM_COLOR_YCBCR_LIMITED_RANGE,
+DRM_COLOR_YCBCR_MID_RANGE,
DRM_COLOR_YCBCR_FULL_RANGE,
DRM_COLOR_RANGE_MAX,
};
Then this table allocation would be wrong.
Perhaps swapping for
> +static const u32
> lcdif_yuv2rgb_coeffs[DRM_COLOR_ENCODING_MAX][DRM_COLOR_RANGE_MAX][6] = {
Would be safer ... but longer :-( ?
Anyway, I think the rest of it looks fine, and perhaps these enums are
in the UAPI which would make them stable anyway:
Reviewed-by: Kieran Bingham
> + [DRM_COLOR_YCBCR_BT601] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> + 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(0x) | CSC0_COEF4_D1(0x01f0),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> + CSC0_COEF1_A3(0x0167) | CSC0_COEF1_B1(0x0100),
> + CSC0_COEF2_B2(0x07a8) | CSC0_COEF2_B3(0x0749),
> + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01c6),
> + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + },
> + [DRM_COLOR_YCBCR_BT709] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> + CSC0_COEF1_A3(0x01d6) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x07c9) | CSC0_COEF2_B3(0x0778),
> + CSC0_COEF3_C1(0x0123) | CSC0_COEF3_C2(0x021d),
> + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x01f0),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + [DRM_COLOR_YCBCR_FULL_RANGE] = {
> + CSC0_COEF0_A1(0x0100) | CSC0_COEF0_A2(0x),
> + CSC0_COEF1_A3(0x0193) | CSC0_COEF1_B1(0x0100),
> + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x0788),
> + CSC0_COEF3_C1(0x0100) | CSC0_COEF3_C2(0x01db),
> + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x),
> + CSC0_COEF5_D2(0x0180) | CSC0_COEF5_D3(0x0180),
> + },
> + },
> + [DRM_COLOR_YCBCR_BT2020] = {
> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = {
> + CSC0_COEF0_A1(0x012a) | CSC0_COEF0_A2(0x),
> + CSC0_COEF1_A3(0x01b8) | CSC0_COEF1_B1(0x0123),
> + CSC0_COEF2_B2(0x07d0) | CSC0_COEF2_B3(0x075a),
> + CSC0_COEF3_C1(0x0124) | CSC0_COEF3_C2(0x0224),
> + CSC0_COEF4_C3(0x) | CSC0_COEF4_D1(0x01
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
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.
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
Hi Marek,
On Wed, Sep 28, 2022 at 03:09:11AM +0200, Marek Vasut wrote:
> 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 ?
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 :-)
> > + } 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.
> Besides those nitpicks:
>
> Reviewed-by: Marek Vasut
--
Regards,
Laurent Pinchart
Re: [PATCH v2 4/4] drm: lcdif: Add support for YUV planes
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
