Maxime Ripard <[email protected]> writes:
> 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.
>
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
