Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 26, 2014 at 05:02:30PM -0200, Paulo Zanoni wrote:
> 2014-12-18 10:47 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Dec 18, 2014 at 02:36:54PM +0200, Jani Nikula wrote:
> >> On Thu, 18 Dec 2014, Chris Wilson <chris@chris-wilson.co.uk> 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@intel.com>
> >> > Date:   Wed Jul 16 17:49:29 2014 -0300
> >> >
> >> >     drm/i915: reorganize the unclaimed register detection code
> >> >
> >> > v2: Automatically enable invalid mmio reporting for the *next* invalid
> >> > access if mmio_debug is disabled by default. This should give us clearer
> >> > debug information without polluting the logs too much.
> >> > v3: Compile fixes, rebase.
> >> >
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
> >> >  drivers/gpu/drm/i915/i915_params.c  |  6 +++---
> >> >  drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++++--
> >> >  3 files changed, 12 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 3047291ff2b9..ca9e21545063 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -2430,7 +2430,7 @@ struct i915_params {
> >> >     bool disable_display;
> >> >     bool disable_vtd_wa;
> >> >     int use_mmio_flip;
> >> > -   bool mmio_debug;
> >> > +   int mmio_debug;
> >> >     bool verbose_state_checks;
> >> >  };
> >> >  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 07252d8dc726..43c1df830531 100644
> >> > --- a/drivers/gpu/drm/i915/i915_params.c
> >> > +++ b/drivers/gpu/drm/i915/i915_params.c
> >> > @@ -170,10 +170,10 @@ 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 (default: off). "
> >> > +   "This may negatively affect performance.");
> >>
> >> Why not describe the new behaviour here instead of a comment in
> >> intel_uncore.c?
> >
> > Secrets and wording.
> > "Enable the MMIO debug code for the first N failures (default: off). "
> >
> >> >  module_param_named(verbose_state_checks, i915.verbose_state_checks, bool, 0600);
> >> >  MODULE_PARM_DESC(verbose_state_checks,
> >> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> > index e9561de382aa..a3b662de1bdb 100644
> >> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> > @@ -722,18 +722,24 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
> >> >             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--; /* Only report the first N failures */
> >> >     }
> >> >  }
> >> >
> >> >  static void
> >> >  hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >> >  {
> >> > -   if (i915.mmio_debug)
> >> > +   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. Please use the i915.mmio_debug=1 to debug this problem.");
> >> > +           DRM_DEBUG("Unclaimed register detected, "
> >> > +                     "enabling oneshot unclaimed register reporting. "
> >> > +                     "Please use i915.mmio_debug=N for more information.\n");
> 
> 
> In addition to Jani's comments: I can already see people trying to use
> a literal N instead of a number and reporting to us that it's broken.
> 
> With at least the improved MODULE_PARM_DESC text that you wrote in your reply:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Help text updated and patch applied, thanks.
-Daniel

> 
> 
> 
> >> >             __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> >> > +           i915.mmio_debug = mmio_debug_once--;
> >>
> >> /me frowns upon the bool assignment to int and bool post-decrement. It's
> >> not quite IOCCC but a demonstration of things that suck about C.
> >>
> >> Is it on purpose that, if you've set i915.mmio_debug=N, you first
> >> decrement it to zero, then enter here and set i915.mmio_debug=1 to do
> >> hsw_unclaimed_reg_debug once more?
> >
> > Only in that, it serves as a nice post-script "oi, there are more
> > errors" and that tracking been here, done that was growing the scope
> > of the patch. N+2 messages is a small price to pay for having a single
> > more accurate mmio warning in the dmesg rather than an unending torrent
> > of "*ERROR* there is a problem, but we can't tell you what".
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx