[Intel-gfx] [PATCH v9 01/11] drm/i915: Add i915 perf infrastructure

[Daniel Vetter]

On Tue, Nov 22, 2016 at 02:29:18PM +0100, Daniel Vetter wrote:
> On Wed, Nov 09, 2016 at 08:00:06PM +0000, Matthew Auld wrote:
> > On 7 November 2016 at 19:49, Robert Bragg <robert at sixbynine.org> wrote:
> > > Adds base i915 perf infrastructure for Gen performance metrics.
> > >
> > > This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64
> > > properties to configure a stream of metrics and returns a new fd usable
> > > with standard VFS system calls including read() to read typed and sized
> > > records; ioctl() to enable or disable capture and poll() to wait for
> > > data.
> > >
> > > A stream is opened something like:
> > >
> > >   uint64_t properties[] = {
> > >       /* Single context sampling */
> > >       DRM_I915_PERF_PROP_CTX_HANDLE,        ctx_handle,
> > >
> > >       /* Include OA reports in samples */
> > >       DRM_I915_PERF_PROP_SAMPLE_OA,         true,
> > >
> > >       /* OA unit configuration */
> > >       DRM_I915_PERF_PROP_OA_METRICS_SET,    metrics_set_id,
> > >       DRM_I915_PERF_PROP_OA_FORMAT,         report_format,
> > >       DRM_I915_PERF_PROP_OA_EXPONENT,       period_exponent,
> > >    };
> > >    struct drm_i915_perf_open_param parm = {
> > >       .flags = I915_PERF_FLAG_FD_CLOEXEC |
> > >                I915_PERF_FLAG_FD_NONBLOCK |
> > >                I915_PERF_FLAG_DISABLED,
> > >       .properties_ptr = (uint64_t)properties,
> > >       .num_properties = sizeof(properties) / 16,
> > >    };
> > >    int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, &param);
> > >
> > > Records read all start with a common { type, size } header with
> > > DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records
> > > contain an extensible number of fields and it's the
> > > DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening that
> > > determine what's included in every sample.
> > >
> > > No specific streams are supported yet so any attempt to open a stream
> > > will return an error.
> > >
> > > v2:
> > >     use i915_gem_context_get() - Chris Wilson
> > > v3:
> > >     update read() interface to avoid passing state struct - Chris Wilson
> > >     fix some rebase fallout, with i915-perf init/deinit
> > > v4:
> > >     s/DRM_IORW/DRM_IOW/ - Emil Velikov
> > >
> > > Signed-off-by: Robert Bragg <robert at sixbynine.org>
> > > Reviewed-by: Matthew Auld <matthew.auld at intel.com>
> > > Reviewed-by: Sourab Gupta <sourab.gupta at intel.com>
> > Minor nit, there are a fair few DRM_ERROR's missing a new line.
> 
> Also, DRM_ERROR for userspace-triggerable failures is no good. igt
> testcase are supposed to exercise all the invalid stuff, and would then
> fail if you spam dmesg. Why was this not caught?
> 
> Fixup patch totally fine, but if this wasn't caught due to missing igt
> that needs to be fixed, too.

Another nitpick for the future: Enabling new features first and then
fixing up the fallout is the wrong way round, if someone bisects over this
range mesa might blow up in really bad ways.

Oh well, this has been out there for way too long, so meh.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch