[Intel-gfx] [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

2014-08-26 9:42 GMT-03:00 Chris Wilson <chris at 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 at 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



-- 
Paulo Zanoni