> -----Original Message----- > From: Kandpal, Suraj <[email protected]> > Sent: 07 July 2026 11:55 > To: Golani, Mitulkumar Ajitkumar <[email protected]>; > [email protected] > Cc: [email protected]; Shankar, Uma <[email protected]>; > Lee, Shawn C <[email protected]>; Vehmanen, Kai > <[email protected]>; Nautiyal, Ankit K <[email protected]>; > Borah, Chaitanya Kumar <[email protected]> > Subject: RE: [PATCH v1] drm/i915/audio: treat UHBR SST like MST for > cpu_transcoder signalling > > > Subject: [PATCH v1] drm/i915/audio: treat UHBR SST like MST for > > cpu_transcoder signalling > > > > Hi Mitul, > > Thanks for the patch. The fix is in correct direction but I think both the > commit message and the code need rework before this can go in. > > > UHBR SST uses the same 128b/132b transport as DP MST, so its audio > > also lives on a meaningful cpu_transcoder (>= 0), not the -1 used for > > legacy SST. Treating it as Non-MST made pin_eld_notify() signal -1 and > > made > > find_audio_state() skip the per-transcoder entry, so after > > suspend/resume or replug the ELD lookup failed and audio went silent. > > > > Detect UHBR SST alongside DP MST when deciding how to address audio > > state, in both the pin_eld_notify() signalling and the find_audio_state() > lookup paths. > > > > Signed-off-by: Kai Vehmanen <[email protected]> > > Signed-off-by: Mitul Golani <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_audio.c | 67 > > ++++++++++++++++++---- > > 1 file changed, 55 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c > > b/drivers/gpu/drm/i915/display/intel_audio.c > > index 9729f1837d2c..4586bf981255 100644 > > --- a/drivers/gpu/drm/i915/display/intel_audio.c > > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > > @@ -37,6 +37,7 @@ > > #include "intel_crtc.h" > > #include "intel_de.h" > > #include "intel_display_types.h" > > +#include "intel_dp.h" > > #include "intel_display_wa.h" > > #include "intel_lpe_audio.h" > > > > @@ -696,6 +697,42 @@ static void ibx_audio_codec_enable(struct > > intel_encoder *encoder, > > mutex_unlock(&display->audio.mutex); > > } > > > > +/* > > + * 128b/132b transport is used for both DP MST and UHBR SST. As far > > +as audio > > + * is concerned the hardware behaves identically in both cases: the > > +port can > > + * carry multiple streams and the cpu_transcoder is a meaningful (>= > > +0, > > + * possibly > 0) identifier of the audio stream on that port. Legacy > > +8b/10b > > + * SST instead carries a single stream per port, for which the audio > > +drivers > > + * expect the cpu_transcoder to be signalled as -1. > > + */ > > > DP MST and 128b/132b are independent: > > MST is a topology feature - multi-stream via MTPs / payload IDs. It runs over > 8b/10b at HBR/HBR2/HBR3 and has done so on virtually every MST hub > shipped to date. > 128b/132b (UHBR) is a link-coding feature from DP 2.0/2.1 and can carry > either SST or MST. > They only overlap at UHBR MST. The commit message along with comment > mentioned here fold them into same thing, which will confuse the next > reader who touches this code. > Also, the comment says the criterion is "128b/132b" but the code is "MST OR > UHBR" (MST-over-HBR3 returns true here). Pick one - I think "MST OR UHBR" > is what you actually want, so please fix the comment. > right. Will fix the commit message and comment block to describe the actual audio-addressing requirement with next revision. > > +static bool intel_audio_has_mst_transcoder(const struct > > +intel_crtc_state *crtc_state) { > > name is misleading. UHBR SST is > single-stream by definition - calling its transcoder an "MST transcoder" bakes > a false concept into the code. Please rename maybe > intel_audio_needs_cpu_transcoder_id(). Agreed. UHBR SST is not MST by definition. Will rename to intel_audio_needs_cpu_transcoder_id() to reflect the actual semantics. > > > + return intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) || > > + intel_dp_is_uhbr(crtc_state); } > > + > > +/* > > + * Same as intel_audio_has_mst_transcoder(), but derived from a > > +stored encoder > > + * for which only the (legacy) crtc pointer is available. DP MST is > > +detected > > + * from the encoder type without dereferencing the crtc, so that the > > +MST path > > + * keeps working exactly as before; UHBR SST is detected from the > > +active crtc > > + * state of the port encoder. > > + */ > > +static bool intel_audio_encoder_has_mst_transcoder(struct > > +intel_encoder > > +*encoder) { > > + struct intel_crtc *crtc; > > + > > + if (encoder->type == INTEL_OUTPUT_DP_MST) > > + return true; > > + > > + if (!encoder->base.crtc) > > + return false; > > + > > + crtc = to_intel_crtc(encoder->base.crtc); > > + > > + return intel_dp_is_uhbr(crtc->config); } > > + > > This is unsafe. It's called from find_audio_state(), which runs from the > audio- > component path under display->audio.mutex only, but > encoder->base.crtc and crtc->config must be accessed under drm_modeset > locks. That's exactly why display->audio.state[cpu_transcoder] is cached in > the first place - see intel_acomp_get_config() which deliberately only touches > the cached ELD. It's also racy vs. a re-modeset flipping the link rate between > store and lookup, so the same entry can be classified UHBR on store and non- > UHBR on lookup. > > Don't derive this from live state. Store it once at codec_enable time (where > crtc_state is fully locked) on struct intel_audio_state: > Regards, Mitul Valid concern. Will eliminate the live crtc access entirely by caching the classification as a bool needs_cpu_transcoder_id in struct intel_audio_state at intel_audio_codec_enable() time (where crtc_state is fully locked), and clear it in intel_audio_codec_disable(). The intel_audio_encoder_has_mst_transcoder() helper will be removed. find_audio_state() will then read only cached state under display->audio.mutex, consistent with how encoder and eld are already handled. > struct intel_audio_state { > struct intel_encoder *encoder; > u8 eld[MAX_ELD_BYTES]; > bool needs_cpu_transcoder_id; /* MST, or SST on 128b/132b */ > }; > > set in intel_audio_codec_enable(): > > audio_state->needs_cpu_transcoder_id = > intel_audio_needs_cpu_transcoder_id(crtc_state); > > and then find_audio_state() has no live-state access at all. > > > bool intel_audio_compute_config(struct intel_encoder *encoder, > > struct intel_crtc_state *crtc_state, > > struct drm_connector_state *conn_state) > @@ > > -769,8 +806,8 @@ void intel_audio_codec_enable(struct intel_encoder > > *encoder, > > > > if (acomp && acomp->base.audio_ops && > > acomp->base.audio_ops->pin_eld_notify) { > > - /* audio drivers expect cpu_transcoder = -1 to indicate Non- > > MST cases */ > > - if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) > > + /* audio drivers expect cpu_transcoder = -1 to indicate Non- > > MST/HBR cases */ > > + if (!intel_audio_has_mst_transcoder(crtc_state)) > > cpu_transcoder = -1; > > acomp->base.audio_ops->pin_eld_notify(acomp- > > >base.audio_ops->audio_ptr, > > (int)port, > > (int)cpu_transcoder); @@ -825,8 +862,8 @@ void > > intel_audio_codec_disable(struct intel_encoder *encoder, > > > > if (acomp && acomp->base.audio_ops && > > acomp->base.audio_ops->pin_eld_notify) { > > - /* audio drivers expect cpu_transcoder = -1 to indicate Non- > > MST cases */ > > - if (!intel_crtc_has_type(old_crtc_state, > > INTEL_OUTPUT_DP_MST)) > > + /* audio drivers expect cpu_transcoder = -1 to indicate Non- > > MST/HBR cases */ > > + if (!intel_audio_has_mst_transcoder(old_crtc_state)) > > cpu_transcoder = -1; > > acomp->base.audio_ops->pin_eld_notify(acomp- > > >base.audio_ops->audio_ptr, > > Please drop this hunk (and the symmetric one in codec_disable). Boot already > works with notify = -1 for UHBR SST, so the notify code doesn't need to > change to fix the reported bug - only the lookup does. > Doing both at once is a wider change than needed and blurs what's being > fixed. > > > (int)port, > > (int)cpu_transcoder); @@ -1119,17 +1156,23 @@ static int > > intel_audio_component_get_cdclk_freq(struct device *kdev) > > > > /* > > * get the intel audio state according to the parameter port and > > cpu_transcoder > > - * MST & (cpu_transcoder >= 0): return the > > audio.state[cpu_transcoder].encoder], > > + * > > + * A "MST transcoder" below means 128b/132b transport, i.e. either DP > > + MST or > > + * UHBR SST, both of which use a meaningful (>= 0) cpu_transcoder to > > + identify > > + * the audio stream on a port (see intel_audio_has_mst_transcoder()): > > + * > > + * MST transcoder & (cpu_transcoder >= 0): return the > > + audio.state[cpu_transcoder], > > * when port is matched > > - * MST & (cpu_transcoder < 0): this is invalid > > - * Non-MST & (cpu_transcoder >= 0): only cpu_transcoder = 0 (the > > first device > > entry) > > - * will get the right intel_encoder with port matched > > - * Non-MST & (cpu_transcoder < 0): get the right intel_encoder with > > port matched > > + * MST transcoder & (cpu_transcoder < 0): this is invalid > > + * Non-MST transcoder & (cpu_transcoder >= 0): only cpu_transcoder = > > + 0 (the > > first > > + * device entry) will get the right intel_encoder with port matched > > + * Non-MST transcoder & (cpu_transcoder < 0): get the right > > + intel_encoder > > with > > + * port matched > > */ > > static struct intel_audio_state *find_audio_state(struct intel_display > *display, > > int port, int cpu_transcoder) > { > > - /* MST */ > > + /* MST or UHBR SST */ > > if (cpu_transcoder >= 0) { > > struct intel_audio_state *audio_state; > > struct intel_encoder *encoder; > > @@ -1142,7 +1185,7 @@ static struct intel_audio_state > > *find_audio_state(struct intel_display *display, > > encoder = audio_state->encoder; > > > > if (encoder && encoder->port == port && > > - encoder->type == INTEL_OUTPUT_DP_MST) > > + intel_audio_encoder_has_mst_transcoder(encoder)) > > return audio_state; > > With the stored flag suggested above, this becomes: > if (encoder && encoder->port == port && > audio_state->needs_cpu_transcoder_id) > return audio_state; > > > > } > > > > @@ -1158,7 +1201,7 @@ static struct intel_audio_state > > *find_audio_state(struct intel_display *display, > > encoder = audio_state->encoder; > > > > if (encoder && encoder->port == port && > > - encoder->type != INTEL_OUTPUT_DP_MST) > > + !intel_audio_encoder_has_mst_transcoder(encoder)) > > With the stored flag suggested above, this becomes: > if (encoder && encoder->port == port && > !audio_state->needs_cpu_transcoder_id) > return audio_state; > > > return audio_state; > > } > > According to me this fix can be made smaller like this, > > struct intel_audio_state { > struct intel_encoder *encoder; > u8 eld[MAX_ELD_BYTES]; > bool needs_cpu_transcoder_id; /* MST, or SST on 128b/132b */ > }; > > In intel_audio_codec_enable(): > audio_state->encoder = encoder; > audio_state->needs_cpu_transcoder_id = > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) || > intel_dp_is_uhbr(crtc_state); > memcpy(audio_state->eld, crtc_state->eld, sizeof(audio_state->eld)); > > In find_audio_state(), We can do this instead od the DP_MST checks: > if (cpu_transcoder >= 0) { > ... > if (encoder && encoder->port == port && > audio_state->needs_cpu_transcoder_id) > return audio_state; > } > ... > /* cpu_transcoder < 0 loop */ > if (encoder && encoder->port == port && > !audio_state->needs_cpu_transcoder_id) > return audio_state; > > Regards, > Suraj Kandpal > > > > > -- > > 2.48.1
RE: [PATCH v1] drm/i915/audio: treat UHBR SST like MST for cpu_transcoder signalling
Golani, Mitulkumar Ajitkumar Wed, 08 Jul 2026 01:33:42 -0700
- [PATCH v1] drm/i915/audio: treat UHBR SST lik... Mitul Golani
- ✗ i915.CI.BAT: failure for drm/i915/audi... Patchwork
- RE: ✗ i915.CI.BAT: failure for drm/i... Golani, Mitulkumar Ajitkumar
- ✓ i915.CI.BAT: success for drm/i915/audi... Patchwork
- RE: [PATCH v1] drm/i915/audio: treat UHB... Kandpal, Suraj
- RE: [PATCH v1] drm/i915/audio: treat... Golani, Mitulkumar Ajitkumar
