Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unconditional CRTC lookup and NULL check bypasses the output disable path, leaking hardware resources and causing spurious warnings. --
--- Patch [4]: [PATCH 4/6] drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in nv50_disp_atomic_commit_core() --- commit 3c40fb984475628cc9b548d32d5295add5289647 Author: Lyude Paul <[email protected]> drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in nv50_disp_atomic_commit_core() This commit replaces direct access to nv_encoder->crtc with a proper lookup from the new atomic state in the core commit function. It adds a warning and continues the loop if the new CRTC is not found. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index f19820dc055ae..63d554e97fb62 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -2171,10 +2171,14 @@ nv50_disp_atomic_commit_core(struct drm_atomic_commit > *state, u32 *interlock) > 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); > + struct nouveau_crtc *nv_crtc = > nv50_outp_get_new_crtc(state, nv_encoder); > + > + if (drm_WARN_ON(drm->dev, !nv_crtc)) > + continue; [Severity: High] When an output is being disabled (for instance, when a monitor is unplugged), wouldn't nv50_outp_get_new_crtc() correctly return NULL here since the encoder is no longer assigned to a CRTC? If this unconditionally triggers the drm_WARN_ON and early return via the continue statement, it appears it will completely bypass the cleanup path below. Could this skip the call to nvif_outp_release(&nv_encoder->outp), leaking the hardware resource and generating a spurious warning every time a display is disabled? > > if (outp->enabled) { > - nv50_audio_enable(outp->encoder, > nouveau_crtc(nv_encoder->crtc), > - nv_encoder->conn, NULL, NULL); > + nv50_audio_enable(outp->encoder, nv_crtc, > nv_encoder->conn, NULL, > + NULL); > outp->enabled = outp->disabled = false; > } else { > if (outp->disabled) { [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
