Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/nouveau: lost plug/unplug bits in nouveau_connector_hpd() when pending - [Medium] drm/nouveau: nouveau_display_hpd_work() skips link check on IRQ with plug --
commit 8aa36f0d839e814a361fc038f601698183c285f9 Author: David R. Hedges <[email protected]> drm/nouveau: don't let an IRQ_HPD hide a plug or unplug event This commit updates nouveau_display_hpd_work() so that the nouveau_dp_link_check() shortcut is only taken when NVIF_CONN_EVENT_V0_IRQ is set without NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG. This prevents plug or unplug events queued alongside an IRQ_HPD from skipping connector detection. > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c > b/drivers/gpu/drm/nouveau/nouveau_display.c > index d71dcfc6ee66b..4398cf0fc541c 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -462,7 +462,12 @@ nouveau_display_hpd_work(struct work_struct *work) > !!(bits & NVIF_CONN_EVENT_V0_UNPLUG), > !!(bits & NVIF_CONN_EVENT_V0_IRQ)); > > - if (bits & NVIF_CONN_EVENT_V0_IRQ) { > + /* A plug or unplug event pending alongside the IRQ, such as a > + * sink count change reported by nouveau_dp_irq(), still needs > + * connector detection. > + */ > + if ((bits & NVIF_CONN_EVENT_V0_IRQ) && > + !(bits & (NVIF_CONN_EVENT_V0_PLUG | > NVIF_CONN_EVENT_V0_UNPLUG))) { [Severity: Medium] Can a separate plug or unplug event still be dropped if it arrives while an earlier NVIF_CONN_EVENT_V0_IRQ is already pending for the same connector? In drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_hpd(), updating nv_connector->hpd_pending is guarded by the drm->hpd_pending check: spin_lock_irqsave(&drm->hpd_lock, flags); if (!(drm->hpd_pending & mask)) { nv_connector->hpd_pending |= bits; drm->hpd_pending |= mask; schedule_work(&drm->hpd_work); } spin_unlock_irqrestore(&drm->hpd_lock, flags); If nouveau_connector_irq() -> nouveau_dp_irq() first queues NVIF_CONN_EVENT_V0_IRQ (before a sink count change is visible), and a plug or unplug event then arrives via nouveau_connector_hotplug() or a follow-up nouveau_dp_irq() before nouveau_display_hpd_work() finishes pm_runtime_get_sync() and clears drm->hpd_pending, drm->hpd_pending & mask is already set. The new NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG bit is never ORed into nv_connector->hpd_pending. When nouveau_display_hpd_work() then reads nv_connector->hpd_pending, bits still only contains NVIF_CONN_EVENT_V0_IRQ, so nouveau_dp_link_check() returns true when no link is trained and connector detection is still skipped. Should nv_connector->hpd_pending |= bits be moved outside the if (!(drm->hpd_pending & mask)) check in nouveau_connector_hpd()? > if (nouveau_dp_link_check(nv_connector)) > continue; > } [Severity: Medium] Does guarding the outer if statement instead of the continue statement skip DP link retraining when NVIF_CONN_EVENT_V0_IRQ is set alongside NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG? In drivers/gpu/drm/nouveau/nouveau_dp.c, nouveau_dp_link_check() -> nouveau_dp_link_check_locked() is the only HPD path that checks channel equalization on an active link (outp->dp.lt.nr != 0) and retrains it via nouveau_dp_train_link(outp, true): if (drm_dp_dpcd_read_phy_link_status(&outp->conn->aux, DP_PHY_DPRX, link_status) < 0) return false; if (drm_dp_channel_eq_ok(link_status, outp->dp.lt.nr)) return true; return nouveau_dp_train_link(outp, true); If a plug event runs first and nouveau_display_hpd_work() clears drm->hpd_pending = 0 before acquiring dev->mode_config.mutex, a subsequent IRQ_HPD in nouveau_dp_irq() -> nouveau_connector_hpd() sees !(drm->hpd_pending & mask), ORs NVIF_CONN_EVENT_V0_IRQ into nv_connector->hpd_pending alongside NVIF_CONN_EVENT_V0_PLUG, and reschedules drm->hpd_work. The same combination also occurs directly when nouveau_dp_irq() observes a sink count change on an active SST branch device while the link needs retraining. The first worker run sees both NVIF_CONN_EVENT_V0_IRQ and NVIF_CONN_EVENT_V0_PLUG in bits, clears nv_connector->hpd_pending = 0, and skips nouveau_dp_link_check(). Because drm_helper_probe_detect() -> nouveau_connector_detect() -> nouveau_dp_detect() does not check link status or retrain the link, and any rescheduled worker run sees bits == 0, the link check is lost. Would it be better to always run nouveau_dp_link_check(nv_connector) when NVIF_CONN_EVENT_V0_IRQ is set, and only guard the continue statement with !(bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG))? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
