* [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires
@ 2014-12-18 10:55 Chris Wilson
2014-12-18 12:36 ` Jani Nikula
2014-12-18 15:25 ` shuang.he
0 siblings, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2014-12-18 10:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Paulo Zanoni
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.");
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");
__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+ i915.mmio_debug = mmio_debug_once--;
}
}
--
2.1.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires 2014-12-18 10:55 [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires Chris Wilson @ 2014-12-18 12:36 ` Jani Nikula 2014-12-18 12:47 ` Chris Wilson 2014-12-18 15:25 ` shuang.he 1 sibling, 1 reply; 6+ messages in thread From: Jani Nikula @ 2014-12-18 12:36 UTC (permalink / raw) To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, Paulo Zanoni 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? > > 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"); > __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? BR, Jani. > } > } > > -- > 2.1.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires 2014-12-18 12:36 ` Jani Nikula @ 2014-12-18 12:47 ` Chris Wilson 2014-12-26 19:02 ` Paulo Zanoni 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2014-12-18 12:47 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx, Paulo Zanoni 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"); > > __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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires 2014-12-18 12:47 ` Chris Wilson @ 2014-12-26 19:02 ` Paulo Zanoni 2015-03-06 14:02 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Paulo Zanoni @ 2014-12-26 19:02 UTC (permalink / raw) To: Chris Wilson, Jani Nikula, Intel Graphics Development, Daniel Vetter, Paulo Zanoni 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> >> > __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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires 2014-12-26 19:02 ` Paulo Zanoni @ 2015-03-06 14:02 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2015-03-06 14:02 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires 2014-12-18 10:55 [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires Chris Wilson 2014-12-18 12:36 ` Jani Nikula @ 2014-12-18 15:25 ` shuang.he 1 sibling, 0 replies; 6+ messages in thread From: shuang.he @ 2014-12-18 15:25 UTC (permalink / raw) To: shuang.he, intel-gfx, chris Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -3 364/364 361/364 ILK +1-4 364/366 361/366 SNB -1 448/450 447/450 IVB -2 497/498 495/498 BYT -1 289/289 288/289 HSW -1 563/564 562/564 BDW -1 416/417 415/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied PNV igt_gem_exec_blt NRUN(2, M23M7)PASS(1, M25) NRUN(1, M7) *PNV igt_gen3_mixed_blits PASS(2, M25M7) FAIL(1, M7) *PNV igt_gen3_render_tiledx_blits PASS(2, M25M7) FAIL(1, M7) ILK igt_gem_exec_blt NRUN(2, M37M26)PASS(1, M26) NRUN(1, M26) *ILK igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible PASS(2, M26) DMESG_WARN(1, M26) ILK igt_kms_render_direct-render DMESG_WARN(1, M26)PASS(1, M26) DMESG_WARN(1, M26) ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible DMESG_WARN(1, M26)PASS(6, M37M26) PASS(1, M26) *ILK igt_kms_flip_rcs-flip-vs-modeset PASS(2, M26) DMESG_WARN(1, M26) SNB igt_gem_exec_blt NRUN(3, M35M22)PASS(1, M35) NRUN(1, M22) IVB igt_gem_exec_blt NRUN(3, M34M21M4)PASS(1, M34) NRUN(1, M21) *IVB igt_kms_cursor_crc_cursor-128x128-random PASS(2, M34M21) DMESG_WARN(1, M21) BYT igt_gem_exec_blt NRUN(3, M48M49M50)PASS(1, M48) NRUN(1, M49) HSW igt_gem_exec_blt NRUN(3, M40M20M19)PASS(1, M40) NRUN(1, M20) BDW igt_gem_exec_blt NRUN(2, M30M28)PASS(1, M28) NRUN(1, M28) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-06 14:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-18 10:55 [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires Chris Wilson 2014-12-18 12:36 ` Jani Nikula 2014-12-18 12:47 ` Chris Wilson 2014-12-26 19:02 ` Paulo Zanoni 2015-03-06 14:02 ` Daniel Vetter 2014-12-18 15:25 ` shuang.he
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.