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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
