[PATCH 5/9] drm/i915/display: check and prune audio frequencies based on bw limits

[Kai Vehmanen]

Hi,

On Tue, 8 Oct 2024, Vinod Govindapillai wrote:

> Calculate the audio bw requirements and check the supported sad
> audio frequencies are feasible with selected pipe configuration.
> If not feasible, prune the audio frequencies from sad list.

a bit of stretch for me to review the display portions, but I did have a 
go and I can see no blockers:
Reviewed-by: Kai Vehmanen <kai.vehmanen at linux.intel.com>

A few minor comments inline if you do update the series.

> @@ -705,6 +705,113 @@ static bool intel_audio_eld_valid(struct drm_connector_state *conn_state)
>  	return true;
>  }
>  
> +static bool
> +intel_audio_frequency_feasible(int line_freq_khz, int hblank_slots_lanes,
> +			       int avail_overhead, int req_overhead,
> +			       int channels, int aud_frequency)

minor note, looking at this from audio point of view (i.e. less background 
with display), I'd find this much easier to follow if the variables had
units in the variable name (like e.g. "line_freq_khz" does).

> +	int aud_samples_per_line =
> +		DIV_ROUND_UP(aud_frequency, line_freq_khz) + 1;
> +	int lines_per_audio_sample =
> +		max(1, line_freq_khz / aud_frequency);
> +	int hblank_bytes_available =
> +		(hblank_slots_lanes - avail_overhead) * lines_per_audio_sample;

E.g. this seems correct in the end, but took me a while to verify that 
hblank_slots_lanes really is bytes.

> +			drm_dbg_kms(&i915->drm, "sad updated. Pruned freq list: 0x%x\n", sad.freq);

Even if a little noisy, I think these are important to have as 
drm_dbg_kms.

> +static bool
> +intel_dp_audio_compute_bw_limits(struct intel_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +	int hblank_bytes_avail_overhead = 64;
> +	int hblank_bytes_req_overhead = 80;

These could perhaps be defines instead rather than magic values in the 
function?

Br, Kai