On Mon, 26 Jun 2023, Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx> 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.
>
> --v1:
> - call intel_audio_compute_eld in this commit as it is defined here
>
> --v2:
> - Handle case when max frequency is less than 32k.
> - remove drm prefix.
> - name change for parse_sad to eld_to_sad.
>
> --v3:
> - Use signed int wherever required.
> - add debug trace when channel is limited.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 69 ++++++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_audio.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> 3 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index e20ffc8e9654..1a1c773c1d41 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -794,6 +794,75 @@ bool intel_audio_compute_config(struct intel_encoder *encoder,
> return true;
> }
>
> +static int sad_to_channels(const u8 *sad)
> +{
> + return 1 + (sad[0] & 0x7);
> +}
> +
> +static inline u8 *eld_to_sad(u8 *eld)
Please don't use inline.
> +{
> + int ver, mnl;
> +
> + ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
> + if (ver != 2 && ver != 31)
> + return NULL;
> +
> + mnl = drm_eld_mnl(eld);
> + if (mnl > 16)
> + return NULL;
> +
> + return eld + DRM_ELD_CEA_SAD(mnl, 0);
Feels like this should be in drm_edid.[ch]... but hey, it actually is!
drm_eld_sad().
> +}
> +
> +static int get_supported_freq_mask(struct intel_crtc_state *crtc_state)
> +{
> + int audio_freq_hz[] = {32000, 44100, 48000, 88000, 96000, 176000, 192000, 0};
This needs a comment referencing the source spec, as apparently the
order is significant.
> + int mask = 0;
> +
> + for (u8 index = 0; index < ARRAY_SIZE(audio_freq_hz); index++) {
int index, and please declare it separately instead of within the for.
> + mask |= 1 << index;
This means the first one is always supported. Is that the case? What if
max_frequency is 0?
> + if (crtc_state->audio.max_frequency != audio_freq_hz[index])
> + continue;
> + else
> + break;
Maybe you mean:
if (audio_freq_hz[index] > crtc_state->audio.max_frequency)
break;
mask |= 1 << index;
> + }
> +
> + return 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;
> + int index, mask = 0;
> +
> + eld = crtc_state->eld;
> + if (!eld) {
> + drm_err(&i915->drm, "failed to locate eld\n");
It doesn't need to be an error.
> + return;
> + }
> +
> + sad = (u8 *)eld_to_sad(eld);
Unnecessary cast.
> + if (sad) {
if (!sad)
return;
and reduce indent below.
> + 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) {
> + sad[0] &= ~0x7;
> + sad[0] |= crtc_state->audio.max_channel - 1;
> + drm_dbg_kms(&i915->drm, "Channel count is limited to %d\n",
> + crtc_state->audio.max_channel - 1);
> + }
> +
> + sad[1] &= mask;
> + }
> + }
> +}
This should also be static within intel_audio.c.
> +
> /**
> * intel_audio_codec_enable - Enable the audio codec for HD audio
> * @encoder: encoder on which to enable audio
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.h b/drivers/gpu/drm/i915/display/intel_audio.h
> index be3edf9c4982..a0162cdc7999 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -17,6 +17,7 @@ struct intel_encoder;
> #define MAX_CHANNEL_COUNT 8
>
> void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> +void intel_audio_compute_eld(struct intel_crtc_state *crtc_state);
> bool intel_audio_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state *conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 6a4d477e8a15..daaa08c0ee47 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2402,6 +2402,8 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> return -EINVAL;
> }
>
> + intel_audio_compute_eld(pipe_config);
> +
> return 0;
> }
--
Jani Nikula, Intel Open Source Graphics Center
