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

[Chris Wilson]

On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> 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?

>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

-- 
Chris Wilson, Intel Open Source Technology Centre