From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Bragg Subject: Re: [Intel-gfx] [PATCH v9 01/11] drm/i915: Add i915 perf infrastructure Date: Tue, 22 Nov 2016 14:02:38 +0000 Message-ID: References: <20161107194957.3385-1-robert@sixbynine.org> <20161107194957.3385-2-robert@sixbynine.org> <20161122132918.artmjbysbrl66qw2@phenom.ffwll.local> <20161122133117.aujnfrcedgnxvdgd@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1206987042==" Return-path: In-Reply-To: <20161122133117.aujnfrcedgnxvdgd@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Intel Graphics Development , Matthew Auld , ML dri-devel , Sourab Gupta , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org --===============1206987042== Content-Type: multipart/alternative; boundary=001a113f6cccdab2170541e43a7e --001a113f6cccdab2170541e43a7e Content-Type: text/plain; charset=UTF-8 On Tue, Nov 22, 2016 at 1:31 PM, Daniel Vetter 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 > 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 > > > > Reviewed-by: Matthew Auld > > > > Reviewed-by: Sourab Gupta > > > 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 > --001a113f6cccdab2170541e43a7e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Tue, Nov 22, 2016 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch><= /span> 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 metri= cs.
> > >
> > > This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an arr= ay of uint64
> > > properties to configure a stream of metrics and returns a ne= w fd usable
> > > with standard VFS system calls including read() to read type= d and sized
> > > records; ioctl() to enable or disable capture and poll() to = wait for
> > > data.
> > >
> > > A stream is opened something like:
> > >
> > >=C2=A0 =C2=A0uint64_t properties[] =3D {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Single context sampling */
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_I915_PERF_PROP_CTX_HANDLE,=C2= =A0 =C2=A0 =C2=A0 =C2=A0 ctx_handle,
> > >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Include OA reports in samples *= /
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_I915_PERF_PROP_SAMPLE_OA,=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0true,
> > >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0/* OA unit configuration */
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_I915_PERF_PROP_OA_METRICS_SET,=C2=A0 =C2=A0 metrics_set_id,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_I915_PERF_PROP_OA_FORMAT,=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0report_format,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_I915_PERF_PROP_OA_EXPONEN= T,=C2=A0 =C2=A0 =C2=A0 =C2=A0period_exponent,
> > >=C2=A0 =C2=A0 };
> > >=C2=A0 =C2=A0 struct drm_i915_perf_open_param parm =3D {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0.flags =3D I915_PERF_FLAG_FD_CLOEX= EC |
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I915_= PERF_FLAG_FD_NONBLOCK |
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I915_= PERF_FLAG_DISABLED,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0.properties_ptr =3D (uint64_t)prop= erties,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0.num_properties =3D sizeof(propert= ies) / 16,
> > >=C2=A0 =C2=A0 };
> > >=C2=A0 =C2=A0 int fd =3D drmIoctl(drm_fd, DRM_IOCTL_I915_PERF= _OPEN, &param);
> > >
> > > Records read all start with a common { type, size } header w= ith
> > > DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample r= ecords
> > > 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:
> > >=C2=A0 =C2=A0 =C2=A0use i915_gem_context_get() - Chris Wilson=
> > > v3:
> > >=C2=A0 =C2=A0 =C2=A0update read() interface to avoid passing = state struct - Chris Wilson
> > >=C2=A0 =C2=A0 =C2=A0fix some rebase fallout, with i915-perf i= nit/deinit
> > > v4:
> > >=C2=A0 =C2=A0 =C2=A0s/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 lin= e.
>
> Also, DRM_ERROR for userspace-triggerable failures is no good. igt
> testcase are supposed to exercise all the invalid stuff, and would the= n
> 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<= br> 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 in= frastructure patch should have no fallout to fix for userspace. The command= parser changes that affect userspace were done before adding oacontrol usa= ge to i915-perf and the cmd parser's EINVAL reporting for access failur= es was changed *before* removing oacontrol from the whitelist.

Did I overlook something in particular?

- Robert

=C2=A0
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http:= //blog.ffwll.ch

--001a113f6cccdab2170541e43a7e-- --===============1206987042== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1206987042==--