From: Dave Stevenson <[email protected]>
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.
Signed-off-by: Dave Stevenson <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
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:
--
2.38.1
Hi
Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <[email protected]>
>
> 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.
Best regards
Thomas
>
> Signed-off-by: Dave Stevenson <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> 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
Hi Thomas
Thanks for the review
On Wed, 11 Jan 2023 at 15:03, Thomas Zimmermann <[email protected]> wrote:
>
> Hi
>
> Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > From: Dave Stevenson <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > 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
Hi,
On Wed, Jan 11, 2023 at 05:00:41PM +0000, Dave Stevenson wrote:
> Hi Thomas
>
> Thanks for the review
>
> On Wed, 11 Jan 2023 at 15:03, Thomas Zimmermann <[email protected]> wrote:
> >
> > Hi
> >
> > Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > > From: Dave Stevenson <[email protected]>
> > >
> > > 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.
Thanks to both of you for that feedback. I've chosen a slightly
different solution since I believe it still makes sens to have the swap
patch separate. I've move the swap function introduction earlier and
removed the two redundant matrices in that patch. And now, that patch
doesn't drop any matrix anymore so I've removed the confusing part of
the commit log.
> 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?
I've squashed it in for the next revision
Thanks!
maxime