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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

>  	} audio;
>  
>  	/*
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel