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

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

[Jani Nikula]

On Tue, 07 Apr 2020, Uma Shankar <uma.shankar@intel.com> wrote:
> For certain DP VDSC bpp settings, hblank asserts before hblank_early,
> leading to a bad audio state. Driver need to program "hblank early
> enable" and "samples per line" parameters in AUDIO_CONFIG_BE
> register.
>
> This is Display Audio WA #1406928334 for 4k+VDSC usecase
> applicable on DP encoders. Implemented the same.
>
> v2: Fixed build failures on 32bit machine.
>
> v3: Dropped u64, added helpers for sample room calculation,
>     other general comments as per Jani Nikula's feedback.
>     Also fixed connector type check (spotted by Anshuman)
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c | 146 +++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  16 +++
>  2 files changed, 162 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index 950160f1a89f..56fd17b65ce0 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -512,6 +512,148 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder,
>  	mutex_unlock(&dev_priv->av_mutex);
>  }
>  
> +/* Add a factor to take care of rounding and truncations */
> +#define ROUNDING_FACTOR 10000
> +static int set_hblank_early_enable_config(struct intel_encoder *encoder,
> +					  const struct intel_crtc_state *crtc_state,
> +					  u32 *val)
> +{
> +	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;
> +
> +	h_active = crtc_state->hw.adjusted_mode.hdisplay;
> +	h_total = crtc_state->hw.adjusted_mode.htotal;
> +	v_total = crtc_state->hw.adjusted_mode.vtotal;
> +	hblank_rise = crtc_state->hw.adjusted_mode.hsync_start;
> +	pixel_clk = crtc_state->hw.adjusted_mode.clock;
> +	refresh_rate = crtc_state->hw.adjusted_mode.vrefresh;
> +	cdclk = i915->cdclk.hw.cdclk;
> +	/* fec= 0.972261, using rounding multiplier of 1000000 */
> +	fec_coeff = 972261;
> +
> +	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;
> +	}
> +
> +	drm_dbg_kms(&i915->drm, "h_active = %u link_clk = %u :"
> +		    "lanes = %u vdsc_bpp = %u cdclk = %u\n",
> +		    h_active, crtc_state->port_clock, crtc_state->lane_count,
> +		    crtc_state->pipe_bpp, cdclk);
> +
> +	link_clks_available = ((((h_total - h_active) *
> +			       ((crtc_state->port_clock * ROUNDING_FACTOR) /
> +				pixel_clk)) / ROUNDING_FACTOR) - 28);
> +
> +	link_clks_required = DIV_ROUND_UP(192000, (refresh_rate *
> +					  v_total)) * ((48 /
> +					  crtc_state->lane_count) + 2);
> +
> +	if (link_clks_available > link_clks_required)
> +		hblank_delta = 32;
> +	else
> +		hblank_delta = DIV_ROUND_UP(((((5 * ROUNDING_FACTOR) /
> +					    crtc_state->port_clock) + ((5 *
> +					    ROUNDING_FACTOR) /
> +					    cdclk)) * pixel_clk),
> +					    ROUNDING_FACTOR);
> +
> +	tu_data = (pixel_clk * crtc_state->pipe_bpp * 8) /
> +		   ((crtc_state->port_clock *
> +		   crtc_state->lane_count * fec_coeff) / 1000000);
> +	tu_line = (((h_active * crtc_state->port_clock * fec_coeff) /
> +		   1000000) / (64 * pixel_clk));
> +	link_clks_active  = (tu_line - 1) * 64 + tu_data;
> +
> +	hblank_rise = ((link_clks_active + 6 * DIV_ROUND_UP(link_clks_active,
> +			250) + 4) * ((pixel_clk * ROUNDING_FACTOR) /
> +			crtc_state->port_clock)) / ROUNDING_FACTOR;
> +
> +	hblank_early_prog = h_active - hblank_rise + hblank_delta;
> +
> +	if (hblank_early_prog < 32) {
> +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_32);
> +	} else if (hblank_early_prog < 64) {
> +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_64);
> +	} else if (hblank_early_prog < 96) {
> +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_96);
> +	} else {
> +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_128);
> +	}
> +
> +	return 0;
> +}
> +
> +static void set_sample_room_config(const struct intel_crtc_state *crtc_state,
> +				   u32 *val)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	enum pipe pipe = crtc->pipe;
> +	u32 h_active, h_total, pixel_clk;
> +	u32 samples_room;
> +
> +	h_active = crtc_state->hw.adjusted_mode.hdisplay;
> +	h_total = crtc_state->hw.adjusted_mode.htotal;
> +	pixel_clk = crtc_state->hw.adjusted_mode.clock;
> +
> +	samples_room = ((((h_total - h_active) * ((crtc_state->port_clock *
> +			ROUNDING_FACTOR) / pixel_clk)) /
> +			ROUNDING_FACTOR) - 12) / ((48 /
> +			crtc_state->lane_count) + 2);
> +
> +	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);
> +	}
> +}
> +
> +static void enable_audio_dsc_wa(struct intel_encoder *encoder,
> +				const struct intel_crtc_state *crtc_state)
> +{
> +	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 val;
> +
> +	if (INTEL_GEN(i915) < 11)
> +		return;
> +
> +	val = intel_de_read(i915, AUD_CONFIG_BE);
> +
> +	if (INTEL_GEN(i915) == 11)
> +		val |= HBLANK_EARLY_ENABLE_ICL(pipe);
> +	else if (INTEL_GEN(i915) >= 12)
> +		val |= HBLANK_EARLY_ENABLE_TGL(pipe);
> +
> +	if (crtc_state->dsc.compression_enable &&
> +	    (crtc_state->hw.adjusted_mode.hdisplay >= 3840 &&
> +	     crtc_state->hw.adjusted_mode.vdisplay >= 2160)) {
> +		if (set_hblank_early_enable_config(encoder, crtc_state, &val))
> +			return;
> +
> +		set_sample_room_config(crtc_state, &val);

Communication is hard. I tried to imply that you'd add helpers that
*return* the values. Then the computations get moved to the helpers, yet
the modifications of the local variable val remain here. The split is
clear, and easy to follow.

BR,
Jani.


> +	}
> +
> +	intel_de_write(i915, AUD_CONFIG_BE, val);
> +}
> +
> +#undef ROUNDING_FACTOR
> +
>  static void hsw_audio_codec_enable(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state)
> @@ -529,6 +671,10 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder,
>  
>  	mutex_lock(&dev_priv->av_mutex);
>  
> +	/* Enable Audio WA for 4k DSC usecases */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> +		enable_audio_dsc_wa(encoder, crtc_state);
> +
>  	/* Enable audio presence detect, invalidate ELD */
>  	tmp = intel_de_read(dev_priv, HSW_AUD_PIN_ELD_CP_VLD);
>  	tmp |= AUDIO_OUTPUT_ENABLE(cpu_transcoder);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8cebb7a86b8c..f72ea2c2a8e3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9395,6 +9395,22 @@ enum {
>  #define AUD_PIN_BUF_CTL		_MMIO(0x48414)
>  #define   AUD_PIN_BUF_ENABLE		REG_BIT(31)
>  
> +/* Display Audio Config Reg */
> +#define AUD_CONFIG_BE			_MMIO(0x65ef0)
> +#define HBLANK_EARLY_ENABLE_ICL(pipe)		(0x1 << (20 - (pipe)))
> +#define HBLANK_EARLY_ENABLE_TGL(pipe)		(0x1 << (24 + (pipe)))
> +#define HBLANK_START_COUNT_MASK(pipe)		(0x7 << (3 + ((pipe) * 6)))
> +#define HBLANK_START_COUNT(pipe, val)		(((val) & 0x7) << (3 + ((pipe)) * 6))
> +#define NUMBER_SAMPLES_PER_LINE_MASK(pipe)	(0x3 << ((pipe) * 6))
> +#define NUMBER_SAMPLES_PER_LINE(pipe, val)	(((val) & 0x3) << ((pipe) * 6))
> +
> +#define HBLANK_START_COUNT_8	0
> +#define HBLANK_START_COUNT_16	1
> +#define HBLANK_START_COUNT_32	2
> +#define HBLANK_START_COUNT_64	3
> +#define HBLANK_START_COUNT_96	4
> +#define HBLANK_START_COUNT_128	5
> +
>  /*
>   * HSW - ICL power wells
>   *

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Uma Shankar]

For certain DP VDSC bpp settings, hblank asserts before hblank_early,
leading to a bad audio state. Driver need to program "hblank early
enable" and "samples per line" parameters in AUDIO_CONFIG_BE
register.

This is Display Audio WA #1406928334 for 4k+VDSC usecase
applicable on DP encoders. Implemented the same.

v2: Fixed build failures on 32bit machine.

v3: Dropped u64, added helpers for sample room calculation,
    other general comments as per Jani Nikula's feedback.
    Also fixed connector type check (spotted by Anshuman)

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/display/intel_audio.c | 146 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  16 +++
 2 files changed, 162 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 950160f1a89f..56fd17b65ce0 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -512,6 +512,148 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder,
 	mutex_unlock(&dev_priv->av_mutex);
 }
 
+/* Add a factor to take care of rounding and truncations */
+#define ROUNDING_FACTOR 10000
+static int set_hblank_early_enable_config(struct intel_encoder *encoder,
+					  const struct intel_crtc_state *crtc_state,
+					  u32 *val)
+{
+	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;
+
+	h_active = crtc_state->hw.adjusted_mode.hdisplay;
+	h_total = crtc_state->hw.adjusted_mode.htotal;
+	v_total = crtc_state->hw.adjusted_mode.vtotal;
+	hblank_rise = crtc_state->hw.adjusted_mode.hsync_start;
+	pixel_clk = crtc_state->hw.adjusted_mode.clock;
+	refresh_rate = crtc_state->hw.adjusted_mode.vrefresh;
+	cdclk = i915->cdclk.hw.cdclk;
+	/* fec= 0.972261, using rounding multiplier of 1000000 */
+	fec_coeff = 972261;
+
+	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;
+	}
+
+	drm_dbg_kms(&i915->drm, "h_active = %u link_clk = %u :"
+		    "lanes = %u vdsc_bpp = %u cdclk = %u\n",
+		    h_active, crtc_state->port_clock, crtc_state->lane_count,
+		    crtc_state->pipe_bpp, cdclk);
+
+	link_clks_available = ((((h_total - h_active) *
+			       ((crtc_state->port_clock * ROUNDING_FACTOR) /
+				pixel_clk)) / ROUNDING_FACTOR) - 28);
+
+	link_clks_required = DIV_ROUND_UP(192000, (refresh_rate *
+					  v_total)) * ((48 /
+					  crtc_state->lane_count) + 2);
+
+	if (link_clks_available > link_clks_required)
+		hblank_delta = 32;
+	else
+		hblank_delta = DIV_ROUND_UP(((((5 * ROUNDING_FACTOR) /
+					    crtc_state->port_clock) + ((5 *
+					    ROUNDING_FACTOR) /
+					    cdclk)) * pixel_clk),
+					    ROUNDING_FACTOR);
+
+	tu_data = (pixel_clk * crtc_state->pipe_bpp * 8) /
+		   ((crtc_state->port_clock *
+		   crtc_state->lane_count * fec_coeff) / 1000000);
+	tu_line = (((h_active * crtc_state->port_clock * fec_coeff) /
+		   1000000) / (64 * pixel_clk));
+	link_clks_active  = (tu_line - 1) * 64 + tu_data;
+
+	hblank_rise = ((link_clks_active + 6 * DIV_ROUND_UP(link_clks_active,
+			250) + 4) * ((pixel_clk * ROUNDING_FACTOR) /
+			crtc_state->port_clock)) / ROUNDING_FACTOR;
+
+	hblank_early_prog = h_active - hblank_rise + hblank_delta;
+
+	if (hblank_early_prog < 32) {
+		*val &= ~HBLANK_START_COUNT_MASK(pipe);
+		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_32);
+	} else if (hblank_early_prog < 64) {
+		*val &= ~HBLANK_START_COUNT_MASK(pipe);
+		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_64);
+	} else if (hblank_early_prog < 96) {
+		*val &= ~HBLANK_START_COUNT_MASK(pipe);
+		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_96);
+	} else {
+		*val &= ~HBLANK_START_COUNT_MASK(pipe);
+		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_128);
+	}
+
+	return 0;
+}
+
+static void set_sample_room_config(const struct intel_crtc_state *crtc_state,
+				   u32 *val)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	enum pipe pipe = crtc->pipe;
+	u32 h_active, h_total, pixel_clk;
+	u32 samples_room;
+
+	h_active = crtc_state->hw.adjusted_mode.hdisplay;
+	h_total = crtc_state->hw.adjusted_mode.htotal;
+	pixel_clk = crtc_state->hw.adjusted_mode.clock;
+
+	samples_room = ((((h_total - h_active) * ((crtc_state->port_clock *
+			ROUNDING_FACTOR) / pixel_clk)) /
+			ROUNDING_FACTOR) - 12) / ((48 /
+			crtc_state->lane_count) + 2);
+
+	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);
+	}
+}
+
+static void enable_audio_dsc_wa(struct intel_encoder *encoder,
+				const struct intel_crtc_state *crtc_state)
+{
+	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 val;
+
+	if (INTEL_GEN(i915) < 11)
+		return;
+
+	val = intel_de_read(i915, AUD_CONFIG_BE);
+
+	if (INTEL_GEN(i915) == 11)
+		val |= HBLANK_EARLY_ENABLE_ICL(pipe);
+	else if (INTEL_GEN(i915) >= 12)
+		val |= HBLANK_EARLY_ENABLE_TGL(pipe);
+
+	if (crtc_state->dsc.compression_enable &&
+	    (crtc_state->hw.adjusted_mode.hdisplay >= 3840 &&
+	     crtc_state->hw.adjusted_mode.vdisplay >= 2160)) {
+		if (set_hblank_early_enable_config(encoder, crtc_state, &val))
+			return;
+
+		set_sample_room_config(crtc_state, &val);
+	}
+
+	intel_de_write(i915, AUD_CONFIG_BE, val);
+}
+
+#undef ROUNDING_FACTOR
+
 static void hsw_audio_codec_enable(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state)
@@ -529,6 +671,10 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder,
 
 	mutex_lock(&dev_priv->av_mutex);
 
+	/* Enable Audio WA for 4k DSC usecases */
+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
+		enable_audio_dsc_wa(encoder, crtc_state);
+
 	/* Enable audio presence detect, invalidate ELD */
 	tmp = intel_de_read(dev_priv, HSW_AUD_PIN_ELD_CP_VLD);
 	tmp |= AUDIO_OUTPUT_ENABLE(cpu_transcoder);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8cebb7a86b8c..f72ea2c2a8e3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9395,6 +9395,22 @@ enum {
 #define AUD_PIN_BUF_CTL		_MMIO(0x48414)
 #define   AUD_PIN_BUF_ENABLE		REG_BIT(31)
 
+/* Display Audio Config Reg */
+#define AUD_CONFIG_BE			_MMIO(0x65ef0)
+#define HBLANK_EARLY_ENABLE_ICL(pipe)		(0x1 << (20 - (pipe)))
+#define HBLANK_EARLY_ENABLE_TGL(pipe)		(0x1 << (24 + (pipe)))
+#define HBLANK_START_COUNT_MASK(pipe)		(0x7 << (3 + ((pipe) * 6)))
+#define HBLANK_START_COUNT(pipe, val)		(((val) & 0x7) << (3 + ((pipe)) * 6))
+#define NUMBER_SAMPLES_PER_LINE_MASK(pipe)	(0x3 << ((pipe) * 6))
+#define NUMBER_SAMPLES_PER_LINE(pipe, val)	(((val) & 0x3) << ((pipe) * 6))
+
+#define HBLANK_START_COUNT_8	0
+#define HBLANK_START_COUNT_16	1
+#define HBLANK_START_COUNT_32	2
+#define HBLANK_START_COUNT_64	3
+#define HBLANK_START_COUNT_96	4
+#define HBLANK_START_COUNT_128	5
+
 /*
  * HSW - ICL power wells
  *
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shankar, Uma]

> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Sent: Tuesday, April 7, 2020 7:42 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Vehmanen, Kai <kai.vehmanen@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/display: Enable DP Display Audio WA
> 
> 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?

No major reasons as such. Will switch to unsigned int.

> > +	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"..?

Ok Sure, will improve this message.
 
> > +	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.

Sure, will add comments to make this more clear.

Thanks Kai for the feedback. Will address and send the next version.

Regards,
Uma Shankar

> Br, Kai
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shankar, Uma]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Tuesday, April 7, 2020 7:26 PM
> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Vehmanen, Kai <kai.vehmanen@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH v3] drm/i915/display: Enable DP Display Audio WA
> 
> On Tue, 07 Apr 2020, Uma Shankar <uma.shankar@intel.com> wrote:
> > For certain DP VDSC bpp settings, hblank asserts before hblank_early,
> > leading to a bad audio state. Driver need to program "hblank early
> > enable" and "samples per line" parameters in AUDIO_CONFIG_BE register.
> >
> > This is Display Audio WA #1406928334 for 4k+VDSC usecase applicable on
> > DP encoders. Implemented the same.
> >
> > v2: Fixed build failures on 32bit machine.
> >
> > v3: Dropped u64, added helpers for sample room calculation,
> >     other general comments as per Jani Nikula's feedback.
> >     Also fixed connector type check (spotted by Anshuman)
> >
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_audio.c | 146 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h            |  16 +++
> >  2 files changed, 162 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> > b/drivers/gpu/drm/i915/display/intel_audio.c
> > index 950160f1a89f..56fd17b65ce0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> > @@ -512,6 +512,148 @@ static void hsw_audio_codec_disable(struct
> intel_encoder *encoder,
> >  	mutex_unlock(&dev_priv->av_mutex);
> >  }
> >
> > +/* Add a factor to take care of rounding and truncations */ #define
> > +ROUNDING_FACTOR 10000 static int
> > +set_hblank_early_enable_config(struct intel_encoder *encoder,
> > +					  const struct intel_crtc_state *crtc_state,
> > +					  u32 *val)
> > +{
> > +	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;
> > +
> > +	h_active = crtc_state->hw.adjusted_mode.hdisplay;
> > +	h_total = crtc_state->hw.adjusted_mode.htotal;
> > +	v_total = crtc_state->hw.adjusted_mode.vtotal;
> > +	hblank_rise = crtc_state->hw.adjusted_mode.hsync_start;
> > +	pixel_clk = crtc_state->hw.adjusted_mode.clock;
> > +	refresh_rate = crtc_state->hw.adjusted_mode.vrefresh;
> > +	cdclk = i915->cdclk.hw.cdclk;
> > +	/* fec= 0.972261, using rounding multiplier of 1000000 */
> > +	fec_coeff = 972261;
> > +
> > +	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;
> > +	}
> > +
> > +	drm_dbg_kms(&i915->drm, "h_active = %u link_clk = %u :"
> > +		    "lanes = %u vdsc_bpp = %u cdclk = %u\n",
> > +		    h_active, crtc_state->port_clock, crtc_state->lane_count,
> > +		    crtc_state->pipe_bpp, cdclk);
> > +
> > +	link_clks_available = ((((h_total - h_active) *
> > +			       ((crtc_state->port_clock * ROUNDING_FACTOR) /
> > +				pixel_clk)) / ROUNDING_FACTOR) - 28);
> > +
> > +	link_clks_required = DIV_ROUND_UP(192000, (refresh_rate *
> > +					  v_total)) * ((48 /
> > +					  crtc_state->lane_count) + 2);
> > +
> > +	if (link_clks_available > link_clks_required)
> > +		hblank_delta = 32;
> > +	else
> > +		hblank_delta = DIV_ROUND_UP(((((5 * ROUNDING_FACTOR) /
> > +					    crtc_state->port_clock) + ((5 *
> > +					    ROUNDING_FACTOR) /
> > +					    cdclk)) * pixel_clk),
> > +					    ROUNDING_FACTOR);
> > +
> > +	tu_data = (pixel_clk * crtc_state->pipe_bpp * 8) /
> > +		   ((crtc_state->port_clock *
> > +		   crtc_state->lane_count * fec_coeff) / 1000000);
> > +	tu_line = (((h_active * crtc_state->port_clock * fec_coeff) /
> > +		   1000000) / (64 * pixel_clk));
> > +	link_clks_active  = (tu_line - 1) * 64 + tu_data;
> > +
> > +	hblank_rise = ((link_clks_active + 6 * DIV_ROUND_UP(link_clks_active,
> > +			250) + 4) * ((pixel_clk * ROUNDING_FACTOR) /
> > +			crtc_state->port_clock)) / ROUNDING_FACTOR;
> > +
> > +	hblank_early_prog = h_active - hblank_rise + hblank_delta;
> > +
> > +	if (hblank_early_prog < 32) {
> > +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> > +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_32);
> > +	} else if (hblank_early_prog < 64) {
> > +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> > +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_64);
> > +	} else if (hblank_early_prog < 96) {
> > +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> > +		*val |= HBLANK_START_COUNT(pipe, HBLANK_START_COUNT_96);
> > +	} else {
> > +		*val &= ~HBLANK_START_COUNT_MASK(pipe);
> > +		*val |= HBLANK_START_COUNT(pipe,
> HBLANK_START_COUNT_128);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void set_sample_room_config(const struct intel_crtc_state *crtc_state,
> > +				   u32 *val)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	enum pipe pipe = crtc->pipe;
> > +	u32 h_active, h_total, pixel_clk;
> > +	u32 samples_room;
> > +
> > +	h_active = crtc_state->hw.adjusted_mode.hdisplay;
> > +	h_total = crtc_state->hw.adjusted_mode.htotal;
> > +	pixel_clk = crtc_state->hw.adjusted_mode.clock;
> > +
> > +	samples_room = ((((h_total - h_active) * ((crtc_state->port_clock *
> > +			ROUNDING_FACTOR) / pixel_clk)) /
> > +			ROUNDING_FACTOR) - 12) / ((48 /
> > +			crtc_state->lane_count) + 2);
> > +
> > +	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);
> > +	}
> > +}
> > +
> > +static void enable_audio_dsc_wa(struct intel_encoder *encoder,
> > +				const struct intel_crtc_state *crtc_state) {
> > +	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 val;
> > +
> > +	if (INTEL_GEN(i915) < 11)
> > +		return;
> > +
> > +	val = intel_de_read(i915, AUD_CONFIG_BE);
> > +
> > +	if (INTEL_GEN(i915) == 11)
> > +		val |= HBLANK_EARLY_ENABLE_ICL(pipe);
> > +	else if (INTEL_GEN(i915) >= 12)
> > +		val |= HBLANK_EARLY_ENABLE_TGL(pipe);
> > +
> > +	if (crtc_state->dsc.compression_enable &&
> > +	    (crtc_state->hw.adjusted_mode.hdisplay >= 3840 &&
> > +	     crtc_state->hw.adjusted_mode.vdisplay >= 2160)) {
> > +		if (set_hblank_early_enable_config(encoder, crtc_state, &val))
> > +			return;
> > +
> > +		set_sample_room_config(crtc_state, &val);
> 
> Communication is hard. I tried to imply that you'd add helpers that
> *return* the values. Then the computations get moved to the helpers, yet the
> modifications of the local variable val remain here. The split is clear, and easy to
> follow.

Oops, got your point Jani. Will add the helpers accordingly to simplify this.

Regards,
Uma Shankar

> BR,
> Jani.
> 
> 
> > +	}
> > +
> > +	intel_de_write(i915, AUD_CONFIG_BE, val); }
> > +
> > +#undef ROUNDING_FACTOR
> > +
> >  static void hsw_audio_codec_enable(struct intel_encoder *encoder,
> >  				   const struct intel_crtc_state *crtc_state,
> >  				   const struct drm_connector_state *conn_state)
> @@ -529,6
> > +671,10 @@ static void hsw_audio_codec_enable(struct intel_encoder
> > *encoder,
> >
> >  	mutex_lock(&dev_priv->av_mutex);
> >
> > +	/* Enable Audio WA for 4k DSC usecases */
> > +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> > +		enable_audio_dsc_wa(encoder, crtc_state);
> > +
> >  	/* Enable audio presence detect, invalidate ELD */
> >  	tmp = intel_de_read(dev_priv, HSW_AUD_PIN_ELD_CP_VLD);
> >  	tmp |= AUDIO_OUTPUT_ENABLE(cpu_transcoder);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 8cebb7a86b8c..f72ea2c2a8e3
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9395,6 +9395,22 @@ enum {
> >  #define AUD_PIN_BUF_CTL		_MMIO(0x48414)
> >  #define   AUD_PIN_BUF_ENABLE		REG_BIT(31)
> >
> > +/* Display Audio Config Reg */
> > +#define AUD_CONFIG_BE			_MMIO(0x65ef0)
> > +#define HBLANK_EARLY_ENABLE_ICL(pipe)		(0x1 << (20 - (pipe)))
> > +#define HBLANK_EARLY_ENABLE_TGL(pipe)		(0x1 << (24 + (pipe)))
> > +#define HBLANK_START_COUNT_MASK(pipe)		(0x7 << (3 + ((pipe) * 6)))
> > +#define HBLANK_START_COUNT(pipe, val)		(((val) & 0x7) << (3 +
> ((pipe)) * 6))
> > +#define NUMBER_SAMPLES_PER_LINE_MASK(pipe)	(0x3 << ((pipe) * 6))
> > +#define NUMBER_SAMPLES_PER_LINE(pipe, val)	(((val) & 0x3) << ((pipe) *
> 6))
> > +
> > +#define HBLANK_START_COUNT_8	0
> > +#define HBLANK_START_COUNT_16	1
> > +#define HBLANK_START_COUNT_32	2
> > +#define HBLANK_START_COUNT_64	3
> > +#define HBLANK_START_COUNT_96	4
> > +#define HBLANK_START_COUNT_128	5
> > +
> >  /*
> >   * HSW - ICL power wells
> >   *
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx