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

[Paulo Zanoni]

2014-08-26 10:18 GMT-03:00 Chris Wilson <chris at 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 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.

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



-- 
Paulo Zanoni