[PATCH 0/9] drm/vc4: hdmi: Broadcast RGB, BT601, BT2020

[PATCH 0/9] drm/vc4: hdmi: Broadcast RGB, BT601, BT2020

[Maxime Ripard]

Hi,

Here's a collection of patches that have been in the downstream tree for a
while to add a bunch of new features to the HDMI controller.

Let me know what you think,
Maxime

To: Emma Anholt <emma@anholt.net>
To: Maxime Ripard <mripard@kernel.org>
To: David Airlie <airlied@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Dave Stevenson (7):
      drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range
      drm/vc4: hdmi: Rename full range helper
      drm/vc4: hdmi: Rework the CSC matrices organization
      drm/vc4: hdmi: Swap CSC matrix channels for YUV444
      drm/vc4: hdmi: Add a function to retrieve the CSC matrix
      drm/vc4: hdmi: Add BT.601 Support
      drm/vc4: hdmi: Add BT.2020 Support

Maxime Ripard (2):
      drm/vc4: hdmi: Update all the planes if the TV margins are changed
      drm/vc4: hdmi: Constify container_of wrappers

 drivers/gpu/drm/vc4/vc4_hdmi.c | 326 ++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  21 ++-
 2 files changed, 289 insertions(+), 58 deletions(-)
---
base-commit: 99e2d98adc738597abcc5d38b03d0e9858db5c00
change-id: 20221207-rpi-hdmi-improvements-3de1c0dba2dc

Best regards,
-- 
Maxime Ripard <maxime@cerno.tech>

[PATCH 1/9] drm/vc4: hdmi: Update all the planes if the TV margins are changed

[Maxime Ripard]

On VC4, the TV margins on the HDMI connector are implemented by scaling
the planes.

However, if only the TV margins or the connector are changed by a new
state, the planes ending up on that connector won't be. Thus, they won't
be updated properly and we'll effectively ignore that change until the
next commit affecting these planes.

Let's make sure to add all the planes attached to the connector so that
we can update them properly.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 12a00d644b61..0eafaf0b76e5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -522,6 +522,22 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 	if (!crtc)
 		return 0;
 
+	if (old_state->tv.margins.left != new_state->tv.margins.left ||
+	    old_state->tv.margins.right != new_state->tv.margins.right ||
+	    old_state->tv.margins.top != new_state->tv.margins.top ||
+	    old_state->tv.margins.bottom != new_state->tv.margins.bottom) {
+		struct drm_crtc_state *crtc_state;
+		int ret;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret)
+			return ret;
+	}
+
 	if (old_state->colorspace != new_state->colorspace ||
 	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
 		struct drm_crtc_state *crtc_state;

-- 
2.38.1

[PATCH 2/9] drm/vc4: hdmi: Constify container_of wrappers

[Maxime Ripard]

None of our wrappers around container_of to access our objects from the
DRM object pointer actually modify the latter.

Let's make them const.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index dc3ccd8002a0..023ea64ef006 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -224,13 +224,13 @@ struct vc4_hdmi {
 };
 
 static inline struct vc4_hdmi *
-connector_to_vc4_hdmi(struct drm_connector *connector)
+connector_to_vc4_hdmi(const struct drm_connector *connector)
 {
 	return container_of(connector, struct vc4_hdmi, connector);
 }
 
 static inline struct vc4_hdmi *
-encoder_to_vc4_hdmi(struct drm_encoder *encoder)
+encoder_to_vc4_hdmi(const struct drm_encoder *encoder)
 {
 	struct vc4_encoder *_encoder = to_vc4_encoder(encoder);
 	return container_of(_encoder, struct vc4_hdmi, encoder);
@@ -244,7 +244,7 @@ struct vc4_hdmi_connector_state {
 };
 
 static inline struct vc4_hdmi_connector_state *
-conn_state_to_vc4_hdmi_conn_state(struct drm_connector_state *conn_state)
+conn_state_to_vc4_hdmi_conn_state(const struct drm_connector_state *conn_state)
 {
 	return container_of(conn_state, struct vc4_hdmi_connector_state, base);
 }

-- 
2.38.1

[PATCH 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Copy Intel's "Broadcast RGB" property semantics to add manual override
of the HDMI pixel range for monitors that don't abide by the content
of the AVI Infoframe.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 87 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.h | 15 ++++++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 0eafaf0b76e5..488a4012d422 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -154,6 +154,11 @@ static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
 {
 	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
 
+	if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
+		return false;
+	else if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
+		return true;
+
 	return !display->is_hdmi ||
 		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
 }
@@ -515,8 +520,12 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 {
 	struct drm_connector_state *old_state =
 		drm_atomic_get_old_connector_state(state, connector);
+	struct vc4_hdmi_connector_state *old_vc4_state =
+		conn_state_to_vc4_hdmi_conn_state(old_state);
 	struct drm_connector_state *new_state =
 		drm_atomic_get_new_connector_state(state, connector);
+	struct vc4_hdmi_connector_state *new_vc4_state =
+		conn_state_to_vc4_hdmi_conn_state(new_state);
 	struct drm_crtc *crtc = new_state->crtc;
 
 	if (!crtc)
@@ -539,6 +548,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 	}
 
 	if (old_state->colorspace != new_state->colorspace ||
+	    old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
 	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
 		struct drm_crtc_state *crtc_state;
 
@@ -552,6 +562,49 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 	return 0;
 }
 
+static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
+					   const struct drm_connector_state *state,
+					   struct drm_property *property,
+					   uint64_t *val)
+{
+	struct drm_device *drm = connector->dev;
+	struct vc4_hdmi *vc4_hdmi =
+		connector_to_vc4_hdmi(connector);
+	struct vc4_hdmi_connector_state *vc4_conn_state =
+		conn_state_to_vc4_hdmi_conn_state(state);
+
+	if (property == vc4_hdmi->broadcast_rgb_property) {
+		*val = vc4_conn_state->broadcast_rgb;
+	} else {
+		drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+			property->base.id, property->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
+					   struct drm_connector_state *state,
+					   struct drm_property *property,
+					   uint64_t val)
+{
+	struct drm_device *drm = connector->dev;
+	struct vc4_hdmi *vc4_hdmi =
+		connector_to_vc4_hdmi(connector);
+	struct vc4_hdmi_connector_state *vc4_conn_state =
+		conn_state_to_vc4_hdmi_conn_state(state);
+
+	if (property == vc4_hdmi->broadcast_rgb_property) {
+		vc4_conn_state->broadcast_rgb = val;
+		return 0;
+	}
+
+	drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+		property->base.id, property->name);
+	return -EINVAL;
+}
+
 static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 {
 	struct vc4_hdmi_connector_state *old_state =
@@ -571,6 +624,7 @@ static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 	new_state->base.max_bpc = 8;
 	new_state->base.max_requested_bpc = 8;
 	new_state->output_format = VC4_HDMI_OUTPUT_RGB;
+	new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
 	drm_atomic_helper_connector_tv_margins_reset(connector);
 }
 
@@ -588,6 +642,7 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
 	new_state->tmds_char_rate = vc4_state->tmds_char_rate;
 	new_state->output_bpc = vc4_state->output_bpc;
 	new_state->output_format = vc4_state->output_format;
+	new_state->broadcast_rgb = vc4_state->broadcast_rgb;
 	__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
 
 	return &new_state->base;
@@ -598,6 +653,8 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
 	.reset = vc4_hdmi_connector_reset,
 	.atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_get_property = vc4_hdmi_connector_get_property,
+	.atomic_set_property = vc4_hdmi_connector_set_property,
 };
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
@@ -606,6 +663,33 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
 	.atomic_check = vc4_hdmi_connector_atomic_check,
 };
 
+static const struct drm_prop_enum_list broadcast_rgb_names[] = {
+	{ VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
+	{ VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
+	{ VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
+};
+
+static void
+vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
+				       struct vc4_hdmi *vc4_hdmi)
+{
+	struct drm_property *prop = vc4_hdmi->broadcast_rgb_property;
+
+	if (!prop) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+						"Broadcast RGB",
+						broadcast_rgb_names,
+						ARRAY_SIZE(broadcast_rgb_names));
+		if (!prop)
+			return;
+
+		vc4_hdmi->broadcast_rgb_property = prop;
+	}
+
+	drm_object_attach_property(&vc4_hdmi->connector.base, prop,
+				   VC4_HDMI_BROADCAST_RGB_AUTO);
+}
+
 static int vc4_hdmi_connector_init(struct drm_device *dev,
 				   struct vc4_hdmi *vc4_hdmi)
 {
@@ -652,6 +736,8 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
 	if (vc4_hdmi->variant->supports_hdr)
 		drm_connector_attach_hdr_output_metadata_property(connector);
 
+	vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
+
 	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
@@ -1690,6 +1776,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	mutex_lock(&vc4_hdmi->mutex);
 	drm_mode_copy(&vc4_hdmi->saved_adjusted_mode,
 		      &crtc_state->adjusted_mode);
+	vc4_hdmi->broadcast_rgb = vc4_state->broadcast_rgb;
 	vc4_hdmi->output_bpc = vc4_state->output_bpc;
 	vc4_hdmi->output_format = vc4_state->output_format;
 	mutex_unlock(&vc4_hdmi->mutex);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 023ea64ef006..d423f175339f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -117,6 +117,12 @@ enum vc4_hdmi_output_format {
 	VC4_HDMI_OUTPUT_YUV420,
 };
 
+enum vc4_hdmi_broadcast_rgb {
+	VC4_HDMI_BROADCAST_RGB_AUTO,
+	VC4_HDMI_BROADCAST_RGB_FULL,
+	VC4_HDMI_BROADCAST_RGB_LIMITED,
+};
+
 /* General HDMI hardware state. */
 struct vc4_hdmi {
 	struct vc4_hdmi_audio audio;
@@ -129,6 +135,8 @@ struct vc4_hdmi {
 
 	struct delayed_work scrambling_work;
 
+	struct drm_property *broadcast_rgb_property;
+
 	struct i2c_adapter *ddc;
 	void __iomem *hdmicore_regs;
 	void __iomem *hd_regs;
@@ -221,6 +229,12 @@ struct vc4_hdmi {
 	 * for use outside of KMS hooks. Protected by @mutex.
 	 */
 	enum vc4_hdmi_output_format output_format;
+
+	/**
+	 * @broadcast_rgb: Copy of @vc4_connector_state.broadcast_rgb
+	 * for use outside of KMS hooks. Protected by @mutex.
+	 */
+	enum vc4_hdmi_broadcast_rgb broadcast_rgb;
 };
 
 static inline struct vc4_hdmi *
@@ -241,6 +255,7 @@ struct vc4_hdmi_connector_state {
 	unsigned long long		tmds_char_rate;
 	unsigned int 			output_bpc;
 	enum vc4_hdmi_output_format	output_format;
+	int				broadcast_rgb;
 };
 
 static inline struct vc4_hdmi_connector_state *

-- 
2.38.1

[PATCH 4/9] drm/vc4: hdmi: Rename full range helper

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The VC4 HDMI driver has a helper function to figure out whether full
range or limited range RGB is being used called
vc4_hdmi_is_full_range_rgb().

We'll need it to support other colorspaces, so let's rename it to
vc4_hdmi_is_full_range().

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 488a4012d422..51469939a8b4 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -149,8 +149,8 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
 	return clock > HDMI_14_MAX_TMDS_CLK;
 }
 
-static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
-				       const struct drm_display_mode *mode)
+static bool vc4_hdmi_is_full_range(struct vc4_hdmi *vc4_hdmi,
+				   const struct drm_display_mode *mode)
 {
 	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
 
@@ -892,7 +892,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
 
 	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
 					   connector, mode,
-					   vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) ?
+					   vc4_hdmi_is_full_range(vc4_hdmi, mode) ?
 					   HDMI_QUANTIZATION_RANGE_FULL :
 					   HDMI_QUANTIZATION_RANGE_LIMITED);
 	drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
@@ -1145,7 +1145,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 	csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
 				VC4_HD_CSC_CTL_ORDER);
 
-	if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) {
+	if (!vc4_hdmi_is_full_range(vc4_hdmi, mode)) {
 		/* CEA VICs other than #1 requre limited range RGB
 		 * output unless overridden by an AVI infoframe.
 		 * Apply a colorspace conversion to squash 0-255 down
@@ -1298,7 +1298,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 	case VC4_HDMI_OUTPUT_RGB:
 		if_xbar = 0x354021;
 
-		if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode))
+		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);

-- 
2.38.1

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

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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 <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 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

[PATCH 6/9] drm/vc4: hdmi: Swap CSC matrix channels for YUV444

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

YUV444 requires the matrix coefficients to be programmed in a different
way than the other formats. Let's add a function to program it properly.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 299a8fe7a2ae..cb92d07680f0 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1259,6 +1259,20 @@ static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
 	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[2][3] << 16) | coeffs[2][2]);
 }
 
+static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
+					 const u16 coeffs[3][4])
+{
+	lockdep_assert_held(&vc4_hdmi->hw_lock);
+
+	/* YUV444 needs the CSC matrices using the channels in a different order */
+	HDMI_WRITE(HDMI_CSC_12_11, (coeffs[2][1] << 16) | coeffs[2][0]);
+	HDMI_WRITE(HDMI_CSC_14_13, (coeffs[2][3] << 16) | coeffs[2][2]);
+	HDMI_WRITE(HDMI_CSC_22_21, (coeffs[0][1] << 16) | coeffs[0][0]);
+	HDMI_WRITE(HDMI_CSC_24_23, (coeffs[0][3] << 16) | coeffs[0][2]);
+	HDMI_WRITE(HDMI_CSC_32_31, (coeffs[1][1] << 16) | coeffs[1][0]);
+	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[1][3] << 16) | coeffs[1][2]);
+}
+
 static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 			       struct drm_connector_state *state,
 			       const struct drm_display_mode *mode)
@@ -1282,7 +1296,8 @@ 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_yuv_bt709[lim_range]);
+		vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi,
+					     vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
 		break;
 
 	case VC4_HDMI_OUTPUT_YUV422:

-- 
2.38.1

[PATCH 7/9] drm/vc4: hdmi: Add a function to retrieve the CSC matrix

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The CSC matrix to use depends on the output format, its range and the
colorspace.

Since we're going to add more colorspaces, let's move the CSC matrix
retrieval to a function.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index cb92d07680f0..cd6775429b5e 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1273,6 +1273,20 @@ static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
 	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[1][3] << 16) | coeffs[1][2]);
 }
 
+static const u16
+(*vc5_hdmi_get_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]
+{
+	switch (colorspace) {
+	default:
+	case DRM_MODE_COLORIMETRY_NO_DATA:
+	case DRM_MODE_COLORIMETRY_BT709_YCC:
+	case DRM_MODE_COLORIMETRY_XVYCC_709:
+	case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
+	case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
+		return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
+	}
+}
+
 static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 			       struct drm_connector_state *state,
 			       const struct drm_display_mode *mode)
@@ -1282,6 +1296,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 		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;
+	const u16 (*csc)[4];
 	u32 if_cfg = 0;
 	u32 if_xbar = 0x543210;
 	u32 csc_chan_ctl = 0;
@@ -1296,11 +1311,16 @@ 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_swap(vc4_hdmi,
-					     vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
+		csc = vc5_hdmi_get_yuv_csc_coeffs(vc4_hdmi, state->colorspace,
+						  vc4_hdmi_is_full_range(vc4_hdmi, mode));
+
+		vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi, csc);
 		break;
 
 	case VC4_HDMI_OUTPUT_YUV422:
+		csc = vc5_hdmi_get_yuv_csc_coeffs(vc4_hdmi, state->colorspace,
+						  vc4_hdmi_is_full_range(vc4_hdmi, mode));
+
 		csc_ctl |= VC4_SET_FIELD(VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422_STANDARD,
 					 VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422) |
 			VC5_MT_CP_CSC_CTL_USE_444_TO_422 |
@@ -1312,7 +1332,7 @@ 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_yuv_bt709[lim_range]);
+		vc5_hdmi_set_csc_coeffs(vc4_hdmi, csc);
 		break;
 
 	case VC4_HDMI_OUTPUT_RGB:

-- 
2.38.1

[PATCH 8/9] drm/vc4: hdmi: Add BT.601 Support

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Even though we report that we support the BT601 Colorspace, we were
always using the BT.709 conversion matrices. Let's add the BT601 ones.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index cd6775429b5e..e3428fb2c22d 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1214,6 +1214,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_rgb[2][3][4] = {
 	},
 };
 
+/*
+ * Conversion between Full Range RGB and YUV using the BT.601 Colorspace
+ *
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
+ */
+static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt601[2][3][4] = {
+	{
+		/*
+		 * Full Range
+		 *
+		 * [  0.299000  0.587000  0.114000  0   ]
+		 * [ -0.168736 -0.331264  0.500000  128 ]
+		 * [  0.500000 -0.418688 -0.081312  128 ]
+		 */
+		{ 0x0991, 0x12c9, 0x03a6, 0x0000 },
+		{ 0xfa9b, 0xf567, 0x1000, 0x2000 },
+		{ 0x1000, 0xf29b, 0xfd67, 0x2000 },
+	},
+	{
+		/* Limited Range
+		 *
+		 * [  0.255785  0.502160  0.097523  16  ]
+		 * [ -0.147644 -0.289856  0.437500  128 ]
+		 * [  0.437500 -0.366352 -0.071148  128 ]
+		 */
+		{ 0x082f, 0x1012, 0x031f, 0x0400 },
+		{ 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
+		{ 0x0e00, 0xf448, 0xfdba, 0x2000 },
+	},
+};
+
 /*
  * Conversion between Full Range RGB and YUV using the BT.709 Colorspace
  *
@@ -1277,6 +1308,13 @@ static const u16
 (*vc5_hdmi_get_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]
 {
 	switch (colorspace) {
+	case DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
+	case DRM_MODE_COLORIMETRY_XVYCC_601:
+	case DRM_MODE_COLORIMETRY_SYCC_601:
+	case DRM_MODE_COLORIMETRY_OPYCC_601:
+	case DRM_MODE_COLORIMETRY_BT601_YCC:
+		return vc5_hdmi_csc_full_rgb_to_yuv_bt601[limited];
+
 	default:
 	case DRM_MODE_COLORIMETRY_NO_DATA:
 	case DRM_MODE_COLORIMETRY_BT709_YCC:

-- 
2.38.1

[PATCH 9/9] drm/vc4: hdmi: Add BT.2020 Support

[Maxime Ripard]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Even though we report that we support the BT.2020 Colorspace, we were
always using the BT.709 conversion matrices. Let's add the BT.2020 ones.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index e3428fb2c22d..2734cab34660 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1277,6 +1277,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt709[2][3][4] = {
 	},
 };
 
+/*
+ * Conversion between Full Range RGB and YUV using the BT.2020 Colorspace
+ *
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
+ */
+static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt2020[2][3][4] = {
+	{
+		/*
+		 * Full Range
+		 *
+		 * [  0.262700  0.678000  0.059300  0   ]
+		 * [ -0.139630 -0.360370  0.500000  128 ]
+		 * [  0.500000 -0.459786 -0.040214  128 ]
+		 */
+		{ 0x0868, 0x15b2, 0x01e6, 0x0000 },
+		{ 0xfb89, 0xf479, 0x1000, 0x2000 },
+		{ 0x1000, 0xf14a, 0xfeb8, 0x2000 },
+	},
+	{
+		/* Limited Range
+		 *
+		 * [  0.224732  0.580008  0.050729  16  ]
+		 * [ -0.122176 -0.315324  0.437500  128 ]
+		 * [  0.437500 -0.402312 -0.035188  128 ]
+		 */
+		{ 0x082f, 0x1012, 0x031f, 0x0400 },
+		{ 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
+		{ 0x0e00, 0xf448, 0xfdba, 0x2000 },
+	},
+};
+
 static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
 				    const u16 coeffs[3][4])
 {
@@ -1322,6 +1353,13 @@ static const u16
 	case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
 	case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
 		return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
+
+	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
+	case DRM_MODE_COLORIMETRY_BT2020_YCC:
+	case DRM_MODE_COLORIMETRY_BT2020_RGB:
+	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
+	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
+		return vc5_hdmi_csc_full_rgb_to_yuv_bt2020[limited];
 	}
 }
 

-- 
2.38.1

Re: [PATCH 1/9] drm/vc4: hdmi: Update all the planes if the TV margins are changed

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2747 bytes --]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> On VC4, the TV margins on the HDMI connector are implemented by scaling
> the planes.
> 
> However, if only the TV margins or the connector are changed by a new
> state, the planes ending up on that connector won't be. Thus, they won't
> be updated properly and we'll effectively ignore that change until the
> next commit affecting these planes.
> 
> Let's make sure to add all the planes attached to the connector so that
> we can update them properly.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 12a00d644b61..0eafaf0b76e5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -522,6 +522,22 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>   	if (!crtc)
>   		return 0;
>   
> +	if (old_state->tv.margins.left != new_state->tv.margins.left ||
> +	    old_state->tv.margins.right != new_state->tv.margins.right ||
> +	    old_state->tv.margins.top != new_state->tv.margins.top ||
> +	    old_state->tv.margins.bottom != new_state->tv.margins.bottom) {
> +		struct drm_crtc_state *crtc_state;
> +		int ret;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		ret = drm_atomic_add_affected_planes(state, crtc);

This is slightly dangerous, but works in the given case. I'd appreciate 
a comment ala 'plane state will be checked by atomic helpers'.

The plane state of the added planes has to be tested via their 
atomic_check helpers. That's no problem here because the connector's 
atomic_check runs before the plane's atomic_check. See _check_modeset vs 
_check_planes in [1].

We had code that invoked drm_atomic_add_affected_planes() from the 
CRTC's atomic_check. At that point, the plane's atomic_check has been 
executed already and the newly added planes were never really state-checked.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.1.4/source/drivers/gpu/drm/drm_atomic_helper.c#L1069

> +		if (ret)
> +			return ret;
> +	}
> +
>   	if (old_state->colorspace != new_state->colorspace ||
>   	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
>   		struct drm_crtc_state *crtc_state;
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/9] drm/vc4: hdmi: Constify container_of wrappers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2245 bytes --]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> None of our wrappers around container_of to access our objects from the
> DRM object pointer actually modify the latter.
> 
> Let's make them const.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Personally, I wouldn't take this patch as it does an implicit const cast 
on the pointer.

For const correctness, it seems better to add new helpers that keep the 
const. Those could be use in places where the caller is not allowed to 
modify the state (i.e., atomic_update, et al).

Something like this:

const struct vc4_hdmi *
const_connector_to_vc4_hdmi(const struct drm_connector *connector)
{
	return container_of(connector, const struct vc4_hdmi,
		connector);
}

Best regards
Thomas

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index dc3ccd8002a0..023ea64ef006 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -224,13 +224,13 @@ struct vc4_hdmi {
>   };
>   
>   static inline struct vc4_hdmi *
> -connector_to_vc4_hdmi(struct drm_connector *connector)
> +connector_to_vc4_hdmi(const struct drm_connector *connector)
>   {
>   	return container_of(connector, struct vc4_hdmi, connector);
>   }
>   
>   static inline struct vc4_hdmi *
> -encoder_to_vc4_hdmi(struct drm_encoder *encoder)
> +encoder_to_vc4_hdmi(const struct drm_encoder *encoder)
>   {
>   	struct vc4_encoder *_encoder = to_vc4_encoder(encoder);
>   	return container_of(_encoder, struct vc4_hdmi, encoder);
> @@ -244,7 +244,7 @@ struct vc4_hdmi_connector_state {
>   };
>   
>   static inline struct vc4_hdmi_connector_state *
> -conn_state_to_vc4_hdmi_conn_state(struct drm_connector_state *conn_state)
> +conn_state_to_vc4_hdmi_conn_state(const struct drm_connector_state *conn_state)
>   {
>   	return container_of(conn_state, struct vc4_hdmi_connector_state, base);
>   }
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9101 bytes --]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Copy Intel's "Broadcast RGB" property semantics to add manual override
> of the HDMI pixel range for monitors that don't abide by the content
> of the AVI Infoframe.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 87 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/vc4/vc4_hdmi.h | 15 ++++++++
>   2 files changed, 102 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 0eafaf0b76e5..488a4012d422 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -154,6 +154,11 @@ static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
>   {
>   	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
>   
> +	if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
> +		return false;
> +	else if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
> +		return true;
> +
>   	return !display->is_hdmi ||
>   		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;

The existing code is now the branch for VC4_HDMI_BROADCAST_RGB_AUTO, AFAIU.

>   }
> @@ -515,8 +520,12 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>   {
>   	struct drm_connector_state *old_state =
>   		drm_atomic_get_old_connector_state(state, connector);
> +	struct vc4_hdmi_connector_state *old_vc4_state =
> +		conn_state_to_vc4_hdmi_conn_state(old_state);
>   	struct drm_connector_state *new_state =
>   		drm_atomic_get_new_connector_state(state, connector);
> +	struct vc4_hdmi_connector_state *new_vc4_state =
> +		conn_state_to_vc4_hdmi_conn_state(new_state);
>   	struct drm_crtc *crtc = new_state->crtc;
>   
>   	if (!crtc)
> @@ -539,6 +548,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>   	}
>   
>   	if (old_state->colorspace != new_state->colorspace ||
> +	    old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
>   	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
>   		struct drm_crtc_state *crtc_state;
>   
> @@ -552,6 +562,49 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>   	return 0;
>   }
>   
> +static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
> +					   const struct drm_connector_state *state,
> +					   struct drm_property *property,
> +					   uint64_t *val)
> +{
> +	struct drm_device *drm = connector->dev;
> +	struct vc4_hdmi *vc4_hdmi =
> +		connector_to_vc4_hdmi(connector);
> +	struct vc4_hdmi_connector_state *vc4_conn_state =
> +		conn_state_to_vc4_hdmi_conn_state(state);
> +
> +	if (property == vc4_hdmi->broadcast_rgb_property) {
> +		*val = vc4_conn_state->broadcast_rgb;
> +	} else {
> +		drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
> +			property->base.id, property->name);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
> +					   struct drm_connector_state *state,
> +					   struct drm_property *property,
> +					   uint64_t val)
> +{
> +	struct drm_device *drm = connector->dev;
> +	struct vc4_hdmi *vc4_hdmi =
> +		connector_to_vc4_hdmi(connector);
> +	struct vc4_hdmi_connector_state *vc4_conn_state =
> +		conn_state_to_vc4_hdmi_conn_state(state);
> +
> +	if (property == vc4_hdmi->broadcast_rgb_property) {
> +		vc4_conn_state->broadcast_rgb = val;
> +		return 0;
> +	}
> +
> +	drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
> +		property->base.id, property->name);
> +	return -EINVAL;
> +}
> +
>   static void vc4_hdmi_connector_reset(struct drm_connector *connector)
>   {
>   	struct vc4_hdmi_connector_state *old_state =
> @@ -571,6 +624,7 @@ static void vc4_hdmi_connector_reset(struct drm_connector *connector)
>   	new_state->base.max_bpc = 8;
>   	new_state->base.max_requested_bpc = 8;
>   	new_state->output_format = VC4_HDMI_OUTPUT_RGB;
> +	new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
>   	drm_atomic_helper_connector_tv_margins_reset(connector);
>   }
>   
> @@ -588,6 +642,7 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
>   	new_state->tmds_char_rate = vc4_state->tmds_char_rate;
>   	new_state->output_bpc = vc4_state->output_bpc;
>   	new_state->output_format = vc4_state->output_format;
> +	new_state->broadcast_rgb = vc4_state->broadcast_rgb;
>   	__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
>   
>   	return &new_state->base;
> @@ -598,6 +653,8 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
>   	.reset = vc4_hdmi_connector_reset,
>   	.atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.atomic_get_property = vc4_hdmi_connector_get_property,
> +	.atomic_set_property = vc4_hdmi_connector_set_property,
>   };
>   
>   static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
> @@ -606,6 +663,33 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
>   	.atomic_check = vc4_hdmi_connector_atomic_check,
>   };
>   
> +static const struct drm_prop_enum_list broadcast_rgb_names[] = {
> +	{ VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
> +	{ VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
> +	{ VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
> +};
> +
> +static void
> +vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
> +				       struct vc4_hdmi *vc4_hdmi)
> +{
> +	struct drm_property *prop = vc4_hdmi->broadcast_rgb_property;
> +
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +						"Broadcast RGB",
> +						broadcast_rgb_names,
> +						ARRAY_SIZE(broadcast_rgb_names));
> +		if (!prop)
> +			return;
> +
> +		vc4_hdmi->broadcast_rgb_property = prop;
> +	}
> +
> +	drm_object_attach_property(&vc4_hdmi->connector.base, prop,
> +				   VC4_HDMI_BROADCAST_RGB_AUTO);
> +}
> +
>   static int vc4_hdmi_connector_init(struct drm_device *dev,
>   				   struct vc4_hdmi *vc4_hdmi)
>   {
> @@ -652,6 +736,8 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
>   	if (vc4_hdmi->variant->supports_hdr)
>   		drm_connector_attach_hdr_output_metadata_property(connector);
>   
> +	vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
> +
>   	drm_connector_attach_encoder(connector, encoder);
>   
>   	return 0;
> @@ -1690,6 +1776,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
>   	mutex_lock(&vc4_hdmi->mutex);
>   	drm_mode_copy(&vc4_hdmi->saved_adjusted_mode,
>   		      &crtc_state->adjusted_mode);
> +	vc4_hdmi->broadcast_rgb = vc4_state->broadcast_rgb;
>   	vc4_hdmi->output_bpc = vc4_state->output_bpc;
>   	vc4_hdmi->output_format = vc4_state->output_format;
>   	mutex_unlock(&vc4_hdmi->mutex);
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 023ea64ef006..d423f175339f 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -117,6 +117,12 @@ enum vc4_hdmi_output_format {
>   	VC4_HDMI_OUTPUT_YUV420,
>   };
>   
> +enum vc4_hdmi_broadcast_rgb {
> +	VC4_HDMI_BROADCAST_RGB_AUTO,
> +	VC4_HDMI_BROADCAST_RGB_FULL,
> +	VC4_HDMI_BROADCAST_RGB_LIMITED,
> +};
> +
>   /* General HDMI hardware state. */
>   struct vc4_hdmi {
>   	struct vc4_hdmi_audio audio;
> @@ -129,6 +135,8 @@ struct vc4_hdmi {
>   
>   	struct delayed_work scrambling_work;
>   
> +	struct drm_property *broadcast_rgb_property;
> +
>   	struct i2c_adapter *ddc;
>   	void __iomem *hdmicore_regs;
>   	void __iomem *hd_regs;
> @@ -221,6 +229,12 @@ struct vc4_hdmi {
>   	 * for use outside of KMS hooks. Protected by @mutex.
>   	 */
>   	enum vc4_hdmi_output_format output_format;
> +
> +	/**
> +	 * @broadcast_rgb: Copy of @vc4_connector_state.broadcast_rgb
> +	 * for use outside of KMS hooks. Protected by @mutex.
> +	 */
> +	enum vc4_hdmi_broadcast_rgb broadcast_rgb;

I cannot seem to find any users of this field. If this is not used, 
please remove it from the patch. It seems questionable to have this 
outside if the connector state anyway.

>   };
>   
>   static inline struct vc4_hdmi *
> @@ -241,6 +255,7 @@ struct vc4_hdmi_connector_state {
>   	unsigned long long		tmds_char_rate;
>   	unsigned int 			output_bpc;
>   	enum vc4_hdmi_output_format	output_format;
> +	int				broadcast_rgb;

Maybe 'enum vc4_hdmi_broadcast_rgb'.

Best regards
Thomas

>   };
>   
>   static inline struct vc4_hdmi_connector_state *
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/9] drm/vc4: hdmi: Rename full range helper

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2866 bytes --]



Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> The VC4 HDMI driver has a helper function to figure out whether full
> range or limited range RGB is being used called
> vc4_hdmi_is_full_range_rgb().
> 
> We'll need it to support other colorspaces, so let's rename it to
> vc4_hdmi_is_full_range().
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 488a4012d422..51469939a8b4 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -149,8 +149,8 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
>   	return clock > HDMI_14_MAX_TMDS_CLK;
>   }
>   
> -static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
> -				       const struct drm_display_mode *mode)
> +static bool vc4_hdmi_is_full_range(struct vc4_hdmi *vc4_hdmi,
> +				   const struct drm_display_mode *mode)
>   {
>   	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
>   
> @@ -892,7 +892,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
>   
>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>   					   connector, mode,
> -					   vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) ?
> +					   vc4_hdmi_is_full_range(vc4_hdmi, mode) ?
>   					   HDMI_QUANTIZATION_RANGE_FULL :
>   					   HDMI_QUANTIZATION_RANGE_LIMITED);
>   	drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
> @@ -1145,7 +1145,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>   	csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
>   				VC4_HD_CSC_CTL_ORDER);
>   
> -	if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) {
> +	if (!vc4_hdmi_is_full_range(vc4_hdmi, mode)) {
>   		/* CEA VICs other than #1 requre limited range RGB
>   		 * output unless overridden by an AVI infoframe.
>   		 * Apply a colorspace conversion to squash 0-255 down
> @@ -1298,7 +1298,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>   	case VC4_HDMI_OUTPUT_RGB:
>   		if_xbar = 0x354021;
>   
> -		if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode))
> +		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);
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/9] drm/vc4: hdmi: Constify container_of wrappers

[Jani Nikula]

On Wed, 11 Jan 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 07.12.22 um 17:07 schrieb Maxime Ripard:
>> None of our wrappers around container_of to access our objects from the
>> DRM object pointer actually modify the latter.
>> 
>> Let's make them const.
>> 
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> Personally, I wouldn't take this patch as it does an implicit const cast 
> on the pointer.
>
> For const correctness, it seems better to add new helpers that keep the 
> const. Those could be use in places where the caller is not allowed to 
> modify the state (i.e., atomic_update, et al).
>
> Something like this:
>
> const struct vc4_hdmi *
> const_connector_to_vc4_hdmi(const struct drm_connector *connector)
> {
> 	return container_of(connector, const struct vc4_hdmi,
> 		connector);
> }

See container_of_const().

BR,
Jani.

>
> Best regards
> Thomas
>
>> ---
>>   drivers/gpu/drm/vc4/vc4_hdmi.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
>> index dc3ccd8002a0..023ea64ef006 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
>> @@ -224,13 +224,13 @@ struct vc4_hdmi {
>>   };
>>   
>>   static inline struct vc4_hdmi *
>> -connector_to_vc4_hdmi(struct drm_connector *connector)
>> +connector_to_vc4_hdmi(const struct drm_connector *connector)
>>   {
>>   	return container_of(connector, struct vc4_hdmi, connector);
>>   }
>>   
>>   static inline struct vc4_hdmi *
>> -encoder_to_vc4_hdmi(struct drm_encoder *encoder)
>> +encoder_to_vc4_hdmi(const struct drm_encoder *encoder)
>>   {
>>   	struct vc4_encoder *_encoder = to_vc4_encoder(encoder);
>>   	return container_of(_encoder, struct vc4_hdmi, encoder);
>> @@ -244,7 +244,7 @@ struct vc4_hdmi_connector_state {
>>   };
>>   
>>   static inline struct vc4_hdmi_connector_state *
>> -conn_state_to_vc4_hdmi_conn_state(struct drm_connector_state *conn_state)
>> +conn_state_to_vc4_hdmi_conn_state(const struct drm_connector_state *conn_state)
>>   {
>>   	return container_of(conn_state, struct vc4_hdmi_connector_state, base);
>>   }
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7248 bytes --]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> 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 <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 6/9] drm/vc4: hdmi: Swap CSC matrix channels for YUV444

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2389 bytes --]



Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> YUV444 requires the matrix coefficients to be programmed in a different
> way than the other formats. Let's add a function to program it properly.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 299a8fe7a2ae..cb92d07680f0 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1259,6 +1259,20 @@ static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
>   	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[2][3] << 16) | coeffs[2][2]);
>   }
>   
> +static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
> +					 const u16 coeffs[3][4])
> +{
> +	lockdep_assert_held(&vc4_hdmi->hw_lock);
> +
> +	/* YUV444 needs the CSC matrices using the channels in a different order */
> +	HDMI_WRITE(HDMI_CSC_12_11, (coeffs[2][1] << 16) | coeffs[2][0]);
> +	HDMI_WRITE(HDMI_CSC_14_13, (coeffs[2][3] << 16) | coeffs[2][2]);
> +	HDMI_WRITE(HDMI_CSC_22_21, (coeffs[0][1] << 16) | coeffs[0][0]);
> +	HDMI_WRITE(HDMI_CSC_24_23, (coeffs[0][3] << 16) | coeffs[0][2]);
> +	HDMI_WRITE(HDMI_CSC_32_31, (coeffs[1][1] << 16) | coeffs[1][0]);
> +	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[1][3] << 16) | coeffs[1][2]);
> +}
> +
>   static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>   			       struct drm_connector_state *state,
>   			       const struct drm_display_mode *mode)
> @@ -1282,7 +1296,8 @@ 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_yuv_bt709[lim_range]);
> +		vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi,
> +					     vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
>   		break;
>   
>   	case VC4_HDMI_OUTPUT_YUV422:
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 7/9] drm/vc4: hdmi: Add a function to retrieve the CSC matrix

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3564 bytes --]

Hi

Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> The CSC matrix to use depends on the output format, its range and the
> colorspace.
> 
> Since we're going to add more colorspaces, let's move the CSC matrix
> retrieval to a function.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index cb92d07680f0..cd6775429b5e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1273,6 +1273,20 @@ static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
>   	HDMI_WRITE(HDMI_CSC_34_33, (coeffs[1][3] << 16) | coeffs[1][2]);
>   }
>   
> +static const u16
> +(*vc5_hdmi_get_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]

I don't like the function name here. The helper 
vc5_hdmi_set_csc_coeffs() programs hardware, but this similarly named 
helper looks up static data.

Best regards
Thomas

> +{
> +	switch (colorspace) {
> +	default:
> +	case DRM_MODE_COLORIMETRY_NO_DATA:
> +	case DRM_MODE_COLORIMETRY_BT709_YCC:
> +	case DRM_MODE_COLORIMETRY_XVYCC_709:
> +	case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
> +	case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
> +		return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
> +	}
> +}
> +
>   static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>   			       struct drm_connector_state *state,
>   			       const struct drm_display_mode *mode)
> @@ -1282,6 +1296,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>   		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;
> +	const u16 (*csc)[4];
>   	u32 if_cfg = 0;
>   	u32 if_xbar = 0x543210;
>   	u32 csc_chan_ctl = 0;
> @@ -1296,11 +1311,16 @@ 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_swap(vc4_hdmi,
> -					     vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
> +		csc = vc5_hdmi_get_yuv_csc_coeffs(vc4_hdmi, state->colorspace,
> +						  vc4_hdmi_is_full_range(vc4_hdmi, mode));
> +
> +		vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi, csc);
>   		break;
>   
>   	case VC4_HDMI_OUTPUT_YUV422:
> +		csc = vc5_hdmi_get_yuv_csc_coeffs(vc4_hdmi, state->colorspace,
> +						  vc4_hdmi_is_full_range(vc4_hdmi, mode));
> +
>   		csc_ctl |= VC4_SET_FIELD(VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422_STANDARD,
>   					 VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422) |
>   			VC5_MT_CP_CSC_CTL_USE_444_TO_422 |
> @@ -1312,7 +1332,7 @@ 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_yuv_bt709[lim_range]);
> +		vc5_hdmi_set_csc_coeffs(vc4_hdmi, csc);
>   		break;
>   
>   	case VC4_HDMI_OUTPUT_RGB:
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 8/9] drm/vc4: hdmi: Add BT.601 Support

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2621 bytes --]



Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Even though we report that we support the BT601 Colorspace, we were
> always using the BT.709 conversion matrices. Let's add the BT601 ones.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index cd6775429b5e..e3428fb2c22d 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1214,6 +1214,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_rgb[2][3][4] = {
>   	},
>   };
>   
> +/*
> + * Conversion between Full Range RGB and YUV using the BT.601 Colorspace
> + *
> + * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
> + */
> +static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt601[2][3][4] = {
> +	{
> +		/*
> +		 * Full Range
> +		 *
> +		 * [  0.299000  0.587000  0.114000  0   ]
> +		 * [ -0.168736 -0.331264  0.500000  128 ]
> +		 * [  0.500000 -0.418688 -0.081312  128 ]
> +		 */
> +		{ 0x0991, 0x12c9, 0x03a6, 0x0000 },
> +		{ 0xfa9b, 0xf567, 0x1000, 0x2000 },
> +		{ 0x1000, 0xf29b, 0xfd67, 0x2000 },
> +	},
> +	{
> +		/* Limited Range
> +		 *
> +		 * [  0.255785  0.502160  0.097523  16  ]
> +		 * [ -0.147644 -0.289856  0.437500  128 ]
> +		 * [  0.437500 -0.366352 -0.071148  128 ]
> +		 */
> +		{ 0x082f, 0x1012, 0x031f, 0x0400 },
> +		{ 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
> +		{ 0x0e00, 0xf448, 0xfdba, 0x2000 },
> +	},
> +};
> +
>   /*
>    * Conversion between Full Range RGB and YUV using the BT.709 Colorspace
>    *
> @@ -1277,6 +1308,13 @@ static const u16
>   (*vc5_hdmi_get_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]
>   {
>   	switch (colorspace) {
> +	case DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> +	case DRM_MODE_COLORIMETRY_XVYCC_601:
> +	case DRM_MODE_COLORIMETRY_SYCC_601:
> +	case DRM_MODE_COLORIMETRY_OPYCC_601:
> +	case DRM_MODE_COLORIMETRY_BT601_YCC:
> +		return vc5_hdmi_csc_full_rgb_to_yuv_bt601[limited];
> +
>   	default:
>   	case DRM_MODE_COLORIMETRY_NO_DATA:
>   	case DRM_MODE_COLORIMETRY_BT709_YCC:
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 9/9] drm/vc4: hdmi: Add BT.2020 Support

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2612 bytes --]



Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Even though we report that we support the BT.2020 Colorspace, we were
> always using the BT.709 conversion matrices. Let's add the BT.2020 ones.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index e3428fb2c22d..2734cab34660 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1277,6 +1277,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt709[2][3][4] = {
>   	},
>   };
>   
> +/*
> + * Conversion between Full Range RGB and YUV using the BT.2020 Colorspace
> + *
> + * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
> + */
> +static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt2020[2][3][4] = {
> +	{
> +		/*
> +		 * Full Range
> +		 *
> +		 * [  0.262700  0.678000  0.059300  0   ]
> +		 * [ -0.139630 -0.360370  0.500000  128 ]
> +		 * [  0.500000 -0.459786 -0.040214  128 ]
> +		 */
> +		{ 0x0868, 0x15b2, 0x01e6, 0x0000 },
> +		{ 0xfb89, 0xf479, 0x1000, 0x2000 },
> +		{ 0x1000, 0xf14a, 0xfeb8, 0x2000 },
> +	},
> +	{
> +		/* Limited Range
> +		 *
> +		 * [  0.224732  0.580008  0.050729  16  ]
> +		 * [ -0.122176 -0.315324  0.437500  128 ]
> +		 * [  0.437500 -0.402312 -0.035188  128 ]
> +		 */
> +		{ 0x082f, 0x1012, 0x031f, 0x0400 },
> +		{ 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
> +		{ 0x0e00, 0xf448, 0xfdba, 0x2000 },
> +	},
> +};
> +
>   static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
>   				    const u16 coeffs[3][4])
>   {
> @@ -1322,6 +1353,13 @@ static const u16
>   	case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
>   	case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
>   		return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
> +
> +	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
> +	case DRM_MODE_COLORIMETRY_BT2020_YCC:
> +	case DRM_MODE_COLORIMETRY_BT2020_RGB:
> +	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> +	case DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> +		return vc5_hdmi_csc_full_rgb_to_yuv_bt2020[limited];
>   	}
>   }
>   
> 

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Dave Stevenson]

Hi Thomas

Thanks for the review

On Wed, 11 Jan 2023 at 15:03, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > 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@raspberrypi.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   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

Re: [PATCH 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

[Maxime Ripard]

Hi,

Thanks for your review

On Wed, Jan 11, 2023 at 03:11:51PM +0100, Thomas Zimmermann wrote:
> Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > 
> > Copy Intel's "Broadcast RGB" property semantics to add manual override
> > of the HDMI pixel range for monitors that don't abide by the content
> > of the AVI Infoframe.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_hdmi.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/vc4/vc4_hdmi.h | 15 ++++++++
> >   2 files changed, 102 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 0eafaf0b76e5..488a4012d422 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -154,6 +154,11 @@ static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
> >   {
> >   	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
> > +	if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
> > +		return false;
> > +	else if (vc4_hdmi->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
> > +		return true;
> > +
> >   	return !display->is_hdmi ||
> >   		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
> 
> The existing code is now the branch for VC4_HDMI_BROADCAST_RGB_AUTO, AFAIU.

I'm not entirely sure what you meant here sorry. The existing code path
is indeed the VC4_HDMI_BROADCAST_RGB_AUTO case, which is the property
default so the current behaviour should remain by default.

Is there anything you want me to clarify?

Maxime

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

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 <tzimmermann@suse.de> wrote:
> >
> > Hi
> >
> > Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >
> > > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]