[Intel-gfx] [RFC 0/2] strace/drm: Add i915 ioctls to strace
patrik.jakobsson at linux.intel.com
Tue May 12 12:35:28 UTC 2015
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 the headers for
> drm. For others, I can't say.
> : 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
More information about the Strace-devel