[Intel-gfx] [PATCH v3] drm/i915/display: Enable DP Display Audio WA

[Kai Vehmanen]

Hi,

thanks Uma! It's good to see the implementation is this localized and 
doesn't need changes elsewhere. Other reviewers already covered most 
parts, but a few notes:

On Tue, 7 Apr 2020, Uma Shankar wrote:

> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	enum pipe pipe = crtc->pipe;
> +	u32 link_clks_available, link_clks_required;
> +	u32 tu_data, tu_line, link_clks_active;
> +	u32 hblank_rise, hblank_early_prog;
> +	u32 h_active, h_total, hblank_delta, pixel_clk, v_total;
> +	u32 fec_coeff, refresh_rate, cdclk;

hmm, minor thing, but why are these u32 and not just unsigned ints? 

> +	if (!(h_active && crtc_state->port_clock && crtc_state->lane_count &&
> +	      crtc_state->pipe_bpp && cdclk)) {
> +		drm_err(&i915->drm, "Null Parameters received\n");
> +		WARN_ON(1);
> +		return -EINVAL;

This is still not very informative. "Invalid parameters for 
hblank_early"..?

> +	if (samples_room < 3) {
> +		*val &= ~NUMBER_SAMPLES_PER_LINE_MASK(pipe);
> +		*val |= NUMBER_SAMPLES_PER_LINE(pipe, samples_room);
> +	} else {
> +		*val &= ~NUMBER_SAMPLES_PER_LINE_MASK(pipe);
> +		*val |= NUMBER_SAMPLES_PER_LINE(pipe, 0x0);
> +	}

This is a bit hard to follow in terms of logic. Maybe worth a comment 
that 0x0 means to take all samples from the buffer. 

Br, Kai