[PATCH v4 0/5] drm: Add decoding for DRM/KMS and i915 ioctls

[PATCH v4 0/5] drm: Add decoding for DRM/KMS and i915 ioctls

[Patrik Jakobsson]

This set of patches adds a dispatcher for handling DRM ioctls. The
kernel headers for DRM might not be available on all distributions
so we depend on libdrm for those. If libdrm is not available we fall
back on the kernel headers. Since DRM drivers share the same range of
private ioctl numbers I've added a function for detecting the driver
based on it's name.

Changes in v2:
* Rebased to master
* Added Changelog to commits
* Keep strace_SOURCES list sorted
* Removed unneeded includes
* Reduced number of driver name checks by adding tcb private data
* Use tprints() for regular strings
* Reworked entering() / exiting() handling for all ioctls
* Use printstr() to print strings in properly quoted form

Changes in v3:
* Moved all umove() into state checks for single state ioctls
* Removed extra curly bracket
* Moved param argument into entering() state in i915_setparam()
* Don't return before private data is freed in drm_ioctl()

Changes in v4:
* Rebased to master
* Rewrote commit messages to GNU changelog standard
* Added private data support to struct tcb
* Reworked drm driver identification
* Reworked drm header detection
* Use recently added return types for decode functions
* Various small fixes

Patrik Jakobsson (5):
  drm: Add config for detecting libdrm
  drm: Add private data field to trace control block
  drm: Add dispatcher and driver identification for DRM
  drm: Add decoding of i915 ioctls
  drm: Add decoding of DRM and KMS ioctls

 Makefile.am                |   2 +
 configure.ac               |   5 +
 defs.h                     |  11 +
 drm.c                      | 663 +++++++++++++++++++++++++++++++++++++++++++++
 drm_i915.c                 | 342 +++++++++++++++++++++++
 ioctl.c                    |   4 +
 strace.c                   |  14 +
 syscall.c                  |   1 +
 xlat/drm_i915_getparams.in |  28 ++
 xlat/drm_i915_ioctls.in    |  51 ++++
 xlat/drm_i915_setparams.in |   4 +
 11 files changed, 1125 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

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 1/5] drm: Add config for detecting libdrm

[Patrik Jakobsson]

Use pkg-config to try to find libdrm headers. If that fails look for
the kernel headers. If no headers are found, drm support will not be
compiled.

* configure.ac: Use pkg-config to find libdrm

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure.ac b/configure.ac
index 0819f83..ef43278 100644
--- a/configure.ac
+++ b/configure.ac
@@ -870,6 +870,11 @@ fi
 AM_CONDITIONAL([USE_LIBUNWIND], [test "x$use_libunwind" = xyes])
 AC_MSG_RESULT([$use_libunwind])
 
+PKG_CHECK_MODULES([LIBDRM], [libdrm],
+		  [CPPFLAGS="$CPPFLAGS $LIBDRM_CFLAGS"
+		   AC_CHECK_HEADERS([drm.h i915_drm.h])],
+		  [AC_CHECK_HEADERS([drm/drm.h drm/i915_drm.h])])
+
 if test "$arch" = mips && test "$no_create" != yes; then
 	mkdir -p linux/mips
 	if $srcdir/linux/mips/genstub.sh linux/mips; then
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/5] drm: Add config for detecting libdrm

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
> +PKG_CHECK_MODULES([LIBDRM], [libdrm],
> +		  [CPPFLAGS="$CPPFLAGS $LIBDRM_CFLAGS"
> +		   AC_CHECK_HEADERS([drm.h i915_drm.h])],
> +		  [AC_CHECK_HEADERS([drm/drm.h drm/i915_drm.h])])

i would make the drm/xxx headers unconditional -- add them to the big
block that does:
AC_CHECK_HEADERS(m4_normalize([
	...
]))

otherwise seems fine
-mike

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

We need to be able to store private data in the tcb across it's
lifetime. To ensure proper destruction of the data a free_priv_data
callback must be provided if an allocation is stored in priv_data. The
callback is executed automatically when the life of the tcb ends.

* defs.h: Add extern declaration of free_tcb_priv_data.
 (struct tcb): Add priv_data and free_priv_data.
* strace.c (free_tcb_priv_data): New function
(drop_tcb): Execute free_tcb_priv_data callback
* syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 defs.h    |  6 ++++++
 strace.c  | 14 ++++++++++++++
 syscall.c |  1 +
 3 files changed, 21 insertions(+)

diff --git a/defs.h b/defs.h
index 9059026..bc3bd83 100644
--- a/defs.h
+++ b/defs.h
@@ -266,6 +266,10 @@ struct tcb {
 	int u_error;		/* Error code */
 	long scno;		/* System call number */
 	long u_arg[MAX_ARGS];	/* System call arguments */
+
+	void *priv_data;	/* Private data for syscall decoding functions */
+	void (*free_priv_data)(void *); /* Callback for freeing priv_data */
+
 #if defined(LINUX_MIPSN32) || defined(X32)
 	long long ext_arg[MAX_ARGS];
 	long long u_lrval;	/* long long return value */
@@ -470,6 +474,8 @@ extern void get_regs(pid_t pid);
 extern int get_scno(struct tcb *tcp);
 extern const char *syscall_name(long scno);
 
+extern void free_tcb_priv_data(struct tcb *tcp);
+
 extern int umoven(struct tcb *, long, unsigned int, void *);
 #define umove(pid, addr, objp)	\
 	umoven((pid), (addr), sizeof(*(objp)), (void *) (objp))
diff --git a/strace.c b/strace.c
index 9b93e79..7a71152 100644
--- a/strace.c
+++ b/strace.c
@@ -711,12 +711,26 @@ alloctcb(int pid)
 	error_msg_and_die("bug in alloctcb");
 }
 
+void
+free_tcb_priv_data(struct tcb *tcp)
+{
+	if (tcp->priv_data) {
+		if (tcp->free_priv_data) {
+			tcp->free_priv_data(tcp->priv_data);
+			tcp->free_priv_data = NULL;
+		}
+		tcp->priv_data = NULL;
+	}
+}
+
 static void
 droptcb(struct tcb *tcp)
 {
 	if (tcp->pid == 0)
 		return;
 
+	free_tcb_priv_data(tcp);
+
 #ifdef USE_LIBUNWIND
 	if (stack_trace_enabled) {
 		unwind_tcb_fin(tcp);
diff --git a/syscall.c b/syscall.c
index 396a7dd..39b973b 100644
--- a/syscall.c
+++ b/syscall.c
@@ -1099,6 +1099,7 @@ trace_syscall_exiting(struct tcb *tcp)
 #endif
 
  ret:
+	free_tcb_priv_data(tcp);
 	tcp->flags &= ~TCB_INSYSCALL;
 	tcp->sys_func_rval = 0;
 	return 0;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 1624 bytes --]

On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
> We need to be able to store private data in the tcb across it's
> lifetime. To ensure proper destruction of the data a free_priv_data
> callback must be provided if an allocation is stored in priv_data. The
> callback is executed automatically when the life of the tcb ends.
> 
> * defs.h: Add extern declaration of free_tcb_priv_data.
>  (struct tcb): Add priv_data and free_priv_data.
> * strace.c (free_tcb_priv_data): New function
> (drop_tcb): Execute free_tcb_priv_data callback
> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
> 
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  defs.h    |  6 ++++++
>  strace.c  | 14 ++++++++++++++
>  syscall.c |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/defs.h b/defs.h
> index 9059026..bc3bd83 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -266,6 +266,10 @@ struct tcb {
>  	int u_error;		/* Error code */
>  	long scno;		/* System call number */
>  	long u_arg[MAX_ARGS];	/* System call arguments */
> +
> +	void *priv_data;	/* Private data for syscall decoding functions */
> +	void (*free_priv_data)(void *); /* Callback for freeing priv_data */

should we name these _priv_data and _free_priv_data and provides accessor
functions ?  i worry that code paths might stomp on each other by accident
and we don't end up noticing.

static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
{
	assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
	...
}
-mike

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]

_______________________________________________
Strace-devel mailing list
Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/strace-devel

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

On Tue, Aug 25, 2015 at 11:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
>> We need to be able to store private data in the tcb across it's
>> lifetime. To ensure proper destruction of the data a free_priv_data
>> callback must be provided if an allocation is stored in priv_data. The
>> callback is executed automatically when the life of the tcb ends.
>>
>> * defs.h: Add extern declaration of free_tcb_priv_data.
>>  (struct tcb): Add priv_data and free_priv_data.
>> * strace.c (free_tcb_priv_data): New function
>> (drop_tcb): Execute free_tcb_priv_data callback
>> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
>> ---
>>  defs.h    |  6 ++++++
>>  strace.c  | 14 ++++++++++++++
>>  syscall.c |  1 +
>>  3 files changed, 21 insertions(+)
>>
>> diff --git a/defs.h b/defs.h
>> index 9059026..bc3bd83 100644
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -266,6 +266,10 @@ struct tcb {
>>       int u_error;            /* Error code */
>>       long scno;              /* System call number */
>>       long u_arg[MAX_ARGS];   /* System call arguments */
>> +
>> +     void *priv_data;        /* Private data for syscall decoding functions */
>> +     void (*free_priv_data)(void *); /* Callback for freeing priv_data */
>
> should we name these _priv_data and _free_priv_data and provides accessor
> functions ?  i worry that code paths might stomp on each other by accident
> and we don't end up noticing.
>
> static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
> {
>         assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
>         ...
> }
> -mike

Yes, that's a good idea. My use case is pretty simple but usage can easliy grow.

I'll resend this patch and take it out of the drm/i915 series.

-Patrik
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

On Wed, Aug 26, 2015 at 03:26:08PM +0200, Patrik Jakobsson wrote:
> On Tue, Aug 25, 2015 at 11:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
> >> We need to be able to store private data in the tcb across it's
> >> lifetime. To ensure proper destruction of the data a free_priv_data
> >> callback must be provided if an allocation is stored in priv_data. The
> >> callback is executed automatically when the life of the tcb ends.
> >>
> >> * defs.h: Add extern declaration of free_tcb_priv_data.
> >>  (struct tcb): Add priv_data and free_priv_data.
> >> * strace.c (free_tcb_priv_data): New function
> >> (drop_tcb): Execute free_tcb_priv_data callback
> >> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
> >>
> >> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> >> ---
> >>  defs.h    |  6 ++++++
> >>  strace.c  | 14 ++++++++++++++
> >>  syscall.c |  1 +
> >>  3 files changed, 21 insertions(+)
> >>
> >> diff --git a/defs.h b/defs.h
> >> index 9059026..bc3bd83 100644
> >> --- a/defs.h
> >> +++ b/defs.h
> >> @@ -266,6 +266,10 @@ struct tcb {
> >>       int u_error;            /* Error code */
> >>       long scno;              /* System call number */
> >>       long u_arg[MAX_ARGS];   /* System call arguments */
> >> +
> >> +     void *priv_data;        /* Private data for syscall decoding functions */
> >> +     void (*free_priv_data)(void *); /* Callback for freeing priv_data */
> >
> > should we name these _priv_data and _free_priv_data and provides accessor
> > functions ?  i worry that code paths might stomp on each other by accident
> > and we don't end up noticing.
> >
> > static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
> > {
> >         assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
> >         ...
> > }
> > -mike
> 
> Yes, that's a good idea. My use case is pretty simple but usage can easliy grow.
> 
> I'll resend this patch and take it out of the drm/i915 series.
> 
> -Patrik

Here's my take on it (I assume it needs some discussion):

int
set_tcb_priv_data(struct tcb *tcp, void *priv_data)
{
	/* A free callback is required before setting private data and private
	 * data must be set back to NULL before being set again.
	 */
	if (tcp->_free_priv_data == NULL ||
	    (tcp->_priv_data && priv_data != NULL))
		return -1;

	tcp->_priv_data = priv_data;
	return 0;
}

void *
get_tcb_priv_data(struct tcb *tcp)
{
	return tcp->_priv_data;
}

int
set_tcb_free_priv_data(struct tcb *tcp, void (*free_priv_data)(void *))
{
 	/* _free_priv_data must be set back to NULL before being set again. */
	if (tcp->_free_priv_data && free_priv_data != NULL)
		return -1;

	tcp->_free_priv_data = free_priv_data;
	return 0;
}

void
free_tcb_priv_data(struct tcb *tcp)
{
	if (tcp->_priv_data) {
		if (tcp->_free_priv_data) {
			tcp->_free_priv_data(tcp->_priv_data);
			tcp->_free_priv_data = NULL;
		}
		tcp->_priv_data = NULL;
	}
}

Cheers
Patrik
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 686 bytes --]

On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
[...]
> Here's my take on it (I assume it needs some discussion):
> 
> int
> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> {
> 	/* A free callback is required before setting private data and private
> 	 * data must be set back to NULL before being set again.
> 	 */

I think a single function initializing both _priv_data and _free_priv_data
would suffice:

int
set_tcb_priv_data(struct tcb *tcp, void *priv_data,
		  void (*free_priv_data)(void *))
{
	if (tcp->_priv_data)
		return -1;

	tcp->_free_priv_data = free_priv_data;
	tcp->_priv_data = priv_data;

	return 0;
}


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]

_______________________________________________
Strace-devel mailing list
Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/strace-devel

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> [...]
>> Here's my take on it (I assume it needs some discussion):
>>
>> int
>> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
>> {
>>       /* A free callback is required before setting private data and private
>>        * data must be set back to NULL before being set again.
>>        */
>
> I think a single function initializing both _priv_data and _free_priv_data
> would suffice:
>
> int
> set_tcb_priv_data(struct tcb *tcp, void *priv_data,
>                   void (*free_priv_data)(void *))
> {
>         if (tcp->_priv_data)
>                 return -1;
>
>         tcp->_free_priv_data = free_priv_data;
>         tcp->_priv_data = priv_data;
>
>         return 0;
> }

Sure, and since they always come in a pairs it might be even better. If it turns
out we need it split up it is easily done later.

Thanks
Patrik
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1196 bytes --]

On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >>       /* A free callback is required before setting private data and private
> >>        * data must be set back to NULL before being set again.
> >>        */
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> >                   void (*free_priv_data)(void *))
> > {
> >         if (tcp->_priv_data)
> >                 return -1;
> >
> >         tcp->_free_priv_data = free_priv_data;
> >         tcp->_priv_data = priv_data;
> >
> >         return 0;
> > }
> 
> Sure, and since they always come in a pairs it might be even better. If it turns
> out we need it split up it is easily done later.

The discussion seems to be stalled.
Patrik, would you like to prepare a patch?


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 411 bytes --]

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]

_______________________________________________
Strace-devel mailing list
Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/strace-devel

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

On Tue, Nov 24, 2015 at 6:46 AM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
>> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
>> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
>> > [...]
>> >> Here's my take on it (I assume it needs some discussion):
>> >>
>> >> int
>> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
>> >> {
>> >>       /* A free callback is required before setting private data and private
>> >>        * data must be set back to NULL before being set again.
>> >>        */
>> >
>> > I think a single function initializing both _priv_data and _free_priv_data
>> > would suffice:
>> >
>> > int
>> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
>> >                   void (*free_priv_data)(void *))
>> > {
>> >         if (tcp->_priv_data)
>> >                 return -1;
>> >
>> >         tcp->_free_priv_data = free_priv_data;
>> >         tcp->_priv_data = priv_data;
>> >
>> >         return 0;
>> > }
>>
>> Sure, and since they always come in a pairs it might be even better. If it turns
>> out we need it split up it is easily done later.
>
> The discussion seems to be stalled.
> Patrik, would you like to prepare a patch?

Hi Dmitry

I'll send you new patches this weekend. Thanks for reminding me.

-Patrik

>
>
> --
> ldv
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/5] drm: Add private data field to trace control block

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --]

On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >>       /* A free callback is required before setting private data and private
> >>        * data must be set back to NULL before being set again.
> >>        */
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> >                   void (*free_priv_data)(void *))
> > {
> >         if (tcp->_priv_data)
> >                 return -1;
> >
> >         tcp->_free_priv_data = free_priv_data;
> >         tcp->_priv_data = priv_data;
> >
> >         return 0;
> > }
> 
> Sure, and since they always come in a pairs it might be even better. If it turns
> out we need it split up it is easily done later.

JFYI, I've finalized and merged this set_tcb_priv_data interface.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 422 bytes --]

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]

_______________________________________________
Strace-devel mailing list
Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/strace-devel

Re: [PATCH v4 2/5] drm: Add private data field to trace control block

[Patrik Jakobsson]

[-- Attachment #1.1: Type: text/plain, Size: 1310 bytes --]

On Jul 20, 2016 4:50 PM, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
> On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> > On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > > [...]
> > >> Here's my take on it (I assume it needs some discussion):
> > >>
> > >> int
> > >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> > >> {
> > >>       /* A free callback is required before setting private data and
private
> > >>        * data must be set back to NULL before being set again.
> > >>        */
> > >
> > > I think a single function initializing both _priv_data and
_free_priv_data
> > > would suffice:
> > >
> > > int
> > > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> > >                   void (*free_priv_data)(void *))
> > > {
> > >         if (tcp->_priv_data)
> > >                 return -1;
> > >
> > >         tcp->_free_priv_data = free_priv_data;
> > >         tcp->_priv_data = priv_data;
> > >
> > >         return 0;
> > > }
> >
> > Sure, and since they always come in a pairs it might be even better. If
it turns
> > out we need it split up it is easily done later.
>
> JFYI, I've finalized and merged this set_tcb_priv_data interface.

Thanks Dmitry

-Patrik

>
>
> --
> ldv

[-- Attachment #1.2: Type: text/html, Size: 1996 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM

[Patrik Jakobsson]

* Makefile.am: Add compilation of drm.c.
* defs.h: Add extern declaration of drm_ioctl when drm headers are found.
* drm.c: New file.
* ioctl.c (ioctl_decode): Dispatch drm ioctls when drm headers are found.

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 Makefile.am |   1 +
 defs.h      |   3 ++
 drm.c       | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioctl.c     |   4 +++
 4 files changed, 115 insertions(+)
 create mode 100644 drm.c

diff --git a/Makefile.am b/Makefile.am
index f70f6d2..c7c6080 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,7 @@ strace_SOURCES =	\
 	count.c		\
 	desc.c		\
 	dirent.c	\
+	drm.c		\
 	epoll.c		\
 	evdev.c		\
 	eventfd.c	\
diff --git a/defs.h b/defs.h
index bc3bd83..dd3c720 100644
--- a/defs.h
+++ b/defs.h
@@ -612,6 +612,9 @@ extern const char *sprint_open_modes(int);
 extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
 
 extern int block_ioctl(struct tcb *, const unsigned int, long);
+#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
+extern int drm_ioctl(struct tcb *, const unsigned int, long);
+#endif
 extern int evdev_ioctl(struct tcb *, const unsigned int, long);
 extern int loop_ioctl(struct tcb *, const unsigned int, long);
 extern int mtd_ioctl(struct tcb *, const unsigned int, long);
diff --git a/drm.c b/drm.c
new file mode 100644
index 0000000..0829803
--- /dev/null
+++ b/drm.c
@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Authors:
+ *    Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
+ */
+
+#include "defs.h"
+
+#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
+
+#ifdef HAVE_DRM_H
+#include <drm.h>
+#else
+#include <drm/drm.h>
+#endif
+
+#include <sys/param.h>
+
+#define DRM_MAX_NAME_LEN 128
+
+inline int drm_is_priv(const unsigned int num)
+{
+	return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
+		_IOC_NR(num) < DRM_COMMAND_END);
+}
+
+static char *drm_get_driver_name(struct tcb *tcp)
+{
+	char path[PATH_MAX];
+	char link[PATH_MAX];
+	int ret;
+
+	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
+		return NULL;
+
+	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
+		 basename(path));
+
+	ret = readlink(link, path, PATH_MAX - 1);
+	if (ret < 0)
+		return NULL;
+
+	path[ret] = '\0';
+	return strdup(basename(path));
+}
+
+static void drm_free_priv(void *data)
+{
+	free(data);
+}
+
+int drm_is_driver(struct tcb *tcp, const char *name)
+{
+	char *priv;
+
+	/*
+	 * If no private data is allocated we are detecting the driver name for
+	 * the first time and must resolve it.
+	 */
+	if (tcp->priv_data == NULL) {
+		tcp->priv_data = drm_get_driver_name(tcp);
+		if (tcp->priv_data == NULL)
+			return 0;
+
+		tcp->free_priv_data = drm_free_priv;
+	}
+
+	priv = tcp->priv_data;
+
+	return strncmp(name, priv, DRM_MAX_NAME_LEN) == 0;
+}
+
+int drm_decode_number(struct tcb *tcp, unsigned int code)
+{
+	return 0;
+}
+
+int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
+{
+	return 0;
+}
+
+#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
diff --git a/ioctl.c b/ioctl.c
index 284828a..dfd35d8 100644
--- a/ioctl.c
+++ b/ioctl.c
@@ -248,6 +248,10 @@ ioctl_decode(struct tcb *tcp)
 	case 0x22:
 		return scsi_ioctl(tcp, code, arg);
 #endif
+#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
+	case 'd':
+		return drm_ioctl(tcp, code, arg);
+#endif
 	case 'L':
 		return loop_ioctl(tcp, code, arg);
 	case 'M':
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2648 bytes --]

On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
> * Makefile.am: Add compilation of drm.c.
> * defs.h: Add extern declaration of drm_ioctl when drm headers are found.
> * drm.c: New file.
> * ioctl.c (ioctl_decode): Dispatch drm ioctls when drm headers are found.

* defs.h (drm_decode_number, drm_ioctl): New prototypes.
* drm.c: New file.
* Makefile.am (strace_SOURCES): Add it.
* ioctl.c (ioctl_decode_command_number, ioctl_decode)
[HAVE_DRM_H || HAVE_DRM_DRM_H]: Dispatch drm ioctls.

> --- a/defs.h
> +++ b/defs.h
> @@ -612,6 +612,9 @@ extern const char *sprint_open_modes(int);
>  extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
>  
>  extern int block_ioctl(struct tcb *, const unsigned int, long);
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> +extern int drm_ioctl(struct tcb *, const unsigned int, long);
> +#endif

I think function these prototypes could be added unconditionally.
Note that v3 version of this patch also declared drm_decode_number().

> --- /dev/null
> +++ b/drm.c
[...]
> +#include "defs.h"
> +
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> +
> +#ifdef HAVE_DRM_H
> +#include <drm.h>
> +#else
> +#include <drm/drm.h>
> +#endif
> +
> +#include <sys/param.h>

Let's include <sys/param.h> before drm stuff.

> +#define DRM_MAX_NAME_LEN 128
> +
> +inline int drm_is_priv(const unsigned int num)
> +{
> +	return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> +		_IOC_NR(num) < DRM_COMMAND_END);
> +}

This function has to be static, and like other static functions,
it has to be added along with its first user, otherwise the project
won't build with --enable-gcc-Werror.

> +static char *drm_get_driver_name(struct tcb *tcp)
> +{
> +	char path[PATH_MAX];
> +	char link[PATH_MAX];
> +	int ret;
> +
> +	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> +		return NULL;
> +
> +	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> +		 basename(path));

if (snprintf(link, sizeof(link), ...) >= sizeof(link))
	return NULL;

> +static void drm_free_priv(void *data)
> +{
> +	free(data);
> +}

Do we really need a wrapper for free(3)?

> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -248,6 +248,10 @@ ioctl_decode(struct tcb *tcp)
>  	case 0x22:
>  		return scsi_ioctl(tcp, code, arg);
>  #endif
> +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> +	case 'd':
> +		return drm_ioctl(tcp, code, arg);
> +#endif
>  	case 'L':
>  		return loop_ioctl(tcp, code, arg);
>  	case 'M':

v3 version also patched ioctl_decode_command_number()
to call drm_decode_number().


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM

[Patrik Jakobsson]

On Tue, Sep 08, 2015 at 03:36:25AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
> > * Makefile.am: Add compilation of drm.c.
> > * defs.h: Add extern declaration of drm_ioctl when drm headers are found.
> > * drm.c: New file.
> > * ioctl.c (ioctl_decode): Dispatch drm ioctls when drm headers are found.
> 
> * defs.h (drm_decode_number, drm_ioctl): New prototypes.
> * drm.c: New file.
> * Makefile.am (strace_SOURCES): Add it.
> * ioctl.c (ioctl_decode_command_number, ioctl_decode)
> [HAVE_DRM_H || HAVE_DRM_DRM_H]: Dispatch drm ioctls.

Ugh, thought I had this correct already. Will fix.

> > --- a/defs.h
> > +++ b/defs.h
> > @@ -612,6 +612,9 @@ extern const char *sprint_open_modes(int);
> >  extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
> >  
> >  extern int block_ioctl(struct tcb *, const unsigned int, long);
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +extern int drm_ioctl(struct tcb *, const unsigned int, long);
> > +#endif
> 
> I think function these prototypes could be added unconditionally.
> Note that v3 version of this patch also declared drm_decode_number().

Ok. Will move the number decoding pieces back into this patch. Not sure why I
did this in the first place.

> > --- /dev/null
> > +++ b/drm.c
> [...]
> > +#include "defs.h"
> > +
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +
> > +#ifdef HAVE_DRM_H
> > +#include <drm.h>
> > +#else
> > +#include <drm/drm.h>
> > +#endif
> > +
> > +#include <sys/param.h>
> 
> Let's include <sys/param.h> before drm stuff.

Ok

> > +#define DRM_MAX_NAME_LEN 128
> > +
> > +inline int drm_is_priv(const unsigned int num)
> > +{
> > +	return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> > +		_IOC_NR(num) < DRM_COMMAND_END);
> > +}
> 
> This function has to be static, and like other static functions,
> it has to be added along with its first user, otherwise the project
> won't build with --enable-gcc-Werror.

Will move it to the correct patch and make it static.

> > +static char *drm_get_driver_name(struct tcb *tcp)
> > +{
> > +	char path[PATH_MAX];
> > +	char link[PATH_MAX];
> > +	int ret;
> > +
> > +	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> > +		return NULL;
> > +
> > +	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> > +		 basename(path));
> 
> if (snprintf(link, sizeof(link), ...) >= sizeof(link))
> 	return NULL;
> 

According to manpage snprintf never returns more than the specified size - 1.
So the only error we can check for is:

if (snprintf(link, sizeof(link), ...) <
    sizeof("/sys/class/drm/%s/device/driver"))

> > +static void drm_free_priv(void *data)
> > +{
> > +	free(data);
> > +}
> 
> Do we really need a wrapper for free(3)?

No. The only reason I see is for clarity on how to use the tcb priv interface.
But that is ofc debatable.

> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -248,6 +248,10 @@ ioctl_decode(struct tcb *tcp)
> >  	case 0x22:
> >  		return scsi_ioctl(tcp, code, arg);
> >  #endif
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +	case 'd':
> > +		return drm_ioctl(tcp, code, arg);
> > +#endif
> >  	case 'L':
> >  		return loop_ioctl(tcp, code, arg);
> >  	case 'M':
> 
> v3 version also patched ioctl_decode_command_number()
> to call drm_decode_number().

Will move it back into this patch.

> 
> 
> -- 
> ldv


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/5] drm: Add dispatcher and driver identification for DRM

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1194 bytes --]

On Fri, Sep 11, 2015 at 12:57:06PM +0200, Patrik Jakobsson wrote:
> On Tue, Sep 08, 2015 at 03:36:25AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:48PM +0200, Patrik Jakobsson wrote:
[...]
> > > +static char *drm_get_driver_name(struct tcb *tcp)
> > > +{
> > > +	char path[PATH_MAX];
> > > +	char link[PATH_MAX];
> > > +	int ret;
> > > +
> > > +	if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
> > > +		return NULL;
> > > +
> > > +	snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> > > +		 basename(path));
> > 
> > if (snprintf(link, sizeof(link), ...) >= sizeof(link))
> > 	return NULL;
> > 
> 
> According to manpage snprintf never returns more than the specified size - 1.

Really?

"The functions snprintf() and vsnprintf() do not write more than size
bytes (including the terminating null byte ('\0')).  If the output was
truncated due to this limit, then the return value is the number of
characters (excluding the terminating null byte) which would have been
written to the final string if enough space had been available.  Thus,
a return value of size or more means that the output was truncated."


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Patrik Jakobsson]

First batch of i915 drm ioctls

* Makefile.am: Add compilation of drm_i915.c.
* defs.h: Add extern i915 declarations
* drm.c (drm_ioctl): Dispatch i915 ioctls.
* drm_i915.c: New file.
* xlat/drm_i915_getparams.in: New file.
* xlat/drm_i915_setparams.in: New file.
* xlat/drm_i915_ioctls.in: New file.

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 Makefile.am                |   1 +
 defs.h                     |   2 +
 drm.c                      |  15 +-
 drm_i915.c                 | 342 +++++++++++++++++++++++++++++++++++++++++++++
 xlat/drm_i915_getparams.in |  28 ++++
 xlat/drm_i915_ioctls.in    |  51 +++++++
 xlat/drm_i915_setparams.in |   4 +
 7 files changed, 442 insertions(+), 1 deletion(-)
 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

diff --git a/Makefile.am b/Makefile.am
index c7c6080..0af46f6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -43,6 +43,7 @@ strace_SOURCES =	\
 	desc.c		\
 	dirent.c	\
 	drm.c		\
+	drm_i915.c	\
 	epoll.c		\
 	evdev.c		\
 	eventfd.c	\
diff --git a/defs.h b/defs.h
index dd3c720..1d54295 100644
--- a/defs.h
+++ b/defs.h
@@ -614,6 +614,8 @@ extern void print_seccomp_filter(struct tcb *tcp, unsigned long);
 extern int block_ioctl(struct tcb *, const unsigned int, long);
 #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
 extern int drm_ioctl(struct tcb *, const unsigned int, long);
+extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int, long);
+extern int drm_i915_decode_number(struct tcb *, unsigned int);
 #endif
 extern int evdev_ioctl(struct tcb *, const unsigned int, long);
 extern int loop_ioctl(struct tcb *, const unsigned int, long);
diff --git a/drm.c b/drm.c
index 0829803..e220309 100644
--- a/drm.c
+++ b/drm.c
@@ -96,12 +96,25 @@ int drm_is_driver(struct tcb *tcp, const char *name)
 
 int drm_decode_number(struct tcb *tcp, unsigned int code)
 {
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			return drm_i915_decode_number(tcp, code);
+	}
+
 	return 0;
 }
 
 int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
 {
-	return 0;
+	int ret = 0;
+
+	/* Check for device specific ioctls */
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			ret = drm_i915_ioctl(tcp, code, arg);
+	}
+
+	return ret;
 }
 
 #endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
diff --git a/drm_i915.c b/drm_i915.c
new file mode 100644
index 0000000..9af3916
--- /dev/null
+++ b/drm_i915.c
@@ -0,0 +1,342 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Authors:
+ *    Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
+ */
+
+#include "defs.h"
+
+#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
+
+#ifdef HAVE_DRM_H
+#include <drm.h>
+#include <i915_drm.h>
+#else
+#include <drm/drm.h>
+#include <drm/i915_drm.h>
+#endif
+
+#include "xlat/drm_i915_ioctls.h"
+#include "xlat/drm_i915_getparams.h"
+#include "xlat/drm_i915_setparams.h"
+
+static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_getparam param;
+	int value;
+
+	if (umove(tcp, arg, &param))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprints(", {param=");
+		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
+	} else if (exiting(tcp)) {
+		if (umove(tcp, (long)param.value, &value))
+			return RVAL_DECODED;
+
+		tprints(", value=");
+		switch (param.param) {
+		case I915_PARAM_CHIPSET_ID:
+			tprintf("0x%04x", value);
+			break;
+		default:
+			tprintf("%d", value);
+		}
+		tprints("}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_setparam param;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &param))
+			return RVAL_DECODED;
+
+		tprints(", {param=");
+		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
+		tprintf(", value=%d}", param.value);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_execbuffer2(struct tcb *tcp, const unsigned int code,
+				long arg)
+{
+	struct drm_i915_gem_execbuffer2 eb;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &eb))
+			return RVAL_DECODED;
+
+		tprintf(", {buffers_ptr=%p, buffer_count=%u, "
+			"batch_start_offset=%x, batch_len=%u, DR1=%u, DR4=%u, "
+			"num_cliprects=%u, cliprects_ptr=%p, flags=0x%Lx}",
+			(void *)eb.buffers_ptr, eb.buffer_count,
+			eb.batch_start_offset, eb.batch_len, eb.DR1, eb.DR4,
+			eb.num_cliprects, (void *)eb.cliprects_ptr, eb.flags);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_busy(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_busy busy;
+
+	if (umove(tcp, arg, &busy))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", busy.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", busy=%c, ring=%u}",
+			(busy.busy & 0x1) ? 'Y' : 'N', (busy.busy >> 16));
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_create(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_create create;
+
+	if (umove(tcp, arg, &create))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {size=%Lu", create.size);
+	} else if (exiting(tcp)) {
+		tprintf(", handle=%u}", create.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_pread(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pread pr;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pr))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pr.handle, pr.offset, pr.size, (void *)pr.data_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_pwrite(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pwrite pw;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pw))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pw.handle, pw.offset, pw.size, (void *)pw.data_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_mmap(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, size=%Lu", mmap.handle, mmap.size);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu, addr_ptr=%p}",
+			mmap.offset, (void *)mmap.addr_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_mmap_gtt(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap_gtt mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", mmap.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu}", mmap.offset);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_set_domain(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_domain dom;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &dom))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u, read_domains=%x, write_domain=%x}",
+			dom.handle, dom.read_domains, dom.write_domain);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_madvise(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_madvise madv;
+
+	if (umove(tcp, arg, &madv))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, madv=%u", madv.handle, madv.madv);
+	} else if (exiting(tcp)) {
+		tprintf(", retained=%u}", madv.retained);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_get_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_get_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", tiling.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", tiling_mode=%u, swizzle_mode=%u}", tiling.tiling_mode,
+			tiling.swizzle_mode);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_set_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, tiling_mode=%u, stride=%u",
+			tiling.handle, tiling.tiling_mode, tiling.stride);
+	} else if (exiting(tcp)) {
+		tprintf(", swizzle_mode=%u}", tiling.swizzle_mode);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int i915_gem_userptr(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_userptr uptr;
+
+	if (umove(tcp, arg, &uptr))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {user_ptr=%p, user_size=%Lu", (void *)uptr.user_ptr,
+			uptr.user_size);
+	} else if (exiting(tcp)) {
+		tprintf(", flags=0x%x, handle=%u}", uptr.flags, uptr.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+int drm_i915_decode_number(struct tcb *tcp, unsigned int arg)
+{
+	const char *str = xlookup(drm_i915_ioctls, arg);
+
+	if (str) {
+		tprintf("%s", str);
+		return 1;
+	}
+
+	return 0;
+}
+
+int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg)
+{
+	switch (code) {
+	case DRM_IOCTL_I915_GETPARAM:
+		return i915_getparam(tcp, code, arg);
+	case DRM_IOCTL_I915_SETPARAM:
+		return i915_setparam(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_EXECBUFFER2:
+		return i915_gem_execbuffer2(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_BUSY:
+		return i915_gem_busy(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_CREATE:
+		return i915_gem_create(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PREAD:
+		return i915_gem_pread(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PWRITE:
+		return i915_gem_pwrite(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP:
+		return i915_gem_mmap(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP_GTT:
+		return i915_gem_mmap_gtt(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_DOMAIN:
+		return i915_gem_set_domain(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MADVISE:
+		return i915_gem_madvise(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_GET_TILING:
+		return i915_gem_get_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_TILING:
+		return i915_gem_set_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_USERPTR:
+		return i915_gem_userptr(tcp, code, arg);
+	}
+
+	return RVAL_DECODED;
+}
+
+#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
diff --git a/xlat/drm_i915_getparams.in b/xlat/drm_i915_getparams.in
new file mode 100644
index 0000000..15275cb
--- /dev/null
+++ b/xlat/drm_i915_getparams.in
@@ -0,0 +1,28 @@
+I915_PARAM_IRQ_ACTIVE
+I915_PARAM_ALLOW_BATCHBUFFER
+I915_PARAM_LAST_DISPATCH
+I915_PARAM_CHIPSET_ID
+I915_PARAM_HAS_GEM
+I915_PARAM_NUM_FENCES_AVAIL
+I915_PARAM_HAS_OVERLAY
+I915_PARAM_HAS_PAGEFLIPPING
+I915_PARAM_HAS_EXECBUF2
+I915_PARAM_HAS_BSD
+I915_PARAM_HAS_BLT
+I915_PARAM_HAS_RELAXED_FENCING
+I915_PARAM_HAS_COHERENT_RINGS
+I915_PARAM_HAS_EXEC_CONSTANTS
+I915_PARAM_HAS_RELAXED_DELTA
+I915_PARAM_HAS_GEN7_SOL_RESET
+I915_PARAM_HAS_LLC
+I915_PARAM_HAS_ALIASING_PPGTT
+I915_PARAM_HAS_WAIT_TIMEOUT
+I915_PARAM_HAS_SEMAPHORES
+I915_PARAM_HAS_PRIME_VMAP_FLUSH
+I915_PARAM_HAS_VEBOX
+I915_PARAM_HAS_SECURE_BATCHES
+I915_PARAM_HAS_PINNED_BATCHES
+I915_PARAM_HAS_EXEC_NO_RELOC
+I915_PARAM_HAS_EXEC_HANDLE_LUT
+I915_PARAM_HAS_WT
+I915_PARAM_CMD_PARSER_VERSION
diff --git a/xlat/drm_i915_ioctls.in b/xlat/drm_i915_ioctls.in
new file mode 100644
index 0000000..21c3397
--- /dev/null
+++ b/xlat/drm_i915_ioctls.in
@@ -0,0 +1,51 @@
+/* Unfortunately i915 collides with other DRM drivers so we must specify these */
+DRM_IOCTL_I915_INIT
+DRM_IOCTL_I915_FLUSH
+DRM_IOCTL_I915_FLIP
+DRM_IOCTL_I915_BATCHBUFFER
+DRM_IOCTL_I915_IRQ_EMIT
+DRM_IOCTL_I915_IRQ_WAIT
+DRM_IOCTL_I915_GETPARAM
+DRM_IOCTL_I915_SETPARAM
+DRM_IOCTL_I915_ALLOC
+DRM_IOCTL_I915_FREE
+DRM_IOCTL_I915_INIT_HEAP
+DRM_IOCTL_I915_CMDBUFFER
+DRM_IOCTL_I915_DESTROY_HEAP
+DRM_IOCTL_I915_SET_VBLANK_PIPE
+DRM_IOCTL_I915_GET_VBLANK_PIPE
+DRM_IOCTL_I915_VBLANK_SWAP
+DRM_IOCTL_I915_HWS_ADDR
+DRM_IOCTL_I915_GEM_INIT
+DRM_IOCTL_I915_GEM_EXECBUFFER
+DRM_IOCTL_I915_GEM_EXECBUFFER2
+DRM_IOCTL_I915_GEM_PIN
+DRM_IOCTL_I915_GEM_UNPIN
+DRM_IOCTL_I915_GEM_BUSY
+DRM_IOCTL_I915_GEM_SET_CACHING
+DRM_IOCTL_I915_GEM_GET_CACHING
+DRM_IOCTL_I915_GEM_THROTTLE
+DRM_IOCTL_I915_GEM_ENTERVT
+DRM_IOCTL_I915_GEM_LEAVEVT
+DRM_IOCTL_I915_GEM_CREATE
+DRM_IOCTL_I915_GEM_PREAD
+DRM_IOCTL_I915_GEM_PWRITE
+DRM_IOCTL_I915_GEM_MMAP
+DRM_IOCTL_I915_GEM_MMAP_GTT
+DRM_IOCTL_I915_GEM_SET_DOMAIN
+DRM_IOCTL_I915_GEM_SW_FINISH
+DRM_IOCTL_I915_GEM_SET_TILING
+DRM_IOCTL_I915_GEM_GET_TILING
+DRM_IOCTL_I915_GEM_GET_APERTURE
+DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID
+DRM_IOCTL_I915_GEM_MADVISE
+DRM_IOCTL_I915_OVERLAY_PUT_IMAGE
+DRM_IOCTL_I915_OVERLAY_ATTRS
+DRM_IOCTL_I915_SET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GEM_WAIT
+DRM_IOCTL_I915_GEM_CONTEXT_CREATE
+DRM_IOCTL_I915_GEM_CONTEXT_DESTROY
+DRM_IOCTL_I915_REG_READ
+DRM_IOCTL_I915_GET_RESET_STATS
+DRM_IOCTL_I915_GEM_USERPTR
diff --git a/xlat/drm_i915_setparams.in b/xlat/drm_i915_setparams.in
new file mode 100644
index 0000000..d93d2ea
--- /dev/null
+++ b/xlat/drm_i915_setparams.in
@@ -0,0 +1,4 @@
+I915_SETPARAM_USE_MI_BATCHBUFFER_START
+I915_SETPARAM_TEX_LRU_LOG_GRANULARITY
+I915_SETPARAM_ALLOW_BATCHBUFFER
+I915_SETPARAM_NUM_USED_FENCES
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2392 bytes --]

On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_i915_getparam param;
> +	int value;
> +
> +	if (umove(tcp, arg, &param))
> +		return RVAL_DECODED;
> +
> +	if (entering(tcp)) {
> +		tprints(", {param=");
> +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> +	} else if (exiting(tcp)) {
> +		if (umove(tcp, (long)param.value, &value))
> +			return RVAL_DECODED;

Since part of param has already been printed, RVAL_DECODED shouldn't be
returned here.  For the same reason, RVAL_DECODED shouldn't be returned
earlier in this function.

> +		tprints(", value=");
> +		switch (param.param) {
> +		case I915_PARAM_CHIPSET_ID:
> +			tprintf("0x%04x", value);

Since the value has been fetched by address stored in param.value,
it has to be printed in brackets like in printnum_int.

> +			break;
> +		default:
> +			tprintf("%d", value);

Likewise.

> +		}
> +		tprints("}");
> +	}
> +
> +	return RVAL_DECODED | 1;

This shouldn't be returned on entering(tcp).

> +}

So the whole function should look smth like this:

static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
{
	struct drm_i915_getparam param;

	if (entering(tcp)) {
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints(", {param=");
		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
		tprints(", value=");
		return 0;
	} else {
		int value;

		if (umove(tcp, arg, &param)) {
			tprints("???");
		} else if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
			switch (param.param) {
			case I915_PARAM_CHIPSET_ID:
				tprintf("[%#04x]", value);
				break;
			default:
				tprintf("[%d]", value);
			}
		}
		tprints("}");
		return 1;
	}
}

Please apply this approach to all DRM_IOWR parsers.

> +
> +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_i915_setparam param;
> +
> +	if (entering(tcp)) {
> +		if (umove(tcp, arg, &param))
> +			return RVAL_DECODED;

In this and other functions I slightly prefer
	if (umove_or_printaddr(tcp, arg, &param))
		return RVAL_DECODED | 1;
over your variant because umove_or_printaddr() handles NULL address
and !verbose(tcp) case better.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1287 bytes --]

On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
[...]
> So the whole function should look smth like this:
> 
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
> 	struct drm_i915_getparam param;
> 
> 	if (entering(tcp)) {
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints(", {param=");

or rather
		tprints(", ");
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints("{param=");


> In this and other functions I slightly prefer
> 	if (umove_or_printaddr(tcp, arg, &param))
> 		return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.

And the whole function will look as simple as this:

static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
{
 	struct drm_i915_setparam param;

	tprints(", ");
	if (!umove_or_printaddr(tcp, arg, &param)) {
		tprints("{param=");
		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
		tprintf(", value=%d}", param.value);
	}

	return RVAL_DECODED | 1;
}

Note the absence of entering(tcp) check because this DRM_IOW parser always
returns a value with RVAL_DECODED flag set.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1739 bytes --]

On Tue, Sep 08, 2015 at 04:30:52AM +0300, Dmitry V. Levin wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> [...]
> > So the whole function should look smth like this:
> > 
> > static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > {
> > 	struct drm_i915_getparam param;
> > 
> > 	if (entering(tcp)) {
> > 		if (umove_or_printaddr(tcp, arg, &param))
> > 			return RVAL_DECODED | 1;
> > 		tprints(", {param=");
> 
> or rather
> 		tprints(", ");
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints("{param=");

or rather

static int
i915_getparam(struct tcb *tcp, const unsigned int code, const long arg)
{
	struct drm_i915_getparam param;
	int value;

	if (entering(tcp)) {
		tprints(", ");
		if (umove_or_printaddr(tcp, arg, &param))
			return RVAL_DECODED | 1;
		tprints("{param=");
		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
		return 0;
	}

	if (!umove(tcp, arg, &param)) {
		tprints(", value=");
		if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
			switch (param.param) {
			case I915_PARAM_CHIPSET_ID:
				tprintf("[%#04x]", value);
				break;
			default:
				tprintf("[%d]", value);
			}
		}
	}
	tprints("}");
	return 1;
}

Note that those structure members that are set by the kernel on exiting
syscall shouldn't normally be printed in case of syserror(tcp).

In case of i915_getparam, no extra check is needed because param.value is
an address supplied by userspace and umove_or_printaddr already performs
all necessary checks, but in other IOWR parsers you might want to use
	if (!syserror(tcp) && !umove(tcp, arg, &param)) {
instead.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Patrik Jakobsson]

On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_i915_getparam param;
> > +	int value;
> > +
> > +	if (umove(tcp, arg, &param))
> > +		return RVAL_DECODED;
> > +
> > +	if (entering(tcp)) {
> > +		tprints(", {param=");
> > +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > +	} else if (exiting(tcp)) {
> > +		if (umove(tcp, (long)param.value, &value))
> > +			return RVAL_DECODED;
> 
> Since part of param has already been printed, RVAL_DECODED shouldn't be
> returned here.  For the same reason, RVAL_DECODED shouldn't be returned
> earlier in this function.
> 

The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
decoded this don't do any fallback". How did you intent it to be used?

> > +		tprints(", value=");
> > +		switch (param.param) {
> > +		case I915_PARAM_CHIPSET_ID:
> > +			tprintf("0x%04x", value);
> 
> Since the value has been fetched by address stored in param.value,
> it has to be printed in brackets like in printnum_int.

Ok

> > +			break;
> > +		default:
> > +			tprintf("%d", value);
> 
> Likewise.
> 
> > +		}
> > +		tprints("}");
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> 
> This shouldn't be returned on entering(tcp).

If all decoding would be done when entering is finished, wouldn't it make sense
to allow RVAL_DECODED here? Still don't understand how this is intended to work.

> > +}
> 
> So the whole function should look smth like this:
> 
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
> 	struct drm_i915_getparam param;
> 
> 	if (entering(tcp)) {
> 		if (umove_or_printaddr(tcp, arg, &param))
> 			return RVAL_DECODED | 1;
> 		tprints(", {param=");
> 		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> 		tprints(", value=");
> 		return 0;
> 	} else {
> 		int value;
> 
> 		if (umove(tcp, arg, &param)) {
> 			tprints("???");
> 		} else if (!umove_or_printaddr(tcp, (long) param.value, &value)) {
> 			switch (param.param) {
> 			case I915_PARAM_CHIPSET_ID:
> 				tprintf("[%#04x]", value);
> 				break;
> 			default:
> 				tprintf("[%d]", value);
> 			}
> 		}
> 		tprints("}");
> 		return 1;
> 	}
> }
> 
> Please apply this approach to all DRM_IOWR parsers.
> 
> > +
> > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_i915_setparam param;
> > +
> > +	if (entering(tcp)) {
> > +		if (umove(tcp, arg, &param))
> > +			return RVAL_DECODED;
> 
> In this and other functions I slightly prefer
> 	if (umove_or_printaddr(tcp, arg, &param))
> 		return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.

Agreed

> 
> 
> -- 
> ldv


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2919 bytes --]

On Fri, Sep 11, 2015 at 01:31:02PM +0200, Patrik Jakobsson wrote:
> On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> > > +{
> > > +	struct drm_i915_getparam param;
> > > +	int value;
> > > +
> > > +	if (umove(tcp, arg, &param))
> > > +		return RVAL_DECODED;
> > > +
> > > +	if (entering(tcp)) {
> > > +		tprints(", {param=");
> > > +		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > > +	} else if (exiting(tcp)) {
> > > +		if (umove(tcp, (long)param.value, &value))
> > > +			return RVAL_DECODED;
> > 
> > Since part of param has already been printed, RVAL_DECODED shouldn't be
> > returned here.  For the same reason, RVAL_DECODED shouldn't be returned
> > earlier in this function.
> 
> The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
> decoded this don't do any fallback". How did you intent it to be used?

RVAL_DECODED itself is trivial: by setting this flag parser tells its
caller that decoding is finished on entering and it shouldn't be called on
exiting of this syscall.  Setting this flag on exiting has no effect.

With regards to ioctl parsers, there might be some confusion because they
also have this unusual return code +1 semantics.  In this example, the
problem with "return RVAL_DECODED" statements is that if they happen on
exiting, it means that something has already been printed on entering
already and by returning RVAL_DECODED parser tells its caller to perform
the default action that also prints something.

That is, ioctl parser should return
- on entering:
  + any value (it's ignored) without RVAL_DECODED flag set:
    continue processing on exiting;
  + RVAL_DECODED:
    skip processing on exiting and tell the caller
    to perform default processing on entering;
  + RVAL_DECODED | (1 + RVAL_*):
    skip processing on exiting and tell the caller
    to skip default processing;
- on exiting:
  + 0 (with or without RVAL_DECODED set, it's ignored anyway):
    tell the caller to perform default processing on exiting;
  + 1 + RVAL_* (RVAL_DECODED is ignored):
    tell the caller to skip default processing.

> > > +			break;
> > > +		default:
> > > +			tprintf("%d", value);
> > 
> > Likewise.
> > 
> > > +		}
> > > +		tprints("}");
> > > +	}
> > > +
> > > +	return RVAL_DECODED | 1;
> > 
> > This shouldn't be returned on entering(tcp).
> 
> If all decoding would be done when entering is finished, wouldn't it make sense
> to allow RVAL_DECODED here? Still don't understand how this is intended to work.

Usally only IOW parsers return codes with RVAL_DECODED set on entering.
IOWR also print something on exiting so they shouldn't normally return
RVAL_DECODED on entering.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Patrik Jakobsson]

First batch of drm / kms ioctls.

* drm.c (drm_version, drm_get_unique, drm_get_magic, drm_wait_vblank,
drm_mode_get_resources, drm_mode_print_modeinfo, drm_mode_get_crtc,
drm_mode_set_crtc, drm_mode_cursor, drm_mode_cursor2,
drm_mode_get_gamma, drm_mode_set_gamma, drm_mode_get_encoder,
drm_mode_get_connector, drm_mode_get_property, drm_mode_set_property,
drm_mode_get_prop_blob, drm_mode_add_fb drm_mode_get_fb, drm_mode_rm_fb,
drm_mode_page_flip, drm_mode_dirty_fb, drm_mode_create_dumb,
drm_mode_map_dumb, drm_mode_destroy_dumb, drm_gem_close): New function
(drm_ioctl): Dispatch drm ioctls

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 drm.c | 543 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 543 insertions(+)

diff --git a/drm.c b/drm.c
index e220309..06f2265 100644
--- a/drm.c
+++ b/drm.c
@@ -104,6 +104,471 @@ int drm_decode_number(struct tcb *tcp, unsigned int code)
 	return 0;
 }
 
+static int drm_version(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_version ver;
+
+	if (exiting(tcp)) {
+		if (umove(tcp, arg, &ver))
+			return RVAL_DECODED;
+
+		tprintf(", {version_major=%d, version_minor=%d, version_patchlevel=%d, "
+			"name_len=%lu, name=", ver.version_major,
+			ver.version_minor, ver.version_patchlevel,
+			ver.name_len);
+		printstr(tcp, (long)ver.name, ver.name_len);
+		tprintf(", date_len=%lu, date=", ver.date_len);
+		printstr(tcp, (long)ver.date, ver.date_len);
+		tprintf(", desc_len=%lu, desc=", ver.desc_len);
+		printstr(tcp, (long)ver.desc, ver.desc_len);
+		tprints("}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_get_unique(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_unique unique;
+
+	if (exiting(tcp)) {
+		if (umove(tcp, arg, &unique))
+			return RVAL_DECODED;
+
+		tprintf(", {unique_len=%lu, unique=", unique.unique_len);
+		printstr(tcp, (long)unique.unique, unique.unique_len);
+		tprints("}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_get_magic(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_auth auth;
+
+	if (exiting(tcp)) {
+		if (umove(tcp, arg, &auth))
+			return RVAL_DECODED;
+
+		tprintf(", {magic=%u}", auth.magic);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_wait_vblank(struct tcb *tcp, const unsigned int code, long arg)
+{
+	union drm_wait_vblank vblank;
+
+	if (umove(tcp, arg, &vblank))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {request={type=%u, sequence=%u, signal=%lu}",
+			vblank.request.type, vblank.request.sequence,
+			vblank.request.signal);
+	} else if (exiting(tcp)) {
+		tprintf(", reply={type=%u, sequence=%u, tval_sec=%ld, tval_usec=%ld}}",
+			vblank.reply.type, vblank.reply.sequence,
+			vblank.reply.tval_sec, vblank.reply.tval_usec);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_resources(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_card_res res;
+
+	if (exiting(tcp)) {
+		if (umove(tcp, arg, &res))
+			return RVAL_DECODED;
+
+		tprintf(", {fb_id_ptr=%p, crtc_id_ptr=%p, connector_id_ptr=%p, "
+			"encoder_id_ptr=%p, count_fbs=%u, count_crtcs=%u, "
+			"count_connectors=%u, count_encoders=%u, min_width=%u, "
+			"max_width=%u, min_height=%u, max_height=%u}",
+			(void *)res.fb_id_ptr, (void *)res.crtc_id_ptr,
+			(void *)res.connector_id_ptr, (void *)res.encoder_id_ptr,
+			res.count_fbs, res.count_crtcs, res.count_connectors,
+			res.count_encoders, res.min_width, res.max_width,
+			res.min_height, res.max_height);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static void drm_mode_print_modeinfo(struct drm_mode_modeinfo *info)
+{
+	tprintf("clock=%u, hdisplay=%hu, hsync_start=%hu, hsync_end=%hu, "
+		"htotal=%hu, hskew=%hu, vdisplay=%hu, vsync_start=%hu, "
+		"vsync_end=%hu, vtotal=%hu, vscan=%hu, vrefresh=%u, "
+		"flags=0x%x, type=%u, name=%s", info->clock, info->hdisplay,
+		info->hsync_start, info->hsync_end, info->htotal, info->hskew,
+		info->vdisplay, info->vsync_start, info->vsync_end,
+		info->vtotal, info->vscan, info->vrefresh, info->flags,
+		info->type, info->name);
+}
+
+static int drm_mode_get_crtc(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_crtc crtc;
+
+	if (umove(tcp, arg, &crtc))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {crtc_id=%u", crtc.crtc_id);
+	} else if (exiting(tcp)) {
+		tprintf(", set_connectors_ptr=%p, count_connectors=%u, "
+			"fb_id=%u, x=%u, y=%u, gamma_size=%u, mode_valid=%u, "
+			"mode={", (void *)crtc.set_connectors_ptr,
+			crtc.count_connectors, crtc.fb_id, crtc.x, crtc.y,
+			crtc.gamma_size, crtc.mode_valid);
+
+		drm_mode_print_modeinfo(&crtc.mode);
+		tprintf("}}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_set_crtc(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_crtc crtc;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &crtc))
+			return RVAL_DECODED;
+
+		tprintf(", {set_connectors_ptr=%p, count_connectors=%u, "
+			"crtc_id=%u, fb_id=%u, x=%u, y=%u, gamma_size=%u, "
+			"mode_valid=%u, mode={", (void *)crtc.set_connectors_ptr,
+			crtc.count_connectors, crtc.crtc_id, crtc.fb_id, crtc.x,
+			crtc.y, crtc.gamma_size, crtc.mode_valid);
+
+		drm_mode_print_modeinfo(&crtc.mode);
+		tprintf("}}");
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_cursor(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_cursor cursor;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &cursor))
+			return RVAL_DECODED;
+
+		tprintf(", {flags=0x%x, crtc_id=%u, x=%d, y=%d, width=%u, "
+			"height=%u, handle=%u}", cursor.flags, cursor.crtc_id,
+			cursor.x, cursor.y, cursor.width, cursor.height,
+			cursor.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_cursor2(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_cursor2 cursor;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &cursor))
+			return RVAL_DECODED;
+
+		tprintf(", {flags=0x%x, crtc_id=%u, x=%d, y=%d, width=%u, "
+			"height=%u, handle=%u, hot_x=%d, hot_y=%d}",
+			cursor.flags, cursor.crtc_id, cursor.x, cursor.y,
+			cursor.width, cursor.height, cursor.handle,
+			cursor.hot_x, cursor.hot_y);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_gamma(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_crtc_lut lut;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &lut))
+			return RVAL_DECODED;
+
+		/* We don't print the entire table, just the pointers */
+		tprintf(", {crtc_id=%u, gamma_size=%u, red=%p, green=%p, "
+			"blue=%p}", lut.crtc_id, lut.gamma_size,
+			(void *)lut.red, (void *)lut.green, (void *)lut.blue);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_set_gamma(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_crtc_lut lut;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &lut))
+			return RVAL_DECODED;
+
+		/* We don't print the entire table, just the rgb pointers */
+		tprintf(", {crtc_id=%u, gamma_size=%u, red=%p, green=%p, "
+			"blue=%p}", lut.crtc_id, lut.gamma_size,
+			(void *)lut.red, (void *)lut.green, (void *)lut.blue);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_encoder(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_get_encoder enc;
+
+	if (umove(tcp, arg, &enc))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {encoder_id=%u", enc.encoder_id);
+	} else if (exiting(tcp)) {
+		/* TODO: Print name of encoder type */
+		tprintf(", encoder_type=%u, crtc_id=%u, possible_crtcs=0x%x, "
+			"possible_clones=0x%x}", enc.encoder_type,
+			enc.crtc_id, enc.possible_crtcs, enc.possible_clones);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_connector(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_get_connector con;
+
+	if (umove(tcp, arg, &con))
+		return RVAL_DECODED;
+
+	/* We could be very verbose here but keep is simple for now */
+	if (entering(tcp)) {
+		tprintf(", {connector_id=%u", con.connector_id);
+	} else if (exiting(tcp)) {
+		tprintf(", encoders_ptr=%p, modes_ptr=%p, props_ptr=%p, "
+			"prop_values_ptr=%p, count_modes=%u, count_props=%u, "
+			"count_encoders=%u, encoder_id=%u, connector_type=%u, "
+			"connector_type_id=%u, connection=%u, mm_width=%u, "
+			"mm_height=%u, subpixel=%u}", (void *)con.encoders_ptr,
+			(void *)con.modes_ptr, (void *)con.props_ptr,
+			(void *)con.prop_values_ptr, con.count_modes,
+			con.count_props, con.count_encoders, con.encoder_id,
+			con.connector_type, con.connector_type_id,
+			con.connection, con.mm_width, con.mm_height,
+			con.subpixel);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_property(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_get_property prop;
+
+	if (umove(tcp, arg, &prop))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {prop_id=%u", prop.prop_id);
+	} else if (exiting(tcp)) {
+		tprintf(", values_ptr=%p, enum_blob_ptr=%p, flags=0x%x, "
+			"name=%s, count_values=%u, count_enum_blobs=%u}",
+			(void *)prop.values_ptr, (void *)prop.enum_blob_ptr,
+			prop.flags, prop.name, prop.count_values,
+			prop.count_enum_blobs);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_set_property(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_connector_set_property prop;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &prop))
+			return RVAL_DECODED;
+
+		tprintf(", {value=%Lu, prop_id=%u, connector_id=%u}",
+			prop.value, prop.prop_id, prop.connector_id);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_prop_blob(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_get_blob blob;
+
+	if (umove(tcp, arg, &blob))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {blob_id=%u", blob.blob_id);
+	} else if (exiting(tcp)) {
+		tprintf(", length=%u, data=%p}", blob.length,
+			(void *)blob.data);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_add_fb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_fb_cmd cmd;
+
+	if (umove(tcp, arg, &cmd))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {width=%u, height=%u, pitch=%u, bpp=%u, depth=%u, "
+			"handle=%u", cmd.width, cmd.height, cmd.pitch,
+			cmd.bpp, cmd.depth, cmd.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", fb_id=%u}", cmd.fb_id);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_get_fb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_fb_cmd cmd;
+
+	if (umove(tcp, arg, &cmd))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {fb_id=%u", cmd.fb_id);
+	} else {
+		tprintf(", width=%u, height=%u, pitch=%u, bpp=%u, depth=%u, "
+		"handle=%u}", cmd.width, cmd.height, cmd.pitch,
+		cmd.bpp, cmd.depth, cmd.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_rm_fb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	unsigned int handle;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &handle))
+			return RVAL_DECODED;
+
+		tprintf(", %u", handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_page_flip(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_crtc_page_flip flip;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &flip))
+			return RVAL_DECODED;
+
+		tprintf(", {crtc_id=%u, fb_id=%u, flags=0x%x, user_data=0x%Lx}",
+			flip.crtc_id, flip.fb_id, flip.flags, flip.user_data);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_dirty_fb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_fb_dirty_cmd cmd;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &cmd))
+			return RVAL_DECODED;
+
+		tprintf(", {fb_id=%u, flags=0x%x, color=0x%x, num_clips=%u, "
+			"clips_ptr=%p}", cmd.fb_id, cmd.flags, cmd.color,
+			cmd.num_clips, (void *)cmd.clips_ptr);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_create_dumb dumb;
+
+	if (umove(tcp, arg, &dumb))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
+			dumb.width, dumb.height, dumb.bpp, dumb.flags);
+	} else if (exiting(tcp)) {
+		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
+			dumb.pitch, dumb.size);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_map_dumb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_map_dumb dumb;
+
+	if (umove(tcp, arg, &dumb))
+		return RVAL_DECODED;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", dumb.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu}", dumb.offset);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_mode_destroy_dumb(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_mode_destroy_dumb dumb;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &dumb))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u}", dumb.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
+static int drm_gem_close(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_gem_close close;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &close))
+			return RVAL_DECODED;
+
+		tprintf(", {handle=%u}", close.handle);
+	}
+
+	return RVAL_DECODED | 1;
+}
+
 int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
 {
 	int ret = 0;
@@ -112,6 +577,84 @@ int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
 	if (drm_is_priv(tcp->u_arg[1])) {
 		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
 			ret = drm_i915_ioctl(tcp, code, arg);
+	} else {
+		switch (code) {
+		case DRM_IOCTL_VERSION:
+			ret = drm_version(tcp, code, arg);
+			break;
+		case DRM_IOCTL_GET_UNIQUE:
+			ret = drm_get_unique(tcp, code, arg);
+			break;
+		case DRM_IOCTL_GET_MAGIC:
+			ret = drm_get_magic(tcp, code, arg);
+			break;
+		case DRM_IOCTL_WAIT_VBLANK:
+			ret = drm_wait_vblank(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETRESOURCES:
+			ret = drm_mode_get_resources(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETCRTC:
+			ret = drm_mode_get_crtc(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_SETCRTC:
+			ret = drm_mode_set_crtc(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_CURSOR:
+			ret = drm_mode_cursor(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_CURSOR2:
+			ret = drm_mode_cursor2(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETGAMMA:
+			ret = drm_mode_get_gamma(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_SETGAMMA:
+			ret = drm_mode_set_gamma(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETENCODER:
+			ret = drm_mode_get_encoder(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETCONNECTOR:
+			ret = drm_mode_get_connector(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETPROPERTY:
+			ret = drm_mode_get_property(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_SETPROPERTY:
+			ret = drm_mode_set_property(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETPROPBLOB:
+			ret = drm_mode_get_prop_blob(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_GETFB:
+			ret = drm_mode_get_fb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_ADDFB:
+			ret = drm_mode_add_fb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_RMFB:
+			ret = drm_mode_rm_fb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_PAGE_FLIP:
+			ret = drm_mode_page_flip(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_DIRTYFB:
+			ret = drm_mode_dirty_fb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_CREATE_DUMB:
+			ret = drm_mode_create_dumb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_MAP_DUMB:
+			ret = drm_mode_map_dumb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_MODE_DESTROY_DUMB:
+			ret = drm_mode_destroy_dumb(tcp, code, arg);
+			break;
+		case DRM_IOCTL_GEM_CLOSE:
+			ret = drm_gem_close(tcp, code, arg);
+			break;
+		}
 	}
 
 	return ret;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1863 bytes --]

On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> First batch of drm / kms ioctls.

Several comments in addition to issues similar to 4/5 patch.

> +static int drm_mode_rm_fb(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	unsigned int handle;
> +
> +
> +	if (entering(tcp)) {
> +		if (umove(tcp, arg, &handle))
> +			return RVAL_DECODED;
> +
> +		tprintf(", %u", handle);
> +	}
> +
> +	return RVAL_DECODED | 1;
> +}

Use printnum_int instead:

	tprints(", ");
	printnum_int(tcp, arg, "%u");
	return RVAL_DECODED | 1;

> +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_mode_create_dumb dumb;
> +
> +	if (umove(tcp, arg, &dumb))
> +		return RVAL_DECODED;
> +
> +	if (entering(tcp)) {
> +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> +	} else if (exiting(tcp)) {
> +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> +			dumb.pitch, dumb.size);
> +	}
> +
> +	return RVAL_DECODED | 1;
> +}

This generates a warning (which turns into an error with
--enable-gcc-Werror) on x86_64 when using kernel drm headers:

drm.c: In function 'drm_mode_create_dumb':
drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]

> @@ -112,6 +577,84 @@ int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
>  	if (drm_is_priv(tcp->u_arg[1])) {
>  		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
>  			ret = drm_i915_ioctl(tcp, code, arg);
> +	} else {
> +		switch (code) {
> +		case DRM_IOCTL_VERSION:
> +			ret = drm_version(tcp, code, arg);
> +			break;

Looks like the return code can be forwarded without further delays.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Patrik Jakobsson]

On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > First batch of drm / kms ioctls.
> 
> Several comments in addition to issues similar to 4/5 patch.
> 
> > +static int drm_mode_rm_fb(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	unsigned int handle;
> > +
> > +
> > +	if (entering(tcp)) {
> > +		if (umove(tcp, arg, &handle))
> > +			return RVAL_DECODED;
> > +
> > +		tprintf(", %u", handle);
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> > +}
> 
> Use printnum_int instead:
> 
> 	tprints(", ");
> 	printnum_int(tcp, arg, "%u");
> 	return RVAL_DECODED | 1;

Ok

> > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_mode_create_dumb dumb;
> > +
> > +	if (umove(tcp, arg, &dumb))
> > +		return RVAL_DECODED;
> > +
> > +	if (entering(tcp)) {
> > +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > +	} else if (exiting(tcp)) {
> > +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > +			dumb.pitch, dumb.size);
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> > +}
> 
> This generates a warning (which turns into an error with
> --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> 
> drm.c: In function 'drm_mode_create_dumb':
> drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]

So this brings us back to whether to include drm kernel headers or not. If
-Werror is a requirement (which is already broken last time I checked) there
will need to be #ifdefs at various places in drm decoding. What would you
prefer. Both options are fine by me.

> > @@ -112,6 +577,84 @@ int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> >  	if (drm_is_priv(tcp->u_arg[1])) {
> >  		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
> >  			ret = drm_i915_ioctl(tcp, code, arg);
> > +	} else {
> > +		switch (code) {
> > +		case DRM_IOCTL_VERSION:
> > +			ret = drm_version(tcp, code, arg);
> > +			break;
> 
> Looks like the return code can be forwarded without further delays.

Yes, this is a leftover from before the tcb priv interface. Will remove.
> 
> 
> -- 
> ldv


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1625 bytes --]

On Fri, Sep 11, 2015 at 01:39:29PM +0200, Patrik Jakobsson wrote:
> On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> > On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> > > +{
> > > +	struct drm_mode_create_dumb dumb;
> > > +
> > > +	if (umove(tcp, arg, &dumb))
> > > +		return RVAL_DECODED;
> > > +
> > > +	if (entering(tcp)) {
> > > +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > > +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > > +	} else if (exiting(tcp)) {
> > > +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > > +			dumb.pitch, dumb.size);
> > > +	}
> > > +
> > > +	return RVAL_DECODED | 1;
> > > +}
> > 
> > This generates a warning (which turns into an error with
> > --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> > 
> > drm.c: In function 'drm_mode_create_dumb':
> > drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]
> 
> So this brings us back to whether to include drm kernel headers or not. If
> -Werror is a requirement (which is already broken last time I checked) there

Is it?  Could you cite the error, please?

> will need to be #ifdefs at various places in drm decoding. What would you
> prefer. Both options are fine by me.

This is the only place where definitions differ to the extent that it's visible.
I'd rather cast the argument to unsigned long long.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Patrik Jakobsson]

On Fri, Sep 11, 2015 at 03:10:05PM +0300, Dmitry V. Levin wrote:
> On Fri, Sep 11, 2015 at 01:39:29PM +0200, Patrik Jakobsson wrote:
> > On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> > > On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > > > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> > > > +{
> > > > +	struct drm_mode_create_dumb dumb;
> > > > +
> > > > +	if (umove(tcp, arg, &dumb))
> > > > +		return RVAL_DECODED;
> > > > +
> > > > +	if (entering(tcp)) {
> > > > +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > > > +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > > > +	} else if (exiting(tcp)) {
> > > > +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > > > +			dumb.pitch, dumb.size);
> > > > +	}
> > > > +
> > > > +	return RVAL_DECODED | 1;
> > > > +}
> > > 
> > > This generates a warning (which turns into an error with
> > > --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> > > 
> > > drm.c: In function 'drm_mode_create_dumb':
> > > drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]
> > 
> > So this brings us back to whether to include drm kernel headers or not. If
> > -Werror is a requirement (which is already broken last time I checked) there
> 
> Is it?  Could you cite the error, please?

It's in aio.c:185 which at a closer look is intentional. Would still break the
build though. But I'm not arguing that letting warnings slip through (with or
without -Werror) is ok, just wondering if it justifies what I'm trying to do
here.

> > will need to be #ifdefs at various places in drm decoding. What would you
> > prefer. Both options are fine by me.
> 
> This is the only place where definitions differ to the extent that it's visible.
> I'd rather cast the argument to unsigned long long.

Sounds reasonable, I'll do that.

> 
> -- 
> ldv



> ------------------------------------------------------------------------------

> _______________________________________________
> Strace-devel mailing list
> Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/strace-devel


------------------------------------------------------------------------------

Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2106 bytes --]

On Fri, Sep 11, 2015 at 02:20:35PM +0200, Patrik Jakobsson wrote:
> On Fri, Sep 11, 2015 at 03:10:05PM +0300, Dmitry V. Levin wrote:
> > On Fri, Sep 11, 2015 at 01:39:29PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> > > > On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > > > > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> > > > > +{
> > > > > +	struct drm_mode_create_dumb dumb;
> > > > > +
> > > > > +	if (umove(tcp, arg, &dumb))
> > > > > +		return RVAL_DECODED;
> > > > > +
> > > > > +	if (entering(tcp)) {
> > > > > +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > > > > +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > > > > +	} else if (exiting(tcp)) {
> > > > > +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > > > > +			dumb.pitch, dumb.size);
> > > > > +	}
> > > > > +
> > > > > +	return RVAL_DECODED | 1;
> > > > > +}
> > > > 
> > > > This generates a warning (which turns into an error with
> > > > --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> > > > 
> > > > drm.c: In function 'drm_mode_create_dumb':
> > > > drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]
> > > 
> > > So this brings us back to whether to include drm kernel headers or not. If
> > > -Werror is a requirement (which is already broken last time I checked) there
> > 
> > Is it?  Could you cite the error, please?
> 
> It's in aio.c:185 which at a closer look is intentional. Would still break the
> build though. But I'm not arguing that letting warnings slip through (with or
> without -Werror) is ok, just wondering if it justifies what I'm trying to do
> here.

strace is expected to build without compilation warnings assuming that the
compiler and headers are supported.  In fact, I routinely build strace
with --enable-gcc-Werror.  If it doesn't build for you, please report.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 0/5] drm: Add decoding for DRM/KMS and i915 ioctls

[Gabriel Laskar]

On Mon, 24 Aug 2015 14:42:45 +0200
Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote:

> This set of patches adds a dispatcher for handling DRM ioctls. The
> kernel headers for DRM might not be available on all distributions
> so we depend on libdrm for those. If libdrm is not available we fall
> back on the kernel headers. Since DRM drivers share the same range of
> private ioctl numbers I've added a function for detecting the driver
> based on it's name.

Besides the reflection about the usage/api for priv_data and
free_priv_data, this patchset seems to be good. Is there any more
remarks for the drm ioctl decoding ?

> Changes in v2:
> * Rebased to master
> * Added Changelog to commits
> * Keep strace_SOURCES list sorted
> * Removed unneeded includes
> * Reduced number of driver name checks by adding tcb private data
> * Use tprints() for regular strings
> * Reworked entering() / exiting() handling for all ioctls
> * Use printstr() to print strings in properly quoted form
> 
> Changes in v3:
> * Moved all umove() into state checks for single state ioctls
> * Removed extra curly bracket
> * Moved param argument into entering() state in i915_setparam()
> * Don't return before private data is freed in drm_ioctl()
> 
> Changes in v4:
> * Rebased to master
> * Rewrote commit messages to GNU changelog standard
> * Added private data support to struct tcb
> * Reworked drm driver identification
> * Reworked drm header detection
> * Use recently added return types for decode functions
> * Various small fixes
> 
> Patrik Jakobsson (5):
>   drm: Add config for detecting libdrm
>   drm: Add private data field to trace control block
>   drm: Add dispatcher and driver identification for DRM
>   drm: Add decoding of i915 ioctls
>   drm: Add decoding of DRM and KMS ioctls
> 
>  Makefile.am                |   2 +
>  configure.ac               |   5 +
>  defs.h                     |  11 +
>  drm.c                      | 663 +++++++++++++++++++++++++++++++++++++++++++++
>  drm_i915.c                 | 342 +++++++++++++++++++++++
>  ioctl.c                    |   4 +
>  strace.c                   |  14 +
>  syscall.c                  |   1 +
>  xlat/drm_i915_getparams.in |  28 ++
>  xlat/drm_i915_ioctls.in    |  51 ++++
>  xlat/drm_i915_setparams.in |   4 +
>  11 files changed, 1125 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
> 



-- 
Gabriel Laskar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx