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

[Javier Martinez Canillas <javierm@redhat.com>]

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