[PATCH 0/3] drm/bridge: it66121: Fix display output on DVI monitors

[PATCH 0/3] drm/bridge: it66121: Fix display output on DVI monitors

[Javier Martinez Canillas]

I wasn't getting any output on a monitor connected to an AM625 BeaglePlay
board, and I tasked a Cursor agent based on Claude Opus 4.6 to conduct a
root cause analysis.

It correctly figured out that it was due to the IT66121 bridge assuming that
the sink type was HDMI and sending AVI infoframes, but this can confuse
DVI monitors.

This patch series fixes the issue, the changes are fairly trivial and I
take full responsibility of the contribution. An Assisted-by tag is added
to the patches in conformance with the AI Coding Assistants [0] policy.

Patch #1 is just a cleanup to avoid doing direct calls to container_of()
and use a helper function which is the convention in most drivers. Patch
by storing the connector sink type in the bridge state, to determine
whether the mode should be HDMI or DVI.

The patches were tested both using a DVI monitor and a HDMI-to-DVI adapter
and a HDMI monitor, to ensure that there are no regressions for HDMI mode.

[0]: https://docs.kernel.org/process/coding-assistants.html


Javier Martinez Canillas (3):
  drm/bridge: it66121: Add bridge_to_it66121_ctx() helper
  drm/bridge: it66121: Add bridge-private atomic state
  drm/bridge: it66121: Select HDMI or DVI mode based on sink type

 drivers/gpu/drm/bridge/ite-it66121.c | 137 +++++++++++++++++++--------
 1 file changed, 100 insertions(+), 37 deletions(-)

-- 
2.54.0

base-commit: 19d584a634fe999786acfb0ac5289710cc84a5f6
branch: drm-misc-next

[PATCH 1/3] drm/bridge: it66121: Add bridge_to_it66121_ctx() helper

[Javier Martinez Canillas]

The driver just calls container_of() to obtain the it66121_ctx from a
drm_bridge, but follow the convention of other bridge drivers and add
a helper function to wrap this duplicated logic.

No functional change.

Assisted-by: Cursor:claude-4.6-opus
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/bridge/ite-it66121.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 19a027d75b61..038a67a32e39 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -315,6 +315,11 @@ struct it66121_ctx {
 	enum chip_id id;
 };
 
+static inline struct it66121_ctx *bridge_to_it66121_ctx(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct it66121_ctx, bridge);
+}
+
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
 	{
 		.name = "it66121",
@@ -589,7 +594,7 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 				 struct drm_encoder *encoder,
 				 enum drm_bridge_attach_flags flags)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	int ret;
 
 	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
@@ -700,7 +705,7 @@ static u32 *it66121_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 						     u32 output_fmt,
 						     unsigned int *num_input_fmts)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	u32 *input_fmts;
 
 	*num_input_fmts = 0;
@@ -724,7 +729,7 @@ static u32 *it66121_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 static void it66121_bridge_enable(struct drm_bridge *bridge,
 				  struct drm_atomic_commit *state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 
 	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 
@@ -734,7 +739,7 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 static void it66121_bridge_disable(struct drm_bridge *bridge,
 				   struct drm_atomic_commit *state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 
 	it66121_set_mute(ctx, true);
 
@@ -746,7 +751,7 @@ static int it66121_bridge_check(struct drm_bridge *bridge,
 				struct drm_crtc_state *crtc_state,
 				struct drm_connector_state *conn_state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 
 	if (ctx->id == ID_IT6610) {
 		/* The IT6610 only supports these settings */
@@ -765,7 +770,7 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 			     const struct drm_display_mode *adjusted_mode)
 {
 	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	int ret;
 
 	mutex_lock(&ctx->lock);
@@ -829,7 +834,7 @@ static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
 						      const struct drm_display_info *info,
 						      const struct drm_display_mode *mode)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	unsigned long max_clock;
 
 	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
@@ -846,7 +851,7 @@ static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
 static enum drm_connector_status
 it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 
 	return it66121_is_hpd_detect(ctx) ? connector_status_connected
 					  : connector_status_disconnected;
@@ -854,7 +859,7 @@ it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector
 
 static void it66121_bridge_hpd_enable(struct drm_bridge *bridge)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	int ret;
 
 	ret = regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG, IT66121_INT_MASK1_HPD, 0);
@@ -864,7 +869,7 @@ static void it66121_bridge_hpd_enable(struct drm_bridge *bridge)
 
 static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	int ret;
 
 	ret = regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
@@ -876,7 +881,7 @@ static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
 static const struct drm_edid *it66121_bridge_edid_read(struct drm_bridge *bridge,
 						       struct drm_connector *connector)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
 	const struct drm_edid *drm_edid;
 	int ret;
 
-- 
2.54.0

[PATCH 2/3] drm/bridge: it66121: Add bridge-private atomic state

[Javier Martinez Canillas]

In preparation to support per-state configuration during atomic commits,
add a private bridge state data structure and access helpers. This will
be needed to store the display information sink mode, to determine if a
connector is HDMI or DVI.

No functional change.

Assisted-by: Cursor:claude-4.6-opus
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/bridge/ite-it66121.c | 42 ++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 038a67a32e39..a203c94a27e5 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -320,6 +320,44 @@ static inline struct it66121_ctx *bridge_to_it66121_ctx(struct drm_bridge *bridg
 	return container_of(bridge, struct it66121_ctx, bridge);
 }
 
+struct it66121_bridge_state {
+	struct drm_bridge_state base;
+};
+
+static inline struct it66121_bridge_state *
+to_it66121_bridge_state(struct drm_bridge_state *state)
+{
+	return container_of(state, struct it66121_bridge_state, base);
+}
+
+static struct drm_bridge_state *
+it66121_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
+{
+	struct it66121_bridge_state *state;
+
+	state = kzalloc_obj(*state);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_bridge_duplicate_state(bridge, &state->base);
+
+	return &state->base;
+}
+
+static struct drm_bridge_state *
+it66121_bridge_atomic_reset(struct drm_bridge *bridge)
+{
+	struct it66121_bridge_state *state;
+
+	state = kzalloc_obj(*state);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_bridge_reset(bridge, &state->base);
+
+	return &state->base;
+}
+
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
 	{
 		.name = "it66121",
@@ -908,9 +946,9 @@ static const struct drm_edid *it66121_bridge_edid_read(struct drm_bridge *bridge
 }
 
 static const struct drm_bridge_funcs it66121_bridge_funcs = {
-	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_duplicate_state = it66121_bridge_atomic_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
-	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_reset = it66121_bridge_atomic_reset,
 	.attach = it66121_bridge_attach,
 	.atomic_get_output_bus_fmts = it66121_bridge_atomic_get_output_bus_fmts,
 	.atomic_get_input_bus_fmts = it66121_bridge_atomic_get_input_bus_fmts,
-- 
2.54.0

[PATCH 3/3] drm/bridge: it66121: Select HDMI or DVI mode based on sink type

[Javier Martinez Canillas]

Currently, the driver assumes that the connector sink type is always HDMI
and configures the IT66121 bridge appropriately. But configuring in this
mode and enabling the transmission of AVI infoframe packets can cause DVI
monitors to fail parsing the video signal.

To prevent this, store the connector display information sink type in the
bridge atomic state and use it to decide whether the bridge should be set
in HDMI or DVI mode, and if the AVI infoframes packets should be sent.

Assisted-by: Cursor:claude-4.6-opus
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/bridge/ite-it66121.c | 68 ++++++++++++++++++----------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index a203c94a27e5..99088277d170 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -322,6 +322,7 @@ static inline struct it66121_ctx *bridge_to_it66121_ctx(struct drm_bridge *bridg
 
 struct it66121_bridge_state {
 	struct drm_bridge_state base;
+	bool sink_is_hdmi;
 };
 
 static inline struct it66121_bridge_state *
@@ -790,6 +791,14 @@ static int it66121_bridge_check(struct drm_bridge *bridge,
 				struct drm_connector_state *conn_state)
 {
 	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
+	struct it66121_bridge_state *state =
+		to_it66121_bridge_state(bridge_state);
+
+	/* Default to HDMI to preserve legacy behavior. */
+	state->sink_is_hdmi = true;
+
+	if (conn_state && conn_state->connector)
+		state->sink_is_hdmi = conn_state->connector->display_info.is_hdmi;
 
 	if (ctx->id == ID_IT6610) {
 		/* The IT6610 only supports these settings */
@@ -809,40 +818,51 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 {
 	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
 	struct it66121_ctx *ctx = bridge_to_it66121_ctx(bridge);
+	struct drm_bridge_state *bridge_state =
+		drm_priv_to_bridge_state(bridge->base.state);
+	struct it66121_bridge_state *state =
+		to_it66121_bridge_state(bridge_state);
+	unsigned int hdmi_mode = state->sink_is_hdmi ?
+		IT66121_HDMI_MODE_HDMI : 0;
+	unsigned int avi_pkt = 0;
 	int ret;
 
 	mutex_lock(&ctx->lock);
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe, ctx->connector,
-						       adjusted_mode);
-	if (ret) {
-		DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
-		goto unlock;
-	}
+	if (hdmi_mode) {
+		ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
+							       ctx->connector,
+							       adjusted_mode);
+		if (ret) {
+			DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
+			goto unlock;
+		}
 
-	ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf, sizeof(buf));
-	if (ret < 0) {
-		DRM_ERROR("Failed to pack infoframe: %d\n", ret);
-		goto unlock;
-	}
+		ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf, sizeof(buf));
+		if (ret < 0) {
+			DRM_ERROR("Failed to pack infoframe: %d\n", ret);
+			goto unlock;
+		}
 
-	/* Write new AVI infoframe packet */
-	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
-				&buf[HDMI_INFOFRAME_HEADER_SIZE],
-				HDMI_AVI_INFOFRAME_SIZE);
-	if (ret)
-		goto unlock;
+		/* Write new AVI infoframe packet */
+		ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
+					&buf[HDMI_INFOFRAME_HEADER_SIZE],
+					HDMI_AVI_INFOFRAME_SIZE);
+		if (ret)
+			goto unlock;
 
-	if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
-		goto unlock;
+		if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
+			goto unlock;
+
+		avi_pkt = IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT;
+	}
 
-	/* Enable AVI infoframe */
-	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
-			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
+	/* Enable or disable AVI infoframe */
+	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, avi_pkt))
 		goto unlock;
 
-	/* Set TX mode to HDMI */
-	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
+	/* Set TX mode to HDMI or DVI */
+	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, hdmi_mode))
 		goto unlock;
 
 	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
-- 
2.54.0