[Intel-gfx] [PATCH 4/4] drm/i915: Reduce frequency of unspecific HSW reg debugging

[Zanoni, Paulo R]

Em Sex, 2015-09-04 às 10:02 +0300, Mika Kuoppala escreveu:
> Paulo Zanoni <paulo.r.zanoni at intel.com> writes:
> 
> > From: Chris Wilson <chris at chris-wilson.co.uk>
> > 
> > Delay the expensive read on the FPGA_DBG register from once per 
> > mmio to
> > once per forcewake section when we are doing the general wellbeing
> > check rather than the targetted error detection. This almost 
> > reduces
> > the overhead of the debug facility (for example when submitting 
> > execlists)
> > to zero whilst keeping the debug checks around.
> > 
> > v2: Enable one-shot mmio debugging from the interrupt check as well 
> > as a
> >     safeguard to catch invalid display writes from outside the 
> > powerwell.
> > v3 (from Paulo): rebase since gen9 addition and 
> > intel_uncore_check_errors
> >     removal
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 52 +++++++++++++++++++++----
> > ------------
> >  1 file changed, 29 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8844c314..1fe63fc 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -148,6 +148,31 @@ fw_domains_put(struct drm_i915_private 
> > *dev_priv, enum forcewake_domains fw_doma
> >  }
> >  
> >  static void
> > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> > +{
> > +	static bool mmio_debug_once = true;
> > +
> > +	if (i915.mmio_debug || !mmio_debug_once)
> > +		return;
> > +
> > +	if (__raw_i915_read32(dev_priv, FPGA_DBG) & 
> > FPGA_DBG_RM_NOCLAIM) {
> > +		DRM_ERROR("Unclaimed register detected, "
> > +			  "enabling oneshot unclaimed register 
> > reporting. "
> > +			  "Please use i915.mmio_debug=N for more 
> > information.\n");
> > +		__raw_i915_write32(dev_priv, FPGA_DBG, 
> > FPGA_DBG_RM_NOCLAIM);
> > +		i915.mmio_debug = mmio_debug_once--;
> > +	}
> > +}
> > +
> > +static void
> > +fw_domains_put_debug(struct drm_i915_private *dev_priv,
> > +		     enum forcewake_domains fw_domains)
> > +{
> > +	hsw_unclaimed_reg_detect(dev_priv);
> > +	fw_domains_put(dev_priv, fw_domains);
> > +}
> > +
> > +static void
> >  fw_domains_posting_read(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_uncore_forcewake_domain *d;
> > @@ -627,26 +652,6 @@ hsw_unclaimed_reg_debug(struct 
> > drm_i915_private *dev_priv, u32 reg, bool read,
> >  	}
> >  }
> >  
> > -static void
> > -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv, u32 
> > reg)
> > -{
> > -	static bool mmio_debug_once = true;
> > -
> > -	if (!UNCLAIMED_CHECK_RANGE(reg))
> > -		return;
> > -
> 
> You lost the range check in the moved version of function.

This was not an accident. After moving the function, the "reg" argument
doesn't make sense anymore, since the function is now called from
fw_domains_put() instead of the register macros.


> -Mika
> 
> > -	if (i915.mmio_debug || !mmio_debug_once)
> > -		return;
> > -
> > -	if (__raw_i915_read32(dev_priv, FPGA_DBG) & 
> > FPGA_DBG_RM_NOCLAIM) {
> > -		DRM_ERROR("Unclaimed register detected, "
> > -			  "enabling oneshot unclaimed register 
> > reporting. "
> > -			  "Please use i915.mmio_debug=N for more 
> > information.\n");
> > -		__raw_i915_write32(dev_priv, FPGA_DBG, 
> > FPGA_DBG_RM_NOCLAIM);
> > -		i915.mmio_debug = mmio_debug_once--;
> > -	}
> > -}
> > -
> >  #define GEN2_READ_HEADER(x) \
> >  	u##x val = 0; \
> >  	assert_device_not_suspended(dev_priv);
> > @@ -900,7 +905,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, 
> > off_t reg, u##x val, bool trace)
> >  		gen6_gt_check_fifodbg(dev_priv); \
> >  	} \
> >  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > -	hsw_unclaimed_reg_detect(dev_priv, reg); \
> >  	GEN6_WRITE_FOOTER; \
> >  }
> >  
> > @@ -942,7 +946,6 @@ gen8_write##x(struct drm_i915_private 
> > *dev_priv, off_t reg, u##x val, bool trace
> >  		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> >  	__raw_i915_write##x(dev_priv, reg, val); \
> >  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > -	hsw_unclaimed_reg_detect(dev_priv, reg); \
> >  	GEN6_WRITE_FOOTER; \
> >  }
> >  
> > @@ -1008,7 +1011,6 @@ gen9_write##x(struct drm_i915_private 
> > *dev_priv, off_t reg, u##x val, \
> >  		__force_wake_get(dev_priv, fw_engine); \
> >  	__raw_i915_write##x(dev_priv, reg, val); \
> >  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > -	hsw_unclaimed_reg_detect(dev_priv, reg); \
> >  	GEN6_WRITE_FOOTER; \
> >  }
> >  
> > @@ -1194,6 +1196,10 @@ static void 
> > intel_uncore_fw_domains_init(struct drm_device *dev)
> >  			       FORCEWAKE, FORCEWAKE_ACK);
> >  	}
> >  
> > +	if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
> > +	    dev_priv->uncore.funcs.force_wake_put == 
> > fw_domains_put)
> > +		dev_priv->uncore.funcs.force_wake_put = 
> > fw_domains_put_debug;
> > +
> >  	/* All future platforms are expected to require complex 
> > power gating */
> >  	WARN_ON(dev_priv->uncore.fw_domains == 0);
> >  }