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/bridge: pass down IRQ_HPD to the drivers Date: Sat, 16 May 2026 11:25:50 +1000 Message-ID: In-Reply-To: <20260513-hpd-irq-events-v3-2-086857017f16@oss.qualcomm.com> References: <20260513-hpd-irq-events-v3-0-086857017f16@oss.qualcomm.com> <20260513-hpd-irq-events-v3-2-086857017f16@oss.qualcomm.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 This is the bulk of the mechanical plumbing =E2=80=94 extending `hpd_cb`, `= hpd_notify` (bridge funcs callback), `oob_hotplug_event` (connector funcs c= allback), and all their callers/implementers to take the extra `enum drm_co= nnector_status_extra` parameter. **Issue: Spurious whitespace change in ti-tfp410.c** ```c - static inline struct tfp410 * drm_connector_to_tfp410(struct drm_connector *connector) ``` A blank line between two `static inline` function definitions is removed. T= his is unrelated to the patch purpose and will create unnecessary merge noi= se. **Issue: `drm_bridge_hpd_notify()` hardcodes `DRM_CONNECTOR_NO_EXTRA_STATUS= `** In `drm_bridge.c`: ```c if (bridge->hpd_cb) - bridge->hpd_cb(bridge->hpd_data, status); + bridge->hpd_cb(bridge->hpd_data, status, DRM_CONNECTOR_NO_EXTRA_STATUS); ``` This is correct for this intermediate step since patch 3 renames this to `d= rm_bridge_hpd_notify_extra()` and makes it pass through. But it does mean p= atch 2 is not fully bisectable for any driver that wants to use `drm_bridge= _hpd_notify()` with extra status between patches 2 and 3. This is fine sinc= e no such caller exists at that point. **Observation: `drm_bridge_connector_handle_hpd` unconditionally sets `conn= ector->status`** ```c mutex_lock(&dev->mode_config.mutex); connector->status =3D status; mutex_unlock(&dev->mode_config.mutex); drm_bridge_connector_hpd_notify(connector, status, extra_status); drm_kms_helper_connector_hotplug_event(connector); ``` When an IRQ_HPD event arrives with `status =3D=3D connector_status_connecte= d` (which is what DP altmode sends), this will still set `connector->status= ` and fire `drm_kms_helper_connector_hotplug_event()`. This means every IRQ= _HPD also triggers a full connector hotplug re-detection cycle. For MST sid= eband messages that can be frequent, this could cause unnecessary work. Is = this intentional? The MSM driver's internal HPD path (via `DP_DP_IRQ_HPD_IN= T_MASK`) only queues `EV_IRQ_HPD_INT` without re-triggering the full hotplu= g cycle. Consider whether `drm_bridge_connector_handle_hpd` should skip the= status update and hotplug event when only `extra_status` is set (i.e., the= status hasn't actually changed). **Intel i915: extra_status is ignored** The `intel_dp_oob_hotplug_event()` function gains the `extra_status` parame= ter but doesn't use it. Looking at the current implementation, i915 only tr= acks connect/disconnect state transitions via `oob_hotplug_last_state`. If = an IRQ_HPD arrives with the same connection status, the `hpd_high !=3D test= _bit(...)` check will be false and the event will be silently dropped. This= means DP MST events via USB-C will be lost on i915. This may be intentiona= l for now (the cover letter says only MSM is wired up), but it's worth noti= ng as a gap =E2=80=94 i915 USB-C DP MST will still be broken. --- Generated by Claude Code Patch Reviewer