diff mbox

drm/i915: Try to shut up more ILK underruns

Message ID 1458746975-11153-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä March 23, 2016, 3:29 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Take a bigger hammer to the underrun suppression on ILK. Instead of
trying to suppress them at specific points in the modeset sequence just
silence them across the entire sequence. This gets rid of some underruns
at least on my ILK. Note that this changes SNB and IVB to follow the
same approach just to keep the code less convoluted. The difference is
that on those platforms we won't suppress CPU underruns for port A since
it doesn't seem to be necessary.

My ILK has port A eDP and two PCH HDMI ports, so I can't be sure this is
as effective on other PCH port types. Perhaps we still need some of
Daniel's extra vblank waits [2]?

I've still been able to trigger an underrun on the other pipe, but
fixing that perhaps needs the LP1+ disable trick I implemented here [1]
which never got merged.

A few details which hamper stress testing on my ILK are that sometimes
the PCH transcoder gets messed up and refuses to shut down, and sometimes
even the panel power sequencer apparently gets stuck on the always on
position.

[1] https://lists.freedesktop.org/archives/intel-gfx/2014-March/041317.html
[2] https://lists.freedesktop.org/archives/intel-gfx/2016-January/086397.html

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_dp.c      | 12 ----------
 2 files changed, 22 insertions(+), 34 deletions(-)

Comments

Daniel Vetter March 23, 2016, 4:32 p.m. UTC | #1 
On Wed, Mar 23, 2016 at 05:29:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Take a bigger hammer to the underrun suppression on ILK. Instead of
> trying to suppress them at specific points in the modeset sequence just
> silence them across the entire sequence. This gets rid of some underruns
> at least on my ILK. Note that this changes SNB and IVB to follow the
> same approach just to keep the code less convoluted. The difference is
> that on those platforms we won't suppress CPU underruns for port A since
> it doesn't seem to be necessary.
> 
> My ILK has port A eDP and two PCH HDMI ports, so I can't be sure this is
> as effective on other PCH port types. Perhaps we still need some of
> Daniel's extra vblank waits [2]?
> 
> I've still been able to trigger an underrun on the other pipe, but
> fixing that perhaps needs the LP1+ disable trick I implemented here [1]
> which never got merged.
> 
> A few details which hamper stress testing on my ILK are that sometimes
> the PCH transcoder gets messed up and refuses to shut down, and sometimes
> even the panel power sequencer apparently gets stuck on the always on
> position.
> 
> [1] https://lists.freedesktop.org/archives/intel-gfx/2014-March/041317.html
> [2] https://lists.freedesktop.org/archives/intel-gfx/2016-January/086397.html
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Agreed this seems sensible - trying to make it all work in the different
cases looks like wasted effort given that we don't plan to ever touch this
code again. For vlv and definitely for ddi platforms we should take a more
measure approach probably, to not reduce test coverage too much.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Wrt the remaining underrun's youre seeing: I'd say let's merge this first,
then figure out how much ilk underruns are still left. Absolutely worst
case we just have to shut them off completely and tune down to debug
level.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_dp.c      | 12 ----------
>  2 files changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 47332a164fcb..44fb3ba4c795 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4128,12 +4128,6 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  	I915_WRITE(FDI_RX_TUSIZE1(pipe),
>  		   I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
>  
> -	/*
> -	 * Sometimes spurious CPU pipe underruns happen during FDI
> -	 * training, at least with VGA+HDMI cloning. Suppress them.
> -	 */
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>  	/* For PCH output, training FDI link */
>  	dev_priv->display.fdi_link_train(crtc);
>  
> @@ -4168,8 +4162,6 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  
>  	intel_fdi_normal_train(crtc);
>  
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -
>  	/* For PCH DP, enable TRANS_DP_CTL */
>  	if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
>  		const struct drm_display_mode *adjusted_mode =
> @@ -4767,6 +4759,17 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	if (WARN_ON(intel_crtc->active))
>  		return;
>  
> +	/*
> +	 * Sometimes spurious CPU pipe underruns happen during FDI
> +	 * training, at least with VGA+HDMI cloning. Suppress them.
> +	 *
> +	 * On ILK we get an occasional spurious CPU pipe underruns
> +	 * between eDP port A enable and vdd enable.
> +	 *
> +	 * Spurious PCH underruns also occur during PCH enabling.
> +	 */
> +	if (intel_crtc->config->has_pch_encoder || IS_GEN5(dev_priv))
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  	if (intel_crtc->config->has_pch_encoder)
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
>  
> @@ -4788,8 +4791,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = true;
>  
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_enable)
>  			encoder->pre_enable(encoder);
> @@ -4831,6 +4832,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	/* Must wait for vblank to avoid spurious PCH FIFO underruns */
>  	if (intel_crtc->config->has_pch_encoder)
>  		intel_wait_for_vblank(dev, pipe);
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>  }
>  
> @@ -4983,8 +4985,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	if (intel_crtc->config->has_pch_encoder)
> +	/*
> +	 * Sometimes spurious CPU pipe underruns happen when the
> +	 * pipe is already disabled, but FDI RX/TX is still enabled.
> +	 * Happens at least with VGA+HDMI cloning. Suppress them.
> +	 */
> +	if (intel_crtc->config->has_pch_encoder) {
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> +	}
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
> @@ -4992,22 +5001,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	drm_crtc_vblank_off(crtc);
>  	assert_vblank_disabled(crtc);
>  
> -	/*
> -	 * Sometimes spurious CPU pipe underruns happen when the
> -	 * pipe is already disabled, but FDI RX/TX is still enabled.
> -	 * Happens at least with VGA+HDMI cloning. Suppress them.
> -	 */
> -	if (intel_crtc->config->has_pch_encoder)
> -		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>  	intel_disable_pipe(intel_crtc);
>  
>  	ironlake_pfit_disable(intel_crtc, false);
>  
> -	if (intel_crtc->config->has_pch_encoder) {
> +	if (intel_crtc->config->has_pch_encoder)
>  		ironlake_fdi_disable(crtc);
> -		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -	}
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->post_disable)
> @@ -5037,6 +5036,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
>  
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3ff8f1d67594..dd8793ff3720 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2640,15 +2640,6 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>  		vlv_init_panel_power_sequencer(intel_dp);
>  
> -	/*
> -	 * We get an occasional spurious underrun between the port
> -	 * enable and vdd enable, when enabling port A eDP.
> -	 *
> -	 * FIXME: Not sure if this applies to (PCH) port D eDP as well
> -	 */
> -	if (port == PORT_A)
> -		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>  	intel_dp_enable_port(intel_dp);
>  
>  	if (port == PORT_A && IS_GEN5(dev_priv)) {
> @@ -2666,9 +2657,6 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	edp_panel_on(intel_dp);
>  	edp_panel_vdd_off(intel_dp, true);
>  
> -	if (port == PORT_A)
> -		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -
>  	pps_unlock(intel_dp);
>  
>  	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> -- 
> 2.7.4
>
Ville Syrjälä March 23, 2016, 6 p.m. UTC | #2 
On Wed, Mar 23, 2016 at 04:33:18PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Try to shut up more ILK underruns
> URL   : https://patchwork.freedesktop.org/series/4811/
> State : failure
> 
> == Summary ==
> 
> Series 4811v1 drm/i915: Try to shut up more ILK underruns
> http://patchwork.freedesktop.org/api/1.0/series/4811/revisions/1/mbox/
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-modeset:
>                 pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE

[  326.096731] [drm:intel_enable_pipe] enabling pipe B
[  326.097054] [drm:ironlake_fdi_link_train] FDI_RX_IIR 0x100
[  326.097058] [drm:ironlake_fdi_link_train] FDI train 1 done.
[  326.097246] [drm:ironlake_fdi_link_train] FDI_RX_IIR 0x200
[  326.097250] [drm:ironlake_fdi_link_train] FDI train 2 done.
[  326.097252] [drm:ironlake_fdi_link_train] FDI train done
[  326.097256] [drm:intel_enable_shared_dpll] enable PCH DPLL B (active
2, on? 0) for crtc 30
[  326.097259] [drm:intel_enable_shared_dpll] enabling PCH DPLL B
[  326.099523] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO underrun

OK so ere the first pipe got an underrun when we enabled the second
pipe. That's where the LP1+ disable thing might help. And it looks like
most of the underrun noise we have is actually of this form.

> Test kms_force_connector_basic:
>         Subgroup force-connector-state:
>                 pass       -> SKIP       (ilk-hp8440p)
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 dmesg-warn -> PASS       (byt-nuc)
>         Subgroup basic-rte:
>                 pass       -> DMESG-WARN (byt-nuc) UNSTABLE
> 
> bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
> bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
> bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
> byt-nuc          total:192  pass:156  dwarn:1   dfail:0   fail:0   skip:35 
> hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
> hsw-gt2          total:192  pass:175  dwarn:0   dfail:0   fail:0   skip:17 
> ilk-hp8440p      total:192  pass:127  dwarn:1   dfail:0   fail:0   skip:64 
> ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
> skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
> skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
> snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1693/
> 
> 6f788978796a35996bea8795db70f9c4194b70a2 drm-intel-nightly: 2016y-03m-23d-12h-24m-24s UTC integration manifest
> 6fea11c3ef42bc12ff37d785524d9a6a46d90d57 drm/i915: Try to shut up more ILK underruns
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 47332a164fcb..44fb3ba4c795 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4128,12 +4128,6 @@  static void ironlake_pch_enable(struct drm_crtc *crtc)
 	I915_WRITE(FDI_RX_TUSIZE1(pipe),
 		   I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
 
-	/*
-	 * Sometimes spurious CPU pipe underruns happen during FDI
-	 * training, at least with VGA+HDMI cloning. Suppress them.
-	 */
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
 	/* For PCH output, training FDI link */
 	dev_priv->display.fdi_link_train(crtc);
 
@@ -4168,8 +4162,6 @@  static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	intel_fdi_normal_train(crtc);
 
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-
 	/* For PCH DP, enable TRANS_DP_CTL */
 	if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
 		const struct drm_display_mode *adjusted_mode =
@@ -4767,6 +4759,17 @@  static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (WARN_ON(intel_crtc->active))
 		return;
 
+	/*
+	 * Sometimes spurious CPU pipe underruns happen during FDI
+	 * training, at least with VGA+HDMI cloning. Suppress them.
+	 *
+	 * On ILK we get an occasional spurious CPU pipe underruns
+	 * between eDP port A enable and vdd enable.
+	 *
+	 * Spurious PCH underruns also occur during PCH enabling.
+	 */
+	if (intel_crtc->config->has_pch_encoder || IS_GEN5(dev_priv))
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 	if (intel_crtc->config->has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
 
@@ -4788,8 +4791,6 @@  static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc->active = true;
 
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
@@ -4831,6 +4832,7 @@  static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	/* Must wait for vblank to avoid spurious PCH FIFO underruns */
 	if (intel_crtc->config->has_pch_encoder)
 		intel_wait_for_vblank(dev, pipe);
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 }
 
@@ -4983,8 +4985,15 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (intel_crtc->config->has_pch_encoder)
+	/*
+	 * Sometimes spurious CPU pipe underruns happen when the
+	 * pipe is already disabled, but FDI RX/TX is still enabled.
+	 * Happens at least with VGA+HDMI cloning. Suppress them.
+	 */
+	if (intel_crtc->config->has_pch_encoder) {
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
@@ -4992,22 +5001,12 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	drm_crtc_vblank_off(crtc);
 	assert_vblank_disabled(crtc);
 
-	/*
-	 * Sometimes spurious CPU pipe underruns happen when the
-	 * pipe is already disabled, but FDI RX/TX is still enabled.
-	 * Happens at least with VGA+HDMI cloning. Suppress them.
-	 */
-	if (intel_crtc->config->has_pch_encoder)
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
 	intel_disable_pipe(intel_crtc);
 
 	ironlake_pfit_disable(intel_crtc, false);
 
-	if (intel_crtc->config->has_pch_encoder) {
+	if (intel_crtc->config->has_pch_encoder)
 		ironlake_fdi_disable(crtc);
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
@@ -5037,6 +5036,7 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
 
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3ff8f1d67594..dd8793ff3720 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2640,15 +2640,6 @@  static void intel_enable_dp(struct intel_encoder *encoder)
 	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
 		vlv_init_panel_power_sequencer(intel_dp);
 
-	/*
-	 * We get an occasional spurious underrun between the port
-	 * enable and vdd enable, when enabling port A eDP.
-	 *
-	 * FIXME: Not sure if this applies to (PCH) port D eDP as well
-	 */
-	if (port == PORT_A)
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
 	intel_dp_enable_port(intel_dp);
 
 	if (port == PORT_A && IS_GEN5(dev_priv)) {
@@ -2666,9 +2657,6 @@  static void intel_enable_dp(struct intel_encoder *encoder)
 	edp_panel_on(intel_dp);
 	edp_panel_vdd_off(intel_dp, true);
 
-	if (port == PORT_A)
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-
 	pps_unlock(intel_dp);
 
 	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {