[Intel-gfx] [PATCH] drm/i915: Disable the mmio.debug WARN after it fires

[Chris Wilson]

On Mon, Nov 24, 2014 at 10:45:38AM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 08:12:57AM +0000, Chris Wilson wrote:
> > If we have a single unclaimed register, we will have lots. A WARN for
> > each one makes the machine unusable and does not aid debugging. Convert
> > the i915.mmio_debug option to a counter for how many WARNs to fire
> > before shutting up. Even when i915.mmio_debug was disabled it would
> > continue to shout an *ERROR* for every interrupt, without any
> > information at all for debugging.
> > 
> > The massive verbiage was added in
> > commit 5978118c39c2f72fd8b39ef9c086723542384809
> > Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Date:   Wed Jul 16 17:49:29 2014 -0300
> > 
> >     drm/i915: reorganize the unclaimed register detection code
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
> >  drivers/gpu/drm/i915/i915_params.c  | 5 ++---
> >  drivers/gpu/drm/i915/intel_uncore.c | 6 ++++--
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5448ce9d1490..314d8a60d55b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2310,7 +2310,7 @@ struct i915_params {
> >  	bool disable_display;
> >  	bool disable_vtd_wa;
> >  	int use_mmio_flip;
> > -	bool mmio_debug;
> > +	int mmio_debug;
> >  };
> >  extern struct i915_params i915 __read_mostly;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index c91cb2033cc5..654a492fcfca 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -169,7 +169,6 @@ module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
> >  MODULE_PARM_DESC(use_mmio_flip,
> >  		 "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)");
> >  
> > -module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
> > +module_param_named(mmio_debug, i915.mmio_debug, int, 0600);
> >  MODULE_PARM_DESC(mmio_debug,
> > -	"Enable the MMIO debug code (default: false). This may negatively "
> > -	"affect performance.");
> > +	"Enable the MMIO debug code. This may negatively affect performance.");
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index f0230b0e8e11..b830407dc2aa 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -714,25 +714,27 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
> >  	const char *op = read ? "reading" : "writing to";
> >  	const char *when = before ? "before" : "after";
> >  
> > -	if (!i915.mmio_debug)
> > +	if (i915.mmio_debug <= 0)
> >  		return;
> >  
> >  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >  		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
> >  		     when, op, reg);
> >  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> > +		i915.mmio_debug--;
> >  	}
> >  }
> >  
> >  static void
> >  hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >  {
> > -	if (i915.mmio_debug)
> > +	if (i915.mmio_debug > 0)
> >  		return;
> >  
> >  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >  		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
> >  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> > +		i915.mmio_debug--;
> 
> Why the decrement here? And if we make this fancy, maybe we should have a
> oneshot set to enable mmio_debug here, i.e.

I only want to see the ERROR once, not every interrupt :)
 
> static mmio_debug_oneshot = 1;
> 
> if (unclaimed_mmio_detected) {
> 	clear();
> 	mmio_debug = mmio_debug_oneshot--;
> }
> 
> Or something similar? That way if it's recurring we'll capture it, but
> only once.

I suggested something similar to do a one shot enabling of mmio.debug if
we hit the error during interrupts to tumbleweeds. I think that would be
much more useful than our current error.

So

index b830407dc2aa..e7a990c449c4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -714,7 +714,7 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
        const char *op = read ? "reading" : "writing to";
        const char *when = before ? "before" : "after";
 
-       if (i915.mmio_debug <= 0)
+       if (i915.mmio_debug)
                return;
 
        if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
@@ -728,13 +728,17 @@ 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)
 {
-       if (i915.mmio_debug > 0)
+       static bool mmio_debug_once = true;
+
+       if (i915.mmio_debug || !i915.mmio_debug_once)
                return;
 
        if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-               DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
+               DRM_DEBUG("Unclaimed register detected, "
+                         "enabling oneshot unclaimed register detection. "
+                         "Please use the i915.mmio_debug=N for more information.\n");
                __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
-               i915.mmio_debug--;
+               i915.mmio_debug = i915.mmio_debug_once--;
        }
 }

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre