> 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
> +
> 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
> + 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.
> + 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
