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

[Robert Bragg]

On Tue, Nov 22, 2016 at 1:31 PM, Daniel Vetter <daniel at ffwll.ch> wrote:

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

Fwiw I'm aware of this, and think I've ordered the patches correctly to
avoid bisect problems in Mesa / userspace. This infrastructure patch should
have no fallout to fix for userspace. The command parser changes that
affect userspace were done before adding oacontrol usage to i915-perf and
the cmd parser's EINVAL reporting for access failures was changed *before*
removing oacontrol from the whitelist.

Did I overlook something in particular?

- Robert



> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161122/a04d5653/attachment.html>