Hi Suraj,
> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> Sent: 05 September 2023 14:47
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@xxxxxxxxx>;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Subject: RE: [PATCH 3/3] drm/i915/display: Configure and initialize
> HDMI audio capabilities
>
> > Subject: [PATCH 3/3] drm/i915/display: Configure and
> > initialize HDMI audio capabilities
> >
> > Initialize the source audio capabilities in the crtc_state property,
> > setting them to
>
> Nit: maybe mention the above as intel_crtc_state rather than crtc_state
> property as property usually refer to as drm_property and it just seems a
> little weird to read. I have seen this in some of your previous patches in this
> series you can make the changes there as well.
>
> > their maximum supported values for max_channel and max_rate. This
> > initialization enables the calculation of audio source capabilities
> > concerning the available mode bandwidth. These capabilities encompass
> > parameters such as supported rate and channel configurations.
> >
> > Additionally, introduces a wrapper function for computing Short Audio
> > Descriptors (SADs). The wrapper function incorporates logic for
> > determining
>
> Typo * introduce
>
> > supported rates and channels according to the capabilities of the audio
> source.
> > It returns a set of SADs that are compatible with the audio source's
> capabilities.
> >
> > --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.
> >
> > --v10:
> > - Merged patch 2 and 3 to deduplicate function calls.
> > - Instead using Calibrate and calculated functions separately, removed
> > code duplication and merged functions.[Nikula, Jani]
> > - Remove magic value for SAD Channel mask. [Nikula, Jani]
> > - Corrected rate values based on HDMI Spec [Nikula, Jani]
> > - Update drm function to extract SAD from ELD [Nikula, Jani]
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_audio.c | 127 ++++++++++++++++++
> > .../drm/i915/display/intel_display_types.h | 6 +
> > 2 files changed, 133 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index e20ffc8e9654..2584096d80a4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -64,6 +64,10 @@
> > * struct &i915_audio_component_audio_ops @audio_ops is called from
> > i915 driver.
> > */
> >
> > +#define AUDIO_SAMPLE_CONTAINER_SIZE 32
> > +#define MAX_CHANNEL_COUNT 8
> > +#define ELD_SAD_CHANNELS_MASK 0x7
>
> Use REG_GENMASK() to create masks should look cleaner
you should use REG_GENMASK when you need to create a bitmask that covers a specific range of bits within a register or variable.
I think If you are defining a simple bitmask like in above example, there's no need to use REG_GENMASK.
> > +
> > struct intel_audio_funcs {
> > void (*audio_codec_enable)(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state,
> @@
> > -770,6 +774,127 @@ void intel_audio_sdp_split_update(struct
> > intel_encoder *encoder,
> > crtc_state->sdp_split_enable ?
> > AUD_ENABLE_SDP_SPLIT : 0); }
> >
> > +static int sad_to_channels(const u8 *sad) {
> > + return 1 + (sad[0] & 0x7);
>
> I think you missed using your defined mask here;
>
> > +}
> > +
> > +static int calc_audio_bw(int channel_count, int rate) {
> > + int bandwidth = channel_count * rate *
> > AUDIO_SAMPLE_CONTAINER_SIZE;
> > + return bandwidth;
>
> Why introduce a variable here why not just return channel_count * rate *
> AUDIO_SAMPLE_CONTAINER_SIZE;
>
> > +}
> > +
> > +static void calc_and_calibrate_audio_config_params(struct
> > +intel_crtc_state
> > *pipe_config,
> > + int channel, bool
> > calibration_required) {
>
> I think this should have a int type function that returns 0 if max_rate and
> max_channel_count are non zero else return -EINVAL
calc_and_calibrate_audio_config_params does not required to get
failed. If it gets failed as well, we will have to go ahead with the current configs available.
So this function does not require to return EINVAL in case it returns value as 0.
That 0 value itself we will compute.
>
> > + struct drm_display_mode *adjusted_mode = &pipe_config-
> > >hw.adjusted_mode;
> > + int channel_count;
> > + int index, rate[] = { 192000, 176400, 96000, 88200, 48000, 44100,
> > 32000 };
>
> Where do we get these rate values from.
> What if we kept them at crtc_state so these can be update if required.
>
> > + 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;
> > +
> > + /*
> > + * Expected calibration of channels and respective rates,
> > + * based on MAX_CHANNEL_COUNT. First calculate channel and
> > + * rate based on Maximum that source can compute, letter
> > + * with respect to sink's maximum channel capacity, calibrate
> > + * supportive rates.
>
> Typo: *maximum and *later and *supported
>
> > + */
> > + if (calibration_required) {
> > + channel_count = channel;
> > + 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;
>
> I think the above lines can be moved to function set_max_rate_and_channel
> as this is duplicated even in the else block
>
> > + return;
> > + }
> > + }
> > + } else {
> > + for (channel_count = channel; 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; }
> > +
> > +static int get_supported_freq_mask(struct intel_crtc_state
> > +*crtc_state) {
> > + int rate[] = { 32000, 44100, 48000, 88200, 96000, 176400, 192000 };
>
> So you do use the same array of rates maybe add these in the
> intel_crtc_state audio struct and which can be filled in
> intel_dp_compute_config , also mention where we get these rates from.
I don't think it's a good idea for this specific case, as we are not going to use it apart from above 2
instances and also these are common sample frequency being used from HDMI and DP spec.
So instead, we can reduce the redundancy. I will push that change with new patch set.
Also, adding max rate and max channel was added considering source restrictions which
we will address. But this rates array will be common to both it seems.
>
> > + int mask = 0, index;
> > +
> > + for (index = 0; index < ARRAY_SIZE(rate); index++) {
> > + if (rate[index] > crtc_state->audio.max_rate)
> > + break;
> > +
> > + mask |= 1 << index;
> > +
> > + if (crtc_state->audio.max_rate != rate[index])
> > + continue;
>
> Why are the above two lines of code needed?
> It's not like there is anything to skip below them.
>
> > + }
> > +
> > + return mask;
> > +}
> > +
> > +static void intel_audio_compute_eld(struct intel_crtc_state
> > +*crtc_state) {
>
> Lets not have this as a void function and lets return the appropriate errors If
> required
>
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > + u8 *eld, *sad;
> > + int index, mask = 0;
> > +
> > + eld = crtc_state->eld;
> > + if (!eld)
> > + return;
> > +
> > + sad = drm_extract_sad_from_eld(eld);
> > + if (!sad)
> > + return;
> > +
> > + calc_and_calibrate_audio_config_params(crtc_state,
> > MAX_CHANNEL_COUNT,
> > + false);
> > +
> > + mask = get_supported_freq_mask(crtc_state);
> > + for (index = 0; index < drm_eld_sad_count(eld); index++, sad += 3) {
> > + /*
> > + * Respect source restricitions. Limit capabilities to a subset
> > that is
> > + * supported both by the source and the sink.
> > + */
> > + if (sad_to_channels(sad) >= crtc_state-
> > >audio.max_channel_count) {
> > + sad[0] &= ~ELD_SAD_CHANNELS_MASK;
> > + sad[0] |= crtc_state->audio.max_channel_count - 1;
> > + drm_dbg_kms(&i915->drm, "Channel count is limited
> > to %d\n",
> > + crtc_state->audio.max_channel_count - 1);
> > + } else {
> > + /*
> > + * calibrate rate when, sink supported channel
> > + * count is slight less than max supported
>
> Typo: *slightly
>
> Regards,
> Suraj Kandpal
> > + * channel count.
> > + */
> > + calc_and_calibrate_audio_config_params(crtc_state,
> > +
> > sad_to_channels(sad),
> > + true);
> > + mask = get_supported_freq_mask(crtc_state);
> > + }
> > +
> > + sad[1] &= mask;
> > + }
> > +}
> > +
> > bool intel_audio_compute_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *crtc_state,
> > struct drm_connector_state *conn_state)
> @@
> > -791,6 +916,8 @@ bool intel_audio_compute_config(struct intel_encoder
> > *encoder,
> >
> > crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode)
> /
> > 2;
> >
> > + intel_audio_compute_eld(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;
> > } audio;
> >
> > /*
> > --
> > 2.25.1
