[Intel-gfx] [RFC 0/2] strace/drm: Add i915 ioctls to strace

[Gabriel Laskar]

On Mon, 11 May 2015 15:54:24 +0200
Patrik Jakobsson <patrik.jakobsson at linux.intel.com> wrote:

> On Mon, May 11, 2015 at 12:50:36PM +0200, Gabriel Laskar wrote:
> > On Wed,  6 May 2015 16:48:01 +0200
> > Patrik Jakobsson <patrik.jakobsson at linux.intel.com> wrote:
> > 
> > > This patch set aims to make strace more useful when tracing i915 ioctls.
> > > The ioctl type is first checked for being drm and then the driver
> > > backing the opened device is identified by looking at sysfs. Other
> > > drivers than i915 can easily be added.
> > > 
> > > Only a subset of the i915 ioctls are included. I will extend this patch
> > > set if the approach looks ok. The generic drm ioctls are also missing.
> > > 
> > > Give it a spin with:
> > >         strace -e trace=ioctl -p `pidof X`
> > > 
> > > Patrik Jakobsson (2):
> > >   strace/drm: Print extended info for drm and i915 ioctls
> > >   strace/drm: Print args for most common i915 ioctls
> > > 
> > >  Makefile.am                |   2 +
> > >  defs.h                     |   2 +
> > >  drm.c                      | 104 +++++++++++++++++
> > >  drm_i915.c                 | 278 +++++++++++++++++++++++++++++++++++++++++++++
> > >  ioctl.c                    |   5 +
> > >  xlat/drm_i915_getparams.in |  28 +++++
> > >  xlat/drm_i915_ioctls.in    |  51 +++++++++
> > >  xlat/drm_i915_setparams.in |   4 +
> > >  8 files changed, 474 insertions(+)
> > >  create mode 100644 drm.c
> > >  create mode 100644 drm_i915.c
> > >  create mode 100644 xlat/drm_i915_getparams.in
> > >  create mode 100644 xlat/drm_i915_ioctls.in
> > >  create mode 100644 xlat/drm_i915_setparams.in
> > > 
> > 
> > This is a great start! We need this kind of decoding. Do you plan to
> > add also the generic drm ioctl decoding?
> 
> Thanks for the review. Yes, my plan is to add generic drm ioctls as well.
> 
> > 
> > Some issues though:
> > 
> > * The way you avoid the ioctl request decoding is quite hard to follow,
> >   but it seems that you don't have much of a choice, except that in
> >   drm_ioctl(), the code from SYS_FUNC(ioctl) is duplicated. It seems it
> >   needs some work here, to allow a simpler code path. Maybe this would
> >   be clearer if the decoding/drm_get_driver_name, etc… was in
> >   ioctl_decode_command_number(). Also, with the actual code, if you are
> >   on i915 with an invalid ioctl number, it will be printed as
> >   "I915_IOCTL_???" and not "_IOC(...)" (see below for an example.) This
> >   will also add an inconsistent result depending whether /sys is
> >   mounted or not.
> 
> Yes, moving it to ioctl_decode_command_number() makes sense. I'll do that.
> And I'll make the output consistent and skip the I915_IOCTL_???. It comes with
> the drawback of possibly duplicated entries when doing the lookup even though we
> know we're talking to i915, but it is still nicer than _???.

If you call all the request decoding code from
ioctl_decode_command_number() you will still be able to determine if
you are on i915, and write the correct request, but the fallback code
will be no longer duplicated. You will have to call xlookup() instead of
printxval(), in order to be able to know when the decoding fail though.

> > * This does not compile on my system (archlinux), because drm.h lives
> >   in libdrm/drm.h and not in drm/drm.h, I know it is an rfc, but this
> >   needs to use pkg-config in order to know where libdrm headers are.
> 
> In theory libdrm is not needed since the headers are available in uapi, so I
> tried to avoid adding an additional dependency. But since distros do not
> guarantee the existance of <drm/drm.h> it might be better to use libdrm. I'll
> look into that.

It seems that on archlinux, they remove intentionally[1] the headers for
drm. For others, I can't say.

[1]: https://projects.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/linux-api-headers#n40

-- 
Gabriel Laskar