Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] drm/nouveau: uncancelled link_bad_work and irq_work in nouveau_display_fini() - [Medium] drm/nouveau: async link_bad_work in nouveau_display_relink() races with hotplug - [Medium] drm/nouveau: nouveau_display_relink() leaves BAD link_status after recovery - [Medium] drm/nouveau: nv50_outp_atomic_check() fails drm_atomic_helper_resume() commits - [Medium] drm/nouveau: nv50_disp_atomic_commit_tail() enables audio on aborted SOR enable - [Medium] drm/nouveau: nv50_pior_create() does not set nv_connector->dp_encoder for DP - [Medium] drm/nouveau/dp: check active and detected DP encoder before relinking --
commit 87d2a7cb9667f947ddf67914d19f058419e967fd Author: David R. Hedges <[email protected]> drm/nouveau/dp: restart an active link when the sink reasserts HPD This commit restarts active DisplayPort links with a full CRTC modeset when the sink signals a plug or unplug HPD event and remains connected, bounded by a rate limit of five restarts within ten-second intervals. > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c > b/drivers/gpu/drm/nouveau/nouveau_display.c > index c5e914602baff..a52db61299626 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c [ ... ] > @@ -443,11 +471,18 @@ nouveau_display_relink(struct drm_connector *connector) > if (ret) > goto out; > > - if (crtc->state->active) { > - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n", > - connector->base.id, connector->name); > - ret = drm_atomic_helper_reset_crtc(crtc, &ctx); > + if (!crtc->state->active) > + goto out; > + > + if (!counted) { > + if (!nouveau_display_relink_allowed(connector)) > + goto out; > + counted = true; > } > + > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n", > + connector->base.id, connector->name); > + ret = drm_atomic_helper_reset_crtc(crtc, &ctx); [Severity: Medium] What happens if connector->state->link_status was previously set to DRM_MODE_LINK_STATUS_BAD (for example after nv50_sor_atomic_enable() or an earlier nouveau_display_relink() failed when the monitor briefly advertised reduced capabilities) and a subsequent HPD event triggers nouveau_display_relink() once full link capabilities are restored? In drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_relink(), drm_atomic_helper_reset_crtc() duplicates connector->state via nouveau_conn_atomic_duplicate_state() without resetting link_status to DRM_MODE_LINK_STATUS_GOOD and without cancelling any pending link_bad_work. Could a successful link restart in nouveau_display_relink() leave connector->state->link_status stuck at DRM_MODE_LINK_STATUS_BAD when userspace processes the hotplug event? [Severity: Medium] Can scheduling link_bad_work asynchronously in nouveau_display_relink() race with the hotplug event sent by nouveau_display_hpd_work()? In drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_relink(): drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); /* The sink may now advertise less than the mode needs. */ if (ret == -EINVAL) schedule_work(&nouveau_connector(connector)->link_bad_work); When a DP sink asserts HPD with reduced capabilities that cannot carry the current CRTC mode, drm_atomic_helper_reset_crtc() fails nv50_outp_atomic_check() with -EINVAL. nouveau_display_relink() drops dev->mode_config.connection_mutex and schedules link_bad_work, and then nouveau_display_hpd_work() immediately sends the hotplug event: if (relink) { drm_connector_list_iter_begin(dev, &conn_iter); nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) { if (relink & drm_connector_mask(connector)) nouveau_display_relink(connector); } drm_connector_list_iter_end(&conn_iter); } if (changed == 1) drm_kms_helper_connector_hotplug_event(first_changed_connector); else if (changed > 0) drm_kms_helper_hotplug_event(dev); Because link_bad_work runs asynchronously in drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_link_bad(): drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD); drm_kms_helper_connector_hotplug_event(connector); userspace can receive the initial hotplug event from nouveau_display_hpd_work() before connector->state->link_status has been set to DRM_MODE_LINK_STATUS_BAD. Furthermore, if userspace or an in-kernel DRM client completes a valid modeset (or a subsequent HPD event retrains the link) before link_bad_work executes, wouldn't nouveau_connector_link_bad() still overwrite connector->state->link_status with DRM_MODE_LINK_STATUS_BAD on the working link? Would it avoid this race to update connector->state->link_status directly while nouveau_display_relink() already holds dev->mode_config.connection_mutex in ctx? > @@ -510,6 +545,14 @@ nouveau_display_hpd_work(struct work_struct *work) > !!(bits & NVIF_CONN_EVENT_V0_UNPLUG), > !!(bits & NVIF_CONN_EVENT_V0_IRQ)); > > + /* If a DP sink dropped HPD while we were driving it, it may > have > + * reset its end of the link whatever its link status says, and > + * userspace won't modeset again if nothing else has changed. > + */ > + if (nv_connector->dp_encoder && > + (bits & (NVIF_CONN_EVENT_V0_PLUG | > NVIF_CONN_EVENT_V0_UNPLUG))) > + modeset = true; [Severity: Medium] On Dual-Mode DisplayPort (DP++) connectors, both a DCB_OUTPUT_DP encoder and a DCB_OUTPUT_TMDS encoder are attached to the same struct nouveau_connector with nv_connector->dp_encoder set to the DP encoder in nv50_sor_create(). If a passive HDMI/DVI (TMDS) sink is connected on a DP++ port, or if a user swaps between a native DP sink and a passive DP++ TMDS adapter while the CRTC is active, nouveau_display_hpd_work() sets modeset = true based only on nv_connector->dp_encoder being non-NULL, and nouveau_display_relink() calls drm_atomic_helper_reset_crtc() before userspace receives the hotplug event: nouveau_display_hpd_work() -> nouveau_display_relink() -> drm_atomic_helper_reset_crtc() Because drm_atomic_helper_reset_crtc() only adds the CRTC to the atomic state before drm_atomic_helper_check_modeset() runs update_connector_routing(), new_connector_state->best_encoder remains old_connector_state->best_encoder. When driving a passive TMDS sink, every HPD plug/unplug event triggers an unnecessary full modeset; when switching between DP and TMDS sinks on the same connector, drm_atomic_helper_reset_crtc() runs against the stale best_encoder (either failing nv50_outp_atomic_check() with -EINVAL and marking link-status BAD, or committing the wrong encoder protocol for the newly detected sink). Should nouveau_display_hpd_work() and nouveau_display_relink() verify that both nv_connector->detected_encoder and connector->state->best_encoder match nv_connector->dp_encoder before restarting the link? [Severity: Medium] In the earlier patch in this series, commit bef9030b7486 ("drm/nouveau/kms: cache the DP encoder after it has been attached"), the find_encoder() lookup for nv_connector->dp_encoder was removed from nouveau_connector_create() and added to nv50_sor_create(): drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_sor_create() { if (dcbe->type == DCB_OUTPUT_DP) { mutex_init(&nv_encoder->dp.hpd_irq_lock); nv_connector->dp_encoder = nv_encoder; ... } } However, drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_pior_create() also creates DCB_OUTPUT_DP encoders for external DisplayPort encoders driven via PIOR on NV50-family GPUs: drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_pior_create() { switch (dcbe->type) { ... case DCB_OUTPUT_DP: aux = nvkm_i2c_aux_find(i2c, nv_encoder->outp.info.dp.aux); ddc = aux ? &aux->i2c : NULL; type = DRM_MODE_ENCODER_TMDS; break; default: return -ENODEV; } nv_encoder->i2c = ddc; mutex_init(&nv_encoder->dp.hpd_irq_lock); } Without setting nv_connector->dp_encoder = nv_encoder in nv50_pior_create(), won't nv_connector->dp_encoder remain NULL on PIOR DP connectors, causing both nouveau_dp_link_check() and this new HPD relink check in nouveau_display_hpd_work() to be skipped? [Severity: High] With nouveau_display_hpd_work() now calling nouveau_display_relink() on DP plug and unplug events across all GPUs (which schedules nv_connector->link_bad_work when drm_atomic_helper_reset_crtc() returns -EINVAL), how are nv_connector->link_bad_work and nv_connector->irq_work synchronized during suspend and driver teardown in nouveau_display_fini()? Looking at drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_fini(): /* disable hotplug interrupts */ drm_connector_list_iter_begin(dev, &conn_iter); nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) { struct nouveau_connector *conn = nouveau_connector(connector); nvif_event_block(&conn->irq); nvif_event_block(&conn->hpd); } drm_connector_list_iter_end(&conn_iter); if (!runtime && !drm->headless) cancel_work_sync(&drm->hpd_work); conn->irq_work and conn->link_bad_work are only cancelled later in drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_destroy(): nvif_event_dtor(&nv_connector->irq); nvif_event_dtor(&nv_connector->hpd); cancel_work_sync(&nv_connector->irq_work); cancel_work_sync(&nv_connector->link_bad_work); This appears to leave two regressions: 1. During system suspend (nouveau_display_suspend() -> nouveau_display_fini()), neither conn->irq_work nor conn->link_bad_work is cancelled. If conn->irq_work was queued just before nvif_event_block(), nouveau_dp_irq() can run during suspend and re-schedule drm->hpd_work after cancel_work_sync(&drm->hpd_work) has returned. Similarly, if drm->hpd_work or nv50_sor_atomic_enable() queued conn->link_bad_work, nouveau_connector_link_bad() can run after drm_atomic_helper_suspend() and nouveau_display_fini() have completed, mutating connector->state and sending hotplug events on a suspended device. 2. During driver unload, nouveau_display_destroy() calls drm_mode_config_cleanup(), which destroys and frees all drm_encoder instances (nv50_sor_destroy(), nv50_pior_destroy()) before destroying connectors in nouveau_connector_destroy(). If conn->irq_work is still running during drm_mode_config_cleanup(), nouveau_dp_irq() can access a freed nouveau_encoder and its destroyed hpd_irq_lock, and re-schedule drm->hpd_work after nouveau_display_fini() already cancelled it. Should nouveau_display_fini() cancel conn->irq_work before cancel_work_sync(&drm->hpd_work) and cancel conn->link_bad_work after it? [Severity: Medium] In commit e5f61700807a ("drm/nouveau/kms/nv50-: reject DP modesets the link can't carry") in this series, nv50_outp_atomic_check() was updated to reject modesets when nv50_outp_atomic_fix_depth() returns false: drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_outp_atomic_check() { if (!nv50_outp_atomic_fix_depth(encoder, crtc_state) && (crtc_state->mode_changed || crtc_state->connectors_changed)) return -EINVAL; } During system resume, nouveau_display_resume() calls drm_atomic_helper_resume() before nouveau_display_hpd_resume(). drm_atomic_helper_resume() resets mode_config state (setting crtc->state->enable = false) and commits the state saved by drm_atomic_helper_suspend() (which has state->duplicated == true). If a DP sink was powered off or disconnected prior to resume (so nouveau_dp_probe_dpcd() zeroed outp->dp.link_nr and outp->dp.link_bw, or cached reduced capabilities) while its CRTC had enable == true when suspended (for example in DPMS off with active == false, or after nv50_sor_atomic_enable() aborted on -EINVAL), drm_atomic_helper_check_modeset() sees old_crtc_state->enable != new_crtc_state->enable and sets mode_changed = true and connectors_changed = true even when new_crtc_state->active is false. Unlike nv50_msto_atomic_check(), which guards its check with !state->duplicated, won't nv50_outp_atomic_check() return -EINVAL here and cause drm_atomic_helper_resume() to fail the entire multi-CRTC resume commit? [Severity: Medium] In commit 9c72b9cc42ad ("drm/nouveau/kms/nv50-: don't enable a DP output with no trained link") in this series, nv50_sor_atomic_enable() returns early when nouveau_dp_train() returns -EINVAL: drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_sor_atomic_enable() { case DCB_OUTPUT_DP: if (nouveau_dp_train(nv_encoder, false, mode->clock, asyh->or.bpc) == -EINVAL) { schedule_work(&nv_connector->link_bad_work); return; } } However, its caller in drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_disp_atomic_commit_tail() still unconditionally marks outp->enabled = true: list_for_each_entry(outp, &atom->outp, head) { ... if (outp->set.mask) { help->atomic_enable(encoder, state); outp->enabled = true; interlock[NV50_DISP_INTERLOCK_CORE] = 1; } } When nv50_disp_atomic_commit_core() then runs: list_for_each_entry(outp, &atom->outp, head) { if (outp->encoder->encoder_type != DRM_MODE_ENCODER_DPMST) { struct nouveau_encoder *nv_encoder = nouveau_encoder(outp->encoder); if (outp->enabled) { nv50_audio_enable(outp->encoder, nouveau_crtc(nv_encoder->crtc), nv_encoder->conn, NULL, NULL); outp->enabled = outp->disabled = false; } doesn't this call nv50_audio_enable() (invoking nvif_outp_hda_eld() and notifying the ALSA audio component) on an untrained, detached SOR whenever the connector's cached display_info.has_audio is true? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
