Claude review: drm/bridge: display-connector: don't autoenable HPD IRQ

Claude review: drm/bridge: display-connector: don't autoenable HPD IRQ

[Claude Code Review Bot]

Patch Review

This patch adds `IRQF_NO_AUTOEN` to the IRQ request flags and implements `hpd_enable`/`hpd_disable` callbacks that call `enable_irq()`/`disable_irq()`.

**Looks correct.** The `hpd_enable`/`hpd_disable` callbacks are the intended mechanism per the `drm_bridge_funcs` documentation: bridges that set `DRM_BRIDGE_OP_HPD` should implement these. Using `IRQF_NO_AUTOEN` prevents the IRQ from firing before the DRM connector is fully set up, which is the bug being fixed.

One minor observation:

- The `hpd_enable`/`hpd_disable` callbacks are unconditionally wired into `display_connector_bridge_funcs`, but `conn->hpd_irq` is only valid (>= 0) when an HPD GPIO with IRQ capability was found. If `hpd_irq` is `-EINVAL` (no HPD GPIO, or IRQ request failed), calling `enable_irq(-EINVAL)` / `disable_irq(-EINVAL)` would be invalid. However, in practice, the `hpd_enable` callback is only invoked by `drm_bridge_hpd_enable()` when `DRM_BRIDGE_OP_HPD` is set in `bridge->ops`, and `DRM_BRIDGE_OP_HPD` is only set when `conn->hpd_irq >= 0`. So this is safe by design, but adding a guard like:

```c
if (conn->hpd_irq < 0)
    return;
```

would be defensive and make the contract clearer. Not strictly necessary though.

**Fixes tag looks appropriate** – the original commit introduced the auto-enabled IRQ.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/2] drm/bridge: display-connector: detect DP state if cable is plugged on boot

[Dmitry Baryshkov]

Unlike the USB-C AltMode handling and unlike the DP controller handling
the HPD on its own, if the display-connector is being used to handle the
HPD pin, DP controller might not be able to get either the HPD
notification nor detect the cable state if the cable is plugged in
before the system has booted.

Make the display-connector send the HPD notifications during system
startup for DisplayPort connectors.

It's typically not a problem for other types of display connectors since
there is no special procedure to detect if the monitor is actually
plugged in or not.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Changes in v2:
- Expanded commit message, explaining difference from HDMI (Laurent)
- Link to v1: https://lore.kernel.org/r/20260314-dp-connector-hpd-v1-0-786044cedc17@oss.qualcomm.com

---
Dmitry Baryshkov (2):
      drm/bridge: display-connector: don't autoenable HPD IRQ
      drm/bridge: display-connector: trigger initial HPD event for DP

 drivers/gpu/drm/bridge/display-connector.c | 36 ++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260314-dp-connector-hpd-f069e66bc6af

Best regards,
--  
With best wishes
Dmitry

[PATCH v2 1/2] drm/bridge: display-connector: don't autoenable HPD IRQ

[Dmitry Baryshkov]

If HPD IRQ is enabled in the display_connector's probe, it can be
triggered too early, before the DRM connector is completely setup. Use
the enable_hpd / disable_hpd callbacks to control enablement of the HPD
IRQ.

Fixes: 0c275c30176b ("drm/bridge: Add bridge driver for display connectors")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/bridge/display-connector.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 16c0631adeb1..6bb1134f75c3 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -87,6 +87,20 @@ display_connector_bridge_detect(struct drm_bridge *bridge, struct drm_connector
 	return display_connector_detect(bridge);
 }
 
+static void display_connector_hpd_enable(struct drm_bridge *bridge)
+{
+	struct display_connector *conn = to_display_connector(bridge);
+
+	enable_irq(conn->hpd_irq);
+}
+
+static void display_connector_hpd_disable(struct drm_bridge *bridge)
+{
+	struct display_connector *conn = to_display_connector(bridge);
+
+	disable_irq(conn->hpd_irq);
+}
+
 static const struct drm_edid *display_connector_edid_read(struct drm_bridge *bridge,
 							  struct drm_connector *connector)
 {
@@ -178,6 +192,8 @@ static u32 *display_connector_get_input_bus_fmts(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs display_connector_bridge_funcs = {
 	.attach = display_connector_attach,
 	.detect = display_connector_bridge_detect,
+	.hpd_enable = display_connector_hpd_enable,
+	.hpd_disable = display_connector_hpd_disable,
 	.edid_read = display_connector_edid_read,
 	.atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts,
 	.atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts,
@@ -307,6 +323,7 @@ static int display_connector_probe(struct platform_device *pdev)
 						NULL, display_connector_hpd_irq,
 						IRQF_TRIGGER_RISING |
 						IRQF_TRIGGER_FALLING |
+						IRQF_NO_AUTOEN |
 						IRQF_ONESHOT,
 						"HPD", conn);
 		if (ret) {

-- 
2.47.3

[PATCH v2 2/2] drm/bridge: display-connector: trigger initial HPD event for DP

[Dmitry Baryshkov]

If the DisplayPort drivers use display-connector for the HPD detection,
the internal HPD state machine might be not active and thus the hardware
might be not able to handle cable detection correctly. Instead it will
depend on the externall HPD notifications to set the cable state,
bypassing the internal HPD state machine (for example this is the case
for the msm DP driver).

However if the cable has been plugged before the HPD IRQ has been
enabled, there will be no HPD event coming. The drivers might fail
detection in such a case. Trigger the HPD notification after enabling
the HPD IRQ, propagating the cable insertion state.

Note, thia issue only affects drivers which set OP_HPD but not OP_DETECT
(like dp-connector). Here DP differs from HDMI. For HDMI there is no
additional state or extra "bridge with no sinks plugged" cases. The HPD
pin state is equalc to the display plugged state. Nor do we have a an
AUX bus with timeouts, etc.

Fixes: 2e2bf3a5584d ("drm/bridge: display-connector: add DP support")
Reported-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/bridge/display-connector.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 6bb1134f75c3..f7a5bfd9c075 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -25,6 +26,8 @@ struct display_connector {
 
 	struct regulator	*supply;
 	struct gpio_desc	*ddc_en;
+
+	struct work_struct	hpd_work;
 };
 
 static inline struct display_connector *
@@ -92,15 +95,29 @@ static void display_connector_hpd_enable(struct drm_bridge *bridge)
 	struct display_connector *conn = to_display_connector(bridge);
 
 	enable_irq(conn->hpd_irq);
+
+	if (conn->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		schedule_work(&conn->hpd_work);
 }
 
 static void display_connector_hpd_disable(struct drm_bridge *bridge)
 {
 	struct display_connector *conn = to_display_connector(bridge);
 
+	if (conn->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		cancel_work_sync(&conn->hpd_work);
+
 	disable_irq(conn->hpd_irq);
 }
 
+static void display_connector_hpd_work(struct work_struct *work)
+{
+	struct display_connector *conn = container_of(work, struct display_connector, hpd_work);
+	struct drm_bridge *bridge = &conn->bridge;
+
+	drm_bridge_hpd_notify(bridge, display_connector_detect(bridge));
+}
+
 static const struct drm_edid *display_connector_edid_read(struct drm_bridge *bridge,
 							  struct drm_connector *connector)
 {
@@ -395,6 +412,8 @@ static int display_connector_probe(struct platform_device *pdev)
 		conn->bridge.ops |= DRM_BRIDGE_OP_DETECT;
 	if (conn->hpd_irq >= 0)
 		conn->bridge.ops |= DRM_BRIDGE_OP_HPD;
+	if (conn->hpd_irq >= 0 && type == DRM_MODE_CONNECTOR_DisplayPort)
+		INIT_WORK(&conn->hpd_work, display_connector_hpd_work);
 
 	dev_dbg(&pdev->dev,
 		"Found %s display connector '%s' %s DDC bus and %s HPD GPIO (ops 0x%x)\n",

-- 
2.47.3

Claude review: drm/bridge: display-connector: detect DP state if cable is plugged on boot

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: display-connector: detect DP state if cable is plugged on boot
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 3
Reviewed: 2026-05-16T11:28:15.749364

---

This is a well-targeted 2-patch series from Dmitry Baryshkov fixing a real boot-time HPD detection problem for DisplayPort connectors using the `display-connector` bridge driver. The core issue: if a DP cable is plugged in before the system boots, the HPD GPIO line is already asserted, so no edge interrupt fires, and the DP controller never learns about the connected display.

The approach is sound: (1) defer IRQ enablement from probe to the `hpd_enable` bridge callback, and (2) schedule a work item to send an initial HPD notification for DP connectors. The work_struct is correctly used to avoid a mutex deadlock — `drm_bridge_hpd_enable()` holds `bridge->hpd_mutex` when calling `hpd_enable`, and `drm_bridge_hpd_notify()` also takes that same mutex, so calling it directly from `hpd_enable` would deadlock.

Both patches are clean, minimal, and correctly ordered. A couple of minor issues noted below.

**Verdict: Looks good, minor nits only.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: display-connector: don't autoenable HPD IRQ

[Claude Code Review Bot]

Patch Review

**Purpose:** Prevent the HPD IRQ from firing before the DRM connector is fully set up by using `IRQF_NO_AUTOEN` and controlling enablement via `hpd_enable`/`hpd_disable` bridge callbacks.

**Analysis:**

The change is straightforward and correct. Adding `IRQF_NO_AUTOEN` to `devm_request_threaded_irq()`:

```c
IRQF_TRIGGER_RISING |
IRQF_TRIGGER_FALLING |
IRQF_NO_AUTOEN |
IRQF_ONESHOT,
```

ensures the IRQ starts disabled. The new callbacks:

```c
static void display_connector_hpd_enable(struct drm_bridge *bridge)
{
	struct display_connector *conn = to_display_connector(bridge);
	enable_irq(conn->hpd_irq);
}

static void display_connector_hpd_disable(struct drm_bridge *bridge)
{
	struct display_connector *conn = to_display_connector(bridge);
	disable_irq(conn->hpd_irq);
}
```

are only invoked when `DRM_BRIDGE_OP_HPD` is set in `bridge->ops`, which only happens when `conn->hpd_irq >= 0` (line 413-414), so `conn->hpd_irq` is always valid when these callbacks run.

The `enable_irq`/`disable_irq` are called under `bridge->hpd_mutex` (held by `drm_bridge_hpd_enable`/`drm_bridge_hpd_disable`). If a threaded IRQ fires immediately after `enable_irq()` and calls `drm_bridge_hpd_notify()`, that will block on the mutex in a separate thread context — no deadlock, just a brief wait. The callback `hpd_cb` is already set before `hpd_enable` is called (drm_bridge.c:1459-1460), so the notification will be properly delivered.

The Fixes tag referencing the original commit that introduced the driver is appropriate.

**No issues found.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: display-connector: trigger initial HPD event for DP

[Claude Code Review Bot]

Patch Review

**Purpose:** After enabling HPD, schedule a work item to send the current cable status for DP connectors, so that cables plugged before boot are detected.

**Analysis:**

The work_struct approach is the correct design. Calling `drm_bridge_hpd_notify()` directly from `hpd_enable` would deadlock on `bridge->hpd_mutex`. The deferred work runs in system workqueue context where the mutex is not held:

```c
static void display_connector_hpd_work(struct work_struct *work)
{
	struct display_connector *conn = container_of(work, struct display_connector, hpd_work);
	struct drm_bridge *bridge = &conn->bridge;
	drm_bridge_hpd_notify(bridge, display_connector_detect(bridge));
}
```

`display_connector_detect()` calls `gpiod_get_value_cansleep()`, which is fine from workqueue context (can sleep).

The ordering in `hpd_disable` is correct — cancel the work before disabling the IRQ:

```c
if (conn->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
	cancel_work_sync(&conn->hpd_work);
disable_irq(conn->hpd_irq);
```

`cancel_work_sync` ensures the work has either completed or been cancelled before proceeding, preventing a use-after-disable scenario.

The conditional `INIT_WORK` in probe:

```c
if (conn->hpd_irq >= 0 && type == DRM_MODE_CONNECTOR_DisplayPort)
	INIT_WORK(&conn->hpd_work, display_connector_hpd_work);
```

is consistent with the conditional usage in `hpd_enable`/`hpd_disable` — both check `conn->bridge.type == DRM_MODE_CONNECTOR_DisplayPort`.

**Minor concern — teardown safety:** `display_connector_remove()` does not call `cancel_work_sync()`. This relies on the DRM framework calling `drm_bridge_hpd_disable()` (which calls `hpd_disable` → `cancel_work_sync`) before the bridge is removed. This is the expected teardown order and is how other bridge drivers work, so it's acceptable. Adding a defensive cancel in `remove()` would be difficult anyway since the work is only initialized for DP connectors.

**Commit message typos:**

```
Note, thia issue only affects drivers which set OP_HPD but not OP_DETECT
```
"thia" → "this"

```
The HPD pin state is equalc to the display plugged state.
```
"equalc" → "equal"

**No functional issues found. Two trivial typos in the commit message.**

---
Generated by Claude Code Patch Reviewer