Re: [PATCH 18/49] drm/i915: Reduce frequency of unspecific HSW reg debugging

[Paulo Zanoni <przanoni@gmail.com>]

2015-03-27 13:12 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Mar 27, 2015 at 12:34:05PM -0300, Paulo Zanoni wrote:
>> 2015-03-27 8:01 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > Delay the expensive read on the FPGA_DBG register from once per mmio to
>> > once per forcewake section when we are doing the general wellbeing
>> > check rather than the targetted error detection. This almost reduces
>> > the overhead of the debug facility (for example when submitting execlists)
>> > to zero whilst keeping the debug checks around.
>> >
>> > v2: Enable one-shot mmio debugging from the interrupt check as well as a
>> >     safeguard to catch invalid display writes from outside the powerwell.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_uncore.c | 56 ++++++++++++++++++++-----------------
>> >  1 file changed, 30 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> > index ab5cc94588e1..0e32bbbcada8 100644
>> > --- a/drivers/gpu/drm/i915/intel_uncore.c
>> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> > @@ -149,6 +149,30 @@ fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
>> >  }
>> >
>> >  static void
>> > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> > +{
>> > +       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_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--;
>> > +       }
>> > +}
>> > +
>> > +static void
>> > +fw_domains_put_debug(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
>> > +{
>> > +       hsw_unclaimed_reg_detect(dev_priv);
>> > +       fw_domains_put(dev_priv, fw_domains);
>> > +}
>>
>> This means we won't check during the forcewake puts that are on the
>> register read/write macros. Is this intentional?
>
> Not really. But the check still catches any mistakes there even though
> we know they are by safe.
>
>> I tried checking the
>> FW code calls, and it seems to me that we're not really going to run
>> hsw_unclaimed_reg_detect very frequently anymore. I wonder if there's
>> the risk of staying a long time without running it. But maybe I'm just
>> wrong.
>
> It gets run after every set of register writes (where set is defined as
> activity on a single cpu within 1ms). It gets run before the powerwell
> is disabled. Look at the profiles, you will see that hsw detect is still
> called quite frequently. And by virtue it does not need to be run very
> often to catch issues anyway.
>
>> > +       if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
>> > +           dev_priv->uncore.funcs.force_wake_put == fw_domains_put)
>>
>> My fear here is that simple changes to the FW code by a future
>> programmer could unintentionally kill the unclaimed register detection
>> feature, and we probably wouldn't notice for a looong time. Why not
>> just omit this fw_domains_put check, since it is true for all
>> platforms where HAS_FPG_DBG_UNCLAIMED is also true? The side effect of
>> calling fw_domains_put() when we shouldn't is probably more noticeable
>> than having unclaimed register detection gone.
>
> Pardon?

My suggestion was to find a way to transform this "if" statement above
somehow into a check just for "if (has_fpga_dbg_unclaimed())", without
relying on whatever is assigned to funcs.force_wake_put, due to the
fear that a refactoring could accidentally kill the unclaimed register
detection. But it was just a suggestion.

>
>> > +               dev_priv->uncore.funcs.force_wake_put = fw_domains_put_debug;
>> > +
>> >         /* All future platforms are expected to require complex power gating */
>> >         WARN_ON(dev_priv->uncore.fw_domains == 0);
>> >  }
>> > @@ -1411,11 +1420,6 @@ int intel_gpu_reset(struct drm_device *dev)
>> >
>> >  void intel_uncore_check_errors(struct drm_device *dev)
>> >  {
>> > -       struct drm_i915_private *dev_priv = dev->dev_private;
>> > -
>> > -       if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
>> > -           (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
>> > -               DRM_ERROR("Unclaimed register before interrupt\n");
>> > -               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> > -       }
>> > +       if (HAS_FPGA_DBG_UNCLAIMED(dev))
>> > +               hsw_unclaimed_reg_detect(to_i915(dev));
>>
>> This means we won't check for unclaimed registers during interrupts if
>> i915.mmio_debug is being used, which is probably not what we want.
>
> It's exactly what you want. It still does the debug check if you have
> mmio_debug unset. If you have mmio_debug set, it means you are debugging
> i915 register mmio, since that is all we can reliably debug.
>
>> One of the things that worries me a little is that now we'll be
>> running a mostly-display-related feature only when certain pieces of
>> non-display-related code run. Instead of doing the checks at the
>> forcewake puts, maybe we could tie the frequency of the checks to
>> something in the display code, or just do the check every X seconds. I
>> don't really know what would be ideal here, I'm just throwing the
>> ideas. I'm also not blocking this patch, just pointing things that
>> could maybe be improved.
>
> Sure, all you would need to do is add the check to every rpm_put() if you
> feel paranoid (it will be run before the powerwell is dropped by
> design).
>
>> Since it's impacting performance, perhaps we could even completely
>> kill unclaimed register detection from the normal use case, hiding it
>> behind i915.mmio_debug=1 (and maybe a kconfig option)? We would then
>> instruct QA and developers to always have the option enabled. Just
>> like QA needs to have lockdep enabled, we could ask it to have
>> mmio_debug enabled all the time too.
>
> Whilst I like the idea, having debug code running in the wild (at a
> frequency high enough to catch bugs, but low enough not to be noticed)
> is invaluable.

Some other ideas that could be worth discussing:

- Adding a check for the range of registers that are covered by
FPGA_DBG. I imagine most/all of your performance sensitive registers
are outside it, so maybe this change would be enough to fix the issues
you're seeing, potentially replacing this patch.

- It seems that most of the times we call I915_WRITE/READ, the
register argument is seen by the compiler as a constant (I checked
this with __builtin_constant_p()). Maybe we could try to exploit this
builtin to make a special-case that allows the compiler to optimize
away all our "if" statements we have on the register writing macros.
Of course, since not all our reg writes are constant, we'd still need
the older/slower version.

- Didn't we ever discuss replacing I915_WRITE with more specialized
macros that would be used just on specific register ranges? On gen8 I
can see, for example: a range requiring forcewake, a range requiring
fpga_dbg and a range requiring nothing. On gen9 I see even more. Would
the little performance gain justify the change?

It seems you're doing some optimizations, so maybe one of the ideas
could be interesting...

Thanks,
Paulo

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx