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

[Patrik Jakobsson]

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 _???.

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

Thanks
Patrik

> 
> 
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <unistd.h>
> 
> #include <drm.h>
> #include <i915_drm.h>
> 
> int main()
> {
> 	int fd = open("/dev/dri/card0", O_RDWR);
> 	ioctl(fd, DRM_IOW(DRM_COMMAND_BASE + DRM_I915_FLUSH, int), 0);
> 	return 0;
> }
> 
> $ CFLAGS=-I/usr/include/libdrm make i915-ioctls
> cc -I/usr/include/libdrm    i915-ioctls.c   -o i915-ioctls
> $ sudo strace -e ioctl ./i915-ioctls 
> ioctl(3, _IOC(_IOC_WRITE, 0x64, 0x41, 0x04), 0) = 0
> +++ exited with 0 +++
> $ sudo ~/source/strace/strace -e ioctl ./i915-ioctls 
> ioctl(3, 0x40046441 /* I915_IOCTL_??? */, 0) = 0
> +++ exited with 0 +++
> 
> 
> -- 
> Gabriel Laskar