[Intel-gfx] [RFC 3/3] drm/i915/display: Add wrapper to Compute SAD

[Kai Vehmanen]

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