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

[Golani, Mitulkumar Ajitkumar]

Hi @Kai Vehmanen

> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> Sent: 19 June 2023 17:56
> 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.
> 

Thanks. Will push the correction in next revision.

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

Thanks. Will push the correction in next revision.

> 
> > +		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?
> 
> Br, Kai

Yes, I understood the issue while validating this on one of the panels. Let's say, with the
obtained audio_req_bandwidth, if I encounter the following combination:
pipe_config->audio.max_channel = 8 and pipe_config->audio.max_frequency = "X" value.
Now, let's assume my sink supports only 7 channels. In this case, pruning will be bypassed,
and there is a possibility that the sink-supported channel multiplied by the sink-supported
rate exceeds the available blank bandwidth, and pruning doesn't occur.

For this situation, I am considering adding an "else" part in intel_audio_compute_eld.
This "else" part would check if (sad_to_channels(sad) < crtc_state->audio.max_channel),
for example, in the GLK case. In that case, the channel would be fixed.

So, if Channel * audio sample container size * (iterating from Max rate to Min rate)
is less than max_audio_samples_per_second, I believe we can eliminate the situation you
mentioned. If the sink's supported channel is lower than pipe_config->audio.max_channel,
we can get another chance to adjust the rate based on the sink's maximum capability.

Please give your opinion on this approach ?

Thanks
Mitul