From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/probe-helper: signal hotplug for force-enabled connectors on HPD
Date: Tue, 31 Mar 2026 17:46:44 +1000
Message-ID: <review-patch1-20260328152550.20222-1-miguelsedek@gmail.com>
In-Reply-To: <20260328152550.20222-1-miguelsedek@gmail.com>
References: <20260328152550.20222-1-miguelsedek@gmail.com> <20260328152550.20222-1-miguelsedek@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 s=
tate.

But this is not a universal DRM convention. The DRM core's own force handli=
ng lives in `drm_helper_probe_single_connector_modes()` (the full probe pat=
h at `drm_probe_helper.c:589-606`), which explicitly checks `connector->for=
ce` and overrides the status. The detect-only path called by `check_connect=
or_changed()` does **not** check `connector->force` =E2=80=94 it goes strai=
ght to the driver's `detect()` callback via `detect_connector_status()`. Wh=
ether a driver respects `connector->force` in its own detect callback is dr=
iver-specific behavior, not a DRM convention. Many drivers will return the =
actual physical status from detect(), meaning the epoch counter **would** c=
hange and this patch's code path would never trigger.

This makes the patch's effect inconsistent across drivers =E2=80=94 it only=
 fires when the driver happens to check `connector->force` in its detect ca=
llback.

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

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

Every HPD event on a forced connector will now trigger a uevent and potenti=
ally a full modeset. HPD lines can be noisy =E2=80=94 spurious events, sign=
al settling, etc. This could cause unnecessary modesetting storms. There's =
no debouncing or consideration of whether the link actually needs re-traini=
ng. The commit message's scenario (TV switching inputs) is valid, but the s=
olution 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 =3D> %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 p=
aths.

**5. Alternative approach to consider**

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

Modifying epoch counters outside of `drm_helper_probe_detect()` creates a s=
econd 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 pat=
h," but `check_connector_changed()` is used by the per-connector `drm_conne=
ctor_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 sam=
e issue but is not addressed.

---
Generated by Claude Code Patch Reviewer