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
