Hi, On Mon, May 11, 2026 at 02:48:24PM +0200, Javier Martinez Canillas wrote: > Maxime Ripard <[email protected]> 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 <[email protected]> > >> --- > >> > >> 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 <[email protected]> > 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 <[email protected]> > --- > > 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
signature.asc
Description: PGP signature
