Re: [PATCH 4/6] drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in nv50_disp_atomic_commit_core()

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