[PATCH 0/6] drm/nouveau/kms/nv50-: Obliterate nv_encoder->crtc

[PATCH 0/6] drm/nouveau/kms/nv50-: Obliterate nv_encoder->crtc

[Lyude Paul]

This is mostly a leftover artifact from the pre-atomic days, and while
we've been using it for a while now - it isn't great. Mostly because having
redundant state tracking for things atomic already keeps track of is pretty
much always error prone, as anyone working on nouveau who isn't already
very well versed in atomic modesetting isn't going to realize this isn't
the right way to see what CRTC is assigned to an encoder.

Also - this patch series does fix an actual bug (patches 1-3).

Lyude Paul (6):
  drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own
    function
  drm/nouveau/kms/nv50-: Add nv50_outp_get_old_crtc()
  drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in
    nv50_sor_atomic_disable()
  drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in
    nv50_disp_atomic_commit_core()
  drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc
  drm/nouveau/kms/nv50-: Obliterate nouveau_encoder->crtc

 drivers/gpu/drm/nouveau/dispnv50/disp.c   | 83 ++++++++++++++++-------
 drivers/gpu/drm/nouveau/nouveau_encoder.h |  4 +-
 2 files changed, 60 insertions(+), 27 deletions(-)


base-commit: 0e118b936dc5904cf0d9859882a40c96057b083d
-- 
2.55.0

[PATCH 1/6] drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own function

[Lyude Paul]

Besides using state->dev to access the nouveau_drm device again, there
should be no functional changes here.

Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org> # v5.12+
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 39 +++++++++++++++++--------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 2c66e480b5116..a885394f7cb92 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1561,24 +1561,39 @@ nv50_sor_update(struct nouveau_encoder *nv_encoder, u8 head,
  * the panel backlight has been shut off? Intel doesn't seem to do this, and uses a
  * fixed time delay from the vbios…
  */
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
+static inline void
+nv50_sor_atomic_disable_backlight(struct nouveau_drm *drm,
+				  struct nouveau_encoder *nv_encoder,
+				  struct drm_atomic_commit *state)
+{
+	struct nouveau_connector *nv_connector;
+	struct nouveau_backlight *backlight;
+	int ret;
+
+	nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
+	if (drm_WARN_ON(drm->dev, !nv_connector))
+		return;
+	backlight = nv_connector->backlight;
+
+	if (!backlight || !backlight->uses_dpcd)
+		return;
+
+	ret = drm_edp_backlight_disable(&nv_connector->aux, &backlight->edp_info);
+	if (ret < 0)
+		NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n",
+			 nv_connector->base.base.id, nv_connector->base.name, ret);
+}
+#endif
+
 static void
 nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
 {
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
 	struct nv50_head *head = nv50_head(nv_encoder->crtc);
-#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
-	struct nouveau_connector *nv_connector = nv50_outp_get_old_connector(state, nv_encoder);
-	struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
-	struct nouveau_backlight *backlight = nv_connector->backlight;
-	struct drm_dp_aux *aux = &nv_connector->aux;
-	int ret;
 
-	if (backlight && backlight->uses_dpcd) {
-		ret = drm_edp_backlight_disable(aux, &backlight->edp_info);
-		if (ret < 0)
-			NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n",
-				 nv_connector->base.base.id, nv_connector->base.name, ret);
-	}
+#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
+	nv50_sor_atomic_disable_backlight(nouveau_drm(state->dev), nv_encoder, state);
 #endif
 
 	if (nv_encoder->dcb->type == DCB_OUTPUT_TMDS && nv_encoder->hdmi.enabled) {
-- 
2.55.0

Re: [PATCH 1/6] drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling the sleeping function `drm_edp_backlight_disable()` from within the `nv50_sor_atomic_disable` encoder atomic disable callback violates DRM atomic context rules. This problem wasn't introduced by this patch, but the logic was moved into the new `nv50_sor_atomic_disable_backlight()` inline function.
--

--- Patch [1]: [PATCH 1/6] drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own function ---
commit a8a2b3da8fed6e6d5dafbcd1c4a9bb461b74def0
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own function

This commit moves the DPCD backlight disable logic into its own separate
inline helper function. Aside from using the state device to access the
drm device, no functional changes are introduced.

Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org> # v5.12+

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
[ ... ]
> +#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
> +static inline void
> +nv50_sor_atomic_disable_backlight(struct nouveau_drm *drm,
> +				  struct nouveau_encoder *nv_encoder,
> +				  struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (!backlight || !backlight->uses_dpcd)
> +		return;
> +
> +	ret = drm_edp_backlight_disable(&nv_connector->aux, &backlight->edp_info);

[Severity: High]
This is a pre-existing issue, but moving the logic into
nv50_sor_atomic_disable_backlight() preserves a potentially problematic
sleeping call in an atomic context.

Is it safe to call drm_edp_backlight_disable() from this path?

Because nv50_sor_atomic_disable_backlight() is invoked by the encoder atomic
disable callback nv50_sor_atomic_disable(), it executes in an atomic context
where sleeping is forbidden.

The drm_edp_backlight_disable() helper performs DPCD transactions over the
AUX channel. These transactions require taking a hardware mutex and can
sleep or block while waiting for hardware responses.

Could this lead to scheduling while atomic kernel warnings or potential
deadlocks during display disables or modesets?

> +	if (ret < 0)
> +		NV_ERROR(drm, "Failed to disable backlight on [CONNECTOR:%d:%s]: %d\n",
> +			 nv_connector->base.base.id, nv_connector->base.name, ret);
> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818235637.1738055-1-lyude@redhat.com?part=1

Re: [PATCH 1/6] drm/nouveau/kms/nv50-: Move DPCD backlight disable into its own function

[lyude]

omg

On Wed, 2026-08-19 at 00:07 +0000, sashiko-bot@kernel.org wrote:
> 
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> [ ... ]
> > +#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
> > +static inline void
> > +nv50_sor_atomic_disable_backlight(struct nouveau_drm *drm,
> > +				  struct nouveau_encoder
> > *nv_encoder,
> > +				  struct drm_atomic_commit *state)
> > +{
> [ ... ]
> > +	if (!backlight || !backlight->uses_dpcd)
> > +		return;
> > +
> > +	ret = drm_edp_backlight_disable(&nv_connector->aux,
> > &backlight->edp_info);
> 
> [Severity: High]
> This is a pre-existing issue, but moving the logic into
> nv50_sor_atomic_disable_backlight() preserves a potentially
> problematic
> sleeping call in an atomic context.
> 
> Is it safe to call drm_edp_backlight_disable() from this path?
> 
> Because nv50_sor_atomic_disable_backlight() is invoked by the encoder
> atomic
> disable callback nv50_sor_atomic_disable(), it executes in an atomic
> context
> where sleeping is forbidden.
> 
> The drm_edp_backlight_disable() helper performs DPCD transactions
> over the
> AUX channel. These transactions require taking a hardware mutex and
> can
> sleep or block while waiting for hardware responses.
> 
> Could this lead to scheduling while atomic kernel warnings or
> potential
> deadlocks during display disables or modesets?

This one gave me a good chuckle. This is a definite hallucination, it
is getting confused by the fact that nv50_sor_atomic_disable() has the
word atomic in it.

> 
> > +	if (ret < 0)
> > +		NV_ERROR(drm, "Failed to disable backlight on
> > [CONNECTOR:%d:%s]: %d\n",
> > +			 nv_connector->base.base.id, nv_connector-
> > >base.name, ret);
> > +}
> > +#endif

[PATCH 2/6] drm/nouveau/kms/nv50-: Add nv50_outp_get_old_crtc()

[Lyude Paul]

Does what it says on the label, unused for now.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org> # v5.12+
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index a885394f7cb92..152361f7feb42 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -459,6 +459,22 @@ nv50_outp_get_old_connector(struct drm_atomic_commit *state, struct nouveau_enco
 	return NULL;
 }
 
+static struct nouveau_crtc * __maybe_unused
+nv50_outp_get_old_crtc(const struct drm_atomic_commit *state, const struct nouveau_encoder *outp)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	const u32 mask = drm_encoder_mask(&outp->base.base);
+	int i;
+
+	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->encoder_mask & mask)
+			return nouveau_crtc(crtc);
+	}
+
+	return NULL;
+}
+
 static struct nouveau_crtc *
 nv50_outp_get_new_crtc(const struct drm_atomic_commit *state, const struct nouveau_encoder *outp)
 {
-- 
2.55.0

[PATCH 3/6] drm/nouveau/kms/nv50-: Stop using nv_encoder->crtc in nv50_sor_atomic_disable()

[Lyude Paul]

This is a leftover CRTC bookkeeping variable from the pre-atomic days.
While it works in most situations, it's prone to breaking - as pointed out
by Marek Czernohous.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reported-by: Marek Czernohous <marek@czernohous.de>
Fixes: f575f2bdb6c3 ("drm/nouveau/kms/nv50-: Remove (nv_encoder->crtc) checks in ->disable callbacks")
Cc: <stable@vger.kernel.org> # v5.12+
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 152361f7feb42..f19820dc055ae 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -459,7 +459,7 @@ nv50_outp_get_old_connector(struct drm_atomic_commit *state, struct nouveau_enco
 	return NULL;
 }
 
-static struct nouveau_crtc * __maybe_unused
+static struct nouveau_crtc *
 nv50_outp_get_old_crtc(const struct drm_atomic_commit *state, const struct nouveau_encoder *outp)
 {
 	struct drm_crtc *crtc;
@@ -1606,12 +1606,18 @@ static void
 nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state)
 {
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
-	struct nv50_head *head = nv50_head(nv_encoder->crtc);
+	struct nouveau_crtc *nv_crtc;
+	struct nv50_head *head;
 
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
 	nv50_sor_atomic_disable_backlight(nouveau_drm(state->dev), nv_encoder, state);
 #endif
 
+	nv_crtc = nv50_outp_get_old_crtc(state, nv_encoder);
+	if (drm_WARN_ON(state->dev, !nv_crtc))
+		return;
+	head = nv50_head(&nv_crtc->base);
+
 	if (nv_encoder->dcb->type == DCB_OUTPUT_TMDS && nv_encoder->hdmi.enabled) {
 		nvif_outp_hdmi(&nv_encoder->outp, head->base.index,
 			       false, 0, 0, 0, false, false, false);
-- 
2.55.0

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

[Lyude Paul]

Another leftover spot where we still use nv_encoder->crtc. Get rid of it
and do the right thing: get the currently assigned CRTC from the new atomic
state.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reported-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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;
 
 			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) {
-- 
2.55.0

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

[sashiko-bot]

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 <lyude@redhat.com>

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/20260818235637.1738055-1-lyude@redhat.com?part=4

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

[lyude]

This is wrong and I only just noticed it before getting ready to finish
up work - whether we need to use the new or old state depends on if
we're enabling or disabling - otherwise we'll end up with an unexpected
!nv_crtc

Will send a respin of this tomorrow

On Tue, 2026-08-18 at 19:48 -0400, Lyude Paul wrote:
> Another leftover spot where we still use nv_encoder->crtc. Get rid of
> it
> and do the right thing: get the currently assigned CRTC from the new
> atomic
> state.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reported-by: Marek Czernohous <marek@czernohous.de>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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;
>  
>  			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) {

[PATCH 5/6] drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc

[Lyude Paul]

This is the only spot in nouveau where we do actually need to keep track of
the currently assigned CRTC for an encoder, and it ideally should be
happening outside of the modesetting locks.

So in preparation for obliterating nouveau_encoder->crtc, let's just add a
variable into nouveau_encoder->audio for tracking this and use it instead
of nouveau_encoder->crtc. This makes it a lot more obvious that this is
only intended for state-tracking for audio.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c   | 4 +++-
 drivers/gpu/drm/nouveau/nouveau_encoder.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 63d554e97fb62..3bfc04f7ef602 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -636,7 +636,7 @@ nv50_audio_component_get_eld(struct device *kdev, int port, int dev_id,
 
 		nv_encoder = nouveau_encoder(encoder);
 		nv_connector = nv_encoder->conn;
-		nv_crtc = nouveau_crtc(nv_encoder->crtc);
+		nv_crtc = nv_encoder->audio.crtc;
 
 		if (!nv_crtc || nv_encoder->outp.or.id != port || nv_crtc->index != dev_id)
 			continue;
@@ -757,6 +757,7 @@ nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc)
 	mutex_lock(&drm->audio.lock);
 	if (nv_encoder->audio.enabled) {
 		nv_encoder->audio.enabled = false;
+		nv_encoder->audio.crtc = NULL;
 		nvif_outp_hda_eld(&nv_encoder->outp, nv_crtc->index, NULL, 0);
 	}
 	mutex_unlock(&drm->audio.lock);
@@ -781,6 +782,7 @@ nv50_audio_enable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc,
 	nvif_outp_hda_eld(&nv_encoder->outp, nv_crtc->index, nv_connector->base.eld,
 			  drm_eld_size(nv_connector->base.eld));
 	nv_encoder->audio.enabled = true;
+	nv_encoder->audio.crtc = nv_crtc;
 
 	mutex_unlock(&drm->audio.lock);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 4422c6185d498..647322ac1c8df 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -62,6 +62,7 @@ struct nouveau_encoder {
 	/* Protected by nouveau_drm.audio.lock */
 	struct {
 		bool enabled;
+		struct nouveau_crtc *crtc;
 	} audio;
 
 	struct drm_display_mode mode;
-- 
2.55.0

Re: [PATCH 5/6] drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PIOR encoders do not clear `audio.crtc` on disable, leading to stale audio state being reported.
--

--- Patch [5]: [PATCH 5/6] drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc ---
commit d00b281328e092a25d08552356c0a7fc11880073
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc

This commit adds an audio.crtc pointer to track the assigned CRTC for an
encoder outside of modesetting locks. It replaces the usage of
nouveau_encoder->crtc in the audio ELD retrieval path.

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 63d554e97fb62..3bfc04f7ef602 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -636,7 +636,7 @@ nv50_audio_component_get_eld(struct device *kdev, int port, int dev_id,
>  
>  		nv_encoder = nouveau_encoder(encoder);
>  		nv_connector = nv_encoder->conn;
> -		nv_crtc = nouveau_crtc(nv_encoder->crtc);
> +		nv_crtc = nv_encoder->audio.crtc;
>  
>  		if (!nv_crtc || nv_encoder->outp.or.id != port || nv_crtc->index != dev_id)
>  			continue;

[Severity: Medium]
Will this cause stale audio state to be reported when PIOR encoders are
disabled?

Before this patch, checking the cleared crtc pointer successfully skipped
the disabled encoder. However, it looks like nv50_pior_atomic_disable()
only clears the old crtc pointer, but not the new audio.crtc pointer:

nv50_pior_atomic_disable()
{
	...
	core->func->pior->ctrl(core, nv_encoder->outp.or.id, ctrl, NULL);
	nv_encoder->crtc = NULL;
}

Does nv_encoder->audio.crtc also need to be cleared here, or does
nv50_audio_disable() need to be called in nv50_pior_atomic_disable() to
prevent userspace from receiving stale ELD data for a deactivated display?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818235637.1738055-1-lyude@redhat.com?part=5

Re: [PATCH 5/6] drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc

[lyude]

This is also a hallucination. We don't have any kind of audio support
for PIOR.

On Wed, 2026-08-19 at 00:10 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] PIOR encoders do not clear `audio.crtc` on disable,
> leading to stale audio state being reported.
> --
> 
> --- Patch [5]: [PATCH 5/6] drm/nouveau/kms/nv50-: Add
> nouveau_encoder->audio.crtc ---
> commit d00b281328e092a25d08552356c0a7fc11880073
> Author: Lyude Paul <lyude@redhat.com>
> 
> drm/nouveau/kms/nv50-: Add nouveau_encoder->audio.crtc
> 
> This commit adds an audio.crtc pointer to track the assigned CRTC for
> an
> encoder outside of modesetting locks. It replaces the usage of
> nouveau_encoder->crtc in the audio ELD retrieval path.
> 
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index 63d554e97fb62..3bfc04f7ef602 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -636,7 +636,7 @@ nv50_audio_component_get_eld(struct device
> > *kdev, int port, int dev_id,
> >  
> >  		nv_encoder = nouveau_encoder(encoder);
> >  		nv_connector = nv_encoder->conn;
> > -		nv_crtc = nouveau_crtc(nv_encoder->crtc);
> > +		nv_crtc = nv_encoder->audio.crtc;
> >  
> >  		if (!nv_crtc || nv_encoder->outp.or.id != port ||
> > nv_crtc->index != dev_id)
> >  			continue;
> 
> [Severity: Medium]
> Will this cause stale audio state to be reported when PIOR encoders
> are
> disabled?
> 
> Before this patch, checking the cleared crtc pointer successfully
> skipped
> the disabled encoder. However, it looks like
> nv50_pior_atomic_disable()
> only clears the old crtc pointer, but not the new audio.crtc pointer:
> 
> nv50_pior_atomic_disable()
> {
> 	...
> 	core->func->pior->ctrl(core, nv_encoder->outp.or.id, ctrl,
> NULL);
> 	nv_encoder->crtc = NULL;
> }
> 
> Does nv_encoder->audio.crtc also need to be cleared here, or does
> nv50_audio_disable() need to be called in nv50_pior_atomic_disable()
> to
> prevent userspace from receiving stale ELD data for a deactivated
> display?
> 
> [ ... ]

[PATCH 6/6] drm/nouveau/kms/nv50-: Obliterate nouveau_encoder->crtc

[Lyude Paul]

This is mostly a leftover artifact from the pre-atomic days, and while
we've been using it for a while now - it isn't great. Mostly because having
redundant state tracking for things atomic already keeps track of is pretty
much always error prone, as anyone working on nouveau who isn't already
very well versed in atomic modesetting isn't going to realize this isn't
the right way to see what CRTC is assigned to an encoder.

Now that we've removed the only legitimate user (DRM audio) and all the
illegitimate ones, let's obliterate it.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c   | 8 --------
 drivers/gpu/drm/nouveau/nouveau_encoder.h | 3 ---
 2 files changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 3bfc04f7ef602..9ab342ebed180 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -502,7 +502,6 @@ nv50_dac_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *s
 	const u32 ctrl = NVDEF(NV507D, DAC_SET_CONTROL, OWNER, NONE);
 
 	core->func->dac->ctrl(core, nv_encoder->outp.or.id, ctrl, NULL);
-	nv_encoder->crtc = NULL;
 }
 
 static void
@@ -532,8 +531,6 @@ nv50_dac_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
 
 	core->func->dac->ctrl(core, nv_encoder->outp.or.id, ctrl, asyh);
 	asyh->or.depth = 0;
-
-	nv_encoder->crtc = &nv_crtc->base;
 }
 
 static enum drm_connector_status
@@ -1634,7 +1631,6 @@ nv50_sor_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *s
 
 	nv_encoder->update(nv_encoder, head->base.index, NULL, 0, 0);
 	nv50_audio_disable(encoder, &head->base);
-	nv_encoder->crtc = NULL;
 }
 
 // common/inc/displayport/displayport.h
@@ -1802,7 +1798,6 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
 	u8 depth = NV837D_SOR_SET_CONTROL_PIXEL_DEPTH_DEFAULT;
 
 	nv_connector = nv50_outp_get_new_connector(state, nv_encoder);
-	nv_encoder->crtc = &nv_crtc->base;
 
 	if ((disp->disp->object.oclass == GT214_DISP ||
 	     disp->disp->object.oclass >= GF110_DISP) &&
@@ -2013,7 +2008,6 @@ nv50_pior_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *
 	const u32 ctrl = NVDEF(NV507D, PIOR_SET_CONTROL, OWNER, NONE);
 
 	core->func->pior->ctrl(core, nv_encoder->outp.or.id, ctrl, NULL);
-	nv_encoder->crtc = NULL;
 }
 
 static void
@@ -2058,7 +2052,6 @@ nv50_pior_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *s
 	}
 
 	core->func->pior->ctrl(core, nv_encoder->outp.or.id, ctrl, asyh);
-	nv_encoder->crtc = &nv_crtc->base;
 }
 
 static const struct drm_encoder_helper_funcs
@@ -2797,7 +2790,6 @@ nv50_display_read_hw_or_state(struct drm_device *dev, struct nv50_disp *disp,
 	armh->state.enable = true;
 	pm_runtime_get_noresume(dev->dev);
 
-	outp->crtc = crtc;
 	outp->ctrl = NVVAL(NV507D, SOR_SET_CONTROL, PROTOCOL, proto) | BIT(crtc->index);
 
 	drm_connector_get(conn);
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 647322ac1c8df..6dcdfd048a35d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -54,9 +54,6 @@ struct nouveau_encoder {
 
 	struct i2c_adapter *i2c;
 
-	/* different to drm_encoder.crtc, this reflects what's
-	 * actually programmed on the hw, not the proposed crtc */
-	struct drm_crtc *crtc;
 	u32 ctrl;
 
 	/* Protected by nouveau_drm.audio.lock */
-- 
2.55.0