From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: Send per-connector hotplug events Date: Thu, 23 Apr 2026 07:22:52 +1000 Message-ID: In-Reply-To: <20260422-hot-plug-passup-v9-2-aef804255986@collabora.com> References: <20260422-hot-plug-passup-v9-0-aef804255986@collabora.com> <20260422-hot-plug-passup-v9-2-aef804255986@collabora.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 **Verdict: Looks good, with minor observations.** **Design approach =E2=80=94 correct and clean:** The key insight in v9 is decomposing `drm_kms_helper_hotplug_event()` (whic= h calls `drm_sysfs_hotplug_event()` + `drm_client_dev_hotplug()`) into its = two parts. The `drm_sysfs_connector_hotplug_event()` calls are safe to make= inside the `mode_config.mutex` since they only do `kobject_uevent_env()`, = while `drm_client_dev_hotplug()` is deferred to after the lock is released.= This avoids the deadlock from earlier versions and avoids needing a tempor= ary allocation for connector IDs. **`output_poll_execute` changes:** The poll path now calls `drm_sysfs_connector_hotplug_event(connector)` inli= ne within the loop (under the mutex), and defers the `drm_client_dev_hotplu= g(dev)` call to after `mutex_unlock()`: ```c + drm_sysfs_connector_hotplug_event(connector); changed =3D true; ``` and later: ```c + if (dev->mode_config.delayed_event) { + dev->mode_config.delayed_event =3D false; + changed =3D true; + drm_sysfs_hotplug_event(dev); + } + if (changed) - drm_kms_helper_hotplug_event(dev); + drm_client_dev_hotplug(dev); ``` This is correct. The `delayed_event` handling has been moved from before th= e lock acquisition to after the lock release in the `out:` label. This is a= subtle but important change: - **Observation 1 (minor):** In the original code, `delayed_event` was cons= umed before the `goto out` on `!drm_kms_helper_poll`, meaning a delayed eve= nt would still fire `drm_kms_helper_hotplug_event()`. In the new code, the = `delayed_event` check is at the `out:` label, so it's also reached from tha= t path =E2=80=94 behavior is preserved. Good. - **Observation 2 (minor):** In the original code, `delayed_event` was cons= umed before the `mutex_trylock` failure path (`goto out` with `repoll =3D t= rue`). In the new code, the same applies =E2=80=94 if the trylock fails, we= still check `delayed_event` at `out:`, which is correct and actually sligh= tly better since we don't lose the delayed event on trylock failure. In the= old code, a trylock failure after consuming `delayed_event` would fire the= hotplug event. The new code does the same via the `out:` label path. Corre= ct. - **Observation 3 (race on `delayed_event`):** The `delayed_event` flag is = read and cleared without holding the `mode_config.mutex` (it's in the `out:= ` block after the unlock). This is the same as the original code (which als= o read it before acquiring the lock), so it's not a regression. The flag is= set under the poll work context which is single-threaded, and races with `= drm_helper_probe_single_connector_modes` setting it are benign (worst case:= an extra poll cycle). Fine. **`drm_helper_hpd_irq_event` changes:** The old code tracked `first_changed_connector` and a `changed` count, then = dispatched either `drm_kms_helper_connector_hotplug_event()` for single-con= nector changes or `drm_kms_helper_hotplug_event()` for multi-connector chan= ges. The new code simply calls `drm_sysfs_connector_hotplug_event()` for ea= ch changed connector within the loop, then calls `drm_client_dev_hotplug()`= once after the lock is released: ```c if (check_connector_changed(connector)) { - if (!first_changed_connector) { - drm_connector_get(connector); - first_changed_connector =3D connector; - } - changed++; + changed =3D true; + drm_sysfs_connector_hotplug_event(connector); } ``` This is a nice simplification =E2=80=94 the `first_changed_connector` / ref= count dance is eliminated entirely. The `drm_sysfs_connector_hotplug_event(= )` call is safe under `mode_config.mutex` since it just does a `kobject_uev= ent_env()`. - **Observation 4 (behavioral change in multi-connector case):** Previously= , if 3 connectors changed simultaneously, userspace got 1 generic `HOTPLUG= =3D1` uevent (without `CONNECTOR=3D`). Now it gets 3 separate `HOTPLUG=3D1 = CONNECTOR=3D` uevents plus a single `drm_client_dev_hotplug()`. This is= intentional per the cover letter and is strictly better for userspace =E2= =80=94 more information, no loss of the old signal. Well-designed composito= rs already handle `CONNECTOR=3D` uevents. Older compositors that ignore `CO= NNECTOR=3D` still see `HOTPLUG=3D1` and re-probe everything, so backwards c= ompatibility is maintained. - **Observation 5 (multiple `drm_client_dev_hotplug` calls):** In both path= s, `drm_client_dev_hotplug()` is called exactly once if anything changed, w= hich is correct and avoids redundant fbdev/fbcon restores. **No issues found.** The patch is a clean improvement that simplifies the c= ode while providing better information to userspace. The locking split is c= orrect =E2=80=94 uevent delivery inside the lock, client notification outsi= de. --- Generated by Claude Code Patch Reviewer