> Subject: [PATCH v2] drm/i915/audio: treat UHBR SST like MST for
> cpu_transcoder signalling
>
> Both DP MST and UHBR SST require the audio component to use a real
> cpu_transcoder (>= 0) value to identify the audio stream on a port.
> For DP MST this was already handled; for UHBR SST it was not, causing
> pin_eld_notify() to signal cpu_transcoder = -1 (the legacy SST sentinel) and
> find_audio_state() to skip the per-transcoder entry for the UHBR stream.
>
> After suspend/resume or replug, the ELD lookup fails and audio goes silent.
> Fix
> by detecting UHBR SST alongside DP MST when deciding whether a real
> cpu_transcoder value must be used for audio state addressing, in both the
> pin_eld_notify() signalling and the
> find_audio_state() lookup paths.
>
> --v2:
> - Commit message and comment update. (Suraj)
> - Change function name has_mst_transcoder. (Suraj)
> - Avoid Live crtc access in find_audio_state. (Suraj)
>
> Signed-off-by: Kai Vehmanen <[email protected]>
> Signed-off-by: Mitul Golani <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 61 ++++++++++++++-----
> .../gpu/drm/i915/display/intel_display_core.h | 1 +
> 2 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> b/drivers/gpu/drm/i915/display/intel_audio.c
> index 9729f1837d2c..7a2bc73744a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -38,6 +38,7 @@
> #include "intel_de.h"
> #include "intel_display_types.h"
> #include "intel_display_wa.h"
> +#include "intel_dp.h"
> #include "intel_lpe_audio.h"
>
> /**
> @@ -696,6 +697,21 @@ 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
I think you need to mention DP MST UHBR and UHBR SST or else it gives the
impression
That MST only uses 128b/132b always.
I recommend we can remove this whole comment altogether.
> + * 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.
> + */
> +static
> +bool intel_audio_needs_cpu_transcoder_id(const struct intel_crtc_state
> +*crtc_state) {
> + return intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) ||
> + intel_dp_is_uhbr(crtc_state);
> +}
> +
> bool intel_audio_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state *conn_state) @@
> -762,6 +778,8 @@ void intel_audio_codec_enable(struct intel_encoder
> *encoder,
> audio_state = &display->audio.state[cpu_transcoder];
>
> audio_state->encoder = encoder;
> + audio_state->needs_cpu_transcoder_id =
> + intel_audio_needs_cpu_transcoder_id(crtc_state);
> BUILD_BUG_ON(sizeof(audio_state->eld) != sizeof(crtc_state->eld));
> memcpy(audio_state->eld, crtc_state->eld, sizeof(audio_state->eld));
>
> @@ -769,8 +787,12 @@ 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. MST and UHBR SST are addressed by
> + * a real cpu_transcoder.
> + */
> + if (!intel_audio_needs_cpu_transcoder_id(crtc_state))
> cpu_transcoder = -1;
> acomp->base.audio_ops->pin_eld_notify(acomp-
> >base.audio_ops->audio_ptr,
> (int)port,
> (int)cpu_transcoder); @@ -819,14 +841,19 @@ void
> intel_audio_codec_disable(struct intel_encoder *encoder,
> audio_state = &display->audio.state[cpu_transcoder];
>
> audio_state->encoder = NULL;
> + audio_state->needs_cpu_transcoder_id = false;
> memset(audio_state->eld, 0, sizeof(audio_state->eld));
>
> mutex_unlock(&display->audio.mutex);
>
> 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. MST and UHBR SST are addressed by
> + * a real cpu_transcoder.
> + */
> + if (!intel_audio_needs_cpu_transcoder_id(old_crtc_state))
> cpu_transcoder = -1;
> acomp->base.audio_ops->pin_eld_notify(acomp-
> >base.audio_ops->audio_ptr,
> (int)port,
> (int)cpu_transcoder); @@ -1118,18 +1145,24 @@ 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],
> + * Get the intel audio state according to the parameter port and
> + cpu_transcoder
> + *
> + * 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_needs_cpu_transcoder_id()):
> + *
> + * 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
> */
I think this above comment saying MST transcoder means 128/132b encoding is
wrong
You can replace the this chunk with
* Get the intel audio state for a given (port, cpu_transcoder).
*
* Streams are addressed either by a real cpu_transcoder (DP MST and UHBR SST,
* i.e. entries whose stored needs_cpu_transcoder_id is true) or by port alone
* (legacy SST). Both the signalling side (pin_eld_notify()) and the lookup
* side use the same predicate, so the two are symmetric.
*
* cpu_transcoder >= 0 & needs_cpu_transcoder_id: return
audio.state[cpu_transcoder]
* when the port matches.
* cpu_transcoder < 0 & !needs_cpu_transcoder_id: return the first
port-matching
* entry.
* cpu_transcoder = 0 & !needs_cpu_transcoder_id: falls through to the
port-only
* loop so the first device entry of a legacy SST port is still found.
*/
Giving a more realistic picture of what we are doing now
Rest looks good to me
With above fixed LGTM,
Reviewed-by: Suraj Kandpal <[email protected]>
> 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,11 +1175,11 @@ 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)
> + audio_state->needs_cpu_transcoder_id)
> return audio_state;
> }
>
> - /* Non-MST */
> + /* Legacy SST. */
> if (cpu_transcoder > 0)
> return NULL;
>
> @@ -1158,7 +1191,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)
> + !audio_state->needs_cpu_transcoder_id)
> return audio_state;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 17f7d3abdb9c..a6129d0b523f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -97,6 +97,7 @@ struct intel_wm_funcs { struct intel_audio_state {
> struct intel_encoder *encoder;
> u8 eld[MAX_ELD_BYTES];
> + bool needs_cpu_transcoder_id; /* MST, or SST on UHBR link */
> };
>
> struct intel_audio {
> --
> 2.48.1
