[Intel-gfx] [PATCH] drm/i915: fix FORCEWAKE posting reads

[Ben Widawsky]

On Fri, Jan 11, 2013 at 09:53:53AM +0200, Jani Nikula wrote:
> We stopped reading FORCEWAKE for posting reads in
> 
> commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd
> Author: Ben Widawsky <ben at bwidawsk.net>
> Date:   Sat Sep 1 22:59:50 2012 -0700
> 
>     drm/i915: Never read FORCEWAKE
> 
> and started using something from the same cacheline instead. This was true
> for ECOBUS, but not for GTFIFODBG. Add posting reads on ECOBUS where
> GTFIFODBG was relied on.
> 
> In true cargo cult spirit, add posting reads for FORCEWAKE_VLV writes as
> well.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=52411
> Reported-and-tested-by: Alexander Bersenev <bay at hackerdom.ru>
> CC: Ben Widawsky <ben at bwidawsk.net>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>

Am I the only skeptic about what is actually fixing the bug here?
Without WC MMIO, the cacheline we readback from makes no difference.

ie. I put ECOBUS there for a warm and fuzzy feeling, nothing else.

> ---
>  drivers/gpu/drm/i915/intel_pm.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index abfff29..06b75a1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4122,7 +4122,8 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  static void __gen6_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(0xffff));
> -	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
> +	/* something from same cacheline, but !FORCEWAKE_MT */
> +	POSTING_READ(ECOBUS);
>  }
>  
>  static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
> @@ -4139,7 +4140,8 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("Timed out waiting for forcewake old ack to clear.\n");
>  
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
> -	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
> +	/* something from same cacheline, but !FORCEWAKE_MT */
> +	POSTING_READ(ECOBUS);
>  
>  	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
> @@ -4176,14 +4178,16 @@ void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>  static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE, 0);
> -	/* gen6_gt_check_fifodbg doubles as the POSTING_READ */
> +	/* something from same cacheline, but !FORCEWAKE */
> +	POSTING_READ(ECOBUS);
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
>  static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
> -	/* gen6_gt_check_fifodbg doubles as the POSTING_READ */
> +	/* something from same cacheline, but !FORCEWAKE_MT */
> +	POSTING_READ(ECOBUS);
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> @@ -4223,6 +4227,8 @@ int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, _MASKED_BIT_DISABLE(0xffff));
> +	/* something from same cacheline, but !FORCEWAKE_VLV */
> +	POSTING_READ(FORCEWAKE_ACK_VLV);
>  }
>  
>  static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
> @@ -4243,7 +4249,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
> -	/* The below doubles as a POSTING_READ */
> +	/* something from same cacheline, but !FORCEWAKE_VLV */
> +	POSTING_READ(FORCEWAKE_ACK_VLV);
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -- 
> 1.7.9.5
> 

-- 
Ben Widawsky, Intel Open Source Technology Center