Hi Ville,
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: 17 August 2023 18:34
> To: Golani, Mitulkumar Ajitkumar
> <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; jyri.sarha@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/3] drm/i915/display: Configure and
> initialize HDMI audio capabilities
>
> On Thu, Aug 17, 2023 at 06:20:06PM +0530, Mitul Golani wrote:
> > Initialize the source audio capabilities in crtc_state property by
> > setting them to their maximum supported values, including max_channel
> > and max_frequency. This allows for the calculation of 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
> >
> > --v2:
> > - Use signed instead of unsigned variables.
> > - Avoid using magic numbers and give them proper name.
> >
> > --v3:
> > - Move defines to intel_audio.c.
> > - use consistent naming convention for rate and channel.
> > - declare num_of_channel and aud_rate separately.
> > - Declare index value outside of for loop.
> > - Move Bandwidth calculation to intel_Audio.c as it is common for both
> > DP and HDMI. Also use static.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_audio.c | 38 +++++++++++++++++++
> > .../drm/i915/display/intel_display_types.h | 6 +++
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index e20ffc8e9654..79377e33a59b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -64,6 +64,9 @@
> > * struct &i915_audio_component_audio_ops @audio_ops is called from
> i915 driver.
> > */
> >
> > +#define AUDIO_SAMPLE_CONTAINER_SIZE 32
> > +#define MAX_CHANNEL_COUNT 8
> > +
> > struct intel_audio_funcs {
> > void (*audio_codec_enable)(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state,
> @@ -770,6 +773,39
> > @@ void intel_audio_sdp_split_update(struct intel_encoder *encoder,
> > crtc_state->sdp_split_enable ?
> AUD_ENABLE_SDP_SPLIT : 0); }
> >
> > +static int calc_audio_bw(int channel_count, int rate) {
> > + int bandwidth = channel_count * rate *
> AUDIO_SAMPLE_CONTAINER_SIZE;
> > + return bandwidth;
> > +}
> > +
> > +static void calc_audio_config_params(struct intel_crtc_state
> > +*pipe_config) {
> > + struct drm_display_mode *adjusted_mode = &pipe_config-
> >hw.adjusted_mode;
> > + int channel_count;
> > + int index, rate[] = { 192000, 176000, 96000, 88000, 48000, 44100,
> 32000 };
> > + 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 (channel_count = MAX_CHANNEL_COUNT; channel_count > 0;
> channel_count--) {
> > + for (index = 0; index < ARRAY_SIZE(rate); index++) {
> > + audio_req_bandwidth =
> calc_audio_bw(channel_count,
> > + rate[index]);
> > + if (audio_req_bandwidth <
> available_blank_bandwidth) {
> > + pipe_config->audio.max_rate = rate[index];
> > + pipe_config->audio.max_channel_count =
> channel_count;
> > + return;
> > + }
> > + }
> > + }
> > +
> > + pipe_config->audio.max_rate = 0;
> > + pipe_config->audio.max_channel_count = 0; }
> > +
> > bool intel_audio_compute_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *crtc_state,
> > struct drm_connector_state *conn_state)
> @@ -791,6 +827,8 @@ bool
> > intel_audio_compute_config(struct intel_encoder *encoder,
> >
> > crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode)
> /
> > 2;
> >
> > + calc_audio_config_params(crtc_state);
> > +
> > return true;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index ebd147180a6e..8815837a95a6 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_rate;
> > +
> > + /* Number of audio channels */
> > + int max_channel_count;
>
> From what I can see you only calculate these when computing the ELD, and
> immediately use them there and nowhere else. So I see no reason to bloat
> the crtc_state with this.
Thanks for the inputs, As mentioned in the cover letter, this initial implementation is
geared towards GLK scenarios with channel and rate constraints, serving as a strategic solution.
Additionally, it proves advantageous for addressing future scenarios involving bandwidth limitations
that may lead to exceedance.
https://gitlab.freedesktop.org/drm/intel/-/issues/5368
Regards,
Mitul
>
> > } audio;
> >
> > /*
> > --
> > 2.25.1
>
> --
> Ville Syrjälä
> Intel
