[Intel-gfx] [PATCH 5/9] drm/i915: Enable i915 perf stream for Haswell OA unit

[Chris Wilson]

On Thu, Apr 21, 2016 at 05:15:10PM +0100, Robert Bragg wrote:
>    On Wed, Apr 20, 2016 at 10:11 PM, Chris Wilson
>    <[1]chris at chris-wilson.co.uk> wrote:
> 
>      On Wed, Apr 20, 2016 at 03:23:10PM +0100, Robert Bragg wrote:
>      > +static void gen7_update_oacontrol_locked(struct drm_i915_private
>      *dev_priv)
>      > +{
>      > +     assert_spin_locked(&dev_priv->perf.hook_lock);
>      > +
>      > +     if (dev_priv->perf.oa.exclusive_stream->enabled) {
>      > +             unsigned long ctx_id = 0;
>      > +
>      > +             if (dev_priv->perf.oa.exclusive_stream->ctx)
>      > +                     ctx_id = dev_priv->perf.oa.specific_ctx_id;
>      > +
>      > +             if (dev_priv->perf.oa.exclusive_stream->ctx == NULL ||
>      ctx_id) {
>      > +                     bool periodic = dev_priv->perf.oa.periodic;
>      > +                     u32 period_exponent =
>      dev_priv->perf.oa.period_exponent;
>      > +                     u32 report_format =
>      dev_priv->perf.oa.oa_buffer.format;
>      > +
>      > +                     I915_WRITE(GEN7_OACONTROL,
>      > +                                (ctx_id & GEN7_OACONTROL_CTX_MASK) |
>      > +                                (period_exponent <<
>      > +                                 GEN7_OACONTROL_TIMER_PERIOD_SHIFT) |
>      > +                                (periodic ?
>      > +                                 GEN7_OACONTROL_TIMER_ENABLE : 0) |
>      > +                                (report_format <<
>      > +                                 GEN7_OACONTROL_FORMAT_SHIFT) |
>      > +                                (ctx_id ?
>      > +                                 GEN7_OACONTROL_PER_CTX_ENABLE : 0) |
>      > +                                GEN7_OACONTROL_ENABLE);
> 
>      So this works by only recording when the OACONTROL context address
>      matches the CCID.
> 
>      Rather than hooking into switch context and checking every batch whether
>      you have the exclusive context in case it changed address, you could
>      just pin the exclusive context when told by the user to bind perf to
>      that context. Then it will also have the same address until oa is
>      finished (and releases it vma pin).
> 
>    Yeah, this was the approach I first went with when the driver was perf
>    based, though we ended up deciding to got with hooking into pinning and
>    updating the OA state in the end.
> 
>    E.g. for reference:
>    [2]https://lists.freedesktop.org/archives/intel-gfx/2014-November/055385.html
>    (wow, sad face after seeing how long I've been kicking this stuff)
> 
>    I'd prefer to stick with this approach now, unless you see a big problem
>    with it.

Given no reason to have the hook, I don't see why we should. Pinning the
context in the GGTT and causing that bit of extra fragmenetation isn't
the worst evil here. and is better practice overall to treat the OA
register as holding the pin on the object is it referencing, along with
lifetime tracking of that register (i.e. unpinning only when we know it
has completed its writes). Given that, the pin_notify is inadequate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre