[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

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

[Maxime Ripard]

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

Hi,

On Sun, May 10, 2026 at 09:14:49PM +0200, Javier Martinez Canillas wrote:
> 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;

It's really not clear to me why you need to have that in the bridge
state. What's wrong with using drm_display_info.is_hdmi directly?

If you really want to rework the driver though, switch to HDMI state
helpers would fix all of it :)

Maxime

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

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

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime!

> Hi,
>
> On Sun, May 10, 2026 at 09:14:49PM +0200, Javier Martinez Canillas wrote:
>> 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;
>
> It's really not clear to me why you need to have that in the bridge
> state. What's wrong with using drm_display_info.is_hdmi directly?
>

I wrongly thought that couldn't get that info from it66121_bridge_mode_set()
so I thought about making it a property of the bridge atomic state.

But I see now that the connector is already stored in struct it66121_ctx *ctx
so the fix indeed becomes even more trivial.

> If you really want to rework the driver though, switch to HDMI state
> helpers would fix all of it :)
>

Sure, I will do. But I think that is still worth to land a minimal fix and
then do the switch to the HDMI state helpers as a follow up. So what do you
think of the following patch?

(Note that I dropped the Assisted-by tag this time since v2 has been manually
written by me, with no assistance from an LLM).

From 02d4e9d306c22375445f062cb4476395aa68df5f Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Mon, 11 May 2026 14:43:17 +0200
Subject: [PATCH v2] drm/bridge: it66121: Select HDMI or DVI mode based on sink
 type

Currently, the driver assumes that the connector sink type is always HDMI,
so it configures the bridge in this mode and enables the transmission of
AVI infoframe packets.

But this can cause problems on DVI monitors that can fail to interpret the
video signal and lead to not having display output.

Check the connector display information sink type to decide whether DVI or
HDMI mode should be set and if the AVI infoframes packets should be sent.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Don't store the sink type in a per-commit bridge state (Maxime).

 drivers/gpu/drm/bridge/ite-it66121.c | 67 +++++++++++++++++-----------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 19a027d75b61..a9c5bab665af 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -182,6 +182,7 @@
 
 #define IT66121_HDMI_MODE_REG			0xC0
 #define IT66121_HDMI_MODE_HDMI			BIT(0)
+#define IT66121_HDMI_MODE_DVI			0
 
 #define IT66121_SYS_STATUS_REG			0x0E
 #define IT66121_SYS_STATUS_ACTIVE_IRQ		BIT(7)
@@ -766,41 +767,55 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 {
 	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct drm_connector *connector = ctx->connector;
 	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 (!connector || connector->display_info.is_hdmi) {
+		ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
+							       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;
 
-	/* Enable AVI infoframe */
-	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
-			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
-		goto unlock;
+		/* Enable AVI infoframe */
+		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
+				 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
+			goto unlock;
 
-	/* Set TX mode to HDMI */
-	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
-		goto unlock;
+		/* Set TX mode to HDMI */
+		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
+				 IT66121_HDMI_MODE_HDMI))
+			goto unlock;
+	} else {
+		/* Disable AVI infoframe */
+		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0))
+			goto unlock;
+
+		/* Set TX mode to DVI */
+		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
+				 IT66121_HDMI_MODE_DVI))
+			goto unlock;
+	}
 
 	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
 	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
-- 
2.54.0

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Maxime Ripard]

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

Hi,

On Mon, May 11, 2026 at 02:48:24PM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <mripard@kernel.org> writes:
> > On Sun, May 10, 2026 at 09:14:49PM +0200, Javier Martinez Canillas wrote:
> >> 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;
> >
> > It's really not clear to me why you need to have that in the bridge
> > state. What's wrong with using drm_display_info.is_hdmi directly?
> >
> 
> I wrongly thought that couldn't get that info from it66121_bridge_mode_set()
> so I thought about making it a property of the bridge atomic state.
> 
> But I see now that the connector is already stored in struct it66121_ctx *ctx
> so the fix indeed becomes even more trivial.

Actually, there's something super fishy there.

ctx->connector is stored only at atomic_enable time, but you'd be using
ctx->it at modeset time which is kind of decorelated to atomic_enable,
ctx->can be called multiple times, etc.

It seems to be already used, so it probably works, but I don't think the
framework provides that guarantee.

> > If you really want to rework the driver though, switch to HDMI state
> > helpers would fix all of it :)
> >
> 
> Sure, I will do. But I think that is still worth to land a minimal fix and
> then do the switch to the HDMI state helpers as a follow up. So what do you
> think of the following patch?
> 
> (Note that I dropped the Assisted-by tag this time since v2 has been manually
> written by me, with no assistance from an LLM).
> 
> From 02d4e9d306c22375445f062cb4476395aa68df5f Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Mon, 11 May 2026 14:43:17 +0200
> Subject: [PATCH v2] drm/bridge: it66121: Select HDMI or DVI mode based on sink
>  type
> 
> Currently, the driver assumes that the connector sink type is always HDMI,
> so it configures the bridge in this mode and enables the transmission of
> AVI infoframe packets.
> 
> But this can cause problems on DVI monitors that can fail to interpret the
> video signal and lead to not having display output.
> 
> Check the connector display information sink type to decide whether DVI or
> HDMI mode should be set and if the AVI infoframes packets should be sent.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v2:
> - Don't store the sink type in a per-commit bridge state (Maxime).
> 
>  drivers/gpu/drm/bridge/ite-it66121.c | 67 +++++++++++++++++-----------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 19a027d75b61..a9c5bab665af 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -182,6 +182,7 @@
>  
>  #define IT66121_HDMI_MODE_REG			0xC0
>  #define IT66121_HDMI_MODE_HDMI			BIT(0)
> +#define IT66121_HDMI_MODE_DVI			0
>  
>  #define IT66121_SYS_STATUS_REG			0x0E
>  #define IT66121_SYS_STATUS_ACTIVE_IRQ		BIT(7)
> @@ -766,41 +767,55 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>  {
>  	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +	struct drm_connector *connector = ctx->connector;
>  	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 (!connector || connector->display_info.is_hdmi) {
> +		ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
> +							       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;
>  
> -	/* Enable AVI infoframe */
> -	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> -			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
> -		goto unlock;
> +		/* Enable AVI infoframe */
> +		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> +				 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
> +			goto unlock;
>  
> -	/* Set TX mode to HDMI */
> -	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
> -		goto unlock;
> +		/* Set TX mode to HDMI */
> +		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
> +				 IT66121_HDMI_MODE_HDMI))
> +			goto unlock;
> +	} else {
> +		/* Disable AVI infoframe */
> +		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0))
> +			goto unlock;
> +
> +		/* Set TX mode to DVI */
> +		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
> +				 IT66121_HDMI_MODE_DVI))
> +			goto unlock;
> +	}

It looks like an early return if !is_hdmi would be more readable?

Maxime

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

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

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Hi,
>
> On Mon, May 11, 2026 at 02:48:24PM +0200, Javier Martinez Canillas wrote:
>> Maxime Ripard <mripard@kernel.org> writes:
>> > On Sun, May 10, 2026 at 09:14:49PM +0200, Javier Martinez Canillas wrote:
>> >> 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;
>> >
>> > It's really not clear to me why you need to have that in the bridge
>> > state. What's wrong with using drm_display_info.is_hdmi directly?
>> >
>> 
>> I wrongly thought that couldn't get that info from it66121_bridge_mode_set()
>> so I thought about making it a property of the bridge atomic state.
>> 
>> But I see now that the connector is already stored in struct it66121_ctx *ctx
>> so the fix indeed becomes even more trivial.
>
> Actually, there's something super fishy there.
>
> ctx->connector is stored only at atomic_enable time, but you'd be using
> ctx->it at modeset time which is kind of decorelated to atomic_enable,
> ctx->can be called multiple times, etc.
>
> It seems to be already used, so it probably works, but I don't think the
> framework provides that guarantee.
>

Yes, I found it strange too but as you said it was already used and I'm
just making the same assumptions that the driver is currently doing. But
that's the reason why I had the condition

if (!connector || connector->display_info.is_hdmi) {

just in case it66121_bridge_mode_set() was called before atomic_enable and
ctx->connector was NULL. Then the driver will continue behaving as before.

[...]

>> @@ -766,41 +767,55 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>>  {
>>  	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
>>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
>> +	struct drm_connector *connector = ctx->connector;
>>  	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 (!connector || connector->display_info.is_hdmi) {
>> +		ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
>> +							       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;
>>  
>> -	/* Enable AVI infoframe */
>> -	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
>> -			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
>> -		goto unlock;
>> +		/* Enable AVI infoframe */
>> +		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
>> +				 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
>> +			goto unlock;
>>  
>> -	/* Set TX mode to HDMI */
>> -	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
>> -		goto unlock;
>> +		/* Set TX mode to HDMI */
>> +		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
>> +				 IT66121_HDMI_MODE_HDMI))
>> +			goto unlock;
>> +	} else {
>> +		/* Disable AVI infoframe */
>> +		if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0))
>> +			goto unlock;
>> +
>> +		/* Set TX mode to DVI */
>> +		if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
>> +				 IT66121_HDMI_MODE_DVI))
>> +			goto unlock;
>> +	}
>
> It looks like an early return if !is_hdmi would be more readable?
>

I'm not sure to follow this comment. The driver needs to take some actions
regardless of the TX mode used, so it66121_bridge_mode_set() can't really
return early for the !is_hdmi case.

> Maxime

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

> Hi,
>
> On Mon, May 11, 2026 at 02:48:24PM +0200, Javier Martinez Canillas wrote:

[...]

>> 
>> I wrongly thought that couldn't get that info from it66121_bridge_mode_set()
>> so I thought about making it a property of the bridge atomic state.
>> 
>> But I see now that the connector is already stored in struct it66121_ctx *ctx
>> so the fix indeed becomes even more trivial.
>
> Actually, there's something super fishy there.
>
> ctx->connector is stored only at atomic_enable time, but you'd be using
> ctx->it at modeset time which is kind of decorelated to atomic_enable,
> ctx->can be called multiple times, etc.
>

Your intuition was correct! The driver has a bug and ctx->connector seems
to always be NULL because .mode_set is called before .atomic_enable, which
is where the ctx->connector is being set.

> It seems to be already used, so it probably works, but I don't think the
> framework provides that guarantee.
>

It seems to work just because drm_hdmi_avi_infoframe_from_display_mode()
allows the connector argument to be NULL, is just that the VIC data is not
sent in the AVI infoframes if I understood correctly.

I'll work on a v3 that moves the TX mode and AVI infoframes enable/disable
to the .atomic_enable callback which I see is what other bridge drivers do
(e.g,: sii902x).

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat