From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 310303F0A8C for ; Mon, 11 May 2026 13:44:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778507092; cv=none; b=apfXBwfNS8LMdwQY0MR0zfQiiJrRm9c/AvukPJGT1tagSnFA6uwcyBZFpPGCvIHoEIiJDBFc3BmMspZWPtJ9ndKkevsBjetU5lvtDlvzvCzwqA/Tn92WjskuhSSEnO4gVQ1JhnMTficc/YKOWEBC0knox4JnEsl9n9lqHfDD9cA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778507092; c=relaxed/simple; bh=DEAs+ltUIKbfanscqB8guftdCMLA/NmLS3C0Y92xCqc=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=R5D6PLfiH7sBZ17Ttz+d6OjyNNkMAnj16eWj3Xc0GZMV5YRsUpCqj4mdpLJOCSiVGNGrYhIBgahQAfHFbqSHaks6qUzzMB+LLIDakfpyoHzux06RuwvH1Q2UbTuhTRYzxzeQ4bHpWmXZSnnq5OHJMOVuMPGQoS2HwSdpOhJhcJo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=agJmQ3hg; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=kqaoj3XJ; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="agJmQ3hg"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="kqaoj3XJ" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778507090; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5HIxXZWET6ZUvg/xyF+wJRy+Khfw5bTC/UU2AdmG+xM=; b=agJmQ3hg5S3OM8D9Jh1vWyk3iDqN7edkqlsDMfQrSzAISNyM+X44e/nUlKaBJxp4Q2cgGs gyg62SUo3zNLbUGN0ZcFNEQOm/VYsRQW+cYlx4+5JB3tsfDEUydn7tmlJtitRfO6J10mtd v9kmwTMP4kWQ+7Kw1ibLBW58q0gMUu8= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-401-9-8ySlemPhC6-5EdfKecRQ-1; Mon, 11 May 2026 09:44:48 -0400 X-MC-Unique: 9-8ySlemPhC6-5EdfKecRQ-1 X-Mimecast-MFC-AGG-ID: 9-8ySlemPhC6-5EdfKecRQ_1778507087 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-48a55de6fb0so29989755e9.3 for ; Mon, 11 May 2026 06:44:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1778507087; x=1779111887; darn=vger.kernel.org; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=5HIxXZWET6ZUvg/xyF+wJRy+Khfw5bTC/UU2AdmG+xM=; b=kqaoj3XJchxuc9cLKcpux/R/LTkN85W3qoA9bLaLTY7TEaoZ+nIcpboZRLIGeSRVmM ciLuG/UnPXs8MlAaaevceFS9Hxsf8zftSmdxU1vzC9CvNZiaHJ3rh0iPLPuL5nAAkK4X qx0Ma4QOy9A/rnNFeMSpt5jPfaIm/sxiI6sGGiVkf/cYgMRUhK3D3Xw0HEta/5KflRoC wueTizNUL8D8V9kuy8SxBueD8q8pqq1urGyiCncTKsRCbG0TdGEuJ72HpfrMCkqfQlFD CWd2cr5CMG2oIIMljB30nTmduSn/KINH+EDcJePnHjMOHqcNv7xC9N+FyZaC7eVHFSxp NnwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778507087; x=1779111887; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5HIxXZWET6ZUvg/xyF+wJRy+Khfw5bTC/UU2AdmG+xM=; b=Ay6l2MGc+SZcVB0QDk3HneJilFICF4zKt04Od4ftdZR2yIy6Pci+bh79UVdfAE5oW/ sPSZ1zqvpg2NmpQ4NdK/LieIGFedwreyHDw7TsaK03bPC8QvU9OjSSsG55BRKQsLFwwW 5nFAUNlyjiwXr22zPX1ed2pB8CMckMQNW37p/jmzkB/Y0b2x5zQ7AebdUvsP7p8EdBON 9yJzMShcPKBRPOU0PyMYFRSbbN0WOOpRy/kUtc/GCMTtNWmOT8GHBKQf7DIH1zd4+M4T aNfxmSdh27jUYvz44n59evSnK4yjVCzId8fluAU+/7iz73Js3thxpLMjUAzs1hTcMvdp h/Cw== X-Gm-Message-State: AOJu0YztrnspAbfD7Di3d8HhmkTUS2Xbj4j+sg8AEo9u9VaW9lemhi7O nOhJkKp251Lh6hJRier0xQI15dAZdr+sxx+q+BKtccp4DrYGfu98NsmcIzbYeLC+P9VjEvXFehH xKPPznEl1lOfiU85Sv8H1uzJvp5FFUhryKy75rSp0r7wGWS6pNmCNfRWhfyVSthNXLQ== X-Gm-Gg: Acq92OH34gUqWsZ0mKUx6b5wy9/NeZeTHPYZ2KE7tdaG4Pxjy+T/aZSIaOixkvzlZA/ CRpoTcgy0JsJvgk5zxtzqQPHr95k2seH+NdU1glPay8qfo8b5McVKvbp7L3CXQ15C1p4pJDQf6j Yc/DO8Rykl5b89q2S8oU+avO4GjFYfeciqXhVwaYPw/QcDbcsgAWdNHfv18jU7pt6chBL3S7Yhp 2ZzZp0h6lpBwdEwcV9Ee4fCHdzG96Kw9HHLPdFcCO2M1aVD18pdyzIGoLbZuXI+DMy2t47Xhjei F94LuzUbf+A6o0khddo/meZVSzadMMBu1OndDSLJSiuj9e9odzpn6EFCNgXMpPOMUIri/2NKPLF ft9NPOShhELaHxz0hEsQ0gfZhPMfNGpHJhpvr+h7ZA4wizx/uoLY6J5hvnr3rahg= X-Received: by 2002:a05:600c:8b65:b0:48a:53ea:140b with SMTP id 5b1f17b1804b1-48e51f4e5fdmr434911215e9.28.1778507087160; Mon, 11 May 2026 06:44:47 -0700 (PDT) X-Received: by 2002:a05:600c:8b65:b0:48a:53ea:140b with SMTP id 5b1f17b1804b1-48e51f4e5fdmr434910375e9.28.1778507086673; Mon, 11 May 2026 06:44:46 -0700 (PDT) Received: from localhost ([195.166.127.210]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48e702e6d41sm238701695e9.7.2026.05.11.06.44.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 May 2026 06:44:45 -0700 (PDT) From: Javier Martinez Canillas To: Maxime Ripard Cc: linux-kernel@vger.kernel.org, Andrzej Hajda , David Airlie , Jernej Skrabec , Jonas Karlman , Laurent Pinchart , Luca Ceresoli , Maarten Lankhorst , Neil Armstrong , Phong LE , Robert Foss , Simona Vetter , Thomas Zimmermann , dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/3] drm/bridge: it66121: Select HDMI or DVI mode based on sink type In-Reply-To: <20260511-gentle-grouse-of-growth-3ac263@houat> References: <20260510191459.90769-1-javierm@redhat.com> <20260510191459.90769-4-javierm@redhat.com> <20260511-spiritual-unique-uakari-5ffce7@houat> <877bpayuw7.fsf@ocarina.mail-host-address-is-not-set> <20260511-gentle-grouse-of-growth-3ac263@houat> Date: Mon, 11 May 2026 15:44:43 +0200 Message-ID: <8733zyysac.fsf@ocarina.mail-host-address-is-not-set> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain Maxime Ripard writes: > Hi, > > On Mon, May 11, 2026 at 02:48:24PM +0200, Javier Martinez Canillas wrote: >> Maxime Ripard 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 >> >> --- >> >> >> >> 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