Re: [RFC 06/44] drm/i915: Fixes for FIFO space queries

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 26 Jun 2014 18:23:57 +0100
John.C.Harrison@Intel.com wrote:

> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The previous code was not correctly masking the value of the GTFIFOCTL register,
> leading to overruns and the message "MMIO read or write has been dropped". In
> addition, the checks were repeated in several different places. This commit
> replaces these various checks with a simple (inline) function to encapsulate the
> read-and-mask operation. In addition, it adds a custom wait-for-fifo function
> for VLV, as the timing parameters are somewhat different from those on earlier
> chips.
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |   49 ++++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 871c284..6a3dddf 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -47,6 +47,12 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv)
>  	     "Device suspended\n");
>  }
>  
> +static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv)
> +{
> +	u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL);
> +	return count & GT_FIFO_FREE_ENTRIES_MASK;
> +}
> +
>  static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
>  {
>  	u32 gt_thread_status_mask;
> @@ -154,6 +160,28 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>  		gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> +static int __vlv_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> +{
> +	u32 free = fifo_free_entries(dev_priv);
> +	int loop1, loop2;
> +
> +	for (loop1 = 0; loop1 < 5000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; ) {
> +		for (loop2 = 0; loop2 < 1000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; loop2 += 10) {
> +			udelay(10);
> +			free = fifo_free_entries(dev_priv);
> +		}
> +		loop1 += loop2;
> +		if (loop1 > 1000 || free < 48)
> +			DRM_DEBUG("after %d us, the FIFO has %d slots", loop1, free);
> +	}
> +
> +	dev_priv->uncore.fifo_count = free;
> +	if (WARN(free < GT_FIFO_NUM_RESERVED_ENTRIES,
> +		"FIFO has insufficient space (%d slots)", free))
> +		return -1;
> +	return 0;
> +}
> +
>  static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  {
>  	int ret = 0;
> @@ -161,16 +189,15 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  	/* On VLV, FIFO will be shared by both SW and HW.
>  	 * So, we need to read the FREE_ENTRIES everytime */
>  	if (IS_VALLEYVIEW(dev_priv->dev))
> -		dev_priv->uncore.fifo_count =
> -			__raw_i915_read32(dev_priv, GTFIFOCTL) &
> -						GT_FIFO_FREE_ENTRIES_MASK;
> +		return __vlv_gt_wait_for_fifo(dev_priv);
>  
>  	if (dev_priv->uncore.fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
>  		int loop = 500;
> -		u32 fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> +		u32 fifo = fifo_free_entries(dev_priv);
> +
>  		while (fifo <= GT_FIFO_NUM_RESERVED_ENTRIES && loop--) {
>  			udelay(10);
> -			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> +			fifo = fifo_free_entries(dev_priv);
>  		}
>  		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
>  			++ret;
> @@ -194,6 +221,11 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
>  static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  						int fw_engine)
>  {
> +#if	1
> +	if (__gen6_gt_wait_for_fifo(dev_priv))
> +		gen6_gt_check_fifodbg(dev_priv);
> +#endif
> +
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine) {
>  		if (wait_for_atomic((__raw_i915_read32(dev_priv,
> @@ -238,6 +270,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  					int fw_engine)
>  {
> +#if	1
> +	if (__gen6_gt_wait_for_fifo(dev_priv))
> +		gen6_gt_check_fifodbg(dev_priv);
> +#endif
>  
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine)
> @@ -355,8 +391,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  
>  		if (IS_GEN6(dev) || IS_GEN7(dev))
>  			dev_priv->uncore.fifo_count =
> -				__raw_i915_read32(dev_priv, GTFIFOCTL) &
> -				GT_FIFO_FREE_ENTRIES_MASK;
> +				fifo_free_entries(dev_priv);
>  	} else {
>  		dev_priv->uncore.forcewake_count = 0;
>  		dev_priv->uncore.fw_rendercount = 0;

It would be best to split out the free_entries cleanup (a good one)
from the vlv bug fix, and also drop the #if 1s.

With that done:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center