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

[Borah, Chaitanya Kumar]

Hello Kai,

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Kai
> Vehmanen
> Sent: Monday, June 19, 2023 5:56 PM
> 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] [RFC 2/3] drm/i915/display: Configure and initialize
> HDMI audio capabilities
> 
> Hey,
> 
> replying to 9th June version (my mistake), but I checked the 15th June patch
> version and comments applied to that one as well:
> 
> On Fri, 9 Jun 2023, Mitul Golani wrote:
> 
> > Initialize the source audio capabilities for HDMI in crtc_state
> > property by setting them to their maximum supported values, including
> > max_channel and max_frequency. This allows for the calculation of HDMI
> > audio source capabilities with respect to the available mode
> > bandwidth. These capabilities encompass parameters such as supported
> > frequency and channel configurations.
> [...]
> > @@ -1131,6 +1131,12 @@ struct intel_crtc_state {
> >
> >  	struct {
> >  		bool has_audio;
> > +
> > +		/* Audio rate in Hz */
> > +		int max_frequency;
> > +
> > +		/* Number of audio channels */
> > +		int max_channel;
> >  	} audio;
> 
> Comment on this below.
> 
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2277,6 +2277,40 @@ bool intel_hdmi_compute_has_hdmi_sink(struct
> intel_encoder *encoder,
> >  		!intel_hdmi_is_cloned(crtc_state);
> >  }
> >
> > +static unsigned int calc_audio_bw(int channel, int frequency) {
> > +	int bits_per_sample = 32;
> > +	unsigned int bandwidth = channel * frequency * bits_per_sample;
> 
> Maybe unsigned for bits_per_sample as well? And not sure how fixed this is,
> but having 32 as a define at start file with more descriptive name might be a
> good idea as well. I.e. this is the audio sample container size used in all
> calculations.
> 
> > +void
> > +intel_hdmi_audio_compute_config(struct intel_crtc_state *pipe_config)
> > +{
> > +	struct drm_display_mode *adjusted_mode = &pipe_config-
> >hw.adjusted_mode;
> > +	int num_of_channel, aud_rates[7] = {192000, 176000, 96000, 88000,
> 48000, 44100, 32000};
> > +	unsigned 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 (num_of_channel = 8; num_of_channel > 0; num_of_channel--) {
> 
> The maximum channel count of 8 would deserve its own define. It's pretty
> much a constant coming from the specs, but still avoid magic numbers in code
> would be preferable. Or we remove this altoghter, see below...
> 
> > +		for (int index = 0; index < 7; index++) {
> > +			audio_req_bandwidth =
> calc_audio_bw(num_of_channel,
> > +							    aud_rates[index]);
> > +			if (audio_req_bandwidth <
> available_blank_bandwidth) {
> 
> <= ?
> 
> > +				pipe_config->audio.max_frequency =
> aud_rates[index];
> > +				pipe_config->audio.max_channel =
> num_of_channel;
> > +				return;
> > +			}
> 
> This will hit a problem if we have a case where bandwidth is not enough for 5.1
> at 192kHz, but it is enough for 2ch 192kHz audio. This approach forces us to
> give preference to either channel acount or sampling rate.
> 
> What if we just store the 'max audio samples per second' into pipe config:
> 
>  - have "int max_audio_samples_per_second;" in pipe_config
>  - pipe_config->audio.max_audio_samples_per_second =
> available_blank_bandwidth / 32;
> 
> Then when filtering SADs, the invidial channels+rate combination of each SAD
> is compared to the max_audio_samples_per_second and based on that, the
> SAD is either filter or passed on. What do you think?
> 

This has been one my concern as well and we have thought about a similar approach as you suggest.
One disadvantage of this approach that I can see, would be that if there are hardware limitations on channels (as was in GLK) or frequencies independently. It will be become tricky with this approach. However, one can argue that these limitations arise from bandwidth itself.

Regarding the bits per sample, Is using 32bit for all audio formats fair or should we take into account different audio formats and their bit depth?

Regards

Chaitanya

> Br, Kai