public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD
@ 2026-03-28 15:25 Miguel Sedek
  2026-03-29 12:40 ` Miguel Sedek
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Miguel Sedek @ 2026-03-28 15:25 UTC (permalink / raw)
  To: dri-devel; +Cc: maarten.lankhorst, mripard, tzimmermann, Miguel Sedek

When a connector has DRM_FORCE_ON (set via video=CONNECTOR:e),
check_connector_changed() calls the driver's detect() callback.
Well-behaved drivers (e.g. NVIDIA's open kernel module) check
connector->force and return connector_status_connected regardless
of actual hardware state. This means the epoch counter never
increments and no uevent reaches userspace.

However, the HPD interrupt itself indicates a physical state
change on the link. A common scenario is an HDMI-connected TV
switching to built-in apps (Netflix, etc.), which causes the TV's
HDMI receiver to stop listening to TMDS signals. When the TV
returns to the HDMI input, the GPU needs to re-negotiate the link,
but no component in the stack detects this because the forced
status masks the physical change.

Add an epoch counter increment for force-enabled connectors after
detect() returns. This ensures the hotplug uevent reaches
userspace compositors so they can trigger a modeset for link
re-training.

Note that detect() is still called normally before the epoch
bump. This preserves driver-side effects like scrambler
re-initialization on vc4/i915. This approach differs from a
prior patch [1] which skipped detect() entirely for forced
connectors and was rejected due to regression concerns on
scrambler re-init.

The software polling path (output_poll_execute) already skips
forced connectors with an explicit guard. This patch addresses
only the hardware HPD IRQ path, which was previously unguarded.

[1] https://patchwork.kernel.org/project/dri-devel/patch/20220826091121.389315-1-mrodin@de.adit-jv.com/

Tested with:
- GPU: NVIDIA RTX 5070 Ti (nvidia-open 595.58.03)
- Display: Samsung 75" 4K TV via HDMI-A-1 (force-enabled)
- Kernel: 6.19.10 (CachyOS)
- Compositor: KWin 6.6.3 (Wayland)

Signed-off-by: Miguel Sedek <miguelsedek@gmail.com>
---
 drm_probe_helper.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drm_probe_helper.c b/drm_probe_helper.c
index 92f2176..b7d60e2 100644
--- a/drm_probe_helper.c
+++ b/drm_probe_helper.c
@@ -984,6 +984,23 @@ static bool check_connector_changed(struct drm_connector *connector)
 	old_epoch_counter = connector->epoch_counter;
 	connector->status = drm_helper_probe_detect(connector, NULL, false);
 
+	/*
+	 * For force-enabled connectors, the driver's detect() returns the
+	 * forced status, so the epoch counter never increments. But an HPD
+	 * event on a forced connector still indicates a physical link state
+	 * change (e.g. a TV switching inputs). Signal the change to
+	 * userspace so the compositor can trigger a modeset for link
+	 * re-training. Note: detect() is still called above so driver-side
+	 * effects (scrambler re-init, etc.) are preserved.
+	 */
+	if (connector->force && old_epoch_counter == connector->epoch_counter) {
+		connector->epoch_counter += 1;
+		drm_dbg_kms(dev,
+			    "[CONNECTOR:%d:%s] HPD on force-enabled connector, signaling link re-train\n",
+			    connector->base.id, connector->name);
+		return true;
+	}
+
 	if (old_epoch_counter == connector->epoch_counter) {
 		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Same epoch counter %llu\n",
 			    connector->base.id,
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD
  2026-03-28 15:25 [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD Miguel Sedek
@ 2026-03-29 12:40 ` Miguel Sedek
  2026-03-31  7:46 ` Claude review: " Claude Code Review Bot
  2026-03-31  7:46 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Miguel Sedek @ 2026-03-29 12:40 UTC (permalink / raw)
  To: dri-devel; +Cc: maarten.lankhorst, mripard, tzimmermann, Miguel Sedek

Withdrawing this patch. The root cause turned out to be a userspace
script running SimulateUserActivity in a loop, which prevented DPMS
from ever cycling. With DPMS working correctly, the HDMI link
re-negotiates on wake without any kernel changes needed.

Apologies for the noise.

Miguel Sedek

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/probe-helper: signal hotplug for force-enabled connectors on HPD
  2026-03-28 15:25 [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD Miguel Sedek
  2026-03-29 12:40 ` Miguel Sedek
@ 2026-03-31  7:46 ` Claude Code Review Bot
  2026-03-31  7:46 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-03-31  7:46 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/probe-helper: signal hotplug for force-enabled connectors on HPD
Author: Miguel Sedek <miguelsedek@gmail.com>
Patches: 2
Reviewed: 2026-03-31T17:46:44.640883

---

This is a single patch that forces an epoch counter bump for force-enabled connectors on the HPD IRQ path, so that userspace receives a uevent even though the connector status doesn't change. The problem is real — a forced connector can mask physical link state changes — but the patch has several issues with the approach and patch formatting that need addressing.

The diff header is malformed (missing `drivers/gpu/drm/` prefix), the fix is arguably too broad (every HPD on any forced connector will trigger a modeset, including spurious events), and the premise relies on driver-specific behavior that isn't universal.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/probe-helper: signal hotplug for force-enabled connectors on HPD
  2026-03-28 15:25 [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD Miguel Sedek
  2026-03-29 12:40 ` Miguel Sedek
  2026-03-31  7:46 ` Claude review: " Claude Code Review Bot
@ 2026-03-31  7:46 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-03-31  7:46 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**1. Malformed diff path**

The diff header shows:

```
diff --git a/drm_probe_helper.c b/drm_probe_helper.c
```

This should be `a/drivers/gpu/drm/drm_probe_helper.c`. This means `git am` will fail to apply the patch. The author likely ran `git diff` from inside `drivers/gpu/drm/` rather than from the repository root.

**2. The premise is driver-specific, not universal**

The commit message states:

> Well-behaved drivers (e.g. NVIDIA's open kernel module) check connector->force and return connector_status_connected regardless of actual hardware state.

But this is not a universal DRM convention. The DRM core's own force handling lives in `drm_helper_probe_single_connector_modes()` (the full probe path at `drm_probe_helper.c:589-606`), which explicitly checks `connector->force` and overrides the status. The detect-only path called by `check_connector_changed()` does **not** check `connector->force` — it goes straight to the driver's `detect()` callback via `detect_connector_status()`. Whether a driver respects `connector->force` in its own detect callback is driver-specific behavior, not a DRM convention. Many drivers will return the actual physical status from detect(), meaning the epoch counter **would** change and this patch's code path would never trigger.

This makes the patch's effect inconsistent across drivers — it only fires when the driver happens to check `connector->force` in its detect callback.

**3. Unconditional epoch bump on every HPD is too aggressive**

```c
if (connector->force && old_epoch_counter == connector->epoch_counter) {
    connector->epoch_counter += 1;
    ...
    return true;
}
```

Every HPD event on a forced connector will now trigger a uevent and potentially a full modeset. HPD lines can be noisy — spurious events, signal settling, etc. This could cause unnecessary modesetting storms. There's no debouncing or consideration of whether the link actually needs re-training. The commit message's scenario (TV switching inputs) is valid, but the solution is a blanket that catches all HPD events.

**4. Early return skips existing debug logging**

The patch returns `true` before reaching the existing code at line 999 that logs the epoch counter change:

```c
drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Changed epoch counter %llu => %llu\n", ...);
```

While the patch adds its own debug print, it uses different format/wording, making debug log analysis inconsistent between the forced and non-forced paths.

**5. Alternative approach to consider**

Since this is really about the nvidia-open driver's `detect()` honoring `connector->force`, it might be more appropriate to either:
- Fix the nvidia driver to not blindly return forced status from its `detect()` callback (the DRM core handles forcing elsewhere), or
- Have `detect_connector_status()` itself understand forced connectors if this is truly meant to be a core behavior.

Modifying epoch counters outside of `drm_helper_probe_detect()` creates a second code path for epoch management, which is fragile.

**6. Minor: the fix should probably also apply to `drm_helper_hpd_irq_event()`**

The commit message says "This patch addresses only the hardware HPD IRQ path," but `check_connector_changed()` is used by the per-connector `drm_connector_helper_hpd_irq_event()`. The multi-connector `drm_helper_hpd_irq_event()` (line 807) has its own inline detect+epoch logic and would have the same issue but is not addressed.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-03-31  7:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-28 15:25 [PATCH] drm/probe-helper: signal hotplug for force-enabled connectors on HPD Miguel Sedek
2026-03-29 12:40 ` Miguel Sedek
2026-03-31  7:46 ` Claude review: " Claude Code Review Bot
2026-03-31  7:46 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox