[PATCH] drm/i915: add register read IOCTL

[PATCH] drm/i915: add register read IOCTL

[Ben Widawsky]

The interface's immediate purpose is to do synchronous timestamp queries
as required by GL_TIMESTAMP. The GPU has a register for reading the
timestamp but because that would normally require root access, the
IOCTL can provide this service.

Currently the implementation whitelists only the render ring timestamp
register, because that is the only thing we need to expose at this time.

Cc: Eric Anholt <eric@anholt.net>
Cc: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>,
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  1 +
 drivers/gpu/drm/i915/i915_drv.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 include/drm/i915_drm.h          |  8 ++++++++
 5 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f64ef4b..5e20f11 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1851,6 +1851,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e754cdf..ae52326 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1152,3 +1152,33 @@ __i915_write(16, w)
 __i915_write(32, l)
 __i915_write(64, q)
 #undef __i915_write
+
+int i915_reg_read_ioctl(struct drm_device *dev,
+			void *data, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_reg_read *reg = data;
+
+	/* Whitelisted for now */
+	if (reg->offset != RING_TIMESTAMP(RENDER_RING_BASE))
+		return -ENXIO;
+
+	switch (reg->size) {
+	case 8:
+		reg->val = I915_READ64(reg->offset);
+		break;
+	case 4:
+		reg->val = I915_READ(reg->offset);
+		break;
+	case 2:
+		reg->val = I915_READ16(reg->offset);
+		break;
+	case 1:
+		reg->val = I915_READ8(reg->offset);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 627fe35..2c0d840 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1529,6 +1529,8 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
 
 extern bool i915_semaphore_is_enabled(struct drm_device *dev);
+int i915_reg_read_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file);
 
 /* overlay */
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc82871..33e66cc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -449,6 +449,7 @@
 #define RING_ACTHD(base)	((base)+0x74)
 #define RING_NOPID(base)	((base)+0x94)
 #define RING_IMR(base)		((base)+0xa8)
+#define RING_TIMESTAMP(base)	((base)+0x358)
 #define   TAIL_ADDR		0x001FFFF8
 #define   HEAD_WRAP_COUNT	0xFFE00000
 #define   HEAD_WRAP_ONE		0x00200000
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 8cc7083..6538d8b 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_WAIT	0x2c
 #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
 #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
+#define DRM_I915_REG_READ	0x30
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -249,6 +250,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
+#define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -918,4 +920,10 @@ struct drm_i915_gem_context_destroy {
 	__u32 pad;
 };
 
+struct drm_i915_reg_read {
+	__u64 offset;
+	__u32 size;
+	__u64 val; /* Return value */
+	__u32 pad;
+};
 #endif				/* _I915_DRM_H_ */
-- 
1.7.11.1

[PATCH] reg_read: basic register read ioctl test

[Ben Widawsky]

This will need to get modified when the ioctl expands, and so is only
here for reference/to make Daniel happy.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/drm_reg_read.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 tests/drm_reg_read.c

diff --git a/tests/drm_reg_read.c b/tests/drm_reg_read.c
new file mode 100644
index 0000000..d28039b
--- /dev/null
+++ b/tests/drm_reg_read.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "i915_drm.h"
+#include "drmtest.h"
+
+struct local_drm_i915_reg_read {
+	__u64 offset;
+	__u32 size;
+	__u64 val; /* Return value */
+	__u32 pad;
+};
+
+
+#define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x30, struct local_drm_i915_reg_read)
+
+static void handle_bad(int ret, int lerrno, int expected, const char *desc)
+{
+	if (ret != 0 && lerrno != expected) {
+		fprintf(stderr, "%s - errno was %d, but should have been %d\n",
+				desc, lerrno, expected);
+		exit(EXIT_FAILURE);
+	} else if (ret == 0) {
+		fprintf(stderr, "%s - Command succeeded, but should have failed\n",
+			desc);
+		exit(EXIT_FAILURE);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct local_drm_i915_reg_read read;
+	int ret, fd;
+
+	read.offset = 0x2358;
+	read.size = 4;
+
+	fd = drm_open_any();
+
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	if (ret) {
+		perror("positive test case failed\n");
+		exit(EXIT_FAILURE);
+	}
+
+	/* bad reg */
+	read.offset = 0x12345678;
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	handle_bad(ret, errno, ENXIO, "bad register");
+
+	/* bad size */
+	read.offset = 0x2358;
+	read.size = 5;
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	handle_bad(ret, errno, EINVAL, "bad size");
+
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.11.1

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

Re: [PATCH] drm/i915: add register read IOCTL

[Daniel Vetter]

On Wed, Jul 11, 2012 at 05:07:36PM -0700, Ben Widawsky wrote:
> The interface's immediate purpose is to do synchronous timestamp queries
> as required by GL_TIMESTAMP. The GPU has a register for reading the
> timestamp but because that would normally require root access, the
> IOCTL can provide this service.
> 
> Currently the implementation whitelists only the render ring timestamp
> register, because that is the only thing we need to expose at this time.
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>,
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Just two minor things below.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  1 +
>  drivers/gpu/drm/i915/i915_drv.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  include/drm/i915_drm.h          |  8 ++++++++
>  5 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f64ef4b..5e20f11 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1851,6 +1851,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e754cdf..ae52326 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1152,3 +1152,33 @@ __i915_write(16, w)
>  __i915_write(32, l)
>  __i915_write(64, q)
>  #undef __i915_write
> +
> +int i915_reg_read_ioctl(struct drm_device *dev,
> +			void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reg_read *reg = data;
> +
> +	/* Whitelisted for now */
> +	if (reg->offset != RING_TIMESTAMP(RENDER_RING_BASE))
> +		return -ENXIO;

I think we should check for both reg offset _and_ size. Just to avoid
people reading 64bit for a 32bit reg to get at the secret stuff in the
next reg ;-) Or in case that the hw has strange semantics if you don't
read the right size (I've seen that). Imo just creating a little table
with { offset; size; } pairs would be good enough.

> +
> +	switch (reg->size) {
> +	case 8:
> +		reg->val = I915_READ64(reg->offset);
> +		break;
> +	case 4:
> +		reg->val = I915_READ(reg->offset);
> +		break;
> +	case 2:
> +		reg->val = I915_READ16(reg->offset);
> +		break;
> +	case 1:
> +		reg->val = I915_READ8(reg->offset);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 627fe35..2c0d840 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1529,6 +1529,8 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>  extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> +int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> +			struct drm_file *file);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc82871..33e66cc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -449,6 +449,7 @@
>  #define RING_ACTHD(base)	((base)+0x74)
>  #define RING_NOPID(base)	((base)+0x94)
>  #define RING_IMR(base)		((base)+0xa8)
> +#define RING_TIMESTAMP(base)	((base)+0x358)
>  #define   TAIL_ADDR		0x001FFFF8
>  #define   HEAD_WRAP_COUNT	0xFFE00000
>  #define   HEAD_WRAP_ONE		0x00200000
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 8cc7083..6538d8b 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_GEM_WAIT	0x2c
>  #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
>  #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
> +#define DRM_I915_REG_READ	0x30
>  
>  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -249,6 +250,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
>  #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
>  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
> +#define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -918,4 +920,10 @@ struct drm_i915_gem_context_destroy {
>  	__u32 pad;
>  };
>  
> +struct drm_i915_reg_read {
> +	__u64 offset;
> +	__u32 size;
> +	__u64 val; /* Return value */
> +	__u32 pad;

The padding is at the wrong place ...

> +};
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.11.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] reg_read: basic register read ioctl test

[Daniel Vetter]

On Wed, Jul 11, 2012 at 05:08:00PM -0700, Ben Widawsky wrote:
> This will need to get modified when the ioctl expands, and so is only
> here for reference/to make Daniel happy.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

If you go with the (offset, size) table to check things, I think you
should add a test to read a valid reg offset, but with the wrong size.
Otherwise this looks good.
-Daniel
> ---
>  tests/drm_reg_read.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 tests/drm_reg_read.c
> 
> diff --git a/tests/drm_reg_read.c b/tests/drm_reg_read.c
> new file mode 100644
> index 0000000..d28039b
> --- /dev/null
> +++ b/tests/drm_reg_read.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include "i915_drm.h"
> +#include "drmtest.h"
> +
> +struct local_drm_i915_reg_read {
> +	__u64 offset;
> +	__u32 size;
> +	__u64 val; /* Return value */
> +	__u32 pad;
> +};
> +
> +
> +#define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x30, struct local_drm_i915_reg_read)
> +
> +static void handle_bad(int ret, int lerrno, int expected, const char *desc)
> +{
> +	if (ret != 0 && lerrno != expected) {
> +		fprintf(stderr, "%s - errno was %d, but should have been %d\n",
> +				desc, lerrno, expected);
> +		exit(EXIT_FAILURE);
> +	} else if (ret == 0) {
> +		fprintf(stderr, "%s - Command succeeded, but should have failed\n",
> +			desc);
> +		exit(EXIT_FAILURE);
> +	}
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct local_drm_i915_reg_read read;
> +	int ret, fd;
> +
> +	read.offset = 0x2358;
> +	read.size = 4;
> +
> +	fd = drm_open_any();
> +
> +	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
> +	if (ret) {
> +		perror("positive test case failed\n");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	/* bad reg */
> +	read.offset = 0x12345678;
> +	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
> +	handle_bad(ret, errno, ENXIO, "bad register");
> +
> +	/* bad size */
> +	read.offset = 0x2358;
> +	read.size = 5;
> +	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
> +	handle_bad(ret, errno, EINVAL, "bad size");
> +
> +	close(fd);
> +
> +	exit(EXIT_SUCCESS);
> +}
> -- 
> 1.7.11.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: add register read IOCTL

[Chris Wilson]

On Thu, 12 Jul 2012 09:58:43 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 11, 2012 at 05:07:36PM -0700, Ben Widawsky wrote:
> I think we should check for both reg offset _and_ size. Just to avoid
> people reading 64bit for a 32bit reg to get at the secret stuff in the
> next reg ;-) Or in case that the hw has strange semantics if you don't
> read the right size (I've seen that). Imo just creating a little table
> with { offset; size; } pairs would be good enough.

Do you want to allow people to read a subregister? That's the only
reason I see to have userspace pass in a size, and extracting a field
from a register can be trivially done in userspace (and will be done as
part of the normal process of extracting a value from the result).
Reading 8,16,32,64 bits all generate a 64-bit read cycle so should be
immaterial performance wise.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm/i915: add register read IOCTL

[Ben Widawsky]

The interface's immediate purpose is to do synchronous timestamp queries
as required by GL_TIMESTAMP. The GPU has a register for reading the
timestamp but because that would normally require root access through
libpciaccess, the IOCTL can provide this service instead.

Currently the implementation whitelists only the render ring timestamp
register, because that is the only thing we need to expose at this time.

v2: make size implicit based on the register offset
Add a generation check

Cc: Eric Anholt <eric@anholt.net>
Cc: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  1 +
 drivers/gpu/drm/i915/i915_drv.c | 46 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 include/drm/i915_drm.h          |  6 ++++++
 5 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f64ef4b..5e20f11 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1851,6 +1851,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e754cdf..77deaea 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1152,3 +1152,49 @@ __i915_write(16, w)
 __i915_write(32, l)
 __i915_write(64, q)
 #undef __i915_write
+
+static const struct register_whitelist {
+	uint64_t offset;
+	uint32_t size;
+	uint32_t gen_bitmask; /* support gens, 0x10 for 4, 0x30 for 4 and 5, etc. */
+} whitelist[] = {
+	{ RING_TIMESTAMP(RENDER_RING_BASE), 8, 0xF0 },
+};
+
+int i915_reg_read_ioctl(struct drm_device *dev,
+			void *data, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_reg_read *reg = data;
+	struct register_whitelist const *entry = whitelist;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
+		if (entry->offset == reg->offset &&
+		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
+			break;
+	}
+
+	if (i == ARRAY_SIZE(whitelist))
+		return -EINVAL;
+
+	switch (entry->size) {
+	case 8:
+		reg->val = I915_READ64(reg->offset);
+		break;
+	case 4:
+		reg->val = I915_READ(reg->offset);
+		break;
+	case 2:
+		reg->val = I915_READ16(reg->offset);
+		break;
+	case 1:
+		reg->val = I915_READ8(reg->offset);
+		break;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 627fe35..2c0d840 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1529,6 +1529,8 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
 
 extern bool i915_semaphore_is_enabled(struct drm_device *dev);
+int i915_reg_read_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file);
 
 /* overlay */
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc82871..33e66cc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -449,6 +449,7 @@
 #define RING_ACTHD(base)	((base)+0x74)
 #define RING_NOPID(base)	((base)+0x94)
 #define RING_IMR(base)		((base)+0xa8)
+#define RING_TIMESTAMP(base)	((base)+0x358)
 #define   TAIL_ADDR		0x001FFFF8
 #define   HEAD_WRAP_COUNT	0xFFE00000
 #define   HEAD_WRAP_ONE		0x00200000
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 8cc7083..fbe7757 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_WAIT	0x2c
 #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
 #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
+#define DRM_I915_REG_READ	0x30
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -249,6 +250,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
+#define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -918,4 +920,8 @@ struct drm_i915_gem_context_destroy {
 	__u32 pad;
 };
 
+struct drm_i915_reg_read {
+	__u64 offset;
+	__u64 val; /* Return value */
+};
 #endif				/* _I915_DRM_H_ */
-- 
1.7.11.1

[PATCH] reg_read: basic register read ioctl test

[Ben Widawsky]

This will need to get modified when the ioctl expands, and so is only
here for reference/to make Daniel happy.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/Makefile.am    |  1 +
 tests/drm_reg_read.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)
 create mode 100644 tests/drm_reg_read.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1f55912..4ee199c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -69,6 +69,7 @@ TESTS_progs = \
 	gem_ctx_exec \
 	gem_ctx_bad_exec \
 	gem_ctx_basic \
+	drm_reg_read \
 	$(NULL)
 
 # IMPORTANT: The ZZ_ tests need to be run last!
diff --git a/tests/drm_reg_read.c b/tests/drm_reg_read.c
new file mode 100644
index 0000000..ab491b5
--- /dev/null
+++ b/tests/drm_reg_read.c
@@ -0,0 +1,90 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "i915_drm.h"
+#include "drmtest.h"
+
+struct local_drm_i915_reg_read {
+	__u64 offset;
+	__u64 val; /* Return value */
+};
+
+
+#define REG_READ_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x30, struct local_drm_i915_reg_read)
+
+static void handle_bad(int ret, int lerrno, int expected, const char *desc)
+{
+	if (ret != 0 && lerrno != expected) {
+		fprintf(stderr, "%s - errno was %d, but should have been %d\n",
+				desc, lerrno, expected);
+		exit(EXIT_FAILURE);
+	} else if (ret == 0) {
+		fprintf(stderr, "%s - Command succeeded, but should have failed\n",
+			desc);
+		exit(EXIT_FAILURE);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct local_drm_i915_reg_read read;
+	int ret, fd;
+	__u64 val;
+
+	read.offset = 0x2358;
+
+	fd = drm_open_any();
+
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	if (ret) {
+		perror("positive test case failed\n");
+		exit(EXIT_FAILURE);
+	}
+	val = read.val;
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	if (ret) {
+		perror("positive test case 2 failed\n");
+		exit(EXIT_FAILURE);
+	}
+
+	if (val == read.val) {
+		fprintf(stderr, "Timer isn't moving, probably busted\n");
+		exit(EXIT_FAILURE);
+	}
+
+
+	/* bad reg */
+	read.offset = 0x12345678;
+	ret = drmIoctl(fd, REG_READ_IOCTL, &read);
+	handle_bad(ret, errno, EINVAL, "bad register");
+
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.11.1

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

Re: [PATCH] drm/i915: add register read IOCTL

[Eric Anholt]

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

Ben Widawsky <ben@bwidawsk.net> writes:

> The interface's immediate purpose is to do synchronous timestamp queries
> as required by GL_TIMESTAMP. The GPU has a register for reading the
> timestamp but because that would normally require root access, the
> IOCTL can provide this service.
>
> Currently the implementation whitelists only the render ring timestamp
> register, because that is the only thing we need to expose at this time.

Thanks. I was just writing this patch yesterday since it still hadn't
landed.  What I was doing was very similar, I was just not including a
size, since we're going to whitelist regs and the correct size is
implied by the register offset.

> +int i915_reg_read_ioctl(struct drm_device *dev,
> +			void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reg_read *reg = data;
> +
> +	/* Whitelisted for now */
> +	if (reg->offset != RING_TIMESTAMP(RENDER_RING_BASE))
> +		return -ENXIO;

Should this be conditional on the gen having the timestamp register?

> +struct drm_i915_reg_read {
> +	__u64 offset;
> +	__u32 size;
> +	__u64 val; /* Return value */
> +	__u32 pad;
> +};

Bad padding here.  On i386 you'll get a struct like:

{
        uint64_t offset
        uint32_t size
        uint32_t implicit_pad
        uint64_t val
        uint32_t pad
}

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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] drm/i915: add register read IOCTL

[Ben Widawsky]

On Thu, 12 Jul 2012 12:42:30 -0700
Eric Anholt <eric@anholt.net> wrote:

> Ben Widawsky <ben@bwidawsk.net> writes:
> 
> > The interface's immediate purpose is to do synchronous timestamp queries
> > as required by GL_TIMESTAMP. The GPU has a register for reading the
> > timestamp but because that would normally require root access, the
> > IOCTL can provide this service.
> >
> > Currently the implementation whitelists only the render ring timestamp
> > register, because that is the only thing we need to expose at this time.
> 
> Thanks. I was just writing this patch yesterday since it still hadn't
> landed.  What I was doing was very similar, I was just not including a
> size, since we're going to whitelist regs and the correct size is
> implied by the register offset.

Please check out:
1342116066-12164-1-git-send-email-ben@bwidawsk.net

We went through this all on IRC already ;-)

> 
> > +int i915_reg_read_ioctl(struct drm_device *dev,
> > +			void *data, struct drm_file *file)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_reg_read *reg = data;
> > +
> > +	/* Whitelisted for now */
> > +	if (reg->offset != RING_TIMESTAMP(RENDER_RING_BASE))
> > +		return -ENXIO;
> 
> Should this be conditional on the gen having the timestamp register?
> 
> > +struct drm_i915_reg_read {
> > +	__u64 offset;
> > +	__u32 size;
> > +	__u64 val; /* Return value */
> > +	__u32 pad;
> > +};
> 
> Bad padding here.  On i386 you'll get a struct like:
> 
> {
>         uint64_t offset
>         uint32_t size
>         uint32_t implicit_pad
>         uint64_t val
>         uint32_t pad
> }



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: add register read IOCTL

[Eric Anholt]

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

Ben Widawsky <ben@bwidawsk.net> writes:

> The interface's immediate purpose is to do synchronous timestamp queries
> as required by GL_TIMESTAMP. The GPU has a register for reading the
> timestamp but because that would normally require root access through
> libpciaccess, the IOCTL can provide this service instead.
>
> Currently the implementation whitelists only the render ring timestamp
> register, because that is the only thing we need to expose at this time.
>
> v2: make size implicit based on the register offset
> Add a generation check

> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 8cc7083..fbe7757 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_GEM_WAIT	0x2c
>  #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
>  #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
> +#define DRM_I915_REG_READ	0x30

Is 0x2f some other outstanding ioctl?

Other than that,

Reviewed-by: Eric Anholt <eric@anholt.net>

Note: we have requests both by Arjan and by Valve for the functionality
that this patch will allow.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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 v2] drm/i915: add register read IOCTL

[Ben Widawsky]

On Wed, 18 Jul 2012 10:14:46 -0700
Eric Anholt <eric@anholt.net> wrote:

> Ben Widawsky <ben@bwidawsk.net> writes:
> 
> > The interface's immediate purpose is to do synchronous timestamp queries
> > as required by GL_TIMESTAMP. The GPU has a register for reading the
> > timestamp but because that would normally require root access through
> > libpciaccess, the IOCTL can provide this service instead.
> >
> > Currently the implementation whitelists only the render ring timestamp
> > register, because that is the only thing we need to expose at this time.
> >
> > v2: make size implicit based on the register offset
> > Add a generation check
> 
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index 8cc7083..fbe7757 100644
> > --- a/include/drm/i915_drm.h
> > +++ b/include/drm/i915_drm.h
> > @@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
> >  #define DRM_I915_GEM_WAIT	0x2c
> >  #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
> >  #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
> > +#define DRM_I915_REG_READ	0x30
> 
> Is 0x2f some other outstanding ioctl?
>

I was saving it for some yet to be realized context ioctl. We can use
0x2f, I don't care. Daniel - feel free to change it or not as you
please when/if you suck it in.

> Other than that,
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> Note: we have requests both by Arjan and by Valve for the functionality
> that this patch will allow.



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: add register read IOCTL

[Daniel Vetter]

On Wed, Jul 18, 2012 at 10:22:01AM -0700, Ben Widawsky wrote:
> On Wed, 18 Jul 2012 10:14:46 -0700
> Eric Anholt <eric@anholt.net> wrote:
> 
> > Ben Widawsky <ben@bwidawsk.net> writes:
> > 
> > > The interface's immediate purpose is to do synchronous timestamp queries
> > > as required by GL_TIMESTAMP. The GPU has a register for reading the
> > > timestamp but because that would normally require root access through
> > > libpciaccess, the IOCTL can provide this service instead.
> > >
> > > Currently the implementation whitelists only the render ring timestamp
> > > register, because that is the only thing we need to expose at this time.
> > >
> > > v2: make size implicit based on the register offset
> > > Add a generation check
> > 
> > > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > > index 8cc7083..fbe7757 100644
> > > --- a/include/drm/i915_drm.h
> > > +++ b/include/drm/i915_drm.h
> > > @@ -203,6 +203,7 @@ typedef struct _drm_i915_sarea {
> > >  #define DRM_I915_GEM_WAIT	0x2c
> > >  #define DRM_I915_GEM_CONTEXT_CREATE	0x2d
> > >  #define DRM_I915_GEM_CONTEXT_DESTROY	0x2e
> > > +#define DRM_I915_REG_READ	0x30
> > 
> > Is 0x2f some other outstanding ioctl?
> >
> 
> I was saving it for some yet to be realized context ioctl. We can use
> 0x2f, I don't care. Daniel - feel free to change it or not as you
> please when/if you suck it in.

Patch queued for -next (with ioctl number 0x31, I've figure when I'll
change it I might as well avoid conflicts with the set_cacheing stuff).
Can you please adjust the i-g-t test and commit that one, too?

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48