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