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
