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@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