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

Reply via email to