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

[Dmitry V. L. <ld...@al...>]

On Tue, May 12, 2015 at 07:37:59PM +0200, Gabriel Laskar wrote:
> On Tue, 12 May 2015 14:35:28 +0200, Patrik Jakobsson wrote:
> > On Mon, May 11, 2015 at 08:08:19PM +0200, Gabriel Laskar wrote:
> > > On Mon, 11 May 2015 15:54:24 +0200, Patrik Jakobsson 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 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?
> 
> Oh yes, I see. We can add tcb context to ioctl_decode_command_number(),
> it should not be a problem. We didn't have the need for it for the
> moment, that's all.
> 
> Dmitry? What do you think of this?

I had no chance to have a look at the code, but anyway, passing
tcb context down to ioctl_decode_command_number() shouldn't be a problem
at all.


-- 
ldv