| Message ID | 20230609174212.1946930-3-mitulkumar.ajitkumar.golani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Get optimal audio frequency and channels | expand |
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
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 >
Hello Kai, > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Kai > Vehmanen > Sent: Monday, June 19, 2023 5:56 PM > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com> > Cc: intel-gfx@lists.freedesktop.org; jyri.sarha@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. > > > +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? > This has been one my concern as well and we have thought about a similar approach as you suggest. One disadvantage of this approach that I can see, would be that if there are hardware limitations on channels (as was in GLK) or frequencies independently. It will be become tricky with this approach. However, one can argue that these limitations arise from bandwidth itself. Regarding the bits per sample, Is using 32bit for all audio formats fair or should we take into account different audio formats and their bit depth? Regards Chaitanya > Br, Kai
Hey, On Tue, 20 Jun 2023, Borah, Chaitanya Kumar wrote: > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Kai > > Vehmanen > > On Fri, 9 Jun 2023, Mitul Golani wrote: [...] > > 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? [...]> > This has been one my concern as well and we have thought about a similar > approach as you suggest. One disadvantage of this approach that I can > see, would be that if there are hardware limitations on channels (as was > in GLK) or frequencies independently. It will be become tricky with this > approach. However, one can argue that these limitations arise from > bandwidth itself. [...] > Regarding the bits per sample, Is using 32bit for all audio formats fair > or should we take into account different audio formats and their bit > depth? hmm, I see the point. This is indeed trickier than it first seems. The 32bit is a good worst-case bound, but in practise actual bandwidth needed will be less. And problem is, we don't really know which bit depth the application will choose, so again we need to limit based on the highest value in SAD. And then you have the problem that this calculation assumes LPCM encoding. If the audio is compressed, e.g. a 8ch DTS stream, the bandwidth calculation needs to be done differently (see linux/sound/pci/hda/hda_eld.c:hdmi_update_short_audio_desc()): So I think there are (at least) two ways to go about this: 1) reduce the scope and make the channel/rate limit more limited, and only cover cases (like) GLK where a specific limitation is known -> max values not set for other platforms 2) go for more generic description and expose the raw bandwidth (in bits per second) available for audio -> then SAD filtering can be done based on raw bandwidth -> can be done only to LPCM at first, extended to compressed formats later -> still the problem that code needs to prioritze between channels/srate/bitdepth when filtering Br, Kai
Hi @Kai Vehmanen > -----Original Message----- > From: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Sent: 19 June 2023 17:56 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com> > Cc: intel-gfx@lists.freedesktop.org; jyri.sarha@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
Hi Kai, > -----Original Message----- > From: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Sent: 21 June 2023 22:36 > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>; Golani, Mitulkumar > Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>; intel- > gfx@lists.freedesktop.org; jyri.sarha@linux.intel.com > Subject: RE: [Intel-gfx] [RFC 2/3] drm/i915/display: Configure and initialize > HDMI audio capabilities > > Hey, > > On Tue, 20 Jun 2023, Borah, Chaitanya Kumar wrote: > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > Of Kai Vehmanen On Fri, 9 Jun 2023, Mitul Golani wrote: > [...] > > > 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? > [...]> > > This has been one my concern as well and we have thought about a > > similar approach as you suggest. One disadvantage of this approach > > that I can see, would be that if there are hardware limitations on > > channels (as was in GLK) or frequencies independently. It will be > > become tricky with this approach. However, one can argue that these > > limitations arise from bandwidth itself. > [...] > > Regarding the bits per sample, Is using 32bit for all audio formats > > fair or should we take into account different audio formats and their > > bit depth? > > hmm, I see the point. This is indeed trickier than it first seems. The 32bit is a > good worst-case bound, but in practise actual bandwidth needed will be less. > And problem is, we don't really know which bit depth the application will > choose, so again we need to limit based on the highest value in SAD. > > And then you have the problem that this calculation assumes LPCM > encoding. > If the audio is compressed, e.g. a 8ch DTS stream, the bandwidth calculation > needs to be done differently (see > linux/sound/pci/hda/hda_eld.c:hdmi_update_short_audio_desc()): > > So I think there are (at least) two ways to go about this: > 1) reduce the scope and make the channel/rate limit more > limited, and only cover cases (like) GLK where a specific limitation > is known -> max values not set for other platforms > > 2) go for more generic description and expose the raw > bandwidth (in bits per second) available for audio > -> then SAD filtering can be done based on raw bandwidth > -> can be done only to LPCM at first, extended to compressed > formats later > -> still the problem that code needs to prioritze > between channels/srate/bitdepth when filtering I tried to prioritise the logic with help of rate first. But in that I found some issues, it is picking lowest channel with highest rate every time. I think apart from current params, adding max_audio_samples_per_second, can solve both the issue with following case, 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, with current implementation, pruning will be bypassed, and there is a possibility that the sink-supported channel multiplied by the sink-supported rate exceeding the available blank bandwidth, but pruning didn'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 also. 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 above mentioned situation. 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. now pruning will happen in 2 cases, 1. If pipe_config->audio.max_channel < sink's supported channels then prune as per obtained combination from, intel_hdmi_audio_compute_config. 2. If pipe_config->audio.max_channel > sink's supported channels, but sink's supported channel * sink supported max rate * audio sample container size exceeds the max_audio_samples_per_second then prune with sink's supported channel and rate (which satisfy bandwidth condition. range: in between Max to min). Please give your opinion. Thanks Mitul > > Br, Kai
Hi, On Mon, 26 Jun 2023, Golani, Mitulkumar Ajitkumar wrote: > 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, with current implementation, > pruning will be bypassed, and there is a possibility that the sink-supported channel multiplied > by the sink-supported rate exceeding the available blank bandwidth, but pruning didn'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 also. > 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 above mentioned situation. > 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. [...] > now pruning will happen in 2 cases, > > 1. If pipe_config->audio.max_channel < sink's supported channels then prune as per obtained combination from, > intel_hdmi_audio_compute_config. > 2. If pipe_config->audio.max_channel > sink's supported channels, but sink's supported channel * sink supported max rate * audio sample container size > exceeds the max_audio_samples_per_second then prune with sink's supported channel and rate (which satisfy bandwidth condition. range: in between Max to min). > > Please give your opinion. ack, I think this is sensible. The SAD filtering cannot be perfect as there can be multiple ways to prune the config to get within bandwidth budget (as we have three varibles for LPCM, channel count, sampling rate and also sample depth). So given limited visibility, I'd say above approach sounds good. I'd also proceed (in your step2) with limiting first the sampling rate and only further limit channel count in second step. Br, Kai
Hi, > -----Original Message----- > From: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Sent: 28 June 2023 22:41 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>; Borah, Chaitanya > Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org; jyri.sarha@linux.intel.com > Subject: RE: [Intel-gfx] [RFC 2/3] drm/i915/display: Configure and initialize > HDMI audio capabilities > > Hi, > > On Mon, 26 Jun 2023, Golani, Mitulkumar Ajitkumar wrote: > > 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, with > > current implementation, pruning will be bypassed, and there is a > > possibility that the sink-supported channel multiplied by the sink- > supported rate exceeding the available blank bandwidth, but pruning didn'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 also. > > 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 above mentioned situation. > > 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. > [...] > > now pruning will happen in 2 cases, > > > > 1. If pipe_config->audio.max_channel < sink's supported channels then > > prune as per obtained combination from, > intel_hdmi_audio_compute_config. > > 2. If pipe_config->audio.max_channel > sink's supported channels, but > > sink's supported channel * sink supported max rate * audio sample > container size exceeds the max_audio_samples_per_second then prune > with sink's supported channel and rate (which satisfy bandwidth condition. > range: in between Max to min). > > > > Please give your opinion. > > ack, I think this is sensible. The SAD filtering cannot be perfect as there can > be multiple ways to prune the config to get within bandwidth budget (as we > have three varibles for LPCM, channel count, sampling rate and also sample > depth). So given limited visibility, I'd say above approach sounds good. I'd > also proceed (in your step2) with limiting first the sampling rate and only > further limit channel count in second step. > > Br, Kai Thanks Kai, I will update changes with new revision. Regards, MItul
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index ebd147180a6e..74eee87d2df1 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -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; /* diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 32157bef2eef..0188a600f9f5 100644 --- 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; + return bandwidth; +} + +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--) { + 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; + } + } + } + + pipe_config->audio.max_frequency = 0; + pipe_config->audio.max_channel = 0; +} + int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -2344,6 +2378,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, pipe_config->hdmi_high_tmds_clock_ratio = true; } } + intel_hdmi_audio_compute_config(pipe_config); intel_hdmi_compute_gcp_infoframe(encoder, pipe_config, conn_state); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.h b/drivers/gpu/drm/i915/display/intel_hdmi.h index 6b39df38d57a..6df303daf348 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.h +++ b/drivers/gpu/drm/i915/display/intel_hdmi.h @@ -27,6 +27,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *dig_port, bool intel_hdmi_compute_has_hdmi_sink(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state); +void intel_hdmi_audio_compute_config(struct intel_crtc_state *pipe_config); int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state);
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. --v1: - Refactor max_channel and max_rate to this commit as it is being initialised here - Remove call for intel_audio_compute_eld to avoid any regression while merge. instead call it in different commit when it is defined. - Use int instead of unsigned int for max_channel and max_frequecy - Update commit message and header Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> --- .../drm/i915/display/intel_display_types.h | 6 ++++ drivers/gpu/drm/i915/display/intel_hdmi.c | 35 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_hdmi.h | 1 + 3 files changed, 42 insertions(+)