Re: [PATCH 2/2] drm/i915: Don't access fifodbg registers on gen8

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Feb 19, 2014 at 06:59:26PM +0200, mika.kuoppala@intel.com wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> as they don't exists.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 25ceac4..cfb8011 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -143,7 +143,9 @@ static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>  			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
>  	/* something from same cacheline, but !FORCEWAKE_MT */
>  	__raw_posting_read(dev_priv, ECOBUS);
> -	gen6_gt_check_fifodbg(dev_priv);
> +
> +	if (IS_GEN6(dev_priv->dev) || IS_GEN7(dev_priv->dev))
> +		gen6_gt_check_fifodbg(dev_priv);
>  }

I think at this point, gen8 specific forcewake functions would be just
as good. Whatever you prefer though.

>  
>  static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> @@ -973,7 +975,10 @@ static int gen6_do_reset(struct drm_device *dev)
>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
>  
>  	/* Restore fifo count */
> -	dev_priv->uncore.fifo_count = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> +	if (IS_GEN6(dev) || IS_GEN7(dev))
> +		dev_priv->uncore.fifo_count =
> +			__raw_i915_read32(dev_priv, GTFIFOCTL) &
> +			GT_FIFO_FREE_ENTRIES_MASK;
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  	return ret;


This code looks somewhat suspicious in that I cannot fathom a case where
fifo_count should be anything other than 0 here. It seems this behavior has
existed since its induction:

commit 286fed412a134e76be55899bc628c6fa59cb70da
Author: Keith Packard <keithp@keithp.com>
Date:   Fri Jan 6 11:44:11 2012 -0800

    drm/i915: Hold gt_lock during reset

I for one would mind another patch with a WARN if it's non-zero... just
saying, and in such a case we could make the WARN gen specific, and the
dev_priv->uncore.fifo_count = 0 for all platforms.


Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center