[Intel-gfx] [PATCH 2/3] drm/i915/display: Configure and initialize HDMI audio capabilities

[Golani, Mitulkumar Ajitkumar]

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: 17 August 2023 18:34
> To: Golani, Mitulkumar Ajitkumar
> <mitulkumar.ajitkumar.golani at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; jyri.sarha at linux.intel.com
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/display: Configure and
> initialize HDMI audio capabilities
> 
> On Thu, Aug 17, 2023 at 06:20:06PM +0530, Mitul Golani wrote:
> > Initialize the source audio capabilities in crtc_state property by
> > setting them to their maximum supported values, including max_channel
> > and max_frequency. This allows for the calculation of audio source
> > capabilities with respect to the available mode bandwidth. These
> > capabilities encompass parameters such as supported frequency and
> > channel configurations.
> >
> > --v1:
> > - Refactor max_channel and max_rate to this commit as it is being
> > initialised here
> > - Remove call for intel_audio_compute_eld to avoid any regression
> > while merge. instead call it in different commit when it is defined.
> > - Use int instead of unsigned int for max_channel and max_frequecy
> > - Update commit message and header
> >
> > --v2:
> > - Use signed instead of unsigned variables.
> > - Avoid using magic numbers and give them proper name.
> >
> > --v3:
> > - Move defines to intel_audio.c.
> > - use consistent naming convention for rate and channel.
> > - declare num_of_channel and aud_rate separately.
> > - Declare index value outside of for loop.
> > - Move Bandwidth calculation to intel_Audio.c as it is common for both
> > DP and HDMI. Also use static.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_audio.c    | 38 +++++++++++++++++++
> >  .../drm/i915/display/intel_display_types.h    |  6 +++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index e20ffc8e9654..79377e33a59b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -64,6 +64,9 @@
> >   * struct &i915_audio_component_audio_ops @audio_ops is called from
> i915 driver.
> >   */
> >
> > +#define AUDIO_SAMPLE_CONTAINER_SIZE	32
> > +#define MAX_CHANNEL_COUNT		8
> > +
> >  struct intel_audio_funcs {
> >  	void (*audio_codec_enable)(struct intel_encoder *encoder,
> >  				   const struct intel_crtc_state *crtc_state,
> @@ -770,6 +773,39
> > @@ void intel_audio_sdp_split_update(struct intel_encoder *encoder,
> >  			     crtc_state->sdp_split_enable ?
> AUD_ENABLE_SDP_SPLIT : 0);  }
> >
> > +static int calc_audio_bw(int channel_count, int rate) {
> > +	int bandwidth = channel_count * rate *
> AUDIO_SAMPLE_CONTAINER_SIZE;
> > +	return bandwidth;
> > +}
> > +
> > +static void calc_audio_config_params(struct intel_crtc_state
> > +*pipe_config) {
> > +	struct drm_display_mode *adjusted_mode = &pipe_config-
> >hw.adjusted_mode;
> > +	int channel_count;
> > +	int index, rate[] = { 192000, 176000, 96000, 88000, 48000, 44100,
> 32000 };
> > +	int audio_req_bandwidth, available_blank_bandwidth, vblank,
> hblank;
> > +
> > +	hblank = adjusted_mode->htotal - adjusted_mode->hdisplay;
> > +	vblank = adjusted_mode->vtotal - adjusted_mode->vdisplay;
> > +	available_blank_bandwidth = hblank * vblank *
> > +				    drm_mode_vrefresh(adjusted_mode) *
> pipe_config->pipe_bpp;
> > +	for (channel_count = MAX_CHANNEL_COUNT; channel_count > 0;
> channel_count--) {
> > +		for (index = 0; index < ARRAY_SIZE(rate); index++) {
> > +			audio_req_bandwidth =
> calc_audio_bw(channel_count,
> > +							    rate[index]);
> > +			if (audio_req_bandwidth <
> available_blank_bandwidth) {
> > +				pipe_config->audio.max_rate = rate[index];
> > +				pipe_config->audio.max_channel_count =
> channel_count;
> > +				return;
> > +			}
> > +		}
> > +	}
> > +
> > +	pipe_config->audio.max_rate = 0;
> > +	pipe_config->audio.max_channel_count = 0; }
> > +
> >  bool intel_audio_compute_config(struct intel_encoder *encoder,
> >  				struct intel_crtc_state *crtc_state,
> >  				struct drm_connector_state *conn_state)
> @@ -791,6 +827,8 @@ bool
> > intel_audio_compute_config(struct intel_encoder *encoder,
> >
> >  	crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode)
> /
> > 2;
> >
> > +	calc_audio_config_params(crtc_state);
> > +
> >  	return true;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index ebd147180a6e..8815837a95a6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1131,6 +1131,12 @@ struct intel_crtc_state {
> >
> >  	struct {
> >  		bool has_audio;
> > +
> > +		/* Audio rate in Hz */
> > +		int max_rate;
> > +
> > +		/* Number of audio channels */
> > +		int max_channel_count;
> 
> From what I can see you only calculate these when computing the ELD, and
> immediately use them there and nowhere else. So I see no reason to bloat
> the crtc_state with this.

Thanks for the inputs, As mentioned in the cover letter, this initial implementation is
geared towards GLK scenarios with channel and rate constraints, serving as a strategic solution.
Additionally, it proves advantageous for addressing future scenarios involving bandwidth limitations
that may lead to exceedance.

https://gitlab.freedesktop.org/drm/intel/-/issues/5368

Regards,
Mitul
> 
> >  	} audio;
> >
> >  	/*
> > --
> > 2.25.1
> 
> --
> Ville Syrjälä
> Intel