[PATCH] drm/i915: Beef up the IPS vs. CRC workaround

[PATCH] drm/i915: Beef up the IPS vs. CRC workaround

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Oneshot disabling of IPS when CRC capturing is started is insufficient.
IPS may get re-enabled by any plane update, and hence tests that keep
CRC capturing on across plane updates will start to see inconsistent
results as soon as IPS kicks back in. Add a new knob into the crtc state
to make sure IPS stays disabled as long as CRC capturing is enabled.

Forcing a modeset is the easiest way to handle this since that's already
how we do the panel fitter workaround. It's a little heavy handed just
for IPS, but seeing as we might already do the panel fitter workaround
I think it's better to follow that. We migth want to optimize both cases
later if someone gets too upset by the extra delay from the modeset.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
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  |  5 +++-
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ef5dde5ab1cf..1ce479614f52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	pipe_config->ips_enabled = i915.enable_ips &&
+		!pipe_config->ips_force_disable &&
 		hsw_crtc_supports_ips(crtc) &&
 		pipe_config_supports_ips(dev_priv, pipe_config);
 }
@@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_crtc_scaler_state scaler_state;
 	struct intel_dpll_hw_state dpll_hw_state;
 	struct intel_shared_dpll *shared_dpll;
-	bool force_thru;
+	bool force_thru, ips_force_disable;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	force_thru = crtc_state->pch_pfit.force_thru;
+	ips_force_disable = crtc_state->ips_force_disable;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->pch_pfit.force_thru = force_thru;
+	crtc_state->ips_force_disable = ips_force_disable;
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8b3e63..cadba9b92cc9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -651,6 +651,7 @@ struct intel_crtc_state {
 	struct intel_link_m_n fdi_m_n;
 
 	bool ips_enabled;
+	bool ips_force_disable;
 
 	bool enable_fbc;
 
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index ef0c0e195164..708cf1d419d4 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
-					bool enable)
+static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
+			      bool enable)
 {
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
@@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		goto out;
 	}
 
-	pipe_config->pch_pfit.force_thru = enable;
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    pipe_config->pch_pfit.enabled != enable)
+	/*
+	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+	 * enabled and disabled dynamically based on package C states,
+	 * user space can't make reliable use of the CRCs, so let's just
+	 * completely disable it.
+	 */
+	pipe_config->ips_force_disable = enable;
+	if (pipe_config->ips_force_disable != enable)
 		pipe_config->base.connectors_changed = true;
 
+	if (IS_HASWELL(dev_priv)) {
+		pipe_config->pch_pfit.force_thru = enable;
+		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+		    pipe_config->pch_pfit.enabled != enable)
+			pipe_config->base.connectors_changed = true;
+	}
+
 	ret = drm_atomic_commit(state);
 out:
 	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
@@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
+		if ((IS_HASWELL(dev_priv) ||
+		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			       enum intel_pipe_crc_source source)
 {
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 	enum intel_display_power_domain power_domain;
 	u32 val = 0; /* shut up gcc */
 	int ret;
@@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			goto out;
 		}
 
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		hsw_disable_ips(crtc);
-
 		spin_lock_irq(&pipe_crc->lock);
 		kfree(pipe_crc->entries);
 		pipe_crc->entries = entries;
@@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
 		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
-		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
-
-		hsw_enable_ips(crtc);
+		else if ((IS_HASWELL(dev_priv) ||
+			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, false);
 	}
 
 	ret = 0;
-- 
2.10.2

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

Re: [PATCH] drm/i915: Beef up the IPS vs. CRC workaround

[Ville Syrjälä]

On Wed, Dec 21, 2016 at 11:31:05AM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Oneshot disabling of IPS when CRC capturing is started is insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep
> CRC capturing on across plane updates will start to see inconsistent
> results as soon as IPS kicks back in. Add a new knob into the crtc state
> to make sure IPS stays disabled as long as CRC capturing is enabled.
> 
> Forcing a modeset is the easiest way to handle this since that's already
> how we do the panel fitter workaround. It's a little heavy handed just
> for IPS, but seeing as we might already do the panel fitter workaround
> I think it's better to follow that. We migth want to optimize both cases
> later if someone gets too upset by the extra delay from the modeset.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 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  |  5 +++-
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ef5dde5ab1cf..1ce479614f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	pipe_config->ips_enabled = i915.enable_ips &&
> +		!pipe_config->ips_force_disable &&
>  		hsw_crtc_supports_ips(crtc) &&
>  		pipe_config_supports_ips(dev_priv, pipe_config);
>  }
> @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	struct intel_crtc_scaler_state scaler_state;
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	struct intel_shared_dpll *shared_dpll;
> -	bool force_thru;
> +	bool force_thru, ips_force_disable;
>  
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
> @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	force_thru = crtc_state->pch_pfit.force_thru;
> +	ips_force_disable = crtc_state->ips_force_disable;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->pch_pfit.force_thru = force_thru;
> +	crtc_state->ips_force_disable = ips_force_disable;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8b3e63..cadba9b92cc9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
>  
>  	bool ips_enabled;
> +	bool ips_force_disable;
>  
>  	bool enable_fbc;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index ef0c0e195164..708cf1d419d4 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> -					bool enable)
> +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> +			      bool enable)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		goto out;
>  	}
>  
> -	pipe_config->pch_pfit.force_thru = enable;
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    pipe_config->pch_pfit.enabled != enable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +	 * enabled and disabled dynamically based on package C states,
> +	 * user space can't make reliable use of the CRCs, so let's just
> +	 * completely disable it.
> +	 */
> +	pipe_config->ips_force_disable = enable;
> +	if (pipe_config->ips_force_disable != enable)

And naturally I typoed that. What I really wanted was:
	if (pipe_config->ips_enabled == enable)

>  		pipe_config->base.connectors_changed = true;
>  
> +	if (IS_HASWELL(dev_priv)) {
> +		pipe_config->pch_pfit.force_thru = enable;
> +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +		    pipe_config->pch_pfit.enabled != enable)
> +			pipe_config->base.connectors_changed = true;
> +	}
> +
>  	ret = drm_atomic_commit(state);
>  out:
>  	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> +		if ((IS_HASWELL(dev_priv) ||
> +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv, true);
>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			       enum intel_pipe_crc_source source)
>  {
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  	enum intel_display_power_domain power_domain;
>  	u32 val = 0; /* shut up gcc */
>  	int ret;
> @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			goto out;
>  		}
>  
> -		/*
> -		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> -		 * enabled and disabled dynamically based on package C states,
> -		 * user space can't make reliable use of the CRCs, so let's just
> -		 * completely disable it.
> -		 */
> -		hsw_disable_ips(crtc);
> -
>  		spin_lock_irq(&pipe_crc->lock);
>  		kfree(pipe_crc->entries);
>  		pipe_crc->entries = entries;
> @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
>  		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
> -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> -
> -		hsw_enable_ips(crtc);
> +		else if ((IS_HASWELL(dev_priv) ||
> +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv, false);
>  	}
>  
>  	ret = 0;
> -- 
> 2.10.2

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Beef up the IPS vs. CRC workaround

[Patchwork]

== Series Details ==

Series: drm/i915: Beef up the IPS vs. CRC workaround
URL   : https://patchwork.freedesktop.org/series/17084/
State : success

== Summary ==

Series 17084v1 drm/i915: Beef up the IPS vs. CRC workaround
https://patchwork.freedesktop.org/api/1.0/series/17084/revisions/1/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:225  dwarn:1   dfail:0   fail:0   skip:21 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

6096aee14ea52e3163729129ee7362e56ff3efb9 drm-tip: 2016y-12m-20d-16h-33m-17s UTC integration manifest
52b9416 drm/i915: Beef up the IPS vs. CRC workaround

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3350/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Beef up the IPS vs. CRC workaround

[Paulo Zanoni]

Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Oneshot disabling of IPS when CRC capturing is started is
> insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep
> CRC capturing on across plane updates will start to see inconsistent
> results as soon as IPS kicks back in. Add a new knob into the crtc
> state
> to make sure IPS stays disabled as long as CRC capturing is enabled.
>
> Forcing a modeset is the easiest way to handle this since that's
> already
> how we do the panel fitter workaround. It's a little heavy handed
> just
> for IPS, but seeing as we might already do the panel fitter
> workaround
> I think it's better to follow that. We migth want to optimize both
> cases
> later if someone gets too upset by the extra delay from the modeset.
> 

Please add a "Testcase:" tag listing the IGTs this patch unbreaks.
Maybe also "Bugzilla:" if there's any.


> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 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  |  5 +++-
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----
> ------------
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ef5dde5ab1cf..1ce479614f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct
> intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	pipe_config->ips_enabled = i915.enable_ips &&
> +		!pipe_config->ips_force_disable &&
>  		hsw_crtc_supports_ips(crtc) &&
>  		pipe_config_supports_ips(dev_priv, pipe_config);
>  }
> @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
>  	struct intel_crtc_scaler_state scaler_state;
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	struct intel_shared_dpll *shared_dpll;
> -	bool force_thru;
> +	bool force_thru, ips_force_disable;
>  
>  	/* FIXME: before the switch to atomic started, a new
> pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero
> should be
> @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	force_thru = crtc_state->pch_pfit.force_thru;
> +	ips_force_disable = crtc_state->ips_force_disable;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->pch_pfit.force_thru = force_thru;
> +	crtc_state->ips_force_disable = ips_force_disable;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8b3e63..cadba9b92cc9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
>  
>  	bool ips_enabled;
> +	bool ips_force_disable;
>  
>  	bool enable_fbc;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index ef0c0e195164..708cf1d419d4 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum
> intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private
> *dev_priv,
> -					bool enable)
> +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> +			      bool enable)

Now we apply more than one WA. So maybe: hsw_pipe_a_crc_workarounds()
or just hsw_pipe_crc_workarounds() (or apply_workarounds). Just "was"
is confusing since it looks like a verb is missing. Also, that capital
A is not exactly following our coding style.


>  {
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> PIPE_A);
> @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct
> drm_i915_private *dev_priv,
>  		goto out;
>  	}
>  
> -	pipe_config->pch_pfit.force_thru = enable;
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    pipe_config->pch_pfit.enabled != enable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS
> gets
> +	 * enabled and disabled dynamically based on package C
> states,
> +	 * user space can't make reliable use of the CRCs, so let's
> just
> +	 * completely disable it.
> +	 */
> +	pipe_config->ips_force_disable = enable;
> +	if (pipe_config->ips_force_disable != enable)

The fix mentioned in your other email makes sense, but I find it easier
to read "if (pipe_config->ips_enable == pipe_config-
>ips_force_disable)".

>  		pipe_config->base.connectors_changed = true;
>  
> +	if (IS_HASWELL(dev_priv)) {
> +		pipe_config->pch_pfit.force_thru = enable;
> +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +		    pipe_config->pch_pfit.enabled != enable)
> +			pipe_config->base.connectors_changed = true;
> +	}
> +
>  	ret = drm_atomic_commit(state);
>  out:
>  	WARN(ret, "Toggling workaround to %i returns %i\n", enable,
> ret);
> @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> +		if ((IS_HASWELL(dev_priv) ||
> +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv, true);

I know this problem is not caused by your patch, but in case you want:
I wonder if there's a better place to call this. Maybe outside this
function. Calling it here sounds like an unrelated side-effect of a
function that's supposed to just return a value for a register.


>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
>  			       enum intel_pipe_crc_source source)
>  {
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> pipe);
>  	enum intel_display_power_domain power_domain;
>  	u32 val = 0; /* shut up gcc */
>  	int ret;
> @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
>  			goto out;
>  		}
>  
> -		/*
> -		 * When IPS gets enabled, the pipe CRC changes.
> Since IPS gets
> -		 * enabled and disabled dynamically based on package
> C states,
> -		 * user space can't make reliable use of the CRCs,
> so let's just
> -		 * completely disable it.
> -		 */
> -		hsw_disable_ips(crtc);
> -
>  		spin_lock_irq(&pipe_crc->lock);
>  		kfree(pipe_crc->entries);
>  		pipe_crc->entries = entries;
> @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
>  			g4x_undo_pipe_scramble_reset(dev_priv,
> pipe);
>  		else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv,
> pipe);
> -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv,
> false);
> -
> -		hsw_enable_ips(crtc);
> +		else if ((IS_HASWELL(dev_priv) ||
> +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)

Moving the gen+pipe checks to inside the function would deduplicate
them and, IMHO, make the code slightly easier to read.

Now here's a question that maybe I never asked myself before: if we
have two pipes enabled (A and B), and our system is properly configured
for power management so we're reaching the deep PC states (depending on
your machine, all you need to do is to run powertop --auto-tune), does
pipe B also change its CRCs when IPS is enabled on pipe A? The reason I
ask this is because IPS enables deeper PC states, but maybe it's the
deeper PC states that causes different CRC calculation, not IPS, and
then this would also affect the CRCs for non-IPS pipes (AKA pipe B). It
would be great if you could check this, because I don't really remember
if I checked for this when I wrote this code. Of course, the fix for
this would be a separate patch since it's not the problem you're
touching here. But then, maybe we could do something else to prevent
deep PC states instead of disabling IPS.

Most of (all?) my suggestions above are bikesheds, so with or without
them:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +			hsw_pipe_A_crc_wa(dev_priv, false);
>  	}
>  
>  	ret = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Beef up the IPS vs. CRC workaround

[Ville Syrjälä]

On Wed, Dec 21, 2016 at 03:04:43PM -0200, Paulo Zanoni wrote:
> Em Qua, 2016-12-21 às 11:31 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Oneshot disabling of IPS when CRC capturing is started is
> > insufficient.
> > IPS may get re-enabled by any plane update, and hence tests that keep
> > CRC capturing on across plane updates will start to see inconsistent
> > results as soon as IPS kicks back in. Add a new knob into the crtc
> > state
> > to make sure IPS stays disabled as long as CRC capturing is enabled.
> >
> > Forcing a modeset is the easiest way to handle this since that's
> > already
> > how we do the panel fitter workaround. It's a little heavy handed
> > just
> > for IPS, but seeing as we might already do the panel fitter
> > workaround
> > I think it's better to follow that. We migth want to optimize both
> > cases
> > later if someone gets too upset by the extra delay from the modeset.
> > 
> 
> Please add a "Testcase:" tag listing the IGTs this patch unbreaks.

The only thin I noticed so far was my kms_plane_blinker which isn't in
yet.

> Maybe also "Bugzilla:" if there's any.

Don't recall any. I can make a quick scan of the list though.

> 
> 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 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  |  5 +++-
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----
> > ------------
> >  3 files changed, 28 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index ef5dde5ab1cf..1ce479614f52 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct
> > intel_crtc *crtc,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  	pipe_config->ips_enabled = i915.enable_ips &&
> > +		!pipe_config->ips_force_disable &&
> >  		hsw_crtc_supports_ips(crtc) &&
> >  		pipe_config_supports_ips(dev_priv, pipe_config);
> >  }
> > @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> >  	struct intel_crtc_scaler_state scaler_state;
> >  	struct intel_dpll_hw_state dpll_hw_state;
> >  	struct intel_shared_dpll *shared_dpll;
> > -	bool force_thru;
> > +	bool force_thru, ips_force_disable;
> >  
> >  	/* FIXME: before the switch to atomic started, a new
> > pipe_config was
> >  	 * kzalloc'd. Code that depends on any field being zero
> > should be
> > @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> >  	shared_dpll = crtc_state->shared_dpll;
> >  	dpll_hw_state = crtc_state->dpll_hw_state;
> >  	force_thru = crtc_state->pch_pfit.force_thru;
> > +	ips_force_disable = crtc_state->ips_force_disable;
> >  
> >  	memset(crtc_state, 0, sizeof *crtc_state);
> >  
> > @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> >  	crtc_state->shared_dpll = shared_dpll;
> >  	crtc_state->dpll_hw_state = dpll_hw_state;
> >  	crtc_state->pch_pfit.force_thru = force_thru;
> > +	crtc_state->ips_force_disable = ips_force_disable;
> >  }
> >  
> >  static int
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 025e4c8b3e63..cadba9b92cc9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -651,6 +651,7 @@ struct intel_crtc_state {
> >  	struct intel_link_m_n fdi_m_n;
> >  
> >  	bool ips_enabled;
> > +	bool ips_force_disable;
> >  
> >  	bool enable_fbc;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index ef0c0e195164..708cf1d419d4 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum
> > intel_pipe_crc_source *source,
> >  	return 0;
> >  }
> >  
> > -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private
> > *dev_priv,
> > -					bool enable)
> > +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > +			      bool enable)
> 
> Now we apply more than one WA. So maybe: hsw_pipe_a_crc_workarounds()
> or just hsw_pipe_crc_workarounds() (or apply_workarounds). Just "was"
> is confusing since it looks like a verb is missing. Also, that capital
> A is not exactly following our coding style.
> 
> 
> >  {
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > PIPE_A);
> > @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> >  		goto out;
> >  	}
> >  
> > -	pipe_config->pch_pfit.force_thru = enable;
> > -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > -	    pipe_config->pch_pfit.enabled != enable)
> > +	/*
> > +	 * When IPS gets enabled, the pipe CRC changes. Since IPS
> > gets
> > +	 * enabled and disabled dynamically based on package C
> > states,
> > +	 * user space can't make reliable use of the CRCs, so let's
> > just
> > +	 * completely disable it.
> > +	 */
> > +	pipe_config->ips_force_disable = enable;
> > +	if (pipe_config->ips_force_disable != enable)
> 
> The fix mentioned in your other email makes sense, but I find it easier
> to read "if (pipe_config->ips_enable == pipe_config-
> >ips_force_disable)".
> 
> >  		pipe_config->base.connectors_changed = true;
> >  
> > +	if (IS_HASWELL(dev_priv)) {
> > +		pipe_config->pch_pfit.force_thru = enable;
> > +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > +		    pipe_config->pch_pfit.enabled != enable)
> > +			pipe_config->base.connectors_changed = true;
> > +	}
> > +
> >  	ret = drm_atomic_commit(state);
> >  out:
> >  	WARN(ret, "Toggling workaround to %i returns %i\n", enable,
> > ret);
> > @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> >  		break;
> >  	case INTEL_PIPE_CRC_SOURCE_PF:
> > -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> > +		if ((IS_HASWELL(dev_priv) ||
> > +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > +			hsw_pipe_A_crc_wa(dev_priv, true);
> 
> I know this problem is not caused by your patch, but in case you want:
> I wonder if there's a better place to call this. Maybe outside this
> function. Calling it here sounds like an unrelated side-effect of a
> function that's supposed to just return a value for a register.

Yeah, the disable call happens from a higher level IIRC, so moving this
might make sense from a consistency POV as well. I'll take another look
at it.

> 
> 
> >  
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> >  		break;
> > @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> >  			       enum intel_pipe_crc_source source)
> >  {
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > pipe);
> >  	enum intel_display_power_domain power_domain;
> >  	u32 val = 0; /* shut up gcc */
> >  	int ret;
> > @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> >  			goto out;
> >  		}
> >  
> > -		/*
> > -		 * When IPS gets enabled, the pipe CRC changes.
> > Since IPS gets
> > -		 * enabled and disabled dynamically based on package
> > C states,
> > -		 * user space can't make reliable use of the CRCs,
> > so let's just
> > -		 * completely disable it.
> > -		 */
> > -		hsw_disable_ips(crtc);
> > -
> >  		spin_lock_irq(&pipe_crc->lock);
> >  		kfree(pipe_crc->entries);
> >  		pipe_crc->entries = entries;
> > @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct
> > drm_i915_private *dev_priv,
> >  			g4x_undo_pipe_scramble_reset(dev_priv,
> > pipe);
> >  		else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> >  			vlv_undo_pipe_scramble_reset(dev_priv,
> > pipe);
> > -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv,
> > false);
> > -
> > -		hsw_enable_ips(crtc);
> > +		else if ((IS_HASWELL(dev_priv) ||
> > +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> 
> Moving the gen+pipe checks to inside the function would deduplicate
> them and, IMHO, make the code slightly easier to read.

Hmm. Not sure. The scrambler reset stuff is here anyway. Or perhaps
we could move that stuff into some function as well.

> 
> Now here's a question that maybe I never asked myself before: if we
> have two pipes enabled (A and B), and our system is properly configured
> for power management so we're reaching the deep PC states (depending on
> your machine, all you need to do is to run powertop --auto-tune), does
> pipe B also change its CRCs when IPS is enabled on pipe A? The reason I
> ask this is because IPS enables deeper PC states, but maybe it's the
> deeper PC states that causes different CRC calculation, not IPS, and
> then this would also affect the CRCs for non-IPS pipes (AKA pipe B). It
> would be great if you could check this, because I don't really remember
> if I checked for this when I wrote this code. Of course, the fix for
> this would be a separate patch since it's not the problem you're
> touching here. But then, maybe we could do something else to prevent
> deep PC states instead of disabling IPS.

I would assume it's IPS itself causing the problem. The package C
state having some undocumented effect on the pipe would sound quite bad
to me. But I must admit that I haven't tried it either. I guess I need
to fire up my HSW again and see how it behaves.

> 
> Most of (all?) my suggestions above are bikesheds, so with or without
> them:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > +			hsw_pipe_A_crc_wa(dev_priv, false);
> >  	}
> >  
> >  	ret = 0;

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Oneshot disabling of IPS when CRC capturing is started is insufficient.
IPS may get re-enabled by any plane update, and hence tests that keep
CRC capturing on across plane updates will start to see inconsistent
results as soon as IPS kicks back in. Add a new knob into the crtc state
to make sure IPS stays disabled as long as CRC capturing is enabled.

Forcing a modeset is the easiest way to handle this since that's already
how we do the panel fitter workaround. It's a little heavy handed just
for IPS, but seeing as we might already do the panel fitter workaround
I think it's better to follow that. We migth want to optimize both cases
later if someone gets too upset by the extra delay from the modeset.

v2: Check the right thing when deciding whether to force a modeset

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  5 +++-
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ef5dde5ab1cf..1ce479614f52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	pipe_config->ips_enabled = i915.enable_ips &&
+		!pipe_config->ips_force_disable &&
 		hsw_crtc_supports_ips(crtc) &&
 		pipe_config_supports_ips(dev_priv, pipe_config);
 }
@@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_crtc_scaler_state scaler_state;
 	struct intel_dpll_hw_state dpll_hw_state;
 	struct intel_shared_dpll *shared_dpll;
-	bool force_thru;
+	bool force_thru, ips_force_disable;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	force_thru = crtc_state->pch_pfit.force_thru;
+	ips_force_disable = crtc_state->ips_force_disable;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->pch_pfit.force_thru = force_thru;
+	crtc_state->ips_force_disable = ips_force_disable;
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8b3e63..cadba9b92cc9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -651,6 +651,7 @@ struct intel_crtc_state {
 	struct intel_link_m_n fdi_m_n;
 
 	bool ips_enabled;
+	bool ips_force_disable;
 
 	bool enable_fbc;
 
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index ef0c0e195164..74780b090d1e 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
-					bool enable)
+static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
+			      bool enable)
 {
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
@@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		goto out;
 	}
 
-	pipe_config->pch_pfit.force_thru = enable;
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    pipe_config->pch_pfit.enabled != enable)
+	/*
+	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+	 * enabled and disabled dynamically based on package C states,
+	 * user space can't make reliable use of the CRCs, so let's just
+	 * completely disable it.
+	 */
+	pipe_config->ips_force_disable = enable;
+	if (pipe_config->ips_enabled == enable)
 		pipe_config->base.connectors_changed = true;
 
+	if (IS_HASWELL(dev_priv)) {
+		pipe_config->pch_pfit.force_thru = enable;
+		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+		    pipe_config->pch_pfit.enabled != enable)
+			pipe_config->base.connectors_changed = true;
+	}
+
 	ret = drm_atomic_commit(state);
 out:
 	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
@@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
+		if ((IS_HASWELL(dev_priv) ||
+		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			       enum intel_pipe_crc_source source)
 {
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 	enum intel_display_power_domain power_domain;
 	u32 val = 0; /* shut up gcc */
 	int ret;
@@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			goto out;
 		}
 
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		hsw_disable_ips(crtc);
-
 		spin_lock_irq(&pipe_crc->lock);
 		kfree(pipe_crc->entries);
 		pipe_crc->entries = entries;
@@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
 		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
-		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
-
-		hsw_enable_ips(crtc);
+		else if ((IS_HASWELL(dev_priv) ||
+			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, false);
 	}
 
 	ret = 0;
-- 
2.13.0

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

Re: [PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround

[Lofstedt, Marta]

FYI, when I rebased to drm-tip to test this there is now also a call to the old: "hsw_trans_edp_pipe_A_crc_wa" from "intel_crtc_set_crc_source". I replaced that in the same manner as you did in " pipe_crc_set_source". 
With that this was tested OK.

Tested-by: Marta Lofsted <marta.lofstedt@intel.com>

> -----Original Message-----
> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, August 16, 2017 5:39 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>; Daniel Vetter
> <daniel.vetter@ffwll.ch>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Lofstedt, Marta
> <marta.lofstedt@intel.com>
> Subject: [PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Oneshot disabling of IPS when CRC capturing is started is insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep CRC
> capturing on across plane updates will start to see inconsistent results as
> soon as IPS kicks back in. Add a new knob into the crtc state to make sure IPS
> stays disabled as long as CRC capturing is enabled.
> 
> Forcing a modeset is the easiest way to handle this since that's already how
> we do the panel fitter workaround. It's a little heavy handed just for IPS, but
> seeing as we might already do the panel fitter workaround I think it's better
> to follow that. We migth want to optimize both cases later if someone gets
> too upset by the extra delay from the modeset.
> 
> v2: Check the right thing when deciding whether to force a modeset
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  5 +++-
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------
> ------
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ef5dde5ab1cf..1ce479614f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct
> intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> 
>  	pipe_config->ips_enabled = i915.enable_ips &&
> +		!pipe_config->ips_force_disable &&
>  		hsw_crtc_supports_ips(crtc) &&
>  		pipe_config_supports_ips(dev_priv,
> pipe_config);  } @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
>  	struct intel_crtc_scaler_state scaler_state;
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	struct intel_shared_dpll *shared_dpll;
> -	bool force_thru;
> +	bool force_thru, ips_force_disable;
> 
>  	/* FIXME: before the switch to atomic started, a new
> pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should
> be @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state
> *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	force_thru = crtc_state->pch_pfit.force_thru;
> +	ips_force_disable = crtc_state->ips_force_disable;
> 
>  	memset(crtc_state, 0, sizeof *crtc_state);
> 
> @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state
> *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->pch_pfit.force_thru = force_thru;
> +	crtc_state->ips_force_disable = ips_force_disable;
>  }
> 
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8b3e63..cadba9b92cc9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
> 
>  	bool ips_enabled;
> +	bool ips_force_disable;
> 
>  	bool enable_fbc;
> 
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index ef0c0e195164..74780b090d1e 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum
> intel_pipe_crc_source *source,
>  	return 0;
>  }
> 
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private
> *dev_priv,
> -					bool
> enable)
> +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> +			      bool enable)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> PIPE_A); @@ -570,11 +570,23 @@ static void
> hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		goto out;
>  	}
> 
> -	pipe_config->pch_pfit.force_thru = enable;
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    pipe_config->pch_pfit.enabled != enable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +	 * enabled and disabled dynamically based on package C
> states,
> +	 * user space can't make reliable use of the CRCs, so let's just
> +	 * completely disable it.
> +	 */
> +	pipe_config->ips_force_disable = enable;
> +	if (pipe_config->ips_enabled == enable)
>  		pipe_config->base.connectors_changed = true;
> 
> +	if (IS_HASWELL(dev_priv)) {
> +		pipe_config->pch_pfit.force_thru = enable;
> +		if (pipe_config->cpu_transcoder ==
> TRANSCODER_EDP &&
> +		    pipe_config->pch_pfit.enabled != enable)
> +			pipe_config-
> >base.connectors_changed = true;
> +	}
> +
>  	ret = drm_atomic_commit(state);
>  out:
>  	WARN(ret, "Toggling workaround to %i returns %i\n", enable,
> ret); @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE |
> PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -
> 	hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> +		if ((IS_HASWELL(dev_priv) ||
> +		     IS_BROADWELL(dev_priv)) && pipe ==
> PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv,
> true);
> 
>  		*val = PIPE_CRC_ENABLE |
> PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
>  			       enum intel_pipe_crc_source
> source)  {
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> pipe);
>  	enum intel_display_power_domain power_domain;
>  	u32 val = 0; /* shut up gcc */
>  	int ret;
> @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
>  			goto out;
>  		}
> 
> -		/*
> -		 * When IPS gets enabled, the pipe CRC
> changes. Since IPS gets
> -		 * enabled and disabled dynamically based on
> package C states,
> -		 * user space can't make reliable use of the
> CRCs, so let's just
> -		 * completely disable it.
> -		 */
> -		hsw_disable_ips(crtc);
> -
>  		spin_lock_irq(&pipe_crc->lock);
>  		kfree(pipe_crc->entries);
>  		pipe_crc->entries = entries;
> @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct
> drm_i915_private *dev_priv,
> 
> 	g4x_undo_pipe_scramble_reset(dev_priv, pipe);
>  		else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
> 
> 	vlv_undo_pipe_scramble_reset(dev_priv, pipe);
> -		else if (IS_HASWELL(dev_priv) && pipe ==
> PIPE_A)
> -
> 	hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> -
> -		hsw_enable_ips(crtc);
> +		else if ((IS_HASWELL(dev_priv) ||
> +			  IS_BROADWELL(dev_priv)) &&
> pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv,
> false);
>  	}
> 
>  	ret = 0;
> --
> 2.13.0

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

Re: [PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround

[Maarten Lankhorst]

Op 16-08-17 om 16:39 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Oneshot disabling of IPS when CRC capturing is started is insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep
> CRC capturing on across plane updates will start to see inconsistent
> results as soon as IPS kicks back in. Add a new knob into the crtc state
> to make sure IPS stays disabled as long as CRC capturing is enabled.
>
> Forcing a modeset is the easiest way to handle this since that's already
> how we do the panel fitter workaround. It's a little heavy handed just
> for IPS, but seeing as we might already do the panel fitter workaround
> I think it's better to follow that. We migth want to optimize both cases
> later if someone gets too upset by the extra delay from the modeset.
>
> v2: Check the right thing when deciding whether to force a modeset
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  5 +++-
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
>  3 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ef5dde5ab1cf..1ce479614f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	pipe_config->ips_enabled = i915.enable_ips &&
> +		!pipe_config->ips_force_disable &&
>  		hsw_crtc_supports_ips(crtc) &&
>  		pipe_config_supports_ips(dev_priv, pipe_config);
>  }
> @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	struct intel_crtc_scaler_state scaler_state;
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	struct intel_shared_dpll *shared_dpll;
> -	bool force_thru;
> +	bool force_thru, ips_force_disable;
>  
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
> @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	force_thru = crtc_state->pch_pfit.force_thru;
> +	ips_force_disable = crtc_state->ips_force_disable;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->pch_pfit.force_thru = force_thru;
> +	crtc_state->ips_force_disable = ips_force_disable;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8b3e63..cadba9b92cc9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
>  
>  	bool ips_enabled;
> +	bool ips_force_disable;
Could we rename this to collecting_crc throughout the patch?

And as Marta noted, intel_crtc_set_crc_source also needs fixing. :)
>  
>  	bool enable_fbc;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index ef0c0e195164..74780b090d1e 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> -					bool enable)
> +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> +			      bool enable)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		goto out;
>  	}
>  
> -	pipe_config->pch_pfit.force_thru = enable;
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    pipe_config->pch_pfit.enabled != enable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +	 * enabled and disabled dynamically based on package C states,
> +	 * user space can't make reliable use of the CRCs, so let's just
> +	 * completely disable it.
> +	 */
> +	pipe_config->ips_force_disable = enable;
> +	if (pipe_config->ips_enabled == enable)
>  		pipe_config->base.connectors_changed = true;
>  
> +	if (IS_HASWELL(dev_priv)) {
> +		pipe_config->pch_pfit.force_thru = enable;
> +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +		    pipe_config->pch_pfit.enabled != enable)
> +			pipe_config->base.connectors_changed = true;
> +	}
> +
>  	ret = drm_atomic_commit(state);
>  out:
>  	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> +		if ((IS_HASWELL(dev_priv) ||
> +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv, true);
>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			       enum intel_pipe_crc_source source)
>  {
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  	enum intel_display_power_domain power_domain;
>  	u32 val = 0; /* shut up gcc */
>  	int ret;
> @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			goto out;
>  		}
>  
> -		/*
> -		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> -		 * enabled and disabled dynamically based on package C states,
> -		 * user space can't make reliable use of the CRCs, so let's just
> -		 * completely disable it.
> -		 */
> -		hsw_disable_ips(crtc);
> -
>  		spin_lock_irq(&pipe_crc->lock);
>  		kfree(pipe_crc->entries);
>  		pipe_crc->entries = entries;
> @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
>  		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
> -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> -
> -		hsw_enable_ips(crtc);
> +		else if ((IS_HASWELL(dev_priv) ||
> +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> +			hsw_pipe_A_crc_wa(dev_priv, false);
>  	}
>  
>  	ret = 0;


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

Re: [PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround

[Ville Syrjälä]

On Thu, Aug 17, 2017 at 10:00:52AM +0200, Maarten Lankhorst wrote:
> Op 16-08-17 om 16:39 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Oneshot disabling of IPS when CRC capturing is started is insufficient.
> > IPS may get re-enabled by any plane update, and hence tests that keep
> > CRC capturing on across plane updates will start to see inconsistent
> > results as soon as IPS kicks back in. Add a new knob into the crtc state
> > to make sure IPS stays disabled as long as CRC capturing is enabled.
> >
> > Forcing a modeset is the easiest way to handle this since that's already
> > how we do the panel fitter workaround. It's a little heavy handed just
> > for IPS, but seeing as we might already do the panel fitter workaround
> > I think it's better to follow that. We migth want to optimize both cases
> > later if someone gets too upset by the extra delay from the modeset.
> >
> > v2: Check the right thing when deciding whether to force a modeset
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  |  5 +++-
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
> >  3 files changed, 28 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ef5dde5ab1cf..1ce479614f52 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  	pipe_config->ips_enabled = i915.enable_ips &&
> > +		!pipe_config->ips_force_disable &&
> >  		hsw_crtc_supports_ips(crtc) &&
> >  		pipe_config_supports_ips(dev_priv, pipe_config);
> >  }
> > @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >  	struct intel_crtc_scaler_state scaler_state;
> >  	struct intel_dpll_hw_state dpll_hw_state;
> >  	struct intel_shared_dpll *shared_dpll;
> > -	bool force_thru;
> > +	bool force_thru, ips_force_disable;
> >  
> >  	/* FIXME: before the switch to atomic started, a new pipe_config was
> >  	 * kzalloc'd. Code that depends on any field being zero should be
> > @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >  	shared_dpll = crtc_state->shared_dpll;
> >  	dpll_hw_state = crtc_state->dpll_hw_state;
> >  	force_thru = crtc_state->pch_pfit.force_thru;
> > +	ips_force_disable = crtc_state->ips_force_disable;
> >  
> >  	memset(crtc_state, 0, sizeof *crtc_state);
> >  
> > @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >  	crtc_state->shared_dpll = shared_dpll;
> >  	crtc_state->dpll_hw_state = dpll_hw_state;
> >  	crtc_state->pch_pfit.force_thru = force_thru;
> > +	crtc_state->ips_force_disable = ips_force_disable;
> >  }
> >  
> >  static int
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 025e4c8b3e63..cadba9b92cc9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -651,6 +651,7 @@ struct intel_crtc_state {
> >  	struct intel_link_m_n fdi_m_n;
> >  
> >  	bool ips_enabled;
> > +	bool ips_force_disable;
> Could we rename this to collecting_crc throughout the patch?

If we do, then we should probably kill off the separate pfit
force_thru boolean as well and just use 'collecting_crc' for
the pipe A routing decisions as well.

> 
> And as Marta noted, intel_crtc_set_crc_source also needs fixing. :)

Doh. I thought I retipped the patch, but apparently I didn't.

> >  
> >  	bool enable_fbc;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index ef0c0e195164..74780b090d1e 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> >  	return 0;
> >  }
> >  
> > -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > -					bool enable)
> > +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > +			      bool enable)
> >  {
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> > @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> >  		goto out;
> >  	}
> >  
> > -	pipe_config->pch_pfit.force_thru = enable;
> > -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > -	    pipe_config->pch_pfit.enabled != enable)
> > +	/*
> > +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > +	 * enabled and disabled dynamically based on package C states,
> > +	 * user space can't make reliable use of the CRCs, so let's just
> > +	 * completely disable it.
> > +	 */
> > +	pipe_config->ips_force_disable = enable;
> > +	if (pipe_config->ips_enabled == enable)
> >  		pipe_config->base.connectors_changed = true;
> >  
> > +	if (IS_HASWELL(dev_priv)) {
> > +		pipe_config->pch_pfit.force_thru = enable;
> > +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > +		    pipe_config->pch_pfit.enabled != enable)
> > +			pipe_config->base.connectors_changed = true;
> > +	}
> > +
> >  	ret = drm_atomic_commit(state);
> >  out:
> >  	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> > @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> >  		break;
> >  	case INTEL_PIPE_CRC_SOURCE_PF:
> > -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> > +		if ((IS_HASWELL(dev_priv) ||
> > +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > +			hsw_pipe_A_crc_wa(dev_priv, true);
> >  
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> >  		break;
> > @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >  			       enum intel_pipe_crc_source source)
> >  {
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >  	enum intel_display_power_domain power_domain;
> >  	u32 val = 0; /* shut up gcc */
> >  	int ret;
> > @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >  			goto out;
> >  		}
> >  
> > -		/*
> > -		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > -		 * enabled and disabled dynamically based on package C states,
> > -		 * user space can't make reliable use of the CRCs, so let's just
> > -		 * completely disable it.
> > -		 */
> > -		hsw_disable_ips(crtc);
> > -
> >  		spin_lock_irq(&pipe_crc->lock);
> >  		kfree(pipe_crc->entries);
> >  		pipe_crc->entries = entries;
> > @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >  			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
> >  		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
> > -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> > -
> > -		hsw_enable_ips(crtc);
> > +		else if ((IS_HASWELL(dev_priv) ||
> > +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > +			hsw_pipe_A_crc_wa(dev_priv, false);
> >  	}
> >  
> >  	ret = 0;
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Beef up the IPS vs. CRC workaround

[Ville Syrjälä]

On Thu, Aug 17, 2017 at 03:16:46PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 17, 2017 at 10:00:52AM +0200, Maarten Lankhorst wrote:
> > Op 16-08-17 om 16:39 schreef ville.syrjala@linux.intel.com:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Oneshot disabling of IPS when CRC capturing is started is insufficient.
> > > IPS may get re-enabled by any plane update, and hence tests that keep
> > > CRC capturing on across plane updates will start to see inconsistent
> > > results as soon as IPS kicks back in. Add a new knob into the crtc state
> > > to make sure IPS stays disabled as long as CRC capturing is enabled.
> > >
> > > Forcing a modeset is the easiest way to handle this since that's already
> > > how we do the panel fitter workaround. It's a little heavy handed just
> > > for IPS, but seeing as we might already do the panel fitter workaround
> > > I think it's better to follow that. We migth want to optimize both cases
> > > later if someone gets too upset by the extra delay from the modeset.
> > >
> > > v2: Check the right thing when deciding whether to force a modeset
> > >
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c  |  5 +++-
> > >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 43 +++++++++++++++++++----------------
> > >  3 files changed, 28 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index ef5dde5ab1cf..1ce479614f52 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -7189,6 +7189,7 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  
> > >  	pipe_config->ips_enabled = i915.enable_ips &&
> > > +		!pipe_config->ips_force_disable &&
> > >  		hsw_crtc_supports_ips(crtc) &&
> > >  		pipe_config_supports_ips(dev_priv, pipe_config);
> > >  }
> > > @@ -12958,7 +12959,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > >  	struct intel_crtc_scaler_state scaler_state;
> > >  	struct intel_dpll_hw_state dpll_hw_state;
> > >  	struct intel_shared_dpll *shared_dpll;
> > > -	bool force_thru;
> > > +	bool force_thru, ips_force_disable;
> > >  
> > >  	/* FIXME: before the switch to atomic started, a new pipe_config was
> > >  	 * kzalloc'd. Code that depends on any field being zero should be
> > > @@ -12970,6 +12971,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > >  	shared_dpll = crtc_state->shared_dpll;
> > >  	dpll_hw_state = crtc_state->dpll_hw_state;
> > >  	force_thru = crtc_state->pch_pfit.force_thru;
> > > +	ips_force_disable = crtc_state->ips_force_disable;
> > >  
> > >  	memset(crtc_state, 0, sizeof *crtc_state);
> > >  
> > > @@ -12978,6 +12980,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > >  	crtc_state->shared_dpll = shared_dpll;
> > >  	crtc_state->dpll_hw_state = dpll_hw_state;
> > >  	crtc_state->pch_pfit.force_thru = force_thru;
> > > +	crtc_state->ips_force_disable = ips_force_disable;
> > >  }
> > >  
> > >  static int
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 025e4c8b3e63..cadba9b92cc9 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -651,6 +651,7 @@ struct intel_crtc_state {
> > >  	struct intel_link_m_n fdi_m_n;
> > >  
> > >  	bool ips_enabled;
> > > +	bool ips_force_disable;
> > Could we rename this to collecting_crc throughout the patch?
> 
> If we do, then we should probably kill off the separate pfit
> force_thru boolean as well and just use 'collecting_crc' for
> the pipe A routing decisions as well.

On further thought that name would be somewhat misleading if we only
set it for HSW/BDW+pipe A.

> 
> > 
> > And as Marta noted, intel_crtc_set_crc_source also needs fixing. :)
> 
> Doh. I thought I retipped the patch, but apparently I didn't.
> 
> > >  
> > >  	bool enable_fbc;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index ef0c0e195164..74780b090d1e 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -547,8 +547,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> > >  	return 0;
> > >  }
> > >  
> > > -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > > -					bool enable)
> > > +static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > > +			      bool enable)
> > >  {
> > >  	struct drm_device *dev = &dev_priv->drm;
> > >  	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> > > @@ -570,11 +570,23 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > >  		goto out;
> > >  	}
> > >  
> > > -	pipe_config->pch_pfit.force_thru = enable;
> > > -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > > -	    pipe_config->pch_pfit.enabled != enable)
> > > +	/*
> > > +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > > +	 * enabled and disabled dynamically based on package C states,
> > > +	 * user space can't make reliable use of the CRCs, so let's just
> > > +	 * completely disable it.
> > > +	 */
> > > +	pipe_config->ips_force_disable = enable;
> > > +	if (pipe_config->ips_enabled == enable)
> > >  		pipe_config->base.connectors_changed = true;
> > >  
> > > +	if (IS_HASWELL(dev_priv)) {
> > > +		pipe_config->pch_pfit.force_thru = enable;
> > > +		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > > +		    pipe_config->pch_pfit.enabled != enable)
> > > +			pipe_config->base.connectors_changed = true;
> > > +	}
> > > +
> > >  	ret = drm_atomic_commit(state);
> > >  out:
> > >  	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> > > @@ -598,8 +610,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> > >  		break;
> > >  	case INTEL_PIPE_CRC_SOURCE_PF:
> > > -		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
> > > +		if ((IS_HASWELL(dev_priv) ||
> > > +		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > +			hsw_pipe_A_crc_wa(dev_priv, true);
> > >  
> > >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> > >  		break;
> > > @@ -618,7 +631,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> > >  			       enum intel_pipe_crc_source source)
> > >  {
> > >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> > > -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > >  	enum intel_display_power_domain power_domain;
> > >  	u32 val = 0; /* shut up gcc */
> > >  	int ret;
> > > @@ -665,14 +677,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> > >  			goto out;
> > >  		}
> > >  
> > > -		/*
> > > -		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > > -		 * enabled and disabled dynamically based on package C states,
> > > -		 * user space can't make reliable use of the CRCs, so let's just
> > > -		 * completely disable it.
> > > -		 */
> > > -		hsw_disable_ips(crtc);
> > > -
> > >  		spin_lock_irq(&pipe_crc->lock);
> > >  		kfree(pipe_crc->entries);
> > >  		pipe_crc->entries = entries;
> > > @@ -713,10 +717,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> > >  			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
> > >  		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >  			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
> > > -		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
> > > -			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> > > -
> > > -		hsw_enable_ips(crtc);
> > > +		else if ((IS_HASWELL(dev_priv) ||
> > > +			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > +			hsw_pipe_A_crc_wa(dev_priv, false);
> > >  	}
> > >  
> > >  	ret = 0;
> > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Beef up the IPS vs. CRC workaround

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Oneshot disabling of IPS when CRC capturing is started is insufficient.
IPS may get re-enabled by any plane update, and hence tests that keep
CRC capturing on across plane updates will start to see inconsistent
results as soon as IPS kicks back in. Add a new knob into the crtc state
to make sure IPS stays disabled as long as CRC capturing is enabled.

Forcing a modeset is the easiest way to handle this since that's already
how we do the panel fitter workaround. It's a little heavy handed just
for IPS, but seeing as we might already do the panel fitter workaround
I think it's better to follow that. We migth want to optimize both cases
later if someone gets too upset by the extra delay from the modeset.

v2: Check the right thing when deciding whether to force a modeset
v3: Rebase, check HAS_IPS before forcing a modeset,
    move ips_force_disable check into pipe_config_supports_ips()

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Marta Lofsted <marta.lofstedt@intel.com> #v2
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  7 +++-
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 62 ++++++++++++++++-------------------
 3 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0e93ec201fe3..e8a24b59c91c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6263,6 +6263,9 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 				     struct intel_crtc_state *pipe_config)
 {
+	if (pipe_config->ips_force_disable)
+		return false;
+
 	if (pipe_config->pipe_bpp > 24)
 		return false;
 
@@ -10830,7 +10833,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	struct intel_shared_dpll *shared_dpll;
 	struct intel_crtc_wm_state wm_state;
-	bool force_thru;
+	bool force_thru, ips_force_disable;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -10841,6 +10844,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	force_thru = crtc_state->pch_pfit.force_thru;
+	ips_force_disable = crtc_state->ips_force_disable;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		wm_state = crtc_state->wm;
@@ -10854,6 +10858,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->pch_pfit.force_thru = force_thru;
+	crtc_state->ips_force_disable = ips_force_disable;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		crtc_state->wm = wm_state;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fa47285918f4..8a9db2279bbd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -753,6 +753,7 @@ struct intel_crtc_state {
 	struct intel_link_m_n fdi_m_n;
 
 	bool ips_enabled;
+	bool ips_force_disable;
 
 	bool enable_fbc;
 
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 8fbd2bd0877f..4e22bb927fed 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -506,8 +506,8 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
-					bool enable)
+static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
+			      bool enable)
 {
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
@@ -533,10 +533,24 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		goto put_state;
 	}
 
-	pipe_config->pch_pfit.force_thru = enable;
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    pipe_config->pch_pfit.enabled != enable)
-		pipe_config->base.connectors_changed = true;
+	if (HAS_IPS(dev_priv)) {
+		/*
+		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+		 * enabled and disabled dynamically based on package C states,
+		 * user space can't make reliable use of the CRCs, so let's just
+		 * completely disable it.
+		 */
+		pipe_config->ips_force_disable = enable;
+		if (pipe_config->ips_enabled == enable)
+			pipe_config->base.connectors_changed = true;
+	}
+
+	if (IS_HASWELL(dev_priv)) {
+		pipe_config->pch_pfit.force_thru = enable;
+		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+		    pipe_config->pch_pfit.enabled != enable)
+			pipe_config->base.connectors_changed = true;
+	}
 
 	ret = drm_atomic_commit(state);
 
@@ -570,8 +584,9 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, true);
+		if ((IS_HASWELL(dev_priv) ||
+		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -606,7 +621,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			       enum intel_pipe_crc_source source)
 {
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 	enum intel_display_power_domain power_domain;
 	u32 val = 0; /* shut up gcc */
 	int ret;
@@ -643,14 +657,6 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			goto out;
 		}
 
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		hsw_disable_ips(crtc);
-
 		spin_lock_irq(&pipe_crc->lock);
 		kfree(pipe_crc->entries);
 		pipe_crc->entries = entries;
@@ -691,10 +697,9 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 			g4x_undo_pipe_scramble_reset(dev_priv, pipe);
 		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, pipe);
-		else if (IS_HASWELL(dev_priv) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
-
-		hsw_enable_ips(crtc);
+		else if ((IS_HASWELL(dev_priv) ||
+			  IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, false);
 	}
 
 	ret = 0;
@@ -935,16 +940,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 	if (ret != 0)
 		goto out;
 
-	if (source) {
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		hsw_disable_ips(intel_crtc);
-	}
-
 	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
 
@@ -953,8 +948,9 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
 		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
-		else if (IS_HASWELL(dev_priv) && crtc->index == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
+		else if ((IS_HASWELL(dev_priv) ||
+			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
+			hsw_pipe_A_crc_wa(dev_priv, false);
 
 		hsw_enable_ips(intel_crtc);
 	}
-- 
2.13.0

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

✗ Fi.CI.BAT: warning for drm/i915: Beef up the IPS vs. CRC workaround (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Beef up the IPS vs. CRC workaround (rev3)
URL   : https://patchwork.freedesktop.org/series/17084/
State : warning

== Summary ==

Series 17084v3 drm/i915: Beef up the IPS vs. CRC workaround
https://patchwork.freedesktop.org/api/1.0/series/17084/revisions/3/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:267  dwarn:1   dfail:0   fail:0   skip:11  time:455s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:443s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:359s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:535s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:531s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:525s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:511s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:607s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:443s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:421s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:506s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:595s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:596s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:530s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:468s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:488s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:438s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:485s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:550s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:403s

e1b18fbd4daecb1c1cf31ca101f64e29a8933bcf drm-tip: 2017y-08m-17d-14h-58m-23s UTC integration manifest
568266b967eb drm/i915: Beef up the IPS vs. CRC workaround

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5430/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: Beef up the IPS vs. CRC workaround (rev3)

[Ville Syrjälä]

On Thu, Aug 17, 2017 at 03:58:19PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Beef up the IPS vs. CRC workaround (rev3)
> URL   : https://patchwork.freedesktop.org/series/17084/
> State : warning
> 
> == Summary ==
> 
> Series 17084v3 drm/i915: Beef up the IPS vs. CRC workaround
> https://patchwork.freedesktop.org/api/1.0/series/17084/revisions/3/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 pass       -> FAIL       (fi-snb-2600) fdo#100007
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (fi-bdw-5557u)

[  362.598196] [drm:intel_fbc_work_fn [i915]] *ERROR* vblank not available for FBC on pipe A

I guess there is some kind of race with crtc enabling/disabling in the
FBC code . I've seen this a few times on my IVB laptop when running
with FBC enabled.

> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-b:
>                 pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705
> 
> fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
> fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
> 
> fi-bdw-5557u     total:279  pass:267  dwarn:1   dfail:0   fail:0   skip:11  time:455s
> fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:443s
> fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:359s
> fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:535s
> fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:531s
> fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:525s
> fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:511s
> fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:607s
> fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:443s
> fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:421s
> fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
> fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:506s
> fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
> fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
> fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:595s
> fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:596s
> fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:530s
> fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:468s
> fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:488s
> fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:438s
> fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:485s
> fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:550s
> fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:403s
> 
> e1b18fbd4daecb1c1cf31ca101f64e29a8933bcf drm-tip: 2017y-08m-17d-14h-58m-23s UTC integration manifest
> 568266b967eb drm/i915: Beef up the IPS vs. CRC workaround
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5430/

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Beef up the IPS vs. CRC workaround

[Ville Syrjälä]

On Thu, Aug 17, 2017 at 05:55:09PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Oneshot disabling of IPS when CRC capturing is started is insufficient.
> IPS may get re-enabled by any plane update, and hence tests that keep
> CRC capturing on across plane updates will start to see inconsistent
> results as soon as IPS kicks back in. Add a new knob into the crtc state
> to make sure IPS stays disabled as long as CRC capturing is enabled.
> 
> Forcing a modeset is the easiest way to handle this since that's already
> how we do the panel fitter workaround. It's a little heavy handed just
> for IPS, but seeing as we might already do the panel fitter workaround
> I think it's better to follow that. We migth want to optimize both cases
> later if someone gets too upset by the extra delay from the modeset.
> 
> v2: Check the right thing when deciding whether to force a modeset
> v3: Rebase, check HAS_IPS before forcing a modeset,
>     move ips_force_disable check into pipe_config_supports_ips()
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Marta Lofsted <marta.lofstedt@intel.com> #v2
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to dinq. Thanks for the review and testing.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx