On Tue, Nov 22, 2016 at 1:31 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
Another nitpick for the future: Enabling new features first and thenOn 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@xxxxxxxxxxxxx> 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, ¶m);
> > >
> > > 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@xxxxxxxxxxxxx>
> > > Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > > Reviewed-by: Sourab Gupta <sourab.gupta@xxxxxxxxx>
> > 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.
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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
