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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 19 Jun 2023, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote:
> 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?

Personally, I'd always go for signed ints. Integer promotions are hard.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Graphics Center