> 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. > +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(). > + 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: 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
