Hi @Kai Vehmanen
> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Sent: 19 June 2023 17:56
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; jyri.sarha@xxxxxxxxxxxxxxx
> Subject: Re: [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
