On Mon, 19 Jun 2023, Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx> 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
