diff mbox

[1/2] drm/i915: reorganize the unclaimed register detection code

Message ID 1405543770-3212-1-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 16, 2014, 8:49 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The current code only runs when we do an I915_WRITE operation. It
checks if the unclaimed register flag is set before we do the
operation, and then it checks it again after we do the operation. This
double check allows us to find out if the I915_WRITE operation in
question is the bad one, or if some previous code is the bad one. When
it finds a problem, our code uses DRM_ERROR to signal it.

The good thing about the current code is that it detects the problem,
so at least we can know we did something wrong. The problem is that
even though we find the problem, we don't really have much information
to actually debug it. So whenever I see one of these DRM_ERROR
messages on my systems, the first thing I do is apply a patch to
change the DRM_ERROR to a WARN and also check for unclaimed registers
on I915_READ operations. This local patch makes things even slower,
but it usually helps a lot in finding the bad code.

The first point here is that since the current code is only useful to
detect whether we have a problem or not, but it is not really good to
find the cause of the problem, I don't think we should be checking
both before and after every I915_WRITE operation: just doing the check
once should be enough for us to quickly detect problems. With this
change, the code that runs by default for every single user will only
do 1 read operation for every single I915_WRITE, instead of 2. This
patch does this change.

The second point is that the local patch I have should be upstream,
but since it makes things slower it should be disabled by default. So
I added the i915.mmio_debug option to enable it.

So after this patch, this is what will happen:
 - By default, we will try to detect unclaimed registers once after
   every I915_WRITE operation. Previously we tried twice for every
   I915_WRITE.
 - When we find an unclaimed register we will still print a DRM_ERROR
   message, but we will now tell the user to try again with
   i915.mmio_debug=1.
 - When we use i915.mmio_debug=1 we will try to find unclaimed
   registers both before and after every I915_READ and I915_WRITE
   operation, and we will print stack traces in case we find them.
   This should really help locating the exact point of the bad code
   (or at least finding out that i915.ko is not the problem).

This commit also opens space for really-slow register debugging
operations on other platforms. In theory we can now add lots and lots
of debug code behind i915.mmio_debug, enable this option on our tests,
and catch more problems.

v2: - Remove not-so-useful comments (Daniel)
    - Fix the param definition macros (Rodrigo)

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_params.c  |  6 ++++++
 drivers/gpu/drm/i915/intel_uncore.c | 27 ++++++++++++++++++++-------
 3 files changed, 27 insertions(+), 7 deletions(-)

Comments

Chris Wilson Aug. 26, 2014, 10:22 a.m. UTC | #1 
On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>  static void
> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>  {
> +	if (i915.mmio_debug)
> +		return;
> +
>  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> -		DRM_ERROR("Unclaimed write to %x\n", reg);
> +		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);
>  	}

What was the point here? You still add an extra read to every register
write and then repeat the request to enable mmio_debug ad infinitum. And
you still check for illegal writes in the irq handler.

Just kill this code.
-Chris
Paulo Zanoni Aug. 26, 2014, 12:17 p.m. UTC | #2 
2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>>  static void
>> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>>  {
>> +     if (i915.mmio_debug)
>> +             return;
>> +
>>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> +             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);
>>       }
>
> What was the point here? You still add an extra read to every register
> write

Well, we previously had 2 extra reads instead of 1, so with this patch
we're in a better position :)


> and then repeat the request to enable mmio_debug ad infinitum.

Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
frequently enough to annoy the user.


> And
> you still check for illegal writes in the irq handler.

That just happens on HSW, not BDW+.

>
> Just kill this code.

If we do it, we won't be checking for unclaimed registers on BDW+
without i915.mmio_debug=1.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Aug. 26, 2014, 12:42 p.m. UTC | #3 
On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
> >>  static void
> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >>  {
> >> +     if (i915.mmio_debug)
> >> +             return;
> >> +
> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
> >> +             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);
> >>       }
> >
> > What was the point here? You still add an extra read to every register
> > write
> 
> Well, we previously had 2 extra reads instead of 1, so with this patch
> we're in a better position :)
> 
> 
> > and then repeat the request to enable mmio_debug ad infinitum.
> 
> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
> frequently enough to annoy the user.

How do you think I came across this?

> > And
> > you still check for illegal writes in the irq handler.
> 
> That just happens on HSW, not BDW+.

Even on hsw, it should be killed. Checking inside the irq handler is
just insane. Just move it to one of the periodic checks since we can't
get any more information than an error occurred, and ask to be re-run
with mmio_debug (and then shut up) - heck you could even automatically
enable it for a one-shot operation.

> >
> > Just kill this code.
> 
> If we do it, we won't be checking for unclaimed registers on BDW+
> without i915.mmio_debug=1.

Then do as above.
-Chris
Paulo Zanoni Aug. 26, 2014, 1:04 p.m. UTC | #4 
2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
>> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>> >>  static void
>> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> >>  {
>> >> +     if (i915.mmio_debug)
>> >> +             return;
>> >> +
>> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> >> +             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);
>> >>       }
>> >
>> > What was the point here? You still add an extra read to every register
>> > write
>>
>> Well, we previously had 2 extra reads instead of 1, so with this patch
>> we're in a better position :)
>>
>>
>> > and then repeat the request to enable mmio_debug ad infinitum.
>>
>> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
>> frequently enough to annoy the user.
>
> How do you think I came across this?
>
>> > And
>> > you still check for illegal writes in the irq handler.
>>
>> That just happens on HSW, not BDW+.
>
> Even on hsw, it should be killed. Checking inside the irq handler is
> just insane. Just move it to one of the periodic checks since we can't
> get any more information than an error occurred, and ask to be re-run
> with mmio_debug (and then shut up) - heck you could even automatically
> enable it for a one-shot operation.

Yeah, looking at how the IRQ handler situation is _today_, I agree it
doesn't really make too much sense. I know it was different in the
past, so I wonder how we ended up reaching this point :)

Anyway, if we just remove the call to intel_uncore_check_errors() that
happens on the irq handler, we'll still end up checking for errors as
soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
thing we could do would be to remove the check from the I915_WRITE
macros and put it on a real periodic check that could be executed at
other specific points of our code (less frequent than every
i915_write), or put it at its own workqueue that gets run every X
jiffies. Or perhaps change the implementation of
hsw_unclaimed_reg_detect() to not do anything when we're in an
interrupt/spinlock context. Which one do you think is better to do?

Of course, we can also implement the one-shot thing on top of the
above, but it won't really help us reducing the amount of reads on the
"happy case" where we never got the error before.

All I have to say in my defense is that I did ask you to look at this
patch before it was merged :)

>
>> >
>> > Just kill this code.
>>
>> If we do it, we won't be checking for unclaimed registers on BDW+
>> without i915.mmio_debug=1.
>
> Then do as above.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Aug. 26, 2014, 1:18 p.m. UTC | #5 
On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> 2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
> >> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
> >> >>  static void
> >> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >> >>  {
> >> >> +     if (i915.mmio_debug)
> >> >> +             return;
> >> >> +
> >> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
> >> >> +             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);
> >> >>       }
> >> >
> >> > What was the point here? You still add an extra read to every register
> >> > write
> >>
> >> Well, we previously had 2 extra reads instead of 1, so with this patch
> >> we're in a better position :)
> >>
> >>
> >> > and then repeat the request to enable mmio_debug ad infinitum.
> >>
> >> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
> >> frequently enough to annoy the user.
> >
> > How do you think I came across this?
> >
> >> > And
> >> > you still check for illegal writes in the irq handler.
> >>
> >> That just happens on HSW, not BDW+.
> >
> > Even on hsw, it should be killed. Checking inside the irq handler is
> > just insane. Just move it to one of the periodic checks since we can't
> > get any more information than an error occurred, and ask to be re-run
> > with mmio_debug (and then shut up) - heck you could even automatically
> > enable it for a one-shot operation.
> 
> Yeah, looking at how the IRQ handler situation is _today_, I agree it
> doesn't really make too much sense. I know it was different in the
> past, so I wonder how we ended up reaching this point :)
> 
> Anyway, if we just remove the call to intel_uncore_check_errors() that
> happens on the irq handler, we'll still end up checking for errors as
> soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
> thing we could do would be to remove the check from the I915_WRITE
> macros and put it on a real periodic check that could be executed at
> other specific points of our code (less frequent than every
> i915_write), or put it at its own workqueue that gets run every X
> jiffies. Or perhaps change the implementation of
> hsw_unclaimed_reg_detect() to not do anything when we're in an
> interrupt/spinlock context. Which one do you think is better to do?

From the irq handler, we can use just use raw mmio interfaces and skip
all the debug and forcewake. I think the solution I prefer is to
instrument modesetting (say check_state) and warn if an error had occurred
outside of mmio_debug.
 
> Of course, we can also implement the one-shot thing on top of the
> above, but it won't really help us reducing the amount of reads on the
> "happy case" where we never got the error before.

Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
the forcewake spinlock when we already hold it. So there won't be any
such logic except when enabled by the user.
-Chris
Paulo Zanoni Aug. 26, 2014, 1:29 p.m. UTC | #6 
2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
>> 2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
>> >> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> >> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>> >> >>  static void
>> >> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> >> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> >> >>  {
>> >> >> +     if (i915.mmio_debug)
>> >> >> +             return;
>> >> >> +
>> >> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> >> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> >> >> +             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);
>> >> >>       }
>> >> >
>> >> > What was the point here? You still add an extra read to every register
>> >> > write
>> >>
>> >> Well, we previously had 2 extra reads instead of 1, so with this patch
>> >> we're in a better position :)
>> >>
>> >>
>> >> > and then repeat the request to enable mmio_debug ad infinitum.
>> >>
>> >> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
>> >> frequently enough to annoy the user.
>> >
>> > How do you think I came across this?
>> >
>> >> > And
>> >> > you still check for illegal writes in the irq handler.
>> >>
>> >> That just happens on HSW, not BDW+.
>> >
>> > Even on hsw, it should be killed. Checking inside the irq handler is
>> > just insane. Just move it to one of the periodic checks since we can't
>> > get any more information than an error occurred, and ask to be re-run
>> > with mmio_debug (and then shut up) - heck you could even automatically
>> > enable it for a one-shot operation.
>>
>> Yeah, looking at how the IRQ handler situation is _today_, I agree it
>> doesn't really make too much sense. I know it was different in the
>> past, so I wonder how we ended up reaching this point :)
>>
>> Anyway, if we just remove the call to intel_uncore_check_errors() that
>> happens on the irq handler, we'll still end up checking for errors as
>> soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
>> thing we could do would be to remove the check from the I915_WRITE
>> macros and put it on a real periodic check that could be executed at
>> other specific points of our code (less frequent than every
>> i915_write), or put it at its own workqueue that gets run every X
>> jiffies. Or perhaps change the implementation of
>> hsw_unclaimed_reg_detect() to not do anything when we're in an
>> interrupt/spinlock context. Which one do you think is better to do?
>
> From the irq handler, we can use just use raw mmio interfaces and skip
> all the debug and forcewake. I think the solution I prefer is to
> instrument modesetting (say check_state) and warn if an error had occurred
> outside of mmio_debug.

My only problem with checking at modesetting is that we often spend
hours and hours without doing a modeset, so it could lead to problems
not ever being detected. So maybe there's a better place, but if
that's what we want I won't block any patches.

>
>> Of course, we can also implement the one-shot thing on top of the
>> above, but it won't really help us reducing the amount of reads on the
>> "happy case" where we never got the error before.
>
> Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> the forcewake spinlock when we already hold it. So there won't be any
> such logic except when enabled by the user.

Should I expect a patch from you, or should I go and write the patch
based on what we already discussed?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Daniel Vetter Aug. 26, 2014, 1:34 p.m. UTC | #7 
On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> >> Of course, we can also implement the one-shot thing on top of the
> >> above, but it won't really help us reducing the amount of reads on the
> >> "happy case" where we never got the error before.
> >
> > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > the forcewake spinlock when we already hold it. So there won't be any
> > such logic except when enabled by the user.
> 
> Should I expect a patch from you, or should I go and write the patch
> based on what we already discussed?

Imo this is crazy - we have no control over what the compiler does and
when exactly it loads vtable entries, so patching them at runtime would be
an interesting excercise at best.

Adding a special version of I915_READ/WRITE for the irq hotpath makes
sense, but only if we can actually show a benefit in benchmarks.
-Daniel
Chris Wilson Aug. 26, 2014, 1:46 p.m. UTC | #8 
On Tue, Aug 26, 2014 at 03:34:07PM +0200, Daniel Vetter wrote:
> On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> > 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> > >> Of course, we can also implement the one-shot thing on top of the
> > >> above, but it won't really help us reducing the amount of reads on the
> > >> "happy case" where we never got the error before.
> > >
> > > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > > the forcewake spinlock when we already hold it. So there won't be any
> > > such logic except when enabled by the user.
> > 
> > Should I expect a patch from you, or should I go and write the patch
> > based on what we already discussed?
> 
> Imo this is crazy - we have no control over what the compiler does and
> when exactly it loads vtable entries, so patching them at runtime would be
> an interesting excercise at best.

Wtf?
-Chris
Daniel Vetter Aug. 26, 2014, 2:08 p.m. UTC | #9 
On Tue, Aug 26, 2014 at 02:46:25PM +0100, Chris Wilson wrote:
> On Tue, Aug 26, 2014 at 03:34:07PM +0200, Daniel Vetter wrote:
> > On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> > > 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> > > >> Of course, we can also implement the one-shot thing on top of the
> > > >> above, but it won't really help us reducing the amount of reads on the
> > > >> "happy case" where we never got the error before.
> > > >
> > > > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > > > the forcewake spinlock when we already hold it. So there won't be any
> > > > such logic except when enabled by the user.
> > > 
> > > Should I expect a patch from you, or should I go and write the patch
> > > based on what we already discussed?
> > 
> > Imo this is crazy - we have no control over what the compiler does and
> > when exactly it loads vtable entries, so patching them at runtime would be
> > an interesting excercise at best.
> 
> Wtf?

I've assumed you'd runtime patch the vtables or something like that. And I
didn't see any other less crazy way to ...
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 991b663..ca0a9dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2135,6 +2135,7 @@  struct i915_params {
 	bool disable_display;
 	bool disable_vtd_wa;
 	int use_mmio_flip;
+	bool 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 bbdee21..62ee830 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -49,6 +49,7 @@  struct i915_params i915 __read_mostly = {
 	.enable_cmd_parser = 1,
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
+	.mmio_debug = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -161,3 +162,8 @@  MODULE_PARM_DESC(enable_cmd_parser,
 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_PARM_DESC(mmio_debug,
+	"Enable the MMIO debug code (default: false). This may negatively "
+	"affect performance.");
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e0f0843..6fee122 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -514,20 +514,30 @@  ilk_dummy_write(struct drm_i915_private *dev_priv)
 }
 
 static void
-hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
+			bool before)
 {
+	const char *op = read ? "reading" : "writing to";
+	const char *when = before ? "before" : "after";
+
+	if (!i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n",
-			  reg);
+		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);
 	}
 }
 
 static void
-hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 {
+	if (i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unclaimed write to %x\n", reg);
+		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);
 	}
 }
@@ -564,6 +574,7 @@  gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_HEADER(x); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
 	if (dev_priv->uncore.forcewake_count == 0 && \
 	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
@@ -574,6 +585,7 @@  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	} else { \
 		val = __raw_i915_read##x(dev_priv, reg); \
 	} \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
 	REG_READ_FOOTER; \
 }
 
@@ -700,12 +712,13 @@  hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
-	hsw_unclaimed_reg_clear(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	hsw_unclaimed_reg_check(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+	hsw_unclaimed_reg_detect(dev_priv); \
 	REG_WRITE_FOOTER; \
 }