Re: [PATCH] drm/i915/audio: Track audio state per-transcoder

[Swati Sharma <swati2.sharma@xxxxxxxxx>]

Hi Ville,

Please add closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8222

On 23-Feb-23 1:48 PM, Shankar, Uma wrote:
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville Syrjala
> > Sent: Wednesday, February 22, 2023 8:45 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject:  [PATCH] drm/i915/audio: Track audio state per-transcoder
> >
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > The audio logic lives in the transcoder rather than the pipe, so start tracking it like
> > that.
> >
> > This is only really important for bigjoiner cases where tracking by pipe doesn't work
> > at all since intel_audio_codec_{enable,disable}()
> > won't even be called for the slave pipe. This means the state checker won't find the
> > ELD for the slave pipe and gets upset.
> > The PD->has_audio readout does currently work since that gets read out from the
> > same transcoder for both pipes.
> >
> > For other cases this doesn't actually matter since it's only the normal pipe
> > transcoders that are audio capable, whereas the more special transcoders (EDP/DSI)
> > are not.
>
> Change Looks Good to me.
> Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/display/intel_audio.c    | 86 +++++++++----------
> >   .../gpu/drm/i915/display/intel_display_core.h |  2 +-
> >   .../gpu/drm/i915/display/intel_lpe_audio.c    |  6 +-
> >   .../gpu/drm/i915/display/intel_lpe_audio.h    |  4 +-
> >   4 files changed, 48 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index a9335c856644..65151f5dcb15 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -581,8 +581,7 @@ static void enable_audio_dsc_wa(struct intel_encoder
> > *encoder,
> >   				const struct intel_crtc_state *crtc_state)  {
> >   	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > -	enum pipe pipe = crtc->pipe;
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >   	unsigned int hblank_early_prog, samples_room;
> >   	unsigned int val;
> >
> > @@ -592,32 +591,32 @@ static void enable_audio_dsc_wa(struct intel_encoder
> > *encoder,
> >   	val = intel_de_read(i915, AUD_CONFIG_BE);
> >
> >   	if (DISPLAY_VER(i915) == 11)
> > -		val |= HBLANK_EARLY_ENABLE_ICL(pipe);
> > +		val |= HBLANK_EARLY_ENABLE_ICL(cpu_transcoder);
> >   	else if (DISPLAY_VER(i915) >= 12)
> > -		val |= HBLANK_EARLY_ENABLE_TGL(pipe);
> > +		val |= HBLANK_EARLY_ENABLE_TGL(cpu_transcoder);
> >
> >   	if (crtc_state->dsc.compression_enable &&
> >   	    crtc_state->hw.adjusted_mode.hdisplay >= 3840 &&
> >   	    crtc_state->hw.adjusted_mode.vdisplay >= 2160) {
> >   		/* Get hblank early enable value required */
> > -		val &= ~HBLANK_START_COUNT_MASK(pipe);
> > +		val &= ~HBLANK_START_COUNT_MASK(cpu_transcoder);
> >   		hblank_early_prog = calc_hblank_early_prog(encoder, crtc_state);
> >   		if (hblank_early_prog < 32)
> > -			val |= HBLANK_START_COUNT(pipe,
> > HBLANK_START_COUNT_32);
> > +			val |= HBLANK_START_COUNT(cpu_transcoder,
> > HBLANK_START_COUNT_32);
> >   		else if (hblank_early_prog < 64)
> > -			val |= HBLANK_START_COUNT(pipe,
> > HBLANK_START_COUNT_64);
> > +			val |= HBLANK_START_COUNT(cpu_transcoder,
> > HBLANK_START_COUNT_64);
> >   		else if (hblank_early_prog < 96)
> > -			val |= HBLANK_START_COUNT(pipe,
> > HBLANK_START_COUNT_96);
> > +			val |= HBLANK_START_COUNT(cpu_transcoder,
> > HBLANK_START_COUNT_96);
> >   		else
> > -			val |= HBLANK_START_COUNT(pipe,
> > HBLANK_START_COUNT_128);
> > +			val |= HBLANK_START_COUNT(cpu_transcoder,
> > HBLANK_START_COUNT_128);
> >
> >   		/* Get samples room value required */
> > -		val &= ~NUMBER_SAMPLES_PER_LINE_MASK(pipe);
> > +		val &= ~NUMBER_SAMPLES_PER_LINE_MASK(cpu_transcoder);
> >   		samples_room = calc_samples_room(crtc_state);
> >   		if (samples_room < 3)
> > -			val |= NUMBER_SAMPLES_PER_LINE(pipe, samples_room);
> > +			val |= NUMBER_SAMPLES_PER_LINE(cpu_transcoder,
> > samples_room);
> >   		else /* Program 0 i.e "All Samples available in buffer" */
> > -			val |= NUMBER_SAMPLES_PER_LINE(pipe, 0x0);
> > +			val |= NUMBER_SAMPLES_PER_LINE(cpu_transcoder, 0x0);
> >   	}
> >
> >   	intel_de_write(i915, AUD_CONFIG_BE, val); @@ -812,9 +811,9 @@ void
> > intel_audio_codec_enable(struct intel_encoder *encoder,
> >   	struct i915_audio_component *acomp = i915->display.audio.component;
> >   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >   	struct intel_connector *connector = to_intel_connector(conn_state-
> > > connector);
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >   	struct intel_audio_state *audio_state;
> >   	enum port port = encoder->port;
> > -	enum pipe pipe = crtc->pipe;
> >
> >   	if (!crtc_state->has_audio)
> >   		return;
> > @@ -832,7 +831,7 @@ void intel_audio_codec_enable(struct intel_encoder
> > *encoder,
> >
> >   	mutex_lock(&i915->display.audio.mutex);
> >
> > -	audio_state = &i915->display.audio.state[pipe];
> > +	audio_state = &i915->display.audio.state[cpu_transcoder];
> >
> >   	audio_state->encoder = encoder;
> >   	BUILD_BUG_ON(sizeof(audio_state->eld) != sizeof(crtc_state->eld)); @@ -
> > 842,14 +841,14 @@ 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 pipe = -1 to indicate Non-MST cases */
> > +		/* audio drivers expect cpu_transcoder = -1 to indicate Non-MST
> > cases
> > +*/
> >   		if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
> > -			pipe = -1;
> > +			cpu_transcoder = -1;
> >   		acomp->base.audio_ops->pin_eld_notify(acomp->base.audio_ops-
> > > audio_ptr,
> > -						      (int)port, (int)pipe);
> > +						      (int)port, (int)cpu_transcoder);
> >   	}
> >
> > -	intel_lpe_audio_notify(i915, pipe, port, crtc_state->eld,
> > +	intel_lpe_audio_notify(i915, cpu_transcoder, port, crtc_state->eld,
> >   			       crtc_state->port_clock,
> >   			       intel_crtc_has_dp_encoder(crtc_state));
> >   }
> > @@ -871,9 +870,9 @@ void intel_audio_codec_disable(struct intel_encoder
> > *encoder,
> >   	struct i915_audio_component *acomp = i915->display.audio.component;
> >   	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >   	struct intel_connector *connector = to_intel_connector(old_conn_state-
> > > connector);
> > +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> >   	struct intel_audio_state *audio_state;
> >   	enum port port = encoder->port;
> > -	enum pipe pipe = crtc->pipe;
> >
> >   	if (!old_crtc_state->has_audio)
> >   		return;
> > @@ -890,7 +889,7 @@ void intel_audio_codec_disable(struct intel_encoder
> > *encoder,
> >
> >   	mutex_lock(&i915->display.audio.mutex);
> >
> > -	audio_state = &i915->display.audio.state[pipe];
> > +	audio_state = &i915->display.audio.state[cpu_transcoder];
> >
> >   	audio_state->encoder = NULL;
> >   	memset(audio_state->eld, 0, sizeof(audio_state->eld)); @@ -899,27
> > +898,26 @@ 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 pipe = -1 to indicate Non-MST cases */
> > +		/* audio drivers expect cpu_transcoder = -1 to indicate Non-MST
> > cases
> > +*/
> >   		if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST))
> > -			pipe = -1;
> > +			cpu_transcoder = -1;
> >   		acomp->base.audio_ops->pin_eld_notify(acomp->base.audio_ops-
> > > audio_ptr,
> > -						      (int)port, (int)pipe);
> > +						      (int)port, (int)cpu_transcoder);
> >   	}
> >
> > -	intel_lpe_audio_notify(i915, pipe, port, NULL, 0, false);
> > +	intel_lpe_audio_notify(i915, cpu_transcoder, port, NULL, 0, false);
> >   }
> >
> >   static void intel_acomp_get_config(struct intel_encoder *encoder,
> >   				   struct intel_crtc_state *crtc_state)  {
> >   	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >   	struct intel_audio_state *audio_state;
> > -	enum pipe pipe = crtc->pipe;
> >
> >   	mutex_lock(&i915->display.audio.mutex);
> >
> > -	audio_state = &i915->display.audio.state[pipe];
> > +	audio_state = &i915->display.audio.state[cpu_transcoder];
> >
> >   	if (audio_state->encoder)
> >   		memcpy(crtc_state->eld, audio_state->eld, sizeof(audio_state-
> > > eld)); @@ -1147,27 +1145,27 @@ static int
> > i915_audio_component_get_cdclk_freq(struct device *kdev)  }
> >
> >   /*
> > - * get the intel audio state according to the parameter port and pipe
> > - * MST & (pipe >= 0): return the audio.state[pipe].encoder],
> > + * get the intel audio state according to the parameter port and
> > + cpu_transcoder
> > + * MST & (cpu_transcoder >= 0): return the
> > + audio.state[cpu_transcoder].encoder],
> >    *   when port is matched
> > - * MST & (pipe < 0): this is invalid
> > - * Non-MST & (pipe >= 0): only pipe = 0 (the first device entry)
> > + * 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 & (pipe < 0): get the right intel_encoder with port matched
> > + * Non-MST & (cpu_transcoder < 0): get the right intel_encoder with
> > + port matched
> >    */
> >   static struct intel_audio_state *find_audio_state(struct drm_i915_private *i915,
> > -						  int port, int pipe)
> > +						  int port, int cpu_transcoder)
> >   {
> >   	/* MST */
> > -	if (pipe >= 0) {
> > +	if (cpu_transcoder >= 0) {
> >   		struct intel_audio_state *audio_state;
> >   		struct intel_encoder *encoder;
> >
> >   		if (drm_WARN_ON(&i915->drm,
> > -				pipe >= ARRAY_SIZE(i915->display.audio.state)))
> > +				cpu_transcoder >= ARRAY_SIZE(i915-
> > > display.audio.state)))
> >   			return NULL;
> >
> > -		audio_state = &i915->display.audio.state[pipe];
> > +		audio_state = &i915->display.audio.state[cpu_transcoder];
> >   		encoder = audio_state->encoder;
> >
> >   		if (encoder && encoder->port == port && @@ -1176,14 +1174,14
> > @@ static struct intel_audio_state *find_audio_state(struct drm_i915_private
> > *i915,
> >   	}
> >
> >   	/* Non-MST */
> > -	if (pipe > 0)
> > +	if (cpu_transcoder > 0)
> >   		return NULL;
> >
> > -	for_each_pipe(i915, pipe) {
> > +	for_each_cpu_transcoder(i915, cpu_transcoder) {
> >   		struct intel_audio_state *audio_state;
> >   		struct intel_encoder *encoder;
> >
> > -		audio_state = &i915->display.audio.state[pipe];
> > +		audio_state = &i915->display.audio.state[cpu_transcoder];
> >   		encoder = audio_state->encoder;
> >
> >   		if (encoder && encoder->port == port && @@ -1195,7 +1193,7 @@
> > static struct intel_audio_state *find_audio_state(struct drm_i915_private *i915,  }
> >
> >   static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
> > -						int pipe, int rate)
> > +						int cpu_transcoder, int rate)
> >   {
> >   	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> >   	struct i915_audio_component *acomp = i915->display.audio.component;
> > @@ -1211,7 +1209,7 @@ static int i915_audio_component_sync_audio_rate(struct
> > device *kdev, int port,
> >   	cookie = i915_audio_component_get_power(kdev);
> >   	mutex_lock(&i915->display.audio.mutex);
> >
> > -	audio_state = find_audio_state(i915, port, pipe);
> > +	audio_state = find_audio_state(i915, port, cpu_transcoder);
> >   	if (!audio_state) {
> >   		drm_dbg_kms(&i915->drm, "Not valid for port %c\n",
> > port_name(port));
> >   		err = -ENODEV;
> > @@ -1223,7 +1221,7 @@ static int i915_audio_component_sync_audio_rate(struct
> > device *kdev, int port,
> >   	/* FIXME stop using the legacy crtc pointer */
> >   	crtc = to_intel_crtc(encoder->base.crtc);
> >
> > -	/* port must be valid now, otherwise the pipe will be invalid */
> > +	/* port must be valid now, otherwise the cpu_transcoder will be
> > +invalid */
> >   	acomp->aud_sample_rate[port] = rate;
> >
> >   	/* FIXME get rid of the crtc->config stuff */ @@ -1236,7 +1234,7 @@ static
> > int i915_audio_component_sync_audio_rate(struct device *kdev, int port,  }
> >
> >   static int i915_audio_component_get_eld(struct device *kdev, int port,
> > -					int pipe, bool *enabled,
> > +					int cpu_transcoder, bool *enabled,
> >   					unsigned char *buf, int max_bytes)  {
> >   	struct drm_i915_private *i915 = kdev_to_i915(kdev); @@ -1245,7 +1243,7
> > @@ static int i915_audio_component_get_eld(struct device *kdev, int port,
> >
> >   	mutex_lock(&i915->display.audio.mutex);
> >
> > -	audio_state = find_audio_state(i915, port, pipe);
> > +	audio_state = find_audio_state(i915, port, cpu_transcoder);
> >   	if (!audio_state) {
> >   		drm_dbg_kms(&i915->drm, "Not valid for port %c\n",
> > port_name(port));
> >   		mutex_unlock(&i915->display.audio.mutex);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index b870f7f47f2b..631a7b17899e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -103,7 +103,7 @@ struct intel_audio {
> >   	u32 freq_cntrl;
> >
> >   	/* current audio state for the audio component hooks */
> > -	struct intel_audio_state state[I915_MAX_PIPES];
> > +	struct intel_audio_state state[I915_MAX_TRANSCODERS];
> >
> >   	/* necessary resource sharing with HDMI LPE audio driver. */
> >   	struct {
> > diff --git a/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> > b/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> > index 8aaaef4d7856..20c8581f4868 100644
> > --- a/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> > @@ -315,7 +315,7 @@ void intel_lpe_audio_teardown(struct drm_i915_private
> > *dev_priv)
> >    * intel_lpe_audio_notify() - notify lpe audio event
> >    * audio driver and i915
> >    * @dev_priv: the i915 drm device private data
> > - * @pipe: pipe
> > + * @cpt_transocer: CPU transcoder
> >    * @port: port
> >    * @eld : ELD data
> >    * @ls_clock: Link symbol clock in kHz
> > @@ -324,7 +324,7 @@ void intel_lpe_audio_teardown(struct drm_i915_private
> > *dev_priv)
> >    * Notify lpe audio driver of eld change.
> >    */
> >   void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > -			    enum pipe pipe, enum port port,
> > +			    enum transcoder cpu_transcoder, enum port port,
> >   			    const void *eld, int ls_clock, bool dp_output)  {
> >   	unsigned long irqflags;
> > @@ -344,7 +344,7 @@ void intel_lpe_audio_notify(struct drm_i915_private
> > *dev_priv,
> >
> >   	if (eld != NULL) {
> >   		memcpy(ppdata->eld, eld, HDMI_MAX_ELD_BYTES);
> > -		ppdata->pipe = pipe;
> > +		ppdata->pipe = cpu_transcoder;
> >   		ppdata->ls_clock = ls_clock;
> >   		ppdata->dp_output = dp_output;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> > b/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> > index f848c5038714..0beecac267ae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> > +++ b/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> > @@ -8,15 +8,15 @@
> >
> >   #include <linux/types.h>
> >
> > -enum pipe;
> >   enum port;
> > +enum transcoder;
> >   struct drm_i915_private;
> >
> >   int  intel_lpe_audio_init(struct drm_i915_private *dev_priv);  void
> > intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);  void
> > intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);  void
> > intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > -			    enum pipe pipe, enum port port,
> > +			    enum transcoder cpu_transcoder, enum port port,
> >   			    const void *eld, int ls_clock, bool dp_output);
> >
> >   #endif /* __INTEL_LPE_AUDIO_H__ */
> > --
> > 2.39.2

--
~Swati Sharma