Re: [PATCH v9 01/11] drm/i915: Add i915 perf infrastructure

From: Daniel Vetter <daniel@ffwll.ch>
To: Robert Bragg <robert@sixbynine.org>
Cc: David Airlie <airlied@linux.ie>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	Sourab Gupta <sourab.gupta@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v9 01/11] drm/i915: Add i915 perf infrastructure
Date: Tue, 22 Nov 2016 16:35:37 +0100	[thread overview]
Message-ID: <20161122153537.64s526oq7a2eph2w@phenom.ffwll.local> (raw)
In-Reply-To: <CAMou1-3OdZ0WVthohoEYPYxXSETARQj+2NJRtSRbySYiU5Qptw@mail.gmail.com>

On Tue, Nov 22, 2016 at 02:02:38PM +0000, Robert Bragg wrote:
> On Tue, Nov 22, 2016 at 1:31 PM, Daniel Vetter <daniel@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@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@sixbynine.org>
> > > > > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> > > > > Reviewed-by: Sourab Gupta <sourab.gupta@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?

Ah, if you key the userspace side off the cmd parser change then I think
it should be all fine. I only looked at this ioctl here, and that alone
looked like it was unsafe. Ordering of the patches later on seemed correct
indeed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx