Hi,
[+Jyri]
On Fri, 9 Jun 2023, Mitul Golani wrote:
> Compute SADs that takes into account the supported rate and channel
> based on the capabilities of the audio source. This wrapper function
> should encapsulate the logic for determining the supported rate and
> channel and should return a set of SADs that are compatible with the
> source.
In general looks good. A few minor comments inline:
> +static u8 get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000,
> 192000, 0};
> + u8 mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
Minor nitpick: the use of "u8" in many places seems a bit misleading. It
seems for many places (like the "index" here), you can just use int.
But right, the SAD mask is 8bit, so maybe the get_support_freq_mask()
is still warranted to return a u8 mask.
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> + u8 *eld, *sad, index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(&i915->drm, "failed to locate eld\n");
> + return;
> + }
> +
> + sad = (u8 *)parse_sad(eld);
> + if (sad) {
> + mask = get_supported_freq_mask(crtc_state);
> +
> + for (index = 0; index < drm_eld_sad_count(eld); index++, sad +=
> 3) {
> + /*
> + * Respect to source restrictions. If source limit is
> greater than sink
> + * capabilities then follow to sink's highest
> supported rate.
> + */
Minor: maybe reword "Respect source restricitions. Limit capabilities to a
subset that is supported both by the source and the sink."?
> + if (drm_sad_to_channels(sad) >=
> crtc_state->audio.max_channel) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
Can we add a debug trace here in case the channel count is limited?
Br, Kai
