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

[Chris Wilson]

On Fri, Apr 22, 2016 at 12:04:26PM +0100, Robert Bragg wrote:
>    On Wed, Apr 20, 2016 at 11:46 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_init_oa_buffer(struct drm_i915_private *dev_priv)
>      > +{
>      > +     /* Pre-DevBDW: OABUFFER must be set with counters off,
>      > +      * before OASTATUS1, but after OASTATUS2
>      > +      */
>      > +     I915_WRITE(GEN7_OASTATUS2,
>      dev_priv->perf.oa.oa_buffer.gtt_offset |
>      > +                OA_MEM_SELECT_GGTT); /* head */
>      > +     I915_WRITE(GEN7_OABUFFER,
>      dev_priv->perf.oa.oa_buffer.gtt_offset);
>      > +     I915_WRITE(GEN7_OASTATUS1,
>      dev_priv->perf.oa.oa_buffer.gtt_offset |
>      > +                OABUFFER_SIZE_16M); /* tail */
>      > +
>      > +     /* On Haswell we have to track which OASTATUS1 flags we've
>      > +      * already seen since they can't be cleared while periodic
>      > +      * sampling is enabled.
>      > +      */
>      > +     dev_priv->perf.oa.gen7_latched_oastatus1 = 0;
>      > +
>      > +     /* We have a sanity check in gen7_append_oa_reports() that
>      > +      * looks at the report-id field to make sure it's non-zero
>      > +      * which relies on the assumption that new reports are
>      > +      * being written to zeroed memory...
>      > +      */
>      > +     memset(dev_priv->perf.oa.oa_buffer.addr, 0, SZ_16M);
> 
>      You allocated zeroed memory.
> 
>    yup. currently I have this memset here because we may re-init the buffer
>    if the stream is disabled then re-enabled (via I915_PERF_IOCTL_ENABLE) or
>    if we have to reset the unit on error. In these cases there may be some
>    number of reports in the buffer with non-zero report-id fields while we
>    still want to be sure new reports are being written to zereod memory so
>    that the sanity check that report-id != 0 will continue to be valid.
> 
>    I've had it in mind to consider optimizing this at some point to minimize
>    how much of the buffer is cleared, maybe just for the _DISABLE/_ENABLE
>    case where I'd expect the buffer will mostly be empty before disabling the
>    stream.

Or just make it clear that you are considering buffer reuse. Having the
memset here allows us to use non-shmemfs allocation, it wasn't that I
objected I just didn't understand the comment in the context of
allocation path.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre