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

[Paulo Zanoni]

2014-11-24 9:07 GMT-02:00 Chris Wilson <chris at chris-wilson.co.uk>:
> On Mon, Nov 24, 2014 at 12:52:23PM +0200, Jani Nikula wrote:
>> On Mon, 24 Nov 2014, Chris Wilson <chris at 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 at 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.
>> >
>> > 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>
>> > Cc: Daniel Vetter <daniel.vetter at 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 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..9de6e8265a14 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -169,7 +169,7 @@ 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.");
>> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> > index f0230b0e8e11..01ae5aec9e02 100644
>> > --- a/drivers/gpu/drm/i915/intel_uncore.c
>> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> > @@ -721,18 +721,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 || !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 reporting. "
>> > +                     "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_once--;
>>
>> I suspect you have uncommitted changes in your work tree.
>
> Or that I only use this tree to write gedankenexperiment against -nightly...

As long as there's still a way to disable the one-shot thing (either
by passing i915.mmio_debug=1000 or by i915.mmio_debug_once=0), this
idea/patch and the other WARN_ONCE patch are: Acked-by: Paulo Zanoni
<paulo.r.zanoni at intel.com>. I see there's still some discussion, so
I'm not sure which one will be the final patch...

Most of the cases I investigated didn't cause enough WARNs to make
dmesg useless, so I still see utility on having a way to print more
than the first error. Bonus points if the default option is set to
something like "print only the first 20 errors" :)

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni