| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ti4tccx5dnupiv5ppglvcg7xmn2twvdjv5fktupgnqp56we4u5@jsd7mdrbhbwj>
Date: Thu, 15 Jan 2026 08:57:24 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Rob Clark <robin.clark@....qualcomm.com>,
Dmitry Baryshkov <lumag@...nel.org>, Abhinav Kumar <abhinav.kumar@...ux.dev>,
Jessica Zhang <jesszhan0024@...il.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Kuogee Hsieh <quic_khsieh@...cinc.com>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, Jessica Zhang <jessica.zhang@....qualcomm.com>
Subject: Re: [PATCH v3 3/8] drm/msm/dp: Read DPCD and sink count in bridge
detect()
On Thu, Jan 15, 2026 at 09:29:08AM +0200, Dmitry Baryshkov wrote:
> From: Jessica Zhang <jessica.zhang@....qualcomm.com>
>
> Instead of relying on the link_ready flag to specify if DP is connected,
> read the DPCD bits and get the sink count to accurately detect if DP is
> connected.
This makes it sounds like the two options are equal, but they most
definitely aren't.
I think this commit message should capture the fact that "link_ready"
not only says that the cable is connected, but that we've managed to
bring up the main link - which is a source of race conditions in the hot
plug detection logic, as well as making it impossible to move link
management to the enable/disable calls.
>
> Signed-off-by: Jessica Zhang <jessica.zhang@....qualcomm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 60 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_drm.c | 20 -------------
> drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
> 3 files changed, 62 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5997cd28ba11..a05144de3b93 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1151,6 +1151,66 @@ static int msm_dp_hpd_event_thread_start(struct msm_dp_display_private *msm_dp_p
> return 0;
> }
>
> +/**
> + * msm_dp_bridge_detect - callback to determine if connector is connected
> + * @bridge: Pointer to drm bridge structure
> + * @connector: Pointer to drm connector structure
> + * Returns: Bridge's 'is connected' status
Could you please rewrite the return definition, to capture what the
value really refers to.
> + */
> +enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(bridge);
> + struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
> + struct msm_dp_display_private *priv;
> + int ret = 0;
First usage is an assignment, so no need for the zero-initialization.
> + int status = connector_status_disconnected;
> + u8 dpcd[DP_RECEIVER_CAP_SIZE];
> + struct drm_dp_desc desc;
> +
> + dp = to_dp_bridge(bridge)->msm_dp_display;
> +
> + priv = container_of(dp, struct msm_dp_display_private, msm_dp_display);
> +
> + if (!dp->link_ready)
> + return status;
So despite the commit message, we're still relying on the link_ready
flag? (With the improvement that even if the code thinks we've trained
the link, we can still determine that we should report it as
disconnected)
Perhaps I'm missing something here? Did we change the meaning of
"link_ready"?
Other than this part, this looks quite familiar to my experiments. Very
happy to see you continue this work!!!
Regards,
Bjorn
> +
> + msm_dp_aux_enable_xfers(priv->aux, true);
> +
> + ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> + if (ret) {
> + DRM_ERROR("failed to pm_runtime_resume\n");
> + msm_dp_aux_enable_xfers(priv->aux, false);
> + return status;
> + }
> +
> + ret = msm_dp_aux_is_link_connected(priv->aux);
> + if (dp->internal_hpd && !ret)
> + goto end;
> +
> + ret = drm_dp_read_dpcd_caps(priv->aux, dpcd);
> + if (ret)
> + goto end;
> +
> + ret = drm_dp_read_desc(priv->aux, &desc, drm_dp_is_branch(dpcd));
> + if (ret)
> + goto end;
> +
> + status = connector_status_connected;
> + if (drm_dp_read_sink_count_cap(connector, dpcd, &desc)) {
> + int sink_count = drm_dp_read_sink_count(priv->aux);
> +
> + drm_dbg_dp(dp->drm_dev, "sink_count = %d\n", sink_count);
> +
> + if (sink_count <= 0)
> + status = connector_status_disconnected;
> + }
> +
> +end:
> + pm_runtime_put_sync(&dp->pdev->dev);
> + return status;
> +}
> +
> static irqreturn_t msm_dp_display_irq_handler(int irq, void *dev_id)
> {
> struct msm_dp_display_private *dp = dev_id;
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index fd6443d2b6ce..e4622c85fb66 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -15,26 +15,6 @@
> #include "dp_audio.h"
> #include "dp_drm.h"
>
> -/**
> - * msm_dp_bridge_detect - callback to determine if connector is connected
> - * @bridge: Pointer to drm bridge structure
> - * @connector: Pointer to drm connector structure
> - * Returns: Bridge's 'is connected' status
> - */
> -static enum drm_connector_status
> -msm_dp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
> -{
> - struct msm_dp *dp;
> -
> - dp = to_dp_bridge(bridge)->msm_dp_display;
> -
> - drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
> - str_true_false(dp->link_ready));
> -
> - return (dp->link_ready) ? connector_status_connected :
> - connector_status_disconnected;
> -}
> -
> static int msm_dp_bridge_atomic_check(struct drm_bridge *bridge,
> struct drm_bridge_state *bridge_state,
> struct drm_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
> index 9eb3431dd93a..6c0426803d78 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.h
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.h
> @@ -25,6 +25,8 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
> struct drm_encoder *encoder,
> bool yuv_supported);
>
> +enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
> + struct drm_connector *connector);
> void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> struct drm_atomic_state *state);
> void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
>
> --
> 2.47.3
>
>
Powered by blists - more mailing lists