* [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 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
* 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
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.