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:

> 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