* [PATCH v5 01/10] drm/msm/dp: fix HPD state status bit shift value
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 02/10] drm/msm/dp: Fix the ISR_* enum values Dmitry Baryshkov
` (10 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
The HPD state status is the 3 most significant bits, not 4 bits of the
HPD_INT_STATUS register.
Fix the bit shift macro so that the correct bits are returned in
msm_dp_aux_is_link_connected().
Fixes: 19e52bcb27c2 ("drm/msm/dp: return correct connection status after suspend")
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_reg.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
index 7c44d4e2cf13..3689642b7fc0 100644
--- a/drivers/gpu/drm/msm/dp/dp_reg.h
+++ b/drivers/gpu/drm/msm/dp/dp_reg.h
@@ -68,8 +68,8 @@
#define DP_DP_IRQ_HPD_INT_ACK (0x00000002)
#define DP_DP_HPD_REPLUG_INT_ACK (0x00000004)
#define DP_DP_HPD_UNPLUG_INT_ACK (0x00000008)
-#define DP_DP_HPD_STATE_STATUS_BITS_MASK (0x0000000F)
-#define DP_DP_HPD_STATE_STATUS_BITS_SHIFT (0x1C)
+#define DP_DP_HPD_STATE_STATUS_BITS_MASK (0x00000007)
+#define DP_DP_HPD_STATE_STATUS_BITS_SHIFT (0x1D)
#define REG_DP_DP_HPD_INT_MASK (0x0000000C)
#define DP_DP_HPD_PLUG_INT_MASK (0x00000001)
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 02/10] drm/msm/dp: Fix the ISR_* enum values
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
2026-03-14 1:09 ` [PATCH v5 01/10] drm/msm/dp: fix HPD state status bit shift value Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect() Dmitry Baryshkov
` (9 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
The ISR_HPD_* enum should represent values that can be read from the
REG_DP_DP_HPD_INT_STATUS register. Swap ISR_HPD_IO_GLITCH_COUNT and
ISR_HPD_REPLUG_COUNT to map them correctly to register values.
While we are at it, correct the spelling for ISR_HPD_REPLUG_COUNT.
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 476848bf8cd1..5997cd28ba11 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -38,9 +38,9 @@ enum {
ISR_DISCONNECTED,
ISR_CONNECT_PENDING,
ISR_CONNECTED,
- ISR_HPD_REPLUG_COUNT,
+ ISR_HPD_IO_GLITCH_COUNT,
ISR_IRQ_HPD_PULSE_COUNT,
- ISR_HPD_LO_GLITH_COUNT,
+ ISR_HPD_REPLUG_COUNT,
};
/* event thread connection state */
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect()
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
2026-03-14 1:09 ` [PATCH v5 01/10] drm/msm/dp: fix HPD state status bit shift value Dmitry Baryshkov
2026-03-14 1:09 ` [PATCH v5 02/10] drm/msm/dp: Fix the ISR_* enum values Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable() Dmitry Baryshkov
` (8 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.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.
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.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
+ */
+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;
+ 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;
+
+ 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: Read DPCD and sink count in bridge detect()
2026-03-14 1:09 ` [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect() Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Moves `msm_dp_bridge_detect()` from `dp_drm.c` to `dp_display.c` and makes it actually read DPCD and sink count over AUX rather than just checking `link_ready`. The function is made non-static and declared in `dp_drm.h`.
**Issues:**
1. There is a redundant assignment of `dp`:
```c
+ struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(bridge);
+ struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
...
+ dp = to_dp_bridge(bridge)->msm_dp_display;
```
The second assignment on line 862 duplicates the initialization. Minor, but should be cleaned up.
2. After the `msm_dp_aux_enable_xfers(priv->aux, true)` call, there is no corresponding `msm_dp_aux_enable_xfers(priv->aux, false)` at the `end:` label — AUX transfers are left enabled after detect returns. This is later addressed in patch 7/9, but it means this patch creates a transient bug if cherry-picked alone.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable()
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (2 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect() Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 05/10] drm/msm/dp: Drop EV_USER_NOTIFICATION Dmitry Baryshkov
` (7 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
Currently, the DP link training is being done during HPD. Move
link training to atomic_enable() in accordance with the atomic_enable()
documentation.
Link disabling is already done in atomic_post_disable() (as part of the
dp_ctrl_off_link_stream() helper).
Finally, call the plug/unplug handlers directly in hpd_notify() instead
of queueing them in the event thread so that they aren't preempted by
other events.
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index a05144de3b93..e7b1ed5491c4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -436,11 +436,6 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false);
msm_dp_link_reset_phy_params_vx_px(dp->link);
- rc = msm_dp_ctrl_on_link(dp->ctrl);
- if (rc) {
- DRM_ERROR("failed to complete DP link training\n");
- goto end;
- }
msm_dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
@@ -1695,6 +1690,11 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
force_link_train = true;
}
+ rc = msm_dp_ctrl_on_link(msm_dp_display->ctrl);
+ if (rc)
+ DRM_ERROR("Failed link training (rc=%d)\n", rc);
+ // TODO: schedule drm_connector_set_link_status_property()
+
msm_dp_display_enable(msm_dp_display, force_link_train);
rc = msm_dp_display_post_enable(dp);
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: Move link training to atomic_enable()
2026-03-14 1:09 ` [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable() Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Moves `msm_dp_ctrl_on_link()` from `msm_dp_display_process_hpd_high()` to `msm_dp_bridge_atomic_enable()`.
**Issues:**
1. The `// TODO: schedule drm_connector_set_link_status_property()` comment should be addressed or at least converted to a proper C-style comment:
```c
+ // TODO: schedule drm_connector_set_link_status_property()
```
The kernel coding style requires `/* */` comments. Also, if link training fails, the function continues into `msm_dp_display_enable()` anyway, which seems wrong — a failed link train should probably bail out.
2. The commit message says it also changes hpd_notify() behavior ("call the plug/unplug handlers directly in hpd_notify() instead of queueing them"), but there's no change to hpd_notify() in this patch. The description seems to be a leftover from an earlier version.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 05/10] drm/msm/dp: Drop EV_USER_NOTIFICATION
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (3 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable() Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 06/10] drm/msm/dp: drop event data Dmitry Baryshkov
` (6 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
Currently, we queue an event for signalling HPD connect/disconnect. This
can mean a delay in plug/unplug handling and notifying DRM core when a
hotplug happens.
Drop EV_USER_NOTIFICATION and signal the IRQ event as part of hotplug
handling.
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 28 ++++++++--------------------
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 2 ++
3 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e7b1ed5491c4..05c292e27126 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -58,7 +58,6 @@ enum {
EV_HPD_PLUG_INT,
EV_IRQ_HPD_INT,
EV_HPD_UNPLUG_INT,
- EV_USER_NOTIFICATION,
};
#define EVENT_TIMEOUT (HZ/10) /* 100ms */
@@ -343,17 +342,6 @@ static const struct component_ops msm_dp_display_comp_ops = {
.unbind = msm_dp_display_unbind,
};
-static void msm_dp_display_send_hpd_event(struct msm_dp *msm_dp_display)
-{
- struct msm_dp_display_private *dp;
- struct drm_connector *connector;
-
- dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
-
- connector = dp->msm_dp_display.connector;
- drm_helper_hpd_irq_event(connector->dev);
-}
-
static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *dp,
bool hpd)
{
@@ -377,7 +365,11 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d
drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
dp->msm_dp_display.connector_type, hpd);
- msm_dp_display_send_hpd_event(&dp->msm_dp_display);
+
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ hpd ?
+ connector_status_connected :
+ connector_status_disconnected);
return 0;
}
@@ -437,7 +429,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
msm_dp_link_reset_phy_params_vx_px(dp->link);
- msm_dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
+ msm_dp_display_send_hpd_notification(dp, true);
end:
return rc;
@@ -506,7 +498,7 @@ static int msm_dp_display_notify_disconnect(struct device *dev)
{
struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
- msm_dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
+ msm_dp_display_send_hpd_notification(dp, false);
return 0;
}
@@ -527,7 +519,7 @@ static int msm_dp_display_handle_port_status_changed(struct msm_dp_display_priva
drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to do\n");
if (dp->hpd_state != ST_DISCONNECTED) {
dp->hpd_state = ST_DISCONNECT_PENDING;
- msm_dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
+ msm_dp_display_send_hpd_notification(dp, false);
}
} else {
if (dp->hpd_state == ST_DISCONNECTED) {
@@ -1121,10 +1113,6 @@ static int hpd_event_thread(void *data)
case EV_IRQ_HPD_INT:
msm_dp_irq_hpd_handle(msm_dp_priv, todo->data);
break;
- case EV_USER_NOTIFICATION:
- msm_dp_display_send_hpd_notification(msm_dp_priv,
- todo->data);
- break;
default:
break;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index cc6e2cab36e9..60094061c102 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -16,6 +16,7 @@ struct msm_dp {
struct platform_device *pdev;
struct drm_connector *connector;
struct drm_bridge *next_bridge;
+ struct drm_bridge *bridge;
bool link_ready;
bool audio_enabled;
bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index e4622c85fb66..f935093c4df4 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -340,6 +340,8 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
}
}
+ msm_dp_display->bridge = bridge;
+
return 0;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: Drop EV_USER_NOTIFICATION
2026-03-14 1:09 ` [PATCH v5 05/10] drm/msm/dp: Drop EV_USER_NOTIFICATION Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Replaces `drm_helper_hpd_irq_event()` with `drm_bridge_hpd_notify()` and adds `struct drm_bridge *bridge` to `struct msm_dp`. This is a clean transition.
**Minor note:** The `msm_dp_display_send_hpd_notification()` function now calls `drm_bridge_hpd_notify()` directly instead of through an event queue. This changes the context in which the notification happens — it's now called from the same context as the HPD handler rather than from the event thread. This should be fine since `drm_bridge_hpd_notify()` schedules work internally.
No blocking issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 06/10] drm/msm/dp: drop event data
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (4 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 05/10] drm/msm/dp: Drop EV_USER_NOTIFICATION Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 07/10] drm/msm/dp: rework HPD handling Dmitry Baryshkov
` (5 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
With EV_USER_NOTIFICATION gone event's data is no longer useful. Drop
it, removing also the argument from event handlers.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 39 +++++++++++++++++--------------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 05c292e27126..bc4078c229cb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -69,7 +69,6 @@ enum {
struct msm_dp_event {
u32 event_id;
- u32 data;
u32 delay;
};
@@ -218,7 +217,7 @@ static struct msm_dp_display_private *dev_get_dp_display_private(struct device *
}
static int msm_dp_add_event(struct msm_dp_display_private *msm_dp_priv, u32 event,
- u32 data, u32 delay)
+ u32 delay)
{
unsigned long flag;
struct msm_dp_event *todo;
@@ -236,7 +235,6 @@ static int msm_dp_add_event(struct msm_dp_display_private *msm_dp_priv, u32 even
todo = &msm_dp_priv->event_list[msm_dp_priv->event_pndx++];
msm_dp_priv->event_pndx %= DP_EVENT_Q_MAX;
todo->event_id = event;
- todo->data = data;
todo->delay = delay;
wake_up(&msm_dp_priv->event_q);
spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
@@ -576,7 +574,7 @@ static int msm_dp_display_usbpd_attention_cb(struct device *dev)
return rc;
}
-static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp, u32 data)
+static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
{
u32 state;
int ret;
@@ -602,7 +600,7 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp, u32 data)
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
+ msm_dp_add_event(dp, EV_HPD_PLUG_INT, 1);
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -644,7 +642,7 @@ static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
plugged);
}
-static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp, u32 data)
+static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
{
u32 state;
struct platform_device *pdev = dp->msm_dp_display.pdev;
@@ -706,7 +704,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp, u32 data)
return 0;
}
-static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data)
+static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
{
u32 state;
@@ -724,7 +722,7 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data)
if (state == ST_MAINLINK_READY || state == ST_DISCONNECT_PENDING) {
/* wait until ST_CONNECTED */
- msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */
+ msm_dp_add_event(dp, EV_IRQ_HPD_INT, 1);
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -1079,7 +1077,6 @@ static int hpd_event_thread(void *data)
todo_next = &msm_dp_priv->event_list[msm_dp_priv->event_pndx++];
msm_dp_priv->event_pndx %= DP_EVENT_Q_MAX;
todo_next->event_id = todo->event_id;
- todo_next->data = todo->data;
todo_next->delay = todo->delay - 1;
/* clean up older event */
@@ -1105,13 +1102,13 @@ static int hpd_event_thread(void *data)
switch (todo->event_id) {
case EV_HPD_PLUG_INT:
- msm_dp_hpd_plug_handle(msm_dp_priv, todo->data);
+ msm_dp_hpd_plug_handle(msm_dp_priv);
break;
case EV_HPD_UNPLUG_INT:
- msm_dp_hpd_unplug_handle(msm_dp_priv, todo->data);
+ msm_dp_hpd_unplug_handle(msm_dp_priv);
break;
case EV_IRQ_HPD_INT:
- msm_dp_irq_hpd_handle(msm_dp_priv, todo->data);
+ msm_dp_irq_hpd_handle(msm_dp_priv);
break;
default:
break;
@@ -1212,19 +1209,19 @@ static irqreturn_t msm_dp_display_irq_handler(int irq, void *dev_id)
dp->msm_dp_display.connector_type, hpd_isr_status);
/* hpd related interrupts */
if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0);
if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
- msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
+ msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0);
}
if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 3);
+ msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
+ msm_dp_add_event(dp, EV_HPD_PLUG_INT, 3);
}
if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
ret = IRQ_HANDLED;
}
@@ -1649,7 +1646,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
}
if (dp->is_edp)
- msm_dp_hpd_plug_handle(msm_dp_display, 0);
+ msm_dp_hpd_plug_handle(msm_dp_display);
mutex_lock(&msm_dp_display->event_mutex);
if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
@@ -1721,7 +1718,7 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
if (dp->is_edp)
- msm_dp_hpd_unplug_handle(msm_dp_display, 0);
+ msm_dp_hpd_unplug_handle(msm_dp_display);
mutex_lock(&msm_dp_display->event_mutex);
@@ -1843,7 +1840,7 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!msm_dp_display->link_ready && status == connector_status_connected)
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0);
else if (msm_dp_display->link_ready && status == connector_status_disconnected)
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 07/10] drm/msm/dp: rework HPD handling
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (5 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 06/10] drm/msm/dp: drop event data Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 08/10] drm/msm/dp: Add sink_count to debug logs Dmitry Baryshkov
` (4 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
Handling of the HPD events in the MSM DP driver is plagued with lots of
problems. It tries to work aside of the main DRM framework, handling the
HPD signals on its own. There are two separate paths, one for the HPD
signals coming from the DP HPD pin and another path for signals coming
from outside (e.g. from the Type-C AltMode). It lies about the connected
state, returning the link established state instead. It is not easy to
understand or modify it. Having a separate event machine doesn't add
extra clarity.
Drop the whole event machine. When the DP receives a HPD event, send it
to the DRM core. Then handle the events in the hpd_notify callback,
unifying paths for HPD signals.
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
Co-developed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 22 --
drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 -
drivers/gpu/drm/msm/dp/dp_display.c | 601 ++++++++----------------------------
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
4 files changed, 134 insertions(+), 491 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index ef298c7d3e5e..3bb08c9a020e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -2593,28 +2593,6 @@ void msm_dp_ctrl_off_link_stream(struct msm_dp_ctrl *msm_dp_ctrl)
phy, phy->init_count, phy->power_count);
}
-void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl)
-{
- struct msm_dp_ctrl_private *ctrl;
- struct phy *phy;
-
- ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
- phy = ctrl->phy;
-
- msm_dp_ctrl_mainlink_disable(ctrl);
-
- dev_pm_opp_set_rate(ctrl->dev, 0);
- msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
-
- DRM_DEBUG_DP("Before, phy=%p init_count=%d power_on=%d\n",
- phy, phy->init_count, phy->power_count);
-
- phy_power_off(phy);
-
- DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
- phy, phy->init_count, phy->power_count);
-}
-
void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl)
{
struct msm_dp_ctrl_private *ctrl;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 124b9b21bb7f..f68bee62713f 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -19,7 +19,6 @@ struct phy;
int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl);
int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train);
void msm_dp_ctrl_off_link_stream(struct msm_dp_ctrl *msm_dp_ctrl);
-void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl);
void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl);
void msm_dp_ctrl_push_idle(struct msm_dp_ctrl *msm_dp_ctrl);
irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bc4078c229cb..d61a8ddd30a8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -43,35 +43,6 @@ enum {
ISR_HPD_REPLUG_COUNT,
};
-/* event thread connection state */
-enum {
- ST_DISCONNECTED,
- ST_MAINLINK_READY,
- ST_CONNECTED,
- ST_DISCONNECT_PENDING,
- ST_DISPLAY_OFF,
-};
-
-enum {
- EV_NO_EVENT,
- /* hpd events */
- EV_HPD_PLUG_INT,
- EV_IRQ_HPD_INT,
- EV_HPD_UNPLUG_INT,
-};
-
-#define EVENT_TIMEOUT (HZ/10) /* 100ms */
-#define DP_EVENT_Q_MAX 8
-
-#define DP_TIMEOUT_NONE 0
-
-#define WAIT_FOR_RESUME_TIMEOUT_JIFFIES (HZ / 2)
-
-struct msm_dp_event {
- u32 event_id;
- u32 delay;
-};
-
struct msm_dp_display_private {
int irq;
@@ -95,15 +66,9 @@ struct msm_dp_display_private {
/* wait for audio signaling */
struct completion audio_comp;
- /* event related only access by event thread */
- struct mutex event_mutex;
- wait_queue_head_t event_q;
- u32 hpd_state;
- u32 event_pndx;
- u32 event_gndx;
- struct task_struct *ev_tsk;
- struct msm_dp_event event_list[DP_EVENT_Q_MAX];
- spinlock_t event_lock;
+ /* HPD IRQ handling */
+ spinlock_t irq_thread_lock;
+ u32 hpd_isr_status;
bool wide_bus_supported;
@@ -216,59 +181,6 @@ static struct msm_dp_display_private *dev_get_dp_display_private(struct device *
return container_of(dp, struct msm_dp_display_private, msm_dp_display);
}
-static int msm_dp_add_event(struct msm_dp_display_private *msm_dp_priv, u32 event,
- u32 delay)
-{
- unsigned long flag;
- struct msm_dp_event *todo;
- int pndx;
-
- spin_lock_irqsave(&msm_dp_priv->event_lock, flag);
- pndx = msm_dp_priv->event_pndx + 1;
- pndx %= DP_EVENT_Q_MAX;
- if (pndx == msm_dp_priv->event_gndx) {
- pr_err("event_q is full: pndx=%d gndx=%d\n",
- msm_dp_priv->event_pndx, msm_dp_priv->event_gndx);
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
- return -EPERM;
- }
- todo = &msm_dp_priv->event_list[msm_dp_priv->event_pndx++];
- msm_dp_priv->event_pndx %= DP_EVENT_Q_MAX;
- todo->event_id = event;
- todo->delay = delay;
- wake_up(&msm_dp_priv->event_q);
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
-
- return 0;
-}
-
-static int msm_dp_del_event(struct msm_dp_display_private *msm_dp_priv, u32 event)
-{
- unsigned long flag;
- struct msm_dp_event *todo;
- u32 gndx;
-
- spin_lock_irqsave(&msm_dp_priv->event_lock, flag);
- if (msm_dp_priv->event_pndx == msm_dp_priv->event_gndx) {
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
- return -ENOENT;
- }
-
- gndx = msm_dp_priv->event_gndx;
- while (msm_dp_priv->event_pndx != gndx) {
- todo = &msm_dp_priv->event_list[gndx];
- if (todo->event_id == event) {
- todo->event_id = EV_NO_EVENT; /* deleted */
- todo->delay = 0;
- }
- gndx++;
- gndx %= DP_EVENT_Q_MAX;
- }
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
-
- return 0;
-}
-
void msm_dp_display_signal_audio_start(struct msm_dp *msm_dp_display)
{
struct msm_dp_display_private *dp;
@@ -287,8 +199,6 @@ void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display)
complete_all(&dp->audio_comp);
}
-static int msm_dp_hpd_event_thread_start(struct msm_dp_display_private *msm_dp_priv);
-
static int msm_dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -308,12 +218,6 @@ static int msm_dp_display_bind(struct device *dev, struct device *master,
goto end;
}
- rc = msm_dp_hpd_event_thread_start(dp);
- if (rc) {
- DRM_ERROR("Event thread create failed\n");
- goto end;
- }
-
return 0;
end:
return rc;
@@ -325,8 +229,6 @@ static void msm_dp_display_unbind(struct device *dev, struct device *master,
struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
struct msm_drm_private *priv = dev_get_drvdata(master);
- kthread_stop(dp->ev_tsk);
-
of_dp_aux_depopulate_bus(dp->aux);
msm_dp_aux_unregister(dp->aux);
@@ -340,38 +242,6 @@ static const struct component_ops msm_dp_display_comp_ops = {
.unbind = msm_dp_display_unbind,
};
-static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *dp,
- bool hpd)
-{
- if ((hpd && dp->msm_dp_display.link_ready) ||
- (!hpd && !dp->msm_dp_display.link_ready)) {
- drm_dbg_dp(dp->drm_dev, "HPD already %s\n", str_on_off(hpd));
- return 0;
- }
-
- /* reset video pattern flag on disconnect */
- if (!hpd) {
- dp->panel->video_test = false;
- if (!dp->msm_dp_display.is_edp)
- drm_dp_set_subconnector_property(dp->msm_dp_display.connector,
- connector_status_disconnected,
- dp->panel->dpcd,
- dp->panel->downstream_ports);
- }
-
- dp->msm_dp_display.link_ready = hpd;
-
- drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
- dp->msm_dp_display.connector_type, hpd);
-
- drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
- hpd ?
- connector_status_connected :
- connector_status_disconnected);
-
- return 0;
-}
-
static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd)
{
int rc, lttpr_count;
@@ -414,6 +284,8 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
dp->panel->dpcd,
dp->panel->downstream_ports);
+ dp->msm_dp_display.link_ready = true;
+
dp->msm_dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
dp->audio_supported = info->has_audio;
@@ -427,8 +299,6 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
msm_dp_link_reset_phy_params_vx_px(dp->link);
- msm_dp_display_send_hpd_notification(dp, true);
-
end:
return rc;
}
@@ -483,24 +353,6 @@ static void msm_dp_display_host_deinit(struct msm_dp_display_private *dp)
dp->core_initialized = false;
}
-static int msm_dp_display_usbpd_configure_cb(struct device *dev)
-{
- struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
-
- msm_dp_display_host_phy_init(dp);
-
- return msm_dp_display_process_hpd_high(dp);
-}
-
-static int msm_dp_display_notify_disconnect(struct device *dev)
-{
- struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
-
- msm_dp_display_send_hpd_notification(dp, false);
-
- return 0;
-}
-
static void msm_dp_display_handle_video_request(struct msm_dp_display_private *dp)
{
if (dp->link->sink_request & DP_TEST_LINK_VIDEO_PATTERN) {
@@ -509,34 +361,12 @@ static void msm_dp_display_handle_video_request(struct msm_dp_display_private *d
}
}
-static int msm_dp_display_handle_port_status_changed(struct msm_dp_display_private *dp)
-{
- int rc = 0;
-
- if (drm_dp_is_branch(dp->panel->dpcd) && dp->link->sink_count == 0) {
- drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to do\n");
- if (dp->hpd_state != ST_DISCONNECTED) {
- dp->hpd_state = ST_DISCONNECT_PENDING;
- msm_dp_display_send_hpd_notification(dp, false);
- }
- } else {
- if (dp->hpd_state == ST_DISCONNECTED) {
- dp->hpd_state = ST_MAINLINK_READY;
- rc = msm_dp_display_process_hpd_high(dp);
- if (rc)
- dp->hpd_state = ST_DISCONNECTED;
- }
- }
-
- return rc;
-}
-
static int msm_dp_display_handle_irq_hpd(struct msm_dp_display_private *dp)
{
u32 sink_request = dp->link->sink_request;
drm_dbg_dp(dp->drm_dev, "%d\n", sink_request);
- if (dp->hpd_state == ST_DISCONNECTED) {
+ if (!dp->msm_dp_display.link_ready) {
if (sink_request & DP_LINK_STATUS_UPDATED) {
drm_dbg_dp(dp->drm_dev, "Disconnected sink_request: %d\n",
sink_request);
@@ -553,76 +383,36 @@ static int msm_dp_display_handle_irq_hpd(struct msm_dp_display_private *dp)
return 0;
}
-static int msm_dp_display_usbpd_attention_cb(struct device *dev)
-{
- int rc = 0;
- u32 sink_request;
- struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
-
- /* check for any test request issued by sink */
- rc = msm_dp_link_process_request(dp->link);
- if (!rc) {
- sink_request = dp->link->sink_request;
- drm_dbg_dp(dp->drm_dev, "hpd_state=%d sink_request=%d\n",
- dp->hpd_state, sink_request);
- if (sink_request & DS_PORT_STATUS_CHANGED)
- rc = msm_dp_display_handle_port_status_changed(dp);
- else
- rc = msm_dp_display_handle_irq_hpd(dp);
- }
-
- return rc;
-}
-
static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
{
- u32 state;
int ret;
struct platform_device *pdev = dp->msm_dp_display.pdev;
- msm_dp_aux_enable_xfers(dp->aux, true);
-
- mutex_lock(&dp->event_mutex);
-
- state = dp->hpd_state;
- drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
-
- if (state == ST_DISPLAY_OFF) {
- mutex_unlock(&dp->event_mutex);
- return 0;
- }
-
- if (state == ST_MAINLINK_READY || state == ST_CONNECTED) {
- mutex_unlock(&dp->event_mutex);
- return 0;
- }
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
+ dp->msm_dp_display.connector_type);
- if (state == ST_DISCONNECT_PENDING) {
- /* wait until ST_DISCONNECTED */
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 1);
- mutex_unlock(&dp->event_mutex);
+ if (dp->msm_dp_display.link_ready)
return 0;
- }
ret = pm_runtime_resume_and_get(&pdev->dev);
if (ret) {
DRM_ERROR("failed to pm_runtime_resume\n");
- mutex_unlock(&dp->event_mutex);
return ret;
}
- ret = msm_dp_display_usbpd_configure_cb(&pdev->dev);
+ msm_dp_aux_enable_xfers(dp->aux, true);
+
+ msm_dp_display_host_phy_init(dp);
+
+ ret = msm_dp_display_process_hpd_high(dp);
if (ret) { /* link train failed */
- dp->hpd_state = ST_DISCONNECTED;
+ dp->msm_dp_display.link_ready = false;
+ msm_dp_aux_enable_xfers(dp->aux, false);
pm_runtime_put_sync(&pdev->dev);
- } else {
- dp->hpd_state = ST_MAINLINK_READY;
}
- drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
- mutex_unlock(&dp->event_mutex);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
+ dp->msm_dp_display.connector_type);
/* uevent will complete connection part */
return 0;
@@ -644,97 +434,69 @@ static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
{
- u32 state;
struct platform_device *pdev = dp->msm_dp_display.pdev;
- msm_dp_aux_enable_xfers(dp->aux, false);
-
- mutex_lock(&dp->event_mutex);
-
- state = dp->hpd_state;
+ dp->panel->video_test = false;
- drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
+ msm_dp_aux_enable_xfers(dp->aux, false);
- /* unplugged, no more irq_hpd handle */
- msm_dp_del_event(dp, EV_IRQ_HPD_INT);
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
+ dp->msm_dp_display.connector_type);
- if (state == ST_DISCONNECTED) {
- /* triggered by irq_hdp with sink_count = 0 */
- if (dp->link->sink_count == 0) {
- msm_dp_display_host_phy_exit(dp);
- }
- msm_dp_display_notify_disconnect(&dp->msm_dp_display.pdev->dev);
- mutex_unlock(&dp->event_mutex);
+ if (!dp->msm_dp_display.link_ready)
return 0;
- } else if (state == ST_DISCONNECT_PENDING) {
- mutex_unlock(&dp->event_mutex);
- return 0;
- } else if (state == ST_MAINLINK_READY) {
- msm_dp_ctrl_off_link(dp->ctrl);
+
+ /* triggered by irq_hdp with sink_count = 0 */
+ if (dp->link->sink_count == 0)
msm_dp_display_host_phy_exit(dp);
- dp->hpd_state = ST_DISCONNECTED;
- msm_dp_display_notify_disconnect(&dp->msm_dp_display.pdev->dev);
- pm_runtime_put_sync(&pdev->dev);
- mutex_unlock(&dp->event_mutex);
- return 0;
- }
/*
* We don't need separate work for disconnect as
* connect/attention interrupts are disabled
*/
- msm_dp_display_notify_disconnect(&dp->msm_dp_display.pdev->dev);
+ if (!dp->msm_dp_display.is_edp)
+ drm_dp_set_subconnector_property(dp->msm_dp_display.connector,
+ connector_status_disconnected,
+ dp->panel->dpcd,
+ dp->panel->downstream_ports);
- if (state == ST_DISPLAY_OFF) {
- dp->hpd_state = ST_DISCONNECTED;
- } else {
- dp->hpd_state = ST_DISCONNECT_PENDING;
- }
+ dp->msm_dp_display.link_ready = false;
/* signal the disconnect event early to ensure proper teardown */
msm_dp_display_handle_plugged_change(&dp->msm_dp_display, false);
- drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
+ dp->msm_dp_display.connector_type);
/* uevent will complete disconnection part */
pm_runtime_put_sync(&pdev->dev);
- mutex_unlock(&dp->event_mutex);
return 0;
}
static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
{
- u32 state;
-
- mutex_lock(&dp->event_mutex);
+ u32 sink_request;
+ int rc = 0;
/* irq_hpd can happen at either connected or disconnected state */
- state = dp->hpd_state;
- drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
+ dp->msm_dp_display.connector_type);
- if (state == ST_DISPLAY_OFF) {
- mutex_unlock(&dp->event_mutex);
- return 0;
- }
-
- if (state == ST_MAINLINK_READY || state == ST_DISCONNECT_PENDING) {
- /* wait until ST_CONNECTED */
- msm_dp_add_event(dp, EV_IRQ_HPD_INT, 1);
- mutex_unlock(&dp->event_mutex);
- return 0;
+ /* check for any test request issued by sink */
+ rc = msm_dp_link_process_request(dp->link);
+ if (!rc) {
+ sink_request = dp->link->sink_request;
+ drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request);
+ if (sink_request & DS_PORT_STATUS_CHANGED)
+ rc = msm_dp_display_process_hpd_high(dp);
+ else
+ rc = msm_dp_display_handle_irq_hpd(dp);
}
- msm_dp_display_usbpd_attention_cb(&dp->msm_dp_display.pdev->dev);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
+ dp->msm_dp_display.connector_type);
- drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
- dp->msm_dp_display.connector_type, state);
-
- mutex_unlock(&dp->event_mutex);
-
- return 0;
+ return rc;
}
static void msm_dp_display_deinit_sub_modules(struct msm_dp_display_private *dp)
@@ -1010,12 +772,8 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
* power_on status before dumping DP registers to avoid crash due
* to unclocked access
*/
- mutex_lock(&msm_dp_display->event_mutex);
-
- if (!dp->power_on) {
- mutex_unlock(&msm_dp_display->event_mutex);
+ if (!dp->power_on)
return;
- }
msm_disp_snapshot_add_block(disp_state, msm_dp_display->ahb_len,
msm_dp_display->ahb_base, "dp_ahb");
@@ -1025,8 +783,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
msm_dp_display->link_base, "dp_link");
msm_disp_snapshot_add_block(disp_state, msm_dp_display->p0_len,
msm_dp_display->p0_base, "dp_p0");
-
- mutex_unlock(&msm_dp_display->event_mutex);
}
void msm_dp_display_set_psr(struct msm_dp *msm_dp_display, bool enter)
@@ -1042,95 +798,6 @@ void msm_dp_display_set_psr(struct msm_dp *msm_dp_display, bool enter)
msm_dp_ctrl_set_psr(dp->ctrl, enter);
}
-static int hpd_event_thread(void *data)
-{
- struct msm_dp_display_private *msm_dp_priv;
- unsigned long flag;
- struct msm_dp_event *todo;
- int timeout_mode = 0;
-
- msm_dp_priv = (struct msm_dp_display_private *)data;
-
- while (1) {
- if (timeout_mode) {
- wait_event_timeout(msm_dp_priv->event_q,
- (msm_dp_priv->event_pndx == msm_dp_priv->event_gndx) ||
- kthread_should_stop(), EVENT_TIMEOUT);
- } else {
- wait_event_interruptible(msm_dp_priv->event_q,
- (msm_dp_priv->event_pndx != msm_dp_priv->event_gndx) ||
- kthread_should_stop());
- }
-
- if (kthread_should_stop())
- break;
-
- spin_lock_irqsave(&msm_dp_priv->event_lock, flag);
- todo = &msm_dp_priv->event_list[msm_dp_priv->event_gndx];
- if (todo->delay) {
- struct msm_dp_event *todo_next;
-
- msm_dp_priv->event_gndx++;
- msm_dp_priv->event_gndx %= DP_EVENT_Q_MAX;
-
- /* re enter delay event into q */
- todo_next = &msm_dp_priv->event_list[msm_dp_priv->event_pndx++];
- msm_dp_priv->event_pndx %= DP_EVENT_Q_MAX;
- todo_next->event_id = todo->event_id;
- todo_next->delay = todo->delay - 1;
-
- /* clean up older event */
- todo->event_id = EV_NO_EVENT;
- todo->delay = 0;
-
- /* switch to timeout mode */
- timeout_mode = 1;
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
- continue;
- }
-
- /* timeout with no events in q */
- if (msm_dp_priv->event_pndx == msm_dp_priv->event_gndx) {
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
- continue;
- }
-
- msm_dp_priv->event_gndx++;
- msm_dp_priv->event_gndx %= DP_EVENT_Q_MAX;
- timeout_mode = 0;
- spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
-
- switch (todo->event_id) {
- case EV_HPD_PLUG_INT:
- msm_dp_hpd_plug_handle(msm_dp_priv);
- break;
- case EV_HPD_UNPLUG_INT:
- msm_dp_hpd_unplug_handle(msm_dp_priv);
- break;
- case EV_IRQ_HPD_INT:
- msm_dp_irq_hpd_handle(msm_dp_priv);
- break;
- default:
- break;
- }
- }
-
- return 0;
-}
-
-static int msm_dp_hpd_event_thread_start(struct msm_dp_display_private *msm_dp_priv)
-{
- /* set event q to empty */
- msm_dp_priv->event_gndx = 0;
- msm_dp_priv->event_pndx = 0;
-
- msm_dp_priv->ev_tsk = kthread_run(hpd_event_thread, msm_dp_priv, "dp_hpd_handler");
- if (IS_ERR(msm_dp_priv->ev_tsk))
- return PTR_ERR(msm_dp_priv->ev_tsk);
-
- return 0;
-}
-
/**
* msm_dp_bridge_detect - callback to determine if connector is connected
* @bridge: Pointer to drm bridge structure
@@ -1155,26 +822,31 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
if (!dp->link_ready)
return status;
- 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;
}
+ msm_dp_aux_enable_xfers(priv->aux, true);
+
ret = msm_dp_aux_is_link_connected(priv->aux);
- if (dp->internal_hpd && !ret)
+ if (!ret) {
+ DRM_DEBUG_DP("aux not connected\n");
goto end;
+ }
ret = drm_dp_read_dpcd_caps(priv->aux, dpcd);
- if (ret)
+ if (ret) {
+ DRM_DEBUG_DP("failed to read caps\n");
goto end;
+ }
ret = drm_dp_read_desc(priv->aux, &desc, drm_dp_is_branch(dpcd));
- if (ret)
+ if (ret) {
+ DRM_DEBUG_DP("failed to read desc\n");
goto end;
+ }
status = connector_status_connected;
if (drm_dp_read_sink_count_cap(connector, dpcd, &desc)) {
@@ -1194,36 +866,20 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
static irqreturn_t msm_dp_display_irq_handler(int irq, void *dev_id)
{
struct msm_dp_display_private *dp = dev_id;
- irqreturn_t ret = IRQ_NONE;
u32 hpd_isr_status;
-
- if (!dp) {
- DRM_ERROR("invalid data\n");
- return IRQ_NONE;
- }
+ unsigned long flags;
+ irqreturn_t ret = IRQ_HANDLED;
hpd_isr_status = msm_dp_aux_get_hpd_intr_status(dp->aux);
if (hpd_isr_status & 0x0F) {
drm_dbg_dp(dp->drm_dev, "type=%d isr=0x%x\n",
dp->msm_dp_display.connector_type, hpd_isr_status);
- /* hpd related interrupts */
- if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0);
-
- if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
- msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0);
- }
- if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 3);
- }
-
- if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
-
- ret = IRQ_HANDLED;
+ spin_lock_irqsave(&dp->irq_thread_lock, flags);
+ dp->hpd_isr_status |= hpd_isr_status;
+ ret = IRQ_WAKE_THREAD;
+ spin_unlock_irqrestore(&dp->irq_thread_lock, flags);
}
/* DP controller isr */
@@ -1232,6 +888,36 @@ static irqreturn_t msm_dp_display_irq_handler(int irq, void *dev_id)
return ret;
}
+static irqreturn_t msm_dp_display_irq_thread(int irq, void *dev_id)
+{
+ struct msm_dp_display_private *dp = dev_id;
+ irqreturn_t ret = IRQ_NONE;
+ unsigned long flags;
+ u32 hpd_isr_status;
+
+ spin_lock_irqsave(&dp->irq_thread_lock, flags);
+ hpd_isr_status = dp->hpd_isr_status;
+ dp->hpd_isr_status = 0;
+ spin_unlock_irqrestore(&dp->irq_thread_lock, flags);
+
+ if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_disconnected);
+
+ if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_connected);
+
+ /* Send HPD as connected and distinguish it in the notifier */
+ if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_connected);
+
+ ret = IRQ_HANDLED;
+
+ return ret;
+}
+
static int msm_dp_display_request_irq(struct msm_dp_display_private *dp)
{
int rc = 0;
@@ -1243,9 +929,13 @@ static int msm_dp_display_request_irq(struct msm_dp_display_private *dp)
return dp->irq;
}
- rc = devm_request_irq(&pdev->dev, dp->irq, msm_dp_display_irq_handler,
- IRQF_TRIGGER_HIGH|IRQF_NO_AUTOEN,
- "dp_display_isr", dp);
+ spin_lock_init(&dp->irq_thread_lock);
+ irq_set_status_flags(dp->irq, IRQ_NOAUTOEN);
+ rc = devm_request_threaded_irq(&pdev->dev, dp->irq,
+ msm_dp_display_irq_handler,
+ msm_dp_display_irq_thread,
+ IRQ_TYPE_LEVEL_HIGH,
+ "dp_display_isr", dp);
if (rc < 0) {
DRM_ERROR("failed to request IRQ%u: %d\n",
@@ -1425,6 +1115,7 @@ static int msm_dp_display_probe(struct platform_device *pdev)
dp->wide_bus_supported = desc->wide_bus_supported;
dp->msm_dp_display.is_edp =
(dp->msm_dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
+ dp->hpd_isr_status = 0;
rc = msm_dp_display_get_io(dp);
if (rc)
@@ -1436,11 +1127,6 @@ static int msm_dp_display_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
- /* setup event q */
- mutex_init(&dp->event_mutex);
- init_waitqueue_head(&dp->event_q);
- spin_lock_init(&dp->event_lock);
-
/* Store DP audio handle inside DP display */
dp->msm_dp_display.msm_dp_audio = dp->audio;
@@ -1636,7 +1322,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
int rc = 0;
struct msm_dp_display_private *msm_dp_display;
- u32 hpd_state;
bool force_link_train = false;
msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
@@ -1648,29 +1333,21 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
if (dp->is_edp)
msm_dp_hpd_plug_handle(msm_dp_display);
- mutex_lock(&msm_dp_display->event_mutex);
if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
DRM_ERROR("failed to pm_runtime_resume\n");
- mutex_unlock(&msm_dp_display->event_mutex);
return;
}
- hpd_state = msm_dp_display->hpd_state;
- if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
- mutex_unlock(&msm_dp_display->event_mutex);
+ if (msm_dp_display->link->sink_count == 0)
return;
- }
rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
if (rc) {
DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
- mutex_unlock(&msm_dp_display->event_mutex);
return;
}
- hpd_state = msm_dp_display->hpd_state;
-
- if (hpd_state == ST_DISPLAY_OFF) {
+ if (dp->link_ready && !dp->power_on) {
msm_dp_display_host_phy_init(msm_dp_display);
force_link_train = true;
}
@@ -1688,11 +1365,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
msm_dp_display_disable(msm_dp_display);
}
- /* completed connection */
- msm_dp_display->hpd_state = ST_CONNECTED;
-
drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
- mutex_unlock(&msm_dp_display->event_mutex);
}
void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
@@ -1712,7 +1385,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
{
struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
- u32 hpd_state;
struct msm_dp_display_private *msm_dp_display;
msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
@@ -1720,27 +1392,14 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
if (dp->is_edp)
msm_dp_hpd_unplug_handle(msm_dp_display);
- mutex_lock(&msm_dp_display->event_mutex);
-
- hpd_state = msm_dp_display->hpd_state;
- if (hpd_state != ST_DISCONNECT_PENDING && hpd_state != ST_CONNECTED)
- drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
- dp->connector_type, hpd_state);
+ if (!dp->link_ready)
+ drm_dbg_dp(dp->drm_dev, "type=%d is disconnected\n", dp->connector_type);
msm_dp_display_disable(msm_dp_display);
- hpd_state = msm_dp_display->hpd_state;
- if (hpd_state == ST_DISCONNECT_PENDING) {
- /* completed disconnection */
- msm_dp_display->hpd_state = ST_DISCONNECTED;
- } else {
- msm_dp_display->hpd_state = ST_DISPLAY_OFF;
- }
-
drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
pm_runtime_put_sync(&dp->pdev->dev);
- mutex_unlock(&msm_dp_display->event_mutex);
}
void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
@@ -1796,18 +1455,13 @@ void msm_dp_bridge_hpd_enable(struct drm_bridge *bridge)
* step-4: DP PHY is initialized at plugin handler before link training
*
*/
- mutex_lock(&dp->event_mutex);
if (pm_runtime_resume_and_get(&msm_dp_display->pdev->dev)) {
DRM_ERROR("failed to resume power\n");
- mutex_unlock(&dp->event_mutex);
return;
}
msm_dp_aux_hpd_enable(dp->aux);
msm_dp_aux_hpd_intr_enable(dp->aux);
-
- msm_dp_display->internal_hpd = true;
- mutex_unlock(&dp->event_mutex);
}
void msm_dp_bridge_hpd_disable(struct drm_bridge *bridge)
@@ -1816,15 +1470,10 @@ void msm_dp_bridge_hpd_disable(struct drm_bridge *bridge)
struct msm_dp *msm_dp_display = msm_dp_bridge->msm_dp_display;
struct msm_dp_display_private *dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
- mutex_lock(&dp->event_mutex);
-
msm_dp_aux_hpd_intr_disable(dp->aux);
msm_dp_aux_hpd_disable(dp->aux);
- msm_dp_display->internal_hpd = false;
-
pm_runtime_put_sync(&msm_dp_display->pdev->dev);
- mutex_unlock(&dp->event_mutex);
}
void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
@@ -1834,13 +1483,31 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(bridge);
struct msm_dp *msm_dp_display = msm_dp_bridge->msm_dp_display;
struct msm_dp_display_private *dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
+ u32 hpd_link_status = 0;
- /* Without next_bridge interrupts are handled by the DP core directly */
- if (msm_dp_display->internal_hpd)
+ if (pm_runtime_resume_and_get(&msm_dp_display->pdev->dev)) {
+ DRM_ERROR("failed to pm_runtime_resume\n");
return;
+ }
+
+ hpd_link_status = msm_dp_aux_is_link_connected(dp->aux);
- if (!msm_dp_display->link_ready && status == connector_status_connected)
- msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0);
- else if (msm_dp_display->link_ready && status == connector_status_disconnected)
- msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0);
+ drm_dbg_dp(dp->drm_dev, "type=%d link hpd_link_status=0x%x, link_ready=%d, status=%d\n",
+ msm_dp_display->connector_type, hpd_link_status,
+ msm_dp_display->link_ready, status);
+
+ if (status == connector_status_connected) {
+ if (hpd_link_status == ISR_HPD_REPLUG_COUNT) {
+ msm_dp_hpd_unplug_handle(dp);
+ msm_dp_hpd_plug_handle(dp);
+ } else if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT) {
+ msm_dp_irq_hpd_handle(dp);
+ } else {
+ msm_dp_hpd_plug_handle(dp);
+ }
+ } else {
+ msm_dp_hpd_unplug_handle(dp);
+ }
+
+ pm_runtime_put_sync(&msm_dp_display->pdev->dev);
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 60094061c102..d2d3d61eb0b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -22,7 +22,6 @@ struct msm_dp {
bool power_on;
unsigned int connector_type;
bool is_edp;
- bool internal_hpd;
struct msm_dp_audio *msm_dp_audio;
bool psr_supported;
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: rework HPD handling
2026-03-14 1:09 ` [PATCH v5 07/10] drm/msm/dp: rework HPD handling Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the big patch — removes the entire event queue/thread, replaces with threaded IRQ, and restructures all HPD handling. The core design is good: the hardcoded IRQ handler accumulates status bits, the threaded handler dispatches `drm_bridge_hpd_notify()`, and `hpd_notify()` calls the actual plug/unplug/irq handlers.
**Issues:**
1. **Replug handling in threaded IRQ vs hpd_notify:** In the threaded IRQ handler, unplug is processed before plug:
```c
+ if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_disconnected);
+
+ if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_connected);
```
But in `hpd_notify()`, a replug is detected by checking `hpd_link_status == ISR_HPD_REPLUG_COUNT`. This means the replug path depends on reading the HPD state status bits from the register at `hpd_notify()` time, which may have changed from the original interrupt. If the replug completes quickly, the status bits may already show "connected" by the time `hpd_notify()` runs. The old code handled this by queueing both unplug and plug events with a delay, which was more robust. Consider whether this race can actually occur in practice.
2. **IRQ_HPD treated as connected:** The threaded IRQ handler sends `connector_status_connected` for IRQ_HPD:
```c
+ /* Send HPD as connected and distinguish it in the notifier */
+ if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK)
+ drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+ connector_status_connected);
```
Then in `hpd_notify()`, the IRQ_HPD case is handled by checking `hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT`. This overloading of `connector_status_connected` to mean both "plug" and "IRQ HPD" is fragile. If an actual plug and IRQ HPD occur at the same time, the two `hpd_notify(connected)` calls cannot be distinguished. The comment acknowledges the hack but doesn't mitigate it.
3. **Missing locking:** The `event_mutex` was removed entirely, but `msm_dp_hpd_plug_handle()`, `msm_dp_hpd_unplug_handle()`, and `msm_dp_irq_hpd_handle()` can now run concurrently (from threaded IRQ and from `hpd_notify()`). Before patch 9 adds `plugged_lock`, there is no synchronization around `link_ready` checks and state transitions. There is a window between patches 7 and 9 where the code has data races.
4. **pm_runtime imbalance in hpd_notify:** `hpd_notify()` does `pm_runtime_resume_and_get()` at the top and `pm_runtime_put_sync()` at the bottom, but `msm_dp_hpd_plug_handle()` also does its own `pm_runtime_resume_and_get()` internally. This means on a plug event, the runtime PM refcount is incremented twice — once in `hpd_notify()` and once in `msm_dp_hpd_plug_handle()`. The `hpd_notify()` then calls `pm_runtime_put_sync()` at the end, but `msm_dp_hpd_plug_handle()` only calls `pm_runtime_put_sync()` on failure. On success, the extra ref from `hpd_notify()` is dropped but the one from `plug_handle()` remains (intentionally, to keep the device powered while connected). This looks correct but is subtle and deserves a comment.
5. **msm_dp_snapshot lost locking:** The `mutex_lock(&msm_dp_display->event_mutex)` around `msm_dp_snapshot()` was removed, but the function reads hardware registers that require the device to be powered. The `power_on` check alone is racy without any lock. Consider whether `pm_runtime_get_if_active()` or similar protection is needed.
6. **`irq_set_status_flags` vs `IRQF_NO_AUTOEN`:** The patch changes from `IRQF_NO_AUTOEN` flag in `devm_request_irq()` to a separate `irq_set_status_flags(dp->irq, IRQ_NOAUTOEN)` call. Using `IRQF_NO_AUTOEN` with `devm_request_threaded_irq()` would be simpler and more conventional.
7. **Default return value changed:**
```c
- irqreturn_t ret = IRQ_NONE;
+ irqreturn_t ret = IRQ_HANDLED;
```
The hardcoded IRQ handler now returns `IRQ_HANDLED` even when only `msm_dp_ctrl_isr()` fires (or when nothing fires at all if `hpd_isr_status` has no bits and `msm_dp_ctrl_isr` returns `IRQ_NONE`). This suppresses spurious IRQ detection. Should be `IRQ_NONE` by default, with `ret` set per-path.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 08/10] drm/msm/dp: Add sink_count to debug logs
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (6 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 07/10] drm/msm/dp: rework HPD handling Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged Dmitry Baryshkov
` (3 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Jessica Zhang
From: Jessica Zhang <jesszhan0024@gmail.com>
Add sink count to the debug logs for [un]plug and HPD IRQ handling.
Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
[DB: dropped link_ready handling]
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d61a8ddd30a8..b4deeea0ec59 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -388,8 +388,9 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
int ret;
struct platform_device *pdev = dp->msm_dp_display.pdev;
- drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
if (dp->msm_dp_display.link_ready)
return 0;
@@ -411,8 +412,9 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
pm_runtime_put_sync(&pdev->dev);
}
- drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
/* uevent will complete connection part */
return 0;
@@ -440,8 +442,9 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
msm_dp_aux_enable_xfers(dp->aux, false);
- drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
if (!dp->msm_dp_display.link_ready)
return 0;
@@ -465,8 +468,9 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
/* signal the disconnect event early to ensure proper teardown */
msm_dp_display_handle_plugged_change(&dp->msm_dp_display, false);
- drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d, sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
/* uevent will complete disconnection part */
pm_runtime_put_sync(&pdev->dev);
@@ -479,8 +483,9 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
int rc = 0;
/* irq_hpd can happen at either connected or disconnected state */
- drm_dbg_dp(dp->drm_dev, "Before, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "Before, type=%d, sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
/* check for any test request issued by sink */
rc = msm_dp_link_process_request(dp->link);
@@ -493,8 +498,9 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
rc = msm_dp_display_handle_irq_hpd(dp);
}
- drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
- dp->msm_dp_display.connector_type);
+ drm_dbg_dp(dp->drm_dev, "After, type=%d, sink_count=%d\n",
+ dp->msm_dp_display.connector_type,
+ dp->link->sink_count);
return rc;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (7 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 08/10] drm/msm/dp: Add sink_count to debug logs Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-14 1:09 ` [PATCH v5 10/10] drm/msm/dp: clear EDID on display unplug Dmitry Baryshkov
` (2 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
Tracking when the DP link is ready isn't that useful from the driver
point of view. It doesn't provide a direct information if the device
should be suspended, etc. Replace it with the 'plugged' boolean, which
is set when the driver knows that there is DPRX plugged.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 94 ++++++++++++++++++++++---------------
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
drivers/gpu/drm/msm/dp/dp_drm.c | 41 ++--------------
3 files changed, 61 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index b4deeea0ec59..0a38957ea901 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -53,6 +53,9 @@ struct msm_dp_display_private {
bool phy_initialized;
bool audio_supported;
+ struct mutex plugged_lock;
+ bool plugged;
+
struct drm_device *drm_dev;
struct drm_dp_aux *aux;
@@ -284,8 +287,6 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
dp->panel->dpcd,
dp->panel->downstream_ports);
- dp->msm_dp_display.link_ready = true;
-
dp->msm_dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
dp->audio_supported = info->has_audio;
@@ -303,7 +304,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
return rc;
}
-static void msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
+static bool msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
{
drm_dbg_dp(dp->drm_dev, "type=%d core_init=%d phy_init=%d\n",
dp->msm_dp_display.connector_type, dp->core_initialized,
@@ -312,7 +313,10 @@ static void msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
if (!dp->phy_initialized) {
msm_dp_ctrl_phy_init(dp->ctrl);
dp->phy_initialized = true;
+ return true;
}
+
+ return false;
}
static void msm_dp_display_host_phy_exit(struct msm_dp_display_private *dp)
@@ -366,14 +370,6 @@ static int msm_dp_display_handle_irq_hpd(struct msm_dp_display_private *dp)
u32 sink_request = dp->link->sink_request;
drm_dbg_dp(dp->drm_dev, "%d\n", sink_request);
- if (!dp->msm_dp_display.link_ready) {
- if (sink_request & DP_LINK_STATUS_UPDATED) {
- drm_dbg_dp(dp->drm_dev, "Disconnected sink_request: %d\n",
- sink_request);
- DRM_ERROR("Disconnected, no DP_LINK_STATUS_UPDATED\n");
- return -EINVAL;
- }
- }
msm_dp_ctrl_handle_sink_request(dp->ctrl);
@@ -392,11 +388,11 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
dp->msm_dp_display.connector_type,
dp->link->sink_count);
- if (dp->msm_dp_display.link_ready)
- return 0;
+ mutex_lock(&dp->plugged_lock);
ret = pm_runtime_resume_and_get(&pdev->dev);
if (ret) {
+ mutex_unlock(&dp->plugged_lock);
DRM_ERROR("failed to pm_runtime_resume\n");
return ret;
}
@@ -406,18 +402,16 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
msm_dp_display_host_phy_init(dp);
ret = msm_dp_display_process_hpd_high(dp);
- if (ret) { /* link train failed */
- dp->msm_dp_display.link_ready = false;
- msm_dp_aux_enable_xfers(dp->aux, false);
- pm_runtime_put_sync(&pdev->dev);
- }
drm_dbg_dp(dp->drm_dev, "After, type=%d sink_count=%d\n",
dp->msm_dp_display.connector_type,
dp->link->sink_count);
- /* uevent will complete connection part */
- return 0;
+ dp->plugged = true;
+
+ mutex_unlock(&dp->plugged_lock);
+
+ return ret;
};
static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
@@ -446,8 +440,12 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
dp->msm_dp_display.connector_type,
dp->link->sink_count);
- if (!dp->msm_dp_display.link_ready)
+ mutex_lock(&dp->plugged_lock);
+ if (!dp->plugged) {
+ mutex_unlock(&dp->plugged_lock);
+
return 0;
+ }
/* triggered by irq_hdp with sink_count = 0 */
if (dp->link->sink_count == 0)
@@ -463,8 +461,6 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
dp->panel->dpcd,
dp->panel->downstream_ports);
- dp->msm_dp_display.link_ready = false;
-
/* signal the disconnect event early to ensure proper teardown */
msm_dp_display_handle_plugged_change(&dp->msm_dp_display, false);
@@ -472,8 +468,12 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
dp->msm_dp_display.connector_type,
dp->link->sink_count);
- /* uevent will complete disconnection part */
- pm_runtime_put_sync(&pdev->dev);
+ if (dp->plugged) {
+ pm_runtime_put_sync(&pdev->dev);
+ dp->plugged = false;
+ }
+ mutex_unlock(&dp->plugged_lock);
+
return 0;
}
@@ -820,41 +820,49 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
int status = connector_status_disconnected;
u8 dpcd[DP_RECEIVER_CAP_SIZE];
struct drm_dp_desc desc;
+ bool phy_deinit;
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;
-
+ mutex_lock(&priv->plugged_lock);
ret = pm_runtime_resume_and_get(&dp->pdev->dev);
if (ret) {
DRM_ERROR("failed to pm_runtime_resume\n");
+ mutex_unlock(&priv->plugged_lock);
return status;
}
+ phy_deinit = msm_dp_display_host_phy_init(priv);
+
msm_dp_aux_enable_xfers(priv->aux, true);
ret = msm_dp_aux_is_link_connected(priv->aux);
- if (!ret) {
+ DRM_DEBUG_DP("aux link status: %x\n", ret);
+ if (!priv->plugged && !ret) {
DRM_DEBUG_DP("aux not connected\n");
+ priv->plugged = false;
goto end;
}
ret = drm_dp_read_dpcd_caps(priv->aux, dpcd);
if (ret) {
DRM_DEBUG_DP("failed to read caps\n");
+ priv->plugged = false;
goto end;
}
ret = drm_dp_read_desc(priv->aux, &desc, drm_dp_is_branch(dpcd));
if (ret) {
DRM_DEBUG_DP("failed to read desc\n");
+ priv->plugged = false;
goto end;
}
status = connector_status_connected;
+ priv->plugged = true;
+
if (drm_dp_read_sink_count_cap(connector, dpcd, &desc)) {
int sink_count = drm_dp_read_sink_count(priv->aux);
@@ -865,7 +873,21 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
}
end:
- pm_runtime_put_sync(&dp->pdev->dev);
+ /*
+ * If we detected the DPRX, leave the controller on so that it doesn't
+ * loose the state.
+ */
+ if (!priv->plugged) {
+ if (phy_deinit) {
+ msm_dp_aux_enable_xfers(priv->aux, false);
+ msm_dp_display_host_phy_exit(priv);
+ }
+
+ pm_runtime_put_sync(&dp->pdev->dev);
+ }
+
+ mutex_unlock(&priv->plugged_lock);
+
return status;
}
@@ -1123,6 +1145,8 @@ static int msm_dp_display_probe(struct platform_device *pdev)
(dp->msm_dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
dp->hpd_isr_status = 0;
+ mutex_init(&dp->plugged_lock);
+
rc = msm_dp_display_get_io(dp);
if (rc)
return rc;
@@ -1353,7 +1377,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
return;
}
- if (dp->link_ready && !dp->power_on) {
+ if (!dp->power_on) {
msm_dp_display_host_phy_init(msm_dp_display);
force_link_train = true;
}
@@ -1398,9 +1422,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
if (dp->is_edp)
msm_dp_hpd_unplug_handle(msm_dp_display);
- if (!dp->link_ready)
- drm_dbg_dp(dp->drm_dev, "type=%d is disconnected\n", dp->connector_type);
-
msm_dp_display_disable(msm_dp_display);
drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
@@ -1498,9 +1519,8 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
hpd_link_status = msm_dp_aux_is_link_connected(dp->aux);
- drm_dbg_dp(dp->drm_dev, "type=%d link hpd_link_status=0x%x, link_ready=%d, status=%d\n",
- msm_dp_display->connector_type, hpd_link_status,
- msm_dp_display->link_ready, status);
+ drm_dbg_dp(dp->drm_dev, "type=%d link hpd_link_status=0x%x, status=%d\n",
+ msm_dp_display->connector_type, hpd_link_status, status);
if (status == connector_status_connected) {
if (hpd_link_status == ISR_HPD_REPLUG_COUNT) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index d2d3d61eb0b0..0b65e16c790d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -17,7 +17,6 @@ struct msm_dp {
struct drm_connector *connector;
struct drm_bridge *next_bridge;
struct drm_bridge *bridge;
- bool link_ready;
bool audio_enabled;
bool power_on;
unsigned int connector_type;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index f935093c4df4..8dc0dabd275c 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -15,36 +15,6 @@
#include "dp_audio.h"
#include "dp_drm.h"
-static int msm_dp_bridge_atomic_check(struct drm_bridge *bridge,
- struct drm_bridge_state *bridge_state,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state)
-{
- 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));
-
- /*
- * There is no protection in the DRM framework to check if the display
- * pipeline has been already disabled before trying to disable it again.
- * Hence if the sink is unplugged, the pipeline gets disabled, but the
- * crtc->active is still true. Any attempt to set the mode or manually
- * disable this encoder will result in the crash.
- *
- * TODO: add support for telling the DRM subsystem that the pipeline is
- * disabled by the hardware and thus all access to it should be forbidden.
- * After that this piece of code can be removed.
- */
- if (bridge->ops & DRM_BRIDGE_OP_HPD)
- return (dp->link_ready) ? 0 : -ENOTCONN;
-
- return 0;
-}
-
-
/**
* msm_dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
* @bridge: Poiner to drm bridge
@@ -62,12 +32,10 @@ static int msm_dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connect
dp = to_dp_bridge(bridge)->msm_dp_display;
/* pluggable case assumes EDID is read when HPD */
- if (dp->link_ready) {
- rc = msm_dp_display_get_modes(dp);
- if (rc <= 0) {
- DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
- return rc;
- }
+ rc = msm_dp_display_get_modes(dp);
+ if (rc <= 0) {
+ DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
+ return rc;
} else {
drm_dbg_dp(connector->dev, "No sink connected\n");
}
@@ -92,7 +60,6 @@ static const struct drm_bridge_funcs msm_dp_bridge_ops = {
.mode_valid = msm_dp_bridge_mode_valid,
.get_modes = msm_dp_bridge_get_modes,
.detect = msm_dp_bridge_detect,
- .atomic_check = msm_dp_bridge_atomic_check,
.hpd_enable = msm_dp_bridge_hpd_enable,
.hpd_disable = msm_dp_bridge_hpd_disable,
.hpd_notify = msm_dp_bridge_hpd_notify,
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: turn link_ready into plugged
2026-03-14 1:09 ` [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Replaces the public `link_ready` flag with a private `plugged` boolean protected by `plugged_lock` mutex. Also removes `msm_dp_bridge_atomic_check()` which returned `-ENOTCONN` when link was not ready.
**Issues:**
1. **msm_dp_bridge_get_modes logic broken:** The patch changes:
```c
- if (dp->link_ready) {
- rc = msm_dp_display_get_modes(dp);
- if (rc <= 0) {
- DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
- return rc;
- }
- } else {
- drm_dbg_dp(connector->dev, "No sink connected\n");
- }
+ rc = msm_dp_display_get_modes(dp);
+ if (rc <= 0) {
+ DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
+ return rc;
+ } else {
+ drm_dbg_dp(connector->dev, "No sink connected\n");
+ }
```
The `else` branch is now unreachable — if `rc <= 0` the function returns, and if `rc > 0` it falls into the `else` printing "No sink connected" which is wrong (it means modes were successfully found). This should be `if (rc > 0)` to reach the else, or the else should be removed.
2. **plug_handle unconditional pm_runtime_get:** After removing the `link_ready` guard:
```c
- if (dp->msm_dp_display.link_ready)
- return 0;
+ mutex_lock(&dp->plugged_lock);
```
Now `msm_dp_hpd_plug_handle()` always calls `pm_runtime_resume_and_get()` even if already plugged. On repeated plug events, this will increment the pm_runtime refcount without matching puts. The old code had `if (link_ready) return 0` which prevented this.
3. **Error handling regression in plug_handle:** After the rework:
```c
ret = msm_dp_display_process_hpd_high(dp);
...
dp->plugged = true;
mutex_unlock(&dp->plugged_lock);
return ret;
```
Even on failure (`ret != 0`), `dp->plugged` is set to `true`. This means a failed plug will leave the device in a state where it thinks it's plugged, preventing future plug attempts from working and leaving a stale pm_runtime ref.
4. **Removal of atomic_check:** Removing `msm_dp_bridge_atomic_check()` that returned `-ENOTCONN` when link was down means the DRM core can now attempt to enable a disconnected display. The commit message mentions "After removing the link_ready flag, DP bridge doesn't have a distinction between link trained and display plugged in" but doesn't explain how the "display enabled while disconnected" case is handled. The `sink_count == 0` check in `atomic_enable()` returns early but doesn't prevent the mode set.
5. **detect() leaves device powered if plugged:** The new detect logic calls `pm_runtime_resume_and_get()` and only calls `pm_runtime_put_sync()` if `!priv->plugged`. This means every detect() call that finds the device plugged increments pm_runtime without a put. Multiple detect() calls would stack refs. The comment says "leave the controller on so that it doesn't lose the state" but this ref stacking is problematic.
6. **Typo:** "loose" should be "lose" in:
```c
+ * If we detected the DPRX, leave the controller on so that it doesn't
+ * loose the state.
```
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 10/10] drm/msm/dp: clear EDID on display unplug
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (8 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged Dmitry Baryshkov
@ 2026-03-14 1:09 ` Dmitry Baryshkov
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
2026-03-15 0:51 ` [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Val Packett
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
11 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2026-03-14 1:09 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
Currently the driver only updates the EDID when it detects a connected
monitor, which results in the connector still listing outdated modes
even after the display is unplugged. Set connector's EDID to NULL on
unplug to clear the list of modes.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 4 ++++
drivers/gpu/drm/msm/dp/dp_panel.c | 8 ++++++++
drivers/gpu/drm/msm/dp/dp_panel.h | 2 ++
3 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0a38957ea901..5c6a24ec140d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -447,6 +447,10 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
return 0;
}
+ /* Don't forget modes for eDP */
+ if (!dp->msm_dp_display.is_edp)
+ msm_dp_panel_unplugged(dp->panel, dp->msm_dp_display.connector);
+
/* triggered by irq_hdp with sink_count = 0 */
if (dp->link->sink_count == 0)
msm_dp_display_host_phy_exit(dp);
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 891211b23202..6bb021820d7c 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -293,6 +293,14 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
return rc;
}
+void msm_dp_panel_unplugged(struct msm_dp_panel *msm_dp_panel,
+ struct drm_connector *connector)
+{
+ drm_edid_connector_update(connector, NULL);
+ drm_edid_free(msm_dp_panel->drm_edid);
+ msm_dp_panel->drm_edid = NULL;
+}
+
u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
u32 mode_edid_bpp, u32 mode_pclk_khz)
{
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 177c1328fd99..9173e90a5053 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -49,6 +49,8 @@ int msm_dp_panel_deinit(struct msm_dp_panel *msm_dp_panel);
int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en);
int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
struct drm_connector *connector);
+void msm_dp_panel_unplugged(struct msm_dp_panel *msm_dp_panel,
+ struct drm_connector *connector);
u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
u32 mode_pclk_khz);
int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
--
2.47.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: drm/msm/dp: clear EDID on display unplug
2026-03-14 1:09 ` [PATCH v5 10/10] drm/msm/dp: clear EDID on display unplug Dmitry Baryshkov
@ 2026-03-16 2:12 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Adds `msm_dp_panel_unplugged()` to clear the EDID on unplug for non-eDP displays. Clean and correct — without this, stale EDID data would persist after unplug.
No issues.
---
**Summary of key concerns:**
- Patch 9 has the most issues: broken `get_modes()` logic, error handling regression in `plug_handle()` where `plugged = true` on failure, and pm_runtime ref stacking in `detect()`.
- Patch 7's IRQ handler default return value should be `IRQ_NONE`, not `IRQ_HANDLED`.
- The series would benefit from better locking documentation explaining the synchronization model that replaces `event_mutex`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (9 preceding siblings ...)
2026-03-14 1:09 ` [PATCH v5 10/10] drm/msm/dp: clear EDID on display unplug Dmitry Baryshkov
@ 2026-03-15 0:51 ` Val Packett
2026-03-15 1:10 ` Val Packett
2026-03-16 2:12 ` Claude review: " Claude Code Review Bot
11 siblings, 1 reply; 24+ messages in thread
From: Val Packett @ 2026-03-15 0:51 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, David Airlie,
Simona Vetter, Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
On 3/13/26 10:09 PM, Dmitry Baryshkov wrote:
> Currently, all HPD interrupt handling must go through the HPD state
> machine.
>
> This has caused many issues where the DRM framework assumes that DP is
> in one state while the state machine is stuck in another state.
>
> As discussed here [1], this series:
>
> - Removes the state machine
> - Moves link training to atomic_enable()
> - Changes the detect() behavior to return true if a display is physically
> plugged in (as opposed to if the DP link is ready).
> - Remove event queue and move internal HPD handling to hpd_notify()
>
> To correctly detect the displays which are plugged on boot on the boards
> which use dp-connector devices, this series depends on [2]. USB-C and
> eDP panels are handled natively.
>
> [1] https://patchwork.freedesktop.org/patch/656312/?series=142010&rev=2#comment_1201738
> [2] https://lore.kernel.org/all/20260314-dp-connector-hpd-v1-0-786044cedc17@oss.qualcomm.com/
Unfortunately this currently seems to mostly break link training with
USB-C, on x1e80100-dell-latitude-7455:
[ 102.190083] [drm:msm_dp_ctrl_link_train_1_2 [msm]] *ERROR* link
training #2 on phy 1 failed. ret=-110
[ 102.192846] [drm:msm_dp_ctrl_setup_main_link [msm]] *ERROR* link
training of LTTPR(s) failed. ret=-110
[ 102.211095] [drm:msm_dp_bridge_atomic_enable [msm]] *ERROR* Failed
link training (rc=-104)
[ 102.211164] [drm:msm_dp_aux_isr [msm]] *ERROR* Unexpected DP AUX IRQ
0x01000000 when not busy
[ 102.247168] [drm:msm_dp_ctrl_link_train_1_2 [msm]] *ERROR* link
training #2 on phy 1 failed. ret=-110
[ 102.252859] [drm:msm_dp_ctrl_setup_main_link [msm]] *ERROR* link
training of LTTPR(s) failed. ret=-110
Once I did get it to connect on one of the ports, but then on unplug it
didn't detect the unplug properly and continued trying to use the link:
[ 1418.418954] disp_cc_mdss_dptx1_link_clk status stuck at 'off'
<trace cut>
[ 1418.420950] Failed to enable clk 'ctrl_link': -16
[ 1418.420959] [drm:msm_dp_ctrl_enable_mainlink_clocks [msm]] *ERROR*
Unable to start link clocks. ret=-16
[ 1418.421058] [drm:msm_dp_bridge_atomic_enable [msm]] *ERROR* Failed
link training (rc=-16)
<trace cut>
[ 1418.423228] Failed to enable clk 'ctrl_link': -16
[ 1418.423234] [drm:msm_dp_ctrl_enable_mainlink_clocks [msm]] *ERROR*
Unable to start link clocks. ret=-16
[ 1418.423325] [drm:msm_dp_ctrl_on_stream [msm]] *ERROR* Failed to start
link clocks. ret=-16
[ 1418.476234] [drm:dpu_encoder_phys_vid_wait_for_commit_done:543] [dpu
error]vblank timeout: 808208c0
[ 1418.476272] [drm:dpu_kms_wait_for_commit_done:527] [dpu error]wait
for commit done returned -110
[ 1418.509204] [drm:dpu_encoder_frame_done_timeout:2731] [dpu
error]enc37 frame done timeout
[ 1419.163337] [drm:dpu_encoder_phys_vid_wait_for_commit_done:543] [dpu
error]vblank timeout: 808208c0
[ 1419.163365] [drm:dpu_kms_wait_for_commit_done:527] [dpu error]wait
for commit done returned -110
But after reboot, link training was failing on both ports..
Thanks,
~val
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine
2026-03-15 0:51 ` [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Val Packett
@ 2026-03-15 1:10 ` Val Packett
0 siblings, 0 replies; 24+ messages in thread
From: Val Packett @ 2026-03-15 1:10 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, David Airlie,
Simona Vetter, Kuogee Hsieh, Yongxing Mou
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
On 3/14/26 9:51 PM, Val Packett wrote:
>
> On 3/13/26 10:09 PM, Dmitry Baryshkov wrote:
>> Currently, all HPD interrupt handling must go through the HPD state
>> machine.
>>
>> This has caused many issues where the DRM framework assumes that DP is
>> in one state while the state machine is stuck in another state.
>>
>> As discussed here [1], this series:
>>
>> - Removes the state machine
>> - Moves link training to atomic_enable()
>> - Changes the detect() behavior to return true if a display is
>> physically
>> plugged in (as opposed to if the DP link is ready).
>> - Remove event queue and move internal HPD handling to hpd_notify()
>>
>> To correctly detect the displays which are plugged on boot on the boards
>> which use dp-connector devices, this series depends on [2]. USB-C and
>> eDP panels are handled natively.
>>
>> [1]
>> https://patchwork.freedesktop.org/patch/656312/?series=142010&rev=2#comment_1201738
>> [2]
>> https://lore.kernel.org/all/20260314-dp-connector-hpd-v1-0-786044cedc17@oss.qualcomm.com/
>
> Unfortunately this currently seems to mostly break link training with
> USB-C, on x1e80100-dell-latitude-7455:
>
> [ 102.190083] [drm:msm_dp_ctrl_link_train_1_2 [msm]] *ERROR* link
> training #2 on phy 1 failed. ret=-110
> [ 102.192846] [drm:msm_dp_ctrl_setup_main_link [msm]] *ERROR* link
> training of LTTPR(s) failed. ret=-110
> [ 102.211095] [drm:msm_dp_bridge_atomic_enable [msm]] *ERROR* Failed
> link training (rc=-104)
> [ 102.211164] [drm:msm_dp_aux_isr [msm]] *ERROR* Unexpected DP AUX
> IRQ 0x01000000 when not busy
> [ 102.247168] [drm:msm_dp_ctrl_link_train_1_2 [msm]] *ERROR* link
> training #2 on phy 1 failed. ret=-110
> [ 102.252859] [drm:msm_dp_ctrl_setup_main_link [msm]] *ERROR* link
> training of LTTPR(s) failed. ret=-110
>
> [..]
Actually looks like that might've been due to having applied the [2]
dp-connector series from above.
Removed it and rebooted, now plugging and unplugging multiple times
between the 2 ports works fine.
Except unplug is still not reliable, the "ghost" monitor often remains
after unplugging.
Almost nothing is logged to dmesg, literally I've only seen this line once:
[drm:msm_dp_panel_read_sink_caps [msm]] *ERROR* panel edid read failed
But I have unplugged the monitor now and I still see:
crtc[108]: crtc-1
enable=1
active=1
self_refresh_active=0
planes_changed=1
mode_changed=0
active_changed=0
connectors_changed=0
color_mgmt_changed=0
plane_mask=2
connector_mask=2
encoder_mask=2
mode: "3840x2560": 60 631750 3840 3888 3920 4000 2560 2563 2573
2633 0x48 0x9
lm[0]=2
ctl[0]=1
lm[1]=3
ctl[1]=1
connector[38]: DP-2
crtc=crtc-1
[..]
and the compositor thinks it's still present, I can move the mouse to
where the screen was etc.
~val
^ permalink raw reply [flat|nested] 24+ messages in thread
* Claude review: drm/msm/dp: Drop the HPD state machine
2026-03-14 1:09 [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
` (10 preceding siblings ...)
2026-03-15 0:51 ` [PATCH v5 00/10] drm/msm/dp: Drop the HPD state machine Val Packett
@ 2026-03-16 2:12 ` Claude Code Review Bot
11 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-16 2:12 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/msm/dp: Drop the HPD state machine
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 13
Reviewed: 2026-03-16T12:12:55.366957
---
This v5 series removes the HPD state machine from the MSM DP driver, which has been a long-standing source of bugs due to state mismatch between the DRM framework and the DP driver's internal state. The approach is sound: replace the custom event queue/thread with threaded IRQs and `drm_bridge_hpd_notify()`, unifying the internal HPD and external (USB-C) HPD paths. The series is well-structured with incremental cleanups leading to the big rework in patch 7.
However, there are several issues worth addressing — mostly around concurrency/locking in the reworked code, a logic error in `msm_dp_bridge_get_modes()`, and some concerns about pm_runtime reference balancing.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread