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

[Patrik Jakobsson]

On Mon, May 11, 2015 at 08:08:19PM +0200, Gabriel Laskar wrote:
> 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.

Unfortunately I cannot check for i915 in _decode_command_number since I don't
have the full tcb context (cannot figure out the path, etc.). That's probably
why I had to duplicate the decoding in the first place. The only other solution
I see is to detect i915 early and store it globally somewhere but that is rather
nasty. Not sure how to do this in a better way. What am I missing?

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

Yes, it's prefered to use the headers from libdrm. It's actually not a bad thing
that Arch Linux enforces this. I'll just have to add libdrm as a dependency for
drm ioctl decoding.

> 
> -- 
> Gabriel Laskar