Re: [PATCH 1/2] drm/i915: Move the GTFIFODBG to the common mmio dbg framework

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 06, 2017 at 06:39:42PM +0300, Mika Kuoppala wrote:
> Remove the per-mmio checking of the FIFO debug register into the common
> conditional mmio debug handling. Based on patch from Chris Wilson.
> 
> v2: postpone warn on fifodbg for unclaimed reg debugs
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 76 +++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 6d1ea26..7a8eb2e 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -172,22 +172,6 @@ static void fw_domains_get_with_thread_status(struct drm_i915_private *dev_priv,
>  	__gen6_gt_wait_for_thread_c0(dev_priv);
>  }
>  
> -static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
> -{
> -	u32 gtfifodbg;
> -
> -	gtfifodbg = __raw_i915_read32(dev_priv, GTFIFODBG);
> -	if (WARN(gtfifodbg, "GT wake FIFO error 0x%x\n", gtfifodbg))
> -		__raw_i915_write32(dev_priv, GTFIFODBG, gtfifodbg);
> -}
> -
> -static void fw_domains_put_with_fifo(struct drm_i915_private *dev_priv,
> -				     enum forcewake_domains fw_domains)
> -{
> -	fw_domains_put(dev_priv, fw_domains);
> -	gen6_gt_check_fifodbg(dev_priv);
> -}
> -
>  static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv)
>  {
>  	u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL);
> @@ -384,15 +368,35 @@ vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  }
>  
>  static bool
> +gen6_check_for_fifo_debug(struct drm_i915_private *dev_priv)
> +{
> +	u32 fifodbg;
> +
> +	fifodbg = __raw_i915_read32(dev_priv, GTFIFODBG);
> +
> +	if (unlikely(fifodbg)) {
> +		DRM_DEBUG_DRIVER("GTFIFODBG = 0x08%x\n", fifodbg);
> +		__raw_i915_write32(dev_priv, GTFIFODBG, fifodbg);
> +	}
> +
> +	return fifodbg;
> +}
> +
> +static bool
>  check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  {
> +	bool ret = false;
> +
>  	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> -		return fpga_check_for_unclaimed_mmio(dev_priv);
> +		ret |= fpga_check_for_unclaimed_mmio(dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		return vlv_check_for_unclaimed_mmio(dev_priv);
> +		ret |= vlv_check_for_unclaimed_mmio(dev_priv);
>  
> -	return false;
> +	if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
> +		ret |= gen6_check_for_fifo_debug(dev_priv);

I'd prefer to keep unclaim vs. wake FIFO separate because the
unclaimned stuff is only for display registers. In my plan to
split the uncore lock into gt and display locks the unclaimed
reg stuff would end up being protected by the display lock rather
than the gt lock.

> +
> +	return ret;
>  }
>  
>  static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> @@ -404,11 +408,6 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>  	if (check_for_unclaimed_mmio(dev_priv))
>  		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>  
> -	/* clear out old GT FIFO errors */
> -	if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
> -		__raw_i915_write32(dev_priv, GTFIFODBG,
> -				   __raw_i915_read32(dev_priv, GTFIFODBG));
> -
>  	/* WaDisableShadowRegForCpd:chv */
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		__raw_i915_write32(dev_priv, GTFIFOCTL,
> @@ -1047,15 +1046,10 @@ __gen2_write(32)
>  #define __gen6_write(x) \
>  static void \
>  gen6_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
> -	u32 __fifo_ret = 0; \
>  	GEN6_WRITE_HEADER; \
> -	if (NEEDS_FORCE_WAKE(offset)) { \
> -		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> -	} \
> +	if (NEEDS_FORCE_WAKE(offset)) \
> +		__gen6_gt_wait_for_fifo(dev_priv); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (unlikely(__fifo_ret)) { \
> -		gen6_gt_check_fifodbg(dev_priv); \
> -	} \
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> @@ -1190,11 +1184,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  			       FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		dev_priv->uncore.funcs.force_wake_get = fw_domains_get;
> -		if (!IS_CHERRYVIEW(dev_priv))
> -			dev_priv->uncore.funcs.force_wake_put =
> -				fw_domains_put_with_fifo;
> -		else
> -			dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
> +		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
>  		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
>  			       FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
>  		fw_domain_init(dev_priv, FW_DOMAIN_ID_MEDIA,
> @@ -1202,11 +1192,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		dev_priv->uncore.funcs.force_wake_get =
>  			fw_domains_get_with_thread_status;
> -		if (IS_HASWELL(dev_priv))
> -			dev_priv->uncore.funcs.force_wake_put =
> -				fw_domains_put_with_fifo;
> -		else
> -			dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
> +		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
>  		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
>  			       FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>  	} else if (IS_IVYBRIDGE(dev_priv)) {
> @@ -1223,8 +1209,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  		 */
>  		dev_priv->uncore.funcs.force_wake_get =
>  			fw_domains_get_with_thread_status;
> -		dev_priv->uncore.funcs.force_wake_put =
> -			fw_domains_put_with_fifo;
> +		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
>  
>  		/* We need to init first for ECOBUS access and then
>  		 * determine later if we want to reinit, in case of MT access is
> @@ -1242,7 +1227,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  		spin_lock_irq(&dev_priv->uncore.lock);
>  		fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_RENDER);
>  		ecobus = __raw_i915_read32(dev_priv, ECOBUS);
> -		fw_domains_put_with_fifo(dev_priv, FORCEWAKE_RENDER);
> +		fw_domains_put(dev_priv, FORCEWAKE_RENDER);
>  		spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  		if (!(ecobus & FORCEWAKE_MT_ENABLE)) {
> @@ -1254,8 +1239,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  	} else if (IS_GEN6(dev_priv)) {
>  		dev_priv->uncore.funcs.force_wake_get =
>  			fw_domains_get_with_thread_status;
> -		dev_priv->uncore.funcs.force_wake_put =
> -			fw_domains_put_with_fifo;
> +		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
>  		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
>  			       FORCEWAKE, FORCEWAKE_ACK);
>  	}
> -- 
> 2.7.4
> 
> _______________________________________________
> 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