Re: [v4 2/2] drm/i915/audio: Prune ELD SADs based on HDMI audio bandwidth

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The bandwidth check incorrectly uses the maximum pixel clock instead of the minimum pixel clock to calculate worst-case audio packet accumulation per line.
--

--- Patch [2]: [v4 2/2] drm/i915/audio: Prune ELD SADs based on HDMI audio bandwidth ---
commit fa98484a48f1957f560711682a22233b5a6fa1f5
Author: Chaitanya Kumar Borah <[email protected]>

drm/i915/audio: Prune ELD SADs based on HDMI audio bandwidth

This commit adds a bandwidth check to determine whether a given audio
sample rate and channel count can be carried within the hblank period
for HDMI TMDS mode. It uses this check to prune unsupported sample rates
from each SAD in the ELD.

> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index eae76e9611055..9eb6867fa3f3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c

[ ... ]

> @@ -697,6 +698,158 @@ static void ibx_audio_codec_enable(struct intel_encoder *encoder,

[ ... ]

> +static bool hdmi_audio_rate_supported(const struct intel_crtc_state *crtc_state,
> +				      int available_tmds,
> +				      int audio_rate, int channels)
> +{
> +	const struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
> +	int pixel_clk_max_hz;
> +	int audio_pkt_factor;
> +	u64 audio_pkt_rate_x4_x1000;
> +	int audio_packets_line;
> +	int hblank_overhead;
> +	int required_tmds;
> +
> +	/*
> +	 * Part 2: Calculate TMDS clock cycles required for Audio Bandwidth
> +	 *
> +	 * Step 1: pixelclk_max = nominal_pixel_rate * (1 + 0.5%)
> +	 * crtc_clock (kHz) * 1000 * 1.005 = crtc_clock * 1005 (Hz)
> +	 */
> +	pixel_clk_max_hz = mode->crtc_clock * 1005;

[ ... ]

> +	/*
> +	 * Steps 2+5-6: Audio packets per line.
> +	 *   AudioPackets_Line = CEIL[R_AP * htotal / f_pixelclk_max]
> +	 *
> +	 * With audio_pkt_rate_x4_x1000 = R_AP * 4 * 1000:
> +	 *   = CEIL[audio_pkt_rate_x4_x1000 * htotal / (4 * 1000 * pixel_clk_max_hz)]
> +	 */
> +	audio_packets_line = DIV64_U64_ROUND_UP(audio_pkt_rate_x4_x1000 * mode->htotal,
> +						(u64)4 * 1000 * pixel_clk_max_hz);

[Severity: Medium]
To calculate the worst-case (maximum possible) required audio packets per line,
should this logic use the minimum possible pixel clock instead of the maximum?

A slower pixel clock means a video line takes longer to transmit in real time,
causing more audio packets to accumulate in the FIFO during that line. These
accumulated packets then require more TMDS bandwidth to transmit during the
corresponding blanking period.

By multiplying the nominal clock by 1005 (adding 0.5% tolerance) instead of
995 (subtracting 0.5% tolerance) and using it as a divisor here, the code
calculates the minimum required packets instead of the maximum. This optimistic
calculation could incorrectly allow audio configurations that will overflow
the HDMI audio FIFO and cause audio dropouts when the pixel clock runs at
nominal or slower speeds.

Although this matches the exact naming in the 'f_pixelclk_max' comment
(suggesting a likely typo in the underlying BSpec/HDMI hardware specification),
is it mathematically flawed for a worst-case capability bounds check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2