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

[Maxime Ripard <mripard@kernel.org>]

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