Re: [PATCH 5/9] drm/vc4: hdmi: Rework the CSC matrices organization

From: Dave Stevenson
Date: Wed Jan 11 2023 - 12:03:47 EST

Next message: Alan Stern: "Re: Internal vs. external barriers (was: Re: Interesting LKMM litmus test)"
Previous message: Johan Hovold: "Re: [PATCH v3 13/13] usb/serial: Rename dtr/rts parameters/variables to active"
In reply to: Thomas Zimmermann: "Re: [PATCH 5/9] drm/vc4: hdmi: Rework the CSC matrices organization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Thomas

Thanks for the review

On Wed, 11 Jan 2023 at 15:03, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >
> > The CSC matrices were stored as separate matrix for each colorspace, and
> > if we wanted a limited or full RGB output.
> >
> > This created some gaps in our support and we would not always pick the
> > relevant matrix.
> >
> > Let's rework our data structure to store one per colorspace, and then a
> > matrix for limited range and one for full range. This makes us add a new
> > matrix to support full range BT709 YUV output, and drops the redundant
> > (but somehow different) BT709 YUV444 vs YUV422 matrix.
>
> The final sentence is confusing and I didn't understand how two
> different matrices can now be just one.

Two changes to accommodate the hardware requirements:

Firstly the driver was only defining
vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709 and
vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709. There was no matrix for
full_rgb_to_full_yuv_bt709, so that had to be added.

Secondly, for some reason when in 444 mode the hardware wants the
matrix rows in a different order. That is why
vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709 differed from
vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709 - it was a simple
reordering of the rows.

This patch dropped the special handling for 444, and in the process
programmed the wrong coefficients into the hardware :-(
Patch 6/9 then reintroduces this reordering, so really should be
squashed into the one patch.

Looking at my more recent work, it looks like I messed up on 6/9 too.
One of the patches on [1] corrects that row swapping for yuv444 - I
was obviously having a bad day.

Maxime: Are you OK to fix that up?

Thanks

Dave

[1] https://github.com/raspberrypi/linux/pull/5249/commits

> Best regards
> Thomas
>
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/vc4/vc4_hdmi.c | 124 +++++++++++++++++++++--------------------
> > 1 file changed, 63 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 51469939a8b4..299a8fe7a2ae 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -1178,68 +1178,72 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> > }
> >
> > /*
> > - * If we need to output Full Range RGB, then use the unity matrix
> > + * Matrices for (internal) RGB to RGB output.
> > *
> > - * [ 1 0 0 0]
> > - * [ 0 1 0 0]
> > - * [ 0 0 1 0]
> > - *
> > - * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
> > - */
> > -static const u16 vc5_hdmi_csc_full_rgb_unity[3][4] = {
> > - { 0x2000, 0x0000, 0x0000, 0x0000 },
> > - { 0x0000, 0x2000, 0x0000, 0x0000 },
> > - { 0x0000, 0x0000, 0x2000, 0x0000 },
> > -};
> > -
> > -/*
> > - * CEA VICs other than #1 require limited range RGB output unless
> > - * overridden by an AVI infoframe. Apply a colorspace conversion to
> > - * squash 0-255 down to 16-235. The matrix here is:
> > - *
> > - * [ 0.8594 0 0 16]
> > - * [ 0 0.8594 0 16]
> > - * [ 0 0 0.8594 16]
> > - *
> > - * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
> > + * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
> > */
> > -static const u16 vc5_hdmi_csc_full_rgb_to_limited_rgb[3][4] = {
> > - { 0x1b80, 0x0000, 0x0000, 0x0400 },
> > - { 0x0000, 0x1b80, 0x0000, 0x0400 },
> > - { 0x0000, 0x0000, 0x1b80, 0x0400 },
> > +static const u16 vc5_hdmi_csc_full_rgb_to_rgb[2][3][4] = {
> > + {
> > + /*
> > + * Full range - unity
> > + *
> > + * [ 1 0 0 0]
> > + * [ 0 1 0 0]
> > + * [ 0 0 1 0]
> > + */
> > + { 0x2000, 0x0000, 0x0000, 0x0000 },
> > + { 0x0000, 0x2000, 0x0000, 0x0000 },
> > + { 0x0000, 0x0000, 0x2000, 0x0000 },
> > + },
> > + {
> > + /*
> > + * Limited range
> > + *
> > + * CEA VICs other than #1 require limited range RGB
> > + * output unless overridden by an AVI infoframe. Apply a
> > + * colorspace conversion to squash 0-255 down to 16-235.
> > + * The matrix here is:
> > + *
> > + * [ 0.8594 0 0 16]
> > + * [ 0 0.8594 0 16]
> > + * [ 0 0 0.8594 16]
> > + */
> > + { 0x1b80, 0x0000, 0x0000, 0x0400 },
> > + { 0x0000, 0x1b80, 0x0000, 0x0400 },
> > + { 0x0000, 0x0000, 0x1b80, 0x0400 },
> > + },
> > };
> >
> > /*
> > - * Conversion between Full Range RGB and Full Range YUV422 using the
> > - * BT.709 Colorspace
> > - *
> > - *
> > - * [ 0.181906 0.611804 0.061758 16 ]
> > - * [ -0.100268 -0.337232 0.437500 128 ]
> > - * [ 0.437500 -0.397386 -0.040114 128 ]
> > - *
> > - * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
> > - */
> > -static const u16 vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709[3][4] = {
> > - { 0x05d2, 0x1394, 0x01fa, 0x0400 },
> > - { 0xfccc, 0xf536, 0x0e00, 0x2000 },
> > - { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
> > -};
> > -
> > -/*
> > - * Conversion between Full Range RGB and Full Range YUV444 using the
> > - * BT.709 Colorspace
> > - *
> > - * [ -0.100268 -0.337232 0.437500 128 ]
> > - * [ 0.437500 -0.397386 -0.040114 128 ]
> > - * [ 0.181906 0.611804 0.061758 16 ]
> > + * Conversion between Full Range RGB and YUV using the BT.709 Colorspace
> > *
> > - * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
> > + * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
> > */
> > -static const u16 vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709[3][4] = {
> > - { 0xfccc, 0xf536, 0x0e00, 0x2000 },
> > - { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
> > - { 0x05d2, 0x1394, 0x01fa, 0x0400 },
> > +static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt709[2][3][4] = {
> > + {
> > + /*
> > + * Full Range
> > + *
> > + * [ 0.212600 0.715200 0.072200 0 ]
> > + * [ -0.114572 -0.385428 0.500000 128 ]
> > + * [ 0.500000 -0.454153 -0.045847 128 ]
> > + */
> > + { 0x06ce, 0x16e3, 0x024f, 0x0000 },
> > + { 0xfc56, 0xf3ac, 0x1000, 0x2000 },
> > + { 0x1000, 0xf179, 0xfe89, 0x2000 },
> > + },
> > + {
> > + /*
> > + * Limited Range
> > + *
> > + * [ 0.181906 0.611804 0.061758 16 ]
> > + * [ -0.100268 -0.337232 0.437500 128 ]
> > + * [ 0.437500 -0.397386 -0.040114 128 ]
> > + */
> > + { 0x05d2, 0x1394, 0x01fa, 0x0400 },
> > + { 0xfccc, 0xf536, 0x0e00, 0x2000 },
> > + { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
> > + },
> > };
> >
> > static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
> > @@ -1262,6 +1266,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> > struct drm_device *drm = vc4_hdmi->connector.dev;
> > struct vc4_hdmi_connector_state *vc4_state =
> > conn_state_to_vc4_hdmi_conn_state(state);
> > + unsigned int lim_range = vc4_hdmi_is_full_range(vc4_hdmi, mode) ? 0 : 1;
> > unsigned long flags;
> > u32 if_cfg = 0;
> > u32 if_xbar = 0x543210;
> > @@ -1277,7 +1282,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> >
> > switch (vc4_state->output_format) {
> > case VC4_HDMI_OUTPUT_YUV444:
> > - vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709);
> > + vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
> > break;
> >
> > case VC4_HDMI_OUTPUT_YUV422:
> > @@ -1292,16 +1297,13 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> > if_cfg |= VC4_SET_FIELD(VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422_FORMAT_422_LEGACY,
> > VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422);
> >
> > - vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709);
> > + vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
> > break;
> >
> > case VC4_HDMI_OUTPUT_RGB:
> > if_xbar = 0x354021;
> >
> > - if (!vc4_hdmi_is_full_range(vc4_hdmi, mode))
> > - vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
> > - else
> > - vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);
> > + vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_rgb[lim_range]);
> > break;
> >
> > default:
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev

Next message: Alan Stern: "Re: Internal vs. external barriers (was: Re: Interesting LKMM litmus test)"
Previous message: Johan Hovold: "Re: [PATCH v3 13/13] usb/serial: Rename dtr/rts parameters/variables to active"
In reply to: Thomas Zimmermann: "Re: [PATCH 5/9] drm/vc4: hdmi: Rework the CSC matrices organization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]