Re: [PATCH 4/4] drm/i915: Reduce frequency of unspecific HSW reg debugging

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Sex, 2015-09-04 às 11:40 +0300, Mika Kuoppala escreveu:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Thu, Sep 03, 2015 at 04:51:45PM -0300, Paulo Zanoni wrote:
> > > From: Chris Wilson <chris@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@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > I'm unclear how this interacts (or how it sould interact) with 
> > patch 2:
> > Forcwake is mostly for GT registers, but patch 2 also tries to 
> > optimize
> > forcwake for GT registers. Do we really need both?
> 
> Assuming the hardware detects access to unpowered domains and
> to unregistered ranges by setting this bit, I would say that patch 2
> is not needed. One could argue that patch 2 is somewhat harmful as
> current register access pattern affects the detection.

Can you please elaborate? I'm more in favor of keeping patch 2 instead
of this. But I'm operating under the assumption that we only flip
FPGA_DBG bit 31 to 1 if we do an access inside the range. See the
discussion on patch 2. So doing the checks outside the range is useless
since it won't catch problems inside i915.ko.

> 
> Also the commit message in patch 2 is not valid wrt the code.

Why?

> 
> With skl, the debug bit seems to decay with time, instead of being
> sticky. So in there we could argue that in patch 4/4, the reading
> should be done before (and after) the forcewake scope.

I tested my patch on SKL and it still does detect the same problems it
was detecting before. Maybe we could write a little debugfs file to do
the accesses in the mentioned pattern and check the forcewake theory.

> 
> Paulo, have you tried if this bit detects access to unpowered
> domain with hsw/bdw?

FPGA_DBG bit 31 becomes 1 when we access an existing register on an
unpowered power well on HSW/BDW.

> 
> -Mika
> 
> > -Daniel
> > 
> > > ---
> > >  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;
> > > -
> > > -	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);
> > >  }
> > > -- 
> > > 2.5.0
> > > 
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx