[PATCH] drm/i915: read/write IOCTLs

[PATCH] drm/i915: read/write IOCTLs

[Ben Widawsky]

With the invention of forcewake (DevGT) , userspace tools to diagnose
problems are no longer reliable. This will provide userspace a mechanism
to read registers through an IOCTL, as well as root permission to write
to registers.

The code tries to be smart about which registers can be read and
written, as hardware behavior may become erratic if writing to reserved
ranges.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  129 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |    2 +
 include/drm/i915_drm.h          |   27 ++++++++
 3 files changed, 158 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..8b5165d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1846,6 +1846,133 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
 
+
+/* If we assume registers are dword aligned, second check is redundant */
+#define REG_IN_RANGE(reg, begin, end) \
+	((reg <= end && reg >= begin) || \
+	((reg+3) <= end && (reg+3) >= begin))
+
+/*
+ * This is here to avoid hanging the GPU from userspace.
+ * The ranges are defined the in "Register Address Map" section of the
+ * documents.
+ */
+static inline int
+reg_in_rsvd_range(struct drm_device *dev, u32 reg)
+{
+	if (INTEL_INFO(dev)->gen < 6) {
+		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
+		    ((IS_BROADWATER(dev) || IS_CRESTLINE(dev)) ?
+			REG_IN_RANGE(reg, 0x4000, 0x4fff) : 0) ||
+		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
+		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
+		    REG_IN_RANGE(reg, 0x14000, 0x2ffff) ||
+		    REG_IN_RANGE(reg, 0x40000, 0x5ffff))
+			return 1;
+
+		if (reg > 0x73fff)
+			return 1;
+	} else {
+		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
+		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
+		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
+		    REG_IN_RANGE(reg, 0x15000, 0x21fff) ||
+		    REG_IN_RANGE(reg, 0x23000, 0x23fff) ||
+		    REG_IN_RANGE(reg, 0x25000, 0x2ffff) ||
+		    REG_IN_RANGE(reg, 0x40000, 0xfffff) ||
+		    REG_IN_RANGE(reg, 0x108000, 0x13ffff))
+			return 1;
+
+		if (reg > 0x17ffff)
+			return 1;
+	}
+
+	return 0;
+}
+
+
+#define REG_IN_READ_BLACKLIST(reg) 0
+#define REG_IN_WRITE_BLACKLIST(reg) 0
+
+static inline int
+reg_read_allowed(struct drm_device *dev, u32 offset)
+{
+	if (offset & 0x3)
+		return 0;
+
+	if (reg_in_rsvd_range(dev, offset))
+		return 0;
+
+	if (REG_IN_READ_BLACKLIST(offset))
+		return 0;
+
+	return 1;
+}
+
+static inline int
+reg_write_allowed(struct drm_device *dev, u32 offset)
+{
+	if (offset & 0x3)
+		return 0;
+
+	if (reg_in_rsvd_range(dev, offset))
+		return 0;
+
+	if (REG_IN_WRITE_BLACKLIST(offset))
+		return 0;
+
+	return 1;
+}
+
+static int
+i915_read_register_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv)
+{
+	struct drm_intel_write_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!reg_read_allowed(dev, args->offset)) {
+		args->value = 0xffffffff;
+		return -EINVAL;
+	}
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		DRM_DEBUG_DRIVER("Couldn't acquire mutex, reading anyway\n");
+
+	args->value = (u32)i915_gt_read(dev_priv, args->offset);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static int
+i915_write_register_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct drm_intel_read_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!reg_write_allowed(dev, args->offset))
+		return -EINVAL;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
+
+	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
+	dev_priv->user_tainted = true;
+
+	i915_gt_write(dev_priv, args->offset, (u32)args->value);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 /**
  * Tells the intel_ips driver that the i915 driver is now loaded, if
  * IPS got loaded first.
@@ -2276,6 +2403,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..4c66734 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,8 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	bool user_tainted;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index c4d6dbf..68eeaa3 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
 #define DRM_I915_OVERLAY_ATTRS	0x28
 #define DRM_I915_GEM_EXECBUFFER2	0x29
+#define DRM_I915_READ_REGISTER	0x2a
+#define DRM_I915_WRITE_REGISTER	0x2b
 
 #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)
@@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
 #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
+#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
+#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -844,4 +848,27 @@ struct drm_intel_overlay_attrs {
 	__u32 gamma5;
 };
 
+struct drm_intel_read_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size, RFU */
+	__u8 size;
+
+	/* return value, high 4 bytes RFU */
+	__u64 value;
+};
+
+struct drm_intel_write_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size,  RFU */
+	__u8 size;
+
+	/* value to write, high 4 bytes RFU */
+	__u64 value;
+
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.3.4

[PATCH v2] drm/i915: read/write IOCTLs

[Ben Widawsky]

With the invention of forcewake (DevGT) , userspace tools to diagnose
problems are no longer reliable. This will provide userspace a mechanism
to read registers through an IOCTL, as well as root permission to write
to registers.

The code tries to be smart about which registers can be read and
written, as hardware behavior may become erratic if writing to reserved
ranges.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  130 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |    2 +
 include/drm/i915_drm.h          |   27 ++++++++
 3 files changed, 159 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..a96602e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1846,6 +1846,134 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
 
+
+/* If we assume registers are dword aligned, second check is redundant */
+#define REG_IN_RANGE(reg, begin, end) \
+	((reg <= end && reg >= begin) || \
+	((reg+3) <= end && (reg+3) >= begin))
+
+/*
+ * This is here to avoid hanging the GPU from userspace.
+ * The ranges are defined the in "Register Address Map" section of the
+ * documents.
+ */
+static inline int
+reg_in_rsvd_range(struct drm_device *dev, u32 reg)
+{
+	if (INTEL_INFO(dev)->gen < 6) {
+		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
+		    ((IS_BROADWATER(dev) || IS_CRESTLINE(dev)) ?
+			REG_IN_RANGE(reg, 0x4000, 0x4fff) : 0) ||
+		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
+		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
+		    REG_IN_RANGE(reg, 0x14000, 0x2ffff) ||
+		    REG_IN_RANGE(reg, 0x40000, 0x5ffff))
+			return 1;
+
+		if (reg > 0x73fff)
+			return 1;
+	} else {
+		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
+		    REG_IN_RANGE(reg, 0x8800, 0x88ff) ||
+		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
+		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
+		    REG_IN_RANGE(reg, 0x15000, 0x21fff) ||
+		    REG_IN_RANGE(reg, 0x23000, 0x23fff) ||
+		    REG_IN_RANGE(reg, 0x25000, 0x2ffff) ||
+		    REG_IN_RANGE(reg, 0x40000, 0xfffff) ||
+		    REG_IN_RANGE(reg, 0x108000, 0x13ffff))
+			return 1;
+
+		if (reg > 0x17ffff)
+			return 1;
+	}
+
+	return 0;
+}
+
+
+#define REG_IN_READ_BLACKLIST(reg) 0
+#define REG_IN_WRITE_BLACKLIST(reg) 0
+
+static inline int
+reg_read_allowed(struct drm_device *dev, u32 offset)
+{
+	if (offset & 0x3)
+		return 0;
+
+	if (reg_in_rsvd_range(dev, offset))
+		return 0;
+
+	if (REG_IN_READ_BLACKLIST(offset))
+		return 0;
+
+	return 1;
+}
+
+static inline int
+reg_write_allowed(struct drm_device *dev, u32 offset)
+{
+	if (offset & 0x3)
+		return 0;
+
+	if (reg_in_rsvd_range(dev, offset))
+		return 0;
+
+	if (REG_IN_WRITE_BLACKLIST(offset))
+		return 0;
+
+	return 1;
+}
+
+static int
+i915_read_register_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv)
+{
+	struct drm_intel_write_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!reg_read_allowed(dev, args->offset)) {
+		args->value = 0xffffffff;
+		return -EINVAL;
+	}
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		DRM_DEBUG_DRIVER("Couldn't acquire mutex, reading anyway\n");
+
+	args->value = (u32)i915_gt_read(dev_priv, args->offset);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static int
+i915_write_register_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct drm_intel_read_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!reg_write_allowed(dev, args->offset))
+		return -EINVAL;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
+
+	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
+	dev_priv->user_tainted = true;
+
+	i915_gt_write(dev_priv, args->offset, (u32)args->value);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 /**
  * Tells the intel_ips driver that the i915 driver is now loaded, if
  * IPS got loaded first.
@@ -2276,6 +2404,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..4c66734 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,8 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	bool user_tainted;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index c4d6dbf..68eeaa3 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
 #define DRM_I915_OVERLAY_ATTRS	0x28
 #define DRM_I915_GEM_EXECBUFFER2	0x29
+#define DRM_I915_READ_REGISTER	0x2a
+#define DRM_I915_WRITE_REGISTER	0x2b
 
 #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)
@@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
 #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
+#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
+#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -844,4 +848,27 @@ struct drm_intel_overlay_attrs {
 	__u32 gamma5;
 };
 
+struct drm_intel_read_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size, RFU */
+	__u8 size;
+
+	/* return value, high 4 bytes RFU */
+	__u64 value;
+};
+
+struct drm_intel_write_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size,  RFU */
+	__u8 size;
+
+	/* value to write, high 4 bytes RFU */
+	__u64 value;
+
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.3.4

Read/Write IOCTL compromise

[Ben Widawsky]

This patch does the 1 register read/write at a time, with a rsvd field for more
ambitious stuff later.

In locking, it does the safe mutex_lock/unlock. We can always
modify this later if we see a need.

So if this isn't okay by everyone, let's get down to the minimum number
of changes required to get this accepted so I can move on to the tools,
and then get back to the debug stuff.

Ben

No more read/write ioctls

[Ben Widawsky]

On Fri, Apr 01, 2011 at 04:54:47PM -0700, Ben Widawsky wrote:

> So if this isn't okay by everyone, let's get down to the minimum number
> of changes required to get this accepted so I can move on to the tools,
> and then get back to the debug stuff.
> 

Some discussion on IRC has led to a new proposal (well 2 new proposals,
but the first one wasn't viable).

The interface will instead of using ioctls use debugfs. The debugfs file
will control force wake. There will be a refcount mechanism for number
of users of the registers in the relevant power-well, and upon opening a
specific file in debugfs (we could have one per power-well if needed),
the refcount will get incremented, and decremented at close.

In other words, for userspace to read/write registers:
fd = open(/sys/kernel/debug/dri...)
normal read write mechanism
close(fd)

There are two side effects which everyone on IRC seems fine with:
* root-only read access (the ioctl read was promiscuous)
* access is only available when debugfs is mounted

As a result, you should ignore both the gpu-tools patches, as well as
these kernel patches.

Thanks.
Ben

Re: No more read/write ioctls

[Eric Anholt]

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

On Wed, 6 Apr 2011 14:38:27 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Apr 01, 2011 at 04:54:47PM -0700, Ben Widawsky wrote:
> 
> > So if this isn't okay by everyone, let's get down to the minimum number
> > of changes required to get this accepted so I can move on to the tools,
> > and then get back to the debug stuff.
> > 
> 
> Some discussion on IRC has led to a new proposal (well 2 new proposals,
> but the first one wasn't viable).
> 
> The interface will instead of using ioctls use debugfs. The debugfs file
> will control force wake. There will be a refcount mechanism for number
> of users of the registers in the relevant power-well, and upon opening a
> specific file in debugfs (we could have one per power-well if needed),
> the refcount will get incremented, and decremented at close.
> 
> In other words, for userspace to read/write registers:
> fd = open(/sys/kernel/debug/dri...)
> normal read write mechanism
> close(fd)
> 
> There are two side effects which everyone on IRC seems fine with:
> * root-only read access (the ioctl read was promiscuous)
> * access is only available when debugfs is mounted
> 
> As a result, you should ignore both the gpu-tools patches, as well as
> these kernel patches.

I thought previously we had issues with debugfs nodes which could wedge
the system (out of range mmio reads)?

[-- 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: No more read/write ioctls

[Chris Wilson]

On Fri, 08 Apr 2011 07:10:14 -1000, Eric Anholt <eric@anholt.net> wrote:
> I thought previously we had issues with debugfs nodes which could wedge
> the system (out of range mmio reads)?

Yes, we have indeed triggered hard hards by trying to read the wrong
registers before. That's why I was interested in seeing Ben codify the
reserved ranges so that we can have at least a warning before doing
something idiotic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 1/2] drm/i915: read/write ioctls for userspace

[Ben Widawsky]

Straight forward IOCTLs which allow userspace to read and write
registers. This is needed because on newer generation products,
registers may become inaccessible due to specific power modes.

To manage this, created a set of register ranges for the different
products which can give fine grained control over what we permit
userspace to read and write. This list was created by hand, so expect
issues/bugs.

The fields in the calls allow register widths to be defined, although
currently this is not used by any products. There is also a rsvd field
which can be used for giving a list of register reads/writes to perform
in the future.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  189 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |   21 +++++
 include/drm/i915_drm.h          |   30 ++++++
 3 files changed, 240 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..30cd576 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1846,6 +1846,174 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
 
+static struct i915_register_range *
+get_register_range(struct drm_device *dev, u32 offset, int mode)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_register_range *range = dev_priv->register_map.map;
+	u8 align = dev_priv->register_map.alignment_mask;
+	u32 range_count = dev_priv->register_map.length;
+
+	if (offset & dev_priv->register_map.alignment_mask)
+		return NULL;
+
+	if (offset >= dev_priv->register_map.top)
+		return NULL;
+
+	while(range_count--) {
+		/*  list is assumed to be in order */
+		if (offset < range->base)
+			break;
+
+		if ( (offset >= range->base) &&
+		     (offset + align) <= (range->base + range->size)) {
+			/*  assume perms ascend in security (ie. rw is > ro) */
+			if (mode > range->flags)
+				return NULL;
+			else
+				return range;
+		}
+		range++;
+	}
+
+	return NULL;
+}
+
+static int
+i915_read_register_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv)
+{
+	struct drm_intel_write_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (args->rsvd != 0)
+		DRM_DEBUG("rsvd field should be zero\n");
+
+	if (get_register_range(dev, args->offset, I915_RANGE_RO) == NULL) {
+		args->value = 0xffffffff;
+		return -EINVAL;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+	args->value = (u32)i915_gt_read(dev_priv, args->offset);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static int
+i915_write_register_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct drm_intel_read_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_register_range *range;
+
+	if (args->rsvd != 0)
+		DRM_DEBUG("rsvd field should be zero\n");
+
+	range = get_register_range(dev, args->offset, I915_RANGE_RW);
+	if (!range)
+		return -EINVAL;
+
+	mutex_lock(&dev->struct_mutex);
+	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
+	range->user_tainted = true;
+	i915_gt_write(dev_priv, args->offset, (u32)args->value);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+struct i915_register_range gen_bwcl_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x000003ff, I915_RANGE_RSVD},
+	{0x00004400, 0x00000bff, I915_RANGE_RSVD},
+	{0x00005000, 0x00000fff, I915_RANGE_RW},
+	{0x00006000, 0x00000fff, I915_RANGE_RW},
+	{0x00007000, 0x000003ff, I915_RANGE_RW},
+	{0x00007400, 0x000014ff, I915_RANGE_RW},
+	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00003fff, I915_RANGE_RW},
+	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
+};
+
+struct i915_register_range genx_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x000003ff, I915_RANGE_RW},
+	{0x00004400, 0x00000bff, I915_RANGE_RW},
+	{0x00005000, 0x00000fff, I915_RANGE_RW},
+	{0x00006000, 0x00000fff, I915_RANGE_RW},
+	{0x00007000, 0x000003ff, I915_RANGE_RW},
+	{0x00007400, 0x000014ff, I915_RANGE_RW},
+	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00003fff, I915_RANGE_RW},
+	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
+};
+
+struct i915_register_range gen6_gt_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x00000fff, I915_RANGE_RW},
+	{0x00005000, 0x0000017f, I915_RANGE_RW},
+	{0x00005180, 0x00000e7f, I915_RANGE_RW},
+	{0x00006000, 0x00001fff, I915_RANGE_RW},
+	{0x00008000, 0x000007ff, I915_RANGE_RW},
+	{0x00008800, 0x000000ff, I915_RANGE_RSVD},
+	{0x00008900, 0x000006ff, I915_RANGE_RW},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00001fff, I915_RANGE_RW},
+	{0x00012000, 0x000003ff, I915_RANGE_RW},
+	{0x00012400, 0x00000bff, I915_RANGE_RW},
+	{0x00013000, 0x00000fff, I915_RANGE_RW},
+	{0x00014000, 0x00000fff, I915_RANGE_RW},
+	{0x00015000, 0x0000cfff, I915_RANGE_RW},
+	{0x00022000, 0x00000fff, I915_RANGE_RW},
+	{0x00023000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00024000, 0x00000fff, I915_RANGE_RW},
+	{0x00025000, 0x0000afff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0008bfff, I915_RANGE_RSVD},
+	{0x00100000, 0x00007fff, I915_RANGE_RW},
+	{0x00108000, 0x00037fff, I915_RANGE_RSVD},
+	{0x00140000, 0x0003ffff, I915_RANGE_RW}
+};
+
 /**
  * Tells the intel_ips driver that the i915 driver is now loaded, if
  * IPS got loaded first.
@@ -2062,6 +2230,25 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ips_ping_for_i915_load();
 
+	if (IS_GEN6(dev)) {
+		dev_priv->register_map.map = gen6_gt_register_map;
+		dev_priv->register_map.length =
+			ARRAY_SIZE(gen6_gt_register_map);
+		dev_priv->register_map.top = 0x180000;
+	} else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev)) {
+		dev_priv->register_map.map = gen_bwcl_register_map;
+		dev_priv->register_map.length =
+			ARRAY_SIZE(gen_bwcl_register_map);
+		dev_priv->register_map.top = 0x80000;
+	} else {
+		dev_priv->register_map.map = genx_register_map;
+		dev_priv->register_map.length =
+			ARRAY_SIZE(genx_register_map);
+		dev_priv->register_map.top = 0x80000;
+	}
+
+	dev_priv->register_map.alignment_mask = 0x3;
+
 	return 0;
 
 out_gem_unload:
@@ -2276,6 +2463,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..355f008 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,14 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	/* User visible register map */
+	struct {
+		struct i915_register_range *map;
+		u32 length;
+		u32 top;
+		u8 alignment_mask;
+	} register_map;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
@@ -1385,4 +1393,17 @@ static inline void i915_gt_write(struct drm_i915_private *dev_priv,
 		__gen6_gt_wait_for_fifo(dev_priv);
 	I915_WRITE(reg, val);
 }
+
+/* Register range interface */
+struct i915_register_range {
+	u32 base;
+	u32 size;
+	u8 flags;
+	bool user_tainted;
+};
+
+#define I915_RANGE_RSVD	(0<<0)
+#define I915_RANGE_RO	(1<<0)
+#define I915_RANGE_RW	(1<<1)
+
 #endif
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index c4d6dbf..9398cb2 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
 #define DRM_I915_OVERLAY_ATTRS	0x28
 #define DRM_I915_GEM_EXECBUFFER2	0x29
+#define DRM_I915_READ_REGISTER	0x2a
+#define DRM_I915_WRITE_REGISTER	0x2b
 
 #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)
@@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
 #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
+#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
+#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -844,4 +848,30 @@ struct drm_intel_overlay_attrs {
 	__u32 gamma5;
 };
 
+struct drm_intel_read_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size, RFU */
+	__u8 size;
+
+	/* return value, high 4 bytes RFU */
+	__u64 value;
+
+	__u64 rsvd;
+};
+
+struct drm_intel_write_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size,  RFU */
+	__u8 size;
+
+	/* value to write, high 4 bytes RFU */
+	__u64 value;
+
+	__u64 rsvd;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.3.4

Re: [PATCH v3 1/2] drm/i915: read/write ioctls for userspace

[Chris Wilson]

Comments inline.

On Fri,  1 Apr 2011 16:54:48 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Straight forward IOCTLs which allow userspace to read and write
> registers. This is needed because on newer generation products,
> registers may become inaccessible due to specific power modes.
> 
> To manage this, created a set of register ranges for the different
> products which can give fine grained control over what we permit
> userspace to read and write. This list was created by hand, so expect
> issues/bugs.
> 
> The fields in the calls allow register widths to be defined, although
> currently this is not used by any products. There is also a rsvd field
> which can be used for giving a list of register reads/writes to perform
> in the future.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  189 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |   21 +++++
>  include/drm/i915_drm.h          |   30 ++++++
>  3 files changed, 240 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..30cd576 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1846,6 +1846,174 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>  
> +static struct i915_register_range *
> +get_register_range(struct drm_device *dev, u32 offset, int mode)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_register_range *range = dev_priv->register_map.map;
> +	u8 align = dev_priv->register_map.alignment_mask;
> +	u32 range_count = dev_priv->register_map.length;
> +
> +	if (offset & dev_priv->register_map.alignment_mask)
> +		return NULL;
> +
> +	if (offset >= dev_priv->register_map.top)
> +		return NULL;
> +
> +	while(range_count--) {
Missingwhitespace.;-)
Rather than range_count/dev_priv->register_map.length, I would prefer a
sentinel.

> +		/*  list is assumed to be in order */
> +		if (offset < range->base)
> +			break;
> +
> +		if ( (offset >= range->base) &&
> +		     (offset + align) <= (range->base + range->size)) {
> +			/*  assume perms ascend in security (ie. rw is > ro) */
> +			if (mode > range->flags)
> +				return NULL;
> +			else
> +				return range;
> +		}
> +		range++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int
> +i915_read_register_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_priv)
> +{
> +	struct drm_intel_write_reg *args = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (args->rsvd != 0)
> +		DRM_DEBUG("rsvd field should be zero\n");

No, don't even mention it, just ignore it.

> +
> +	if (get_register_range(dev, args->offset, I915_RANGE_RO) == NULL) {
> +		args->value = 0xffffffff;
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);

Will be happier with i915_mutex_lock_interruptible() just in case.

> +	args->value = (u32)i915_gt_read(dev_priv, args->offset);
And we need to start doing
intel_register_read_get(dev, args->offset);
switch (args->size) {
switch 8: args->value = ioread8(regs+args->offset); break;
...
}
intel_register_read_put(dev, args->offset);

where intel_register_read_get() looks something like

int intel_register_read_get(struct drm_device *dev,
                            int offset)
{
	switch (INTEL_INFO(dev)->gen) {
	case 6:
		if (offset < 0x40000)
			__gen6_gt_force_wake_get(dev);
		return 0;

	case 5:
	case 4:
 	case 3:
		return 0;
	}
}

> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
> +static int
> +i915_write_register_ioctl(struct drm_device *dev, void *data,
> +			  struct drm_file *file_priv)
> +{
> +	struct drm_intel_read_reg *args = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_register_range *range;
> +
> +	if (args->rsvd != 0)
> +		DRM_DEBUG("rsvd field should be zero\n");
> +
> +	range = get_register_range(dev, args->offset, I915_RANGE_RW);
> +	if (!range)
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> +	range->user_tainted = true;
> +	i915_gt_write(dev_priv, args->offset, (u32)args->value);
ret = intel_register_write_get()
if (ret == 0) {
	switch (args->size) {
		case 8: iowrite8(args->value, regs+args->offset); break;
	}
	intel_register_write_put()
}
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
> +struct i915_register_range gen_bwcl_register_map[] = {
static const struct i915_register_range ...

Keep sparse quiet.

> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x000003ff, I915_RANGE_RSVD},
> +	{0x00004400, 0x00000bff, I915_RANGE_RSVD},
> +	{0x00005000, 0x00000fff, I915_RANGE_RW},
> +	{0x00006000, 0x00000fff, I915_RANGE_RW},
> +	{0x00007000, 0x000003ff, I915_RANGE_RW},
> +	{0x00007400, 0x000014ff, I915_RANGE_RW},
> +	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00003fff, I915_RANGE_RW},
> +	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range genx_register_map[] = {
> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x000003ff, I915_RANGE_RW},
> +	{0x00004400, 0x00000bff, I915_RANGE_RW},
> +	{0x00005000, 0x00000fff, I915_RANGE_RW},
> +	{0x00006000, 0x00000fff, I915_RANGE_RW},
> +	{0x00007000, 0x000003ff, I915_RANGE_RW},
> +	{0x00007400, 0x000014ff, I915_RANGE_RW},
> +	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00003fff, I915_RANGE_RW},
> +	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range gen6_gt_register_map[] = {
> +	{0x00000000, 0x00000fff, I915_RANGE_RW},
> +	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00002000, 0x00000fff, I915_RANGE_RW},
> +	{0x00003000, 0x000001ff, I915_RANGE_RW},
> +	{0x00003200, 0x00000dff, I915_RANGE_RW},
> +	{0x00004000, 0x00000fff, I915_RANGE_RW},
> +	{0x00005000, 0x0000017f, I915_RANGE_RW},
> +	{0x00005180, 0x00000e7f, I915_RANGE_RW},
> +	{0x00006000, 0x00001fff, I915_RANGE_RW},
> +	{0x00008000, 0x000007ff, I915_RANGE_RW},
> +	{0x00008800, 0x000000ff, I915_RANGE_RSVD},
> +	{0x00008900, 0x000006ff, I915_RANGE_RW},
> +	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x0000a000, 0x00000fff, I915_RANGE_RW},
> +	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> +	{0x00010000, 0x00001fff, I915_RANGE_RW},
> +	{0x00012000, 0x000003ff, I915_RANGE_RW},
> +	{0x00012400, 0x00000bff, I915_RANGE_RW},
> +	{0x00013000, 0x00000fff, I915_RANGE_RW},
> +	{0x00014000, 0x00000fff, I915_RANGE_RW},
> +	{0x00015000, 0x0000cfff, I915_RANGE_RW},
> +	{0x00022000, 0x00000fff, I915_RANGE_RW},
> +	{0x00023000, 0x00000fff, I915_RANGE_RSVD},
> +	{0x00024000, 0x00000fff, I915_RANGE_RW},
> +	{0x00025000, 0x0000afff, I915_RANGE_RSVD},
> +	{0x00030000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> +	{0x00060000, 0x0000ffff, I915_RANGE_RW},
> +	{0x00070000, 0x00002fff, I915_RANGE_RW},
> +	{0x00073000, 0x00000fff, I915_RANGE_RW},
> +	{0x00074000, 0x0008bfff, I915_RANGE_RSVD},
> +	{0x00100000, 0x00007fff, I915_RANGE_RW},
> +	{0x00108000, 0x00037fff, I915_RANGE_RSVD},
> +	{0x00140000, 0x0003ffff, I915_RANGE_RW}
> +};
> +
>  /**
>   * Tells the intel_ips driver that the i915 driver is now loaded, if
>   * IPS got loaded first.
> @@ -2062,6 +2230,25 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ips_ping_for_i915_load();
>  
> +	if (IS_GEN6(dev)) {
> +		dev_priv->register_map.map = gen6_gt_register_map;
> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(gen6_gt_register_map);
> +		dev_priv->register_map.top = 0x180000;
> +	} else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev)) {
> +		dev_priv->register_map.map = gen_bwcl_register_map;
> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(gen_bwcl_register_map);
> +		dev_priv->register_map.top = 0x80000;
> +	} else {
> +		dev_priv->register_map.map = genx_register_map;

Wow, didn't expect the gen5 register map to match gen2.

> +		dev_priv->register_map.length =
> +			ARRAY_SIZE(genx_register_map);
> +		dev_priv->register_map.top = 0x80000;
> +	}
> +
> +	dev_priv->register_map.alignment_mask = 0x3;
> +
>  	return 0;
>  
>  out_gem_unload:
> @@ -2276,6 +2463,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..355f008 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -703,6 +703,14 @@ typedef struct drm_i915_private {
>  	struct intel_fbdev *fbdev;
>  
>  	struct drm_property *broadcast_rgb_property;
> +
> +	/* User visible register map */
> +	struct {
> +		struct i915_register_range *map;
> +		u32 length;
As noted we can remove this. I hope.

> +		u32 top;
> +		u8 alignment_mask;
> +	} register_map;
>  } drm_i915_private_t;
>  
>  struct drm_i915_gem_object {
> @@ -1385,4 +1393,17 @@ static inline void i915_gt_write(struct drm_i915_private *dev_priv,
>  		__gen6_gt_wait_for_fifo(dev_priv);
>  	I915_WRITE(reg, val);
>  }
> +
> +/* Register range interface */
> +struct i915_register_range {
> +	u32 base;
> +	u32 size;
> +	u8 flags;
> +	bool user_tainted;
Save a puppy, merge this into flags.

> +};
> +
> +#define I915_RANGE_RSVD	(0<<0)
> +#define I915_RANGE_RO	(1<<0)
> +#define I915_RANGE_RW	(1<<1)
Mind renaming this as I915_RANGE_READ, I915_RANGE_WRITE and use them as
distinct permission bits and then I915_RANGE_RW is (I915_RANGE_READ |
I915_RANGE_WRITE). Yes, there are the occasional write-only registers out
there.

> +
>  #endif
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index c4d6dbf..9398cb2 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
>  #define DRM_I915_OVERLAY_ATTRS	0x28
>  #define DRM_I915_GEM_EXECBUFFER2	0x29
> +#define DRM_I915_READ_REGISTER	0x2a
> +#define DRM_I915_WRITE_REGISTER	0x2b
>  
>  #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)
> @@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
>  #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
>  #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> +#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
> +#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -844,4 +848,30 @@ struct drm_intel_overlay_attrs {
>  	__u32 gamma5;
>  };
>  
> +struct drm_intel_read_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size, RFU */
> +	__u8 size;

Make this a _u32 for alignment's sake. Leave a note saying that are a
spare 26 bits if you want ;-)

> +
> +	/* return value, high 4 bytes RFU */
> +	__u64 value;
> +
> +	__u64 rsvd;
> +};
> +
> +struct drm_intel_write_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size,  RFU */
> +	__u8 size;
> +
> +	/* value to write, high 4 bytes RFU */
> +	__u64 value;
> +
> +	__u64 rsvd;
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre

Read/Write ioctls

[Ben Widawsky]

Went back to 1 patch instead of 2...

Two things from your comments not addressed (which I think should be
addressed separately):

* < gen4 register maps (userspace tools will fall back to old method)

* intel_register_write_get - I think the whole register read/write
situation needs cleanup, and I'd like to look at those changes in
isolation from this change.

Ben

[PATCH v4] drm/i915: read/write ioctls for userspace

[Ben Widawsky]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 15453 bytes --]

Straight forward IOCTLs which allow userspace to read and write
registers. This is needed because on newer generation products,
registers may become inaccessible due to specific power modes.

To manage this, created a set of register ranges for the different
products which can give fine grained control over what we permit
userspace to read and write. This list was created by hand, so expect
issues/bugs.

The fields in the calls allow register widths to be defined, although
currently this is not used by any products. There is also a rsvd field
which can be used for giving a list of register reads/writes to perform
in the future.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/Makefile        |    1 +
 drivers/gpu/drm/i915/i915_debugfs.c  |   19 +++++
 drivers/gpu/drm/i915/i915_dma.c      |   87 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h      |   25 ++++++
 drivers/gpu/drm/i915/i915_reg_maps.c |  143 ++++++++++++++++++++++++++++++++++
 include/drm/i915_drm.h               |   30 +++++++
 6 files changed, 305 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_reg_maps.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0ae6a7c..6544076 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -13,6 +13,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o i915_mem.o \
 	  i915_gem_gtt.o \
 	  i915_gem_tiling.o \
 	  i915_trace_points.o \
+	  i915_reg_maps.o \
 	  intel_display.o \
 	  intel_crt.o \
 	  intel_lvds.o \
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 87c8e29..81f49d0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1186,6 +1186,24 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_user_tainted_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct i915_register_range *range = dev_priv->register_map.map;
+
+	while (!(range->flags & I915_RANGE_END)) {
+		if (range->flags & I915_RANGE_TAINTED) {
+			seq_printf(m, "tainted register range 0x%08x->0x%08x\n",
+				   range->base, range->size);
+		}
+		range++;
+	}
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1324,6 +1342,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_user_tainted", i915_user_tainted_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..cce61a8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1846,6 +1846,89 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
 
+static struct i915_register_range *
+get_register_range(struct drm_device *dev, u32 offset, int mode)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_register_range *range = dev_priv->register_map.map;
+	u8 align = dev_priv->register_map.alignment_mask;
+
+	if (offset & dev_priv->register_map.alignment_mask)
+		return NULL;
+
+	if (offset >= dev_priv->register_map.top)
+		return NULL;
+
+	while (!(range->flags & I915_RANGE_END)) {
+		/*  list is assumed to be in order */
+		if (offset < range->base)
+			break;
+
+		if ( (offset >= range->base) &&
+		     (offset + align) <= (range->base + range->size)) {
+			if ((mode & range->flags) == mode)
+				return range;
+		}
+		range++;
+	}
+
+	return NULL;
+}
+
+static int
+i915_read_register_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv)
+{
+	struct drm_intel_write_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (dev_priv->register_map.map == NULL)
+		return -ENOTSUPP;
+
+	if (get_register_range(dev, args->offset, I915_RANGE_READ) == NULL) {
+		args->value = 0xffffffff;
+		return -EINVAL;
+	}
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	args->value = (u32)i915_gt_read(dev_priv, args->offset);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static int
+i915_write_register_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct drm_intel_read_reg *args = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_register_range *range;
+	int ret;
+
+	if (dev_priv->register_map.map == NULL)
+		return -ENOTSUPP;
+
+	range = get_register_range(dev, args->offset, I915_RANGE_WRITE);
+	if (!range)
+		return -EINVAL;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
+	range->flags |= I915_RANGE_TAINTED;
+	i915_gt_write(dev_priv, args->offset, (u32)args->value);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 /**
  * Tells the intel_ips driver that the i915 driver is now loaded, if
  * IPS got loaded first.
@@ -2062,6 +2145,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ips_ping_for_i915_load();
 
+	i915_init_register_map(dev);
+
 	return 0;
 
 out_gem_unload:
@@ -2276,6 +2361,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..ab48a1b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,14 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	/* User visible register map */
+	struct {
+		struct i915_register_range *map;
+		u32 length;
+		u32 top;
+		u8 alignment_mask;
+	} register_map;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
@@ -1385,4 +1393,21 @@ static inline void i915_gt_write(struct drm_i915_private *dev_priv,
 		__gen6_gt_wait_for_fifo(dev_priv);
 	I915_WRITE(reg, val);
 }
+
+/* Register range interface */
+struct i915_register_range {
+	u32 base;
+	u32 size;
+	u32 flags; /* protected by struct_mutex */
+};
+
+#define I915_RANGE_RSVD		(0<<0) /* Shouldn't be read or written */
+#define I915_RANGE_READ		(1<<0)
+#define I915_RANGE_WRITE	(1<<1)
+#define I915_RANGE_RW		(I915_RANGE_READ | I915_RANGE_WRITE)
+#define I915_RANGE_TAINTED	(1<<30)
+#define I915_RANGE_END		(1<<31)
+
+extern int i915_init_register_map(struct drm_device *dev);
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_reg_maps.c b/drivers/gpu/drm/i915/i915_reg_maps.c
new file mode 100644
index 0000000..21f008d
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_reg_maps.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright © 2011 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 "i915_drv.h"
+
+static struct i915_register_range gen_bwcl_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x000003ff, I915_RANGE_RSVD},
+	{0x00004400, 0x00000bff, I915_RANGE_RSVD},
+	{0x00005000, 0x00000fff, I915_RANGE_RW},
+	{0x00006000, 0x00000fff, I915_RANGE_RW},
+	{0x00007000, 0x000003ff, I915_RANGE_RW},
+	{0x00007400, 0x000014ff, I915_RANGE_RW},
+	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00003fff, I915_RANGE_RW},
+	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0000bfff, I915_RANGE_RSVD},
+	{0x00000000, 0x00000000, I915_RANGE_END}
+};
+
+static struct i915_register_range gen4_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x000003ff, I915_RANGE_RW},
+	{0x00004400, 0x00000bff, I915_RANGE_RW},
+	{0x00005000, 0x00000fff, I915_RANGE_RW},
+	{0x00006000, 0x00000fff, I915_RANGE_RW},
+	{0x00007000, 0x000003ff, I915_RANGE_RW},
+	{0x00007400, 0x000014ff, I915_RANGE_RW},
+	{0x00008900, 0x000006ff, I915_RANGE_RSVD},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00003fff, I915_RANGE_RW},
+	{0x00014000, 0x0001bfff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0000bfff, I915_RANGE_RSVD},
+	{0x00000000, 0x00000000, I915_RANGE_END}
+};
+
+/* The documentation is a little sketchy on these register ranges. */
+static struct i915_register_range gen6_gt_register_map[] = {
+	{0x00000000, 0x00000fff, I915_RANGE_RW},
+	{0x00001000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00002000, 0x00000fff, I915_RANGE_RW},
+	{0x00003000, 0x000001ff, I915_RANGE_RW},
+	{0x00003200, 0x00000dff, I915_RANGE_RW},
+	{0x00004000, 0x00000fff, I915_RANGE_RW},
+	{0x00005000, 0x0000017f, I915_RANGE_RW},
+	{0x00005180, 0x00000e7f, I915_RANGE_RW},
+	{0x00006000, 0x00001fff, I915_RANGE_RW},
+	{0x00008000, 0x000007ff, I915_RANGE_RW},
+	{0x00008800, 0x000000ff, I915_RANGE_RSVD},
+	{0x00008900, 0x000006ff, I915_RANGE_RW},
+	{0x00009000, 0x00000fff, I915_RANGE_RSVD},
+	{0x0000a000, 0x00000fff, I915_RANGE_RW},
+	{0x0000b000, 0x00004fff, I915_RANGE_RSVD},
+	{0x00010000, 0x00001fff, I915_RANGE_RW},
+	{0x00012000, 0x000003ff, I915_RANGE_RW},
+	{0x00012400, 0x00000bff, I915_RANGE_RW},
+	{0x00013000, 0x00000fff, I915_RANGE_RW},
+	{0x00014000, 0x00000fff, I915_RANGE_RW},
+	{0x00015000, 0x0000cfff, I915_RANGE_RW},
+	{0x00022000, 0x00000fff, I915_RANGE_RW},
+	{0x00023000, 0x00000fff, I915_RANGE_RSVD},
+	{0x00024000, 0x00000fff, I915_RANGE_RW},
+	{0x00025000, 0x0000afff, I915_RANGE_RSVD},
+	{0x00030000, 0x0000ffff, I915_RANGE_RW},
+	{0x00040000, 0x0001ffff, I915_RANGE_RSVD},
+	{0x00060000, 0x0000ffff, I915_RANGE_RW},
+	{0x00070000, 0x00002fff, I915_RANGE_RW},
+	{0x00073000, 0x00000fff, I915_RANGE_RW},
+	{0x00074000, 0x0008bfff, I915_RANGE_RSVD},
+	{0x00100000, 0x00007fff, I915_RANGE_RW},
+	{0x00108000, 0x00037fff, I915_RANGE_RSVD},
+	{0x00140000, 0x0003ffff, I915_RANGE_RW},
+	{0x00000000, 0x00000000, I915_RANGE_END}
+};
+
+int i915_init_register_map(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_GEN6(dev)) {
+		dev_priv->register_map.map = gen6_gt_register_map;
+		dev_priv->register_map.top = 0x180000;
+	} else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev)) {
+		dev_priv->register_map.map = gen_bwcl_register_map;
+		dev_priv->register_map.top = 0x80000;
+	} else if (INTEL_INFO(dev)->gen >= 4) {
+		dev_priv->register_map.map = gen4_register_map;
+		dev_priv->register_map.top = 0x80000;
+	} else {
+		dev_priv->register_map.map = NULL;;
+	}
+
+	dev_priv->register_map.alignment_mask = 0x3;
+
+	return 0;
+}
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index c4d6dbf..ff136af 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
 #define DRM_I915_OVERLAY_ATTRS	0x28
 #define DRM_I915_GEM_EXECBUFFER2	0x29
+#define DRM_I915_READ_REGISTER	0x2a
+#define DRM_I915_WRITE_REGISTER	0x2b
 
 #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)
@@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
 #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
+#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
+#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -844,4 +848,30 @@ struct drm_intel_overlay_attrs {
 	__u32 gamma5;
 };
 
+struct drm_intel_read_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size */
+	__u32 size;
+
+	/* return value, high 4 bytes RFU */
+	__u64 value;
+
+	__u64 rsvd;
+};
+
+struct drm_intel_write_reg {
+	/* register offset to read */
+	__u32 offset;
+
+	/* register size */
+	__u32 size;
+
+	/* value to write, high 4 bytes RFU */
+	__u64 value;
+
+	__u64 rsvd;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.3.4


[-- 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 v3 2/2] drm/i915: debugfs for register write taint

[Ben Widawsky]

Provide an interface to see which register ranges have been tainted by
the user's interaction.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 87c8e29..7142a0c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1186,6 +1186,25 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_user_tainted_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct i915_register_range *range = dev_priv->register_map.map;
+	u32 range_count = dev_priv->register_map.length;
+
+	while(range_count--) {
+		if (range->user_tainted) {
+			seq_printf(m, "tainted register range 0x%08x->0x%08x\n",
+				   range->base, range->size);
+		}
+		range++;
+	}
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1324,6 +1343,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_user_tainted", i915_user_tainted_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.3.4

Re: [PATCH] drm/i915: read/write IOCTLs

[Chris Wilson]

Nice. The consensus is that this ioctl is required, just a few comments
inline.

On Thu, 31 Mar 2011 18:31:49 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> With the invention of forcewake (DevGT) , userspace tools to diagnose
> problems are no longer reliable. This will provide userspace a mechanism
> to read registers through an IOCTL, as well as root permission to write
> to registers.
> 
> The code tries to be smart about which registers can be read and
> written, as hardware behavior may become erratic if writing to reserved
> ranges.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  129 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |    2 +
>  include/drm/i915_drm.h          |   27 ++++++++
>  3 files changed, 158 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..8b5165d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1846,6 +1846,133 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>  
> +
> +/* If we assume registers are dword aligned, second check is redundant */
> +#define REG_IN_RANGE(reg, begin, end) \
> +	((reg <= end && reg >= begin) || \
> +	((reg+3) <= end && (reg+3) >= begin))
> +
> +/*
> + * This is here to avoid hanging the GPU from userspace.
> + * The ranges are defined the in "Register Address Map" section of the
> + * documents.
> + */
> +static inline int
> +reg_in_rsvd_range(struct drm_device *dev, u32 reg)
> +{
> +	if (INTEL_INFO(dev)->gen < 6) {
> +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> +		    ((IS_BROADWATER(dev) || IS_CRESTLINE(dev)) ?
> +			REG_IN_RANGE(reg, 0x4000, 0x4fff) : 0) ||
> +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> +		    REG_IN_RANGE(reg, 0x14000, 0x2ffff) ||
> +		    REG_IN_RANGE(reg, 0x40000, 0x5ffff))
> +			return 1;
> +
> +		if (reg > 0x73fff)
> +			return 1;
> +	} else {
> +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> +		    REG_IN_RANGE(reg, 0x15000, 0x21fff) ||
> +		    REG_IN_RANGE(reg, 0x23000, 0x23fff) ||
> +		    REG_IN_RANGE(reg, 0x25000, 0x2ffff) ||
> +		    REG_IN_RANGE(reg, 0x40000, 0xfffff) ||
> +		    REG_IN_RANGE(reg, 0x108000, 0x13ffff))
> +			return 1;
> +
> +		if (reg > 0x17ffff)
> +			return 1;
> +	}

I think this would be more maintainable as a set of tables with the
ranges. And a r/w flag could be stored in the table for the individual
read/write blacklists.

> +
> +	return 0;
> +}
> +
> +
> +#define REG_IN_READ_BLACKLIST(reg) 0
> +#define REG_IN_WRITE_BLACKLIST(reg) 0
> +
> +static inline int
> +reg_read_allowed(struct drm_device *dev, u32 offset)
> +{
> +	if (offset & 0x3)
> +		return 0;
> +
> +	if (reg_in_rsvd_range(dev, offset))
> +		return 0;
> +
> +	if (REG_IN_READ_BLACKLIST(offset))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static inline int
> +reg_write_allowed(struct drm_device *dev, u32 offset)
> +{
> +	if (offset & 0x3)
> +		return 0;
> +
> +	if (reg_in_rsvd_range(dev, offset))
> +		return 0;
> +
> +	if (REG_IN_WRITE_BLACKLIST(offset))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int
> +i915_read_register_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_priv)
> +{
> +	struct drm_intel_write_reg *args = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	if (!reg_read_allowed(dev, args->offset)) {
> +		args->value = 0xffffffff;
> +		return -EINVAL;
> +	}
> +
> +	ret = i915_mutex_lock_interruptible(dev);

I don't think you want i915_mutex_lock_interuptible() here just an
ordinary mutex_lock_interruptible() - we don't need or desire to check if
the GPU is wedged.

> +	if (ret)
> +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, reading anyway\n");
> +
> +	args->value = (u32)i915_gt_read(dev_priv, args->offset);
> +
> +	mutex_unlock(&dev->struct_mutex);

Don't even think about unlocking an unlocked mutex. We either apply
strict locking or we don't bother. I think we can assume that in the
scenario where the mutex is blocked (from experience that is only from an
OOPS within i915.ko) we can play with the registers to our hearts contents
directly from userspace.

> +
> +	return 0;
> +}
> +
> +static int
> +i915_write_register_ioctl(struct drm_device *dev, void *data,
> +			  struct drm_file *file_priv)
> +{
> +	struct drm_intel_read_reg *args = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	if (!reg_write_allowed(dev, args->offset))
> +		return -EINVAL;
> +
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
> +
> +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> +	dev_priv->user_tainted = true;

Nice touch. Let's try add_taint(TAINT_USER) instead.

> +
> +	i915_gt_write(dev_priv, args->offset, (u32)args->value);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
>  /**
>   * Tells the intel_ips driver that the i915 driver is now loaded, if
>   * IPS got loaded first.
> @@ -2276,6 +2403,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..4c66734 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -703,6 +703,8 @@ typedef struct drm_i915_private {
>  	struct intel_fbdev *fbdev;
>  
>  	struct drm_property *broadcast_rgb_property;
> +
> +	bool user_tainted;
>  } drm_i915_private_t;
>  
>  struct drm_i915_gem_object {
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index c4d6dbf..68eeaa3 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
>  #define DRM_I915_OVERLAY_ATTRS	0x28
>  #define DRM_I915_GEM_EXECBUFFER2	0x29
> +#define DRM_I915_READ_REGISTER	0x2a
> +#define DRM_I915_WRITE_REGISTER	0x2b
>  
>  #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)
> @@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
>  #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
>  #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> +#define DRM_IOCTL_I915_READ_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
> +#define DRM_IOCTL_I915_WRITE_REGISTER	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -844,4 +848,27 @@ struct drm_intel_overlay_attrs {
>  	__u32 gamma5;
>  };
>  
> +struct drm_intel_read_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size, RFU */
> +	__u8 size;
> +
> +	/* return value, high 4 bytes RFU */
> +	__u64 value;
> +};

Haha! I was going to ask for this to handle arbitrary register sizes.
What I think is also necessary is to be able to read/write a block of
registers in a single call - thinking of some of the more complex
procedures we may want to try from userspace (the pcode messages for
example) or for snapshotting a set of registers.

My dream would be a small domain specific language that would allow me to
write a set of registers and poll for a result and then return that
result. In the meantime I'd settle for passing an array of registers to be
written and read in a single ioctl.

> +
> +struct drm_intel_write_reg {
> +	/* register offset to read */
> +	__u32 offset;
> +
> +	/* register size,  RFU */
> +	__u8 size;
> +
> +	/* value to write, high 4 bytes RFU */
> +	__u64 value;
> +
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: read/write IOCTLs

[Ben Widawsky]

On Fri, Apr 01, 2011 at 07:36:56AM +0100, Chris Wilson wrote:
> Nice. The consensus is that this ioctl is required, just a few comments
> inline.

Nobody likes it, but it could pay off, especially if we end up with
weird non-mappable registers we wish to allow user space to read. I had
actually decided to add another field (something like a bus) to the
interface just in case.

> > +static inline int
> > +reg_in_rsvd_range(struct drm_device *dev, u32 reg)
> > +{
> > +	if (INTEL_INFO(dev)->gen < 6) {
> > +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> > +		    ((IS_BROADWATER(dev) || IS_CRESTLINE(dev)) ?
> > +			REG_IN_RANGE(reg, 0x4000, 0x4fff) : 0) ||
> > +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> > +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> > +		    REG_IN_RANGE(reg, 0x14000, 0x2ffff) ||
> > +		    REG_IN_RANGE(reg, 0x40000, 0x5ffff))
> > +			return 1;
> > +
> > +		if (reg > 0x73fff)
> > +			return 1;
> > +	} else {
> > +		if (REG_IN_RANGE(reg, 0x1000, 0x1fff) ||
> > +		    REG_IN_RANGE(reg, 0x9000, 0x9fff) ||
> > +		    REG_IN_RANGE(reg, 0xb000, 0xffff) ||
> > +		    REG_IN_RANGE(reg, 0x15000, 0x21fff) ||
> > +		    REG_IN_RANGE(reg, 0x23000, 0x23fff) ||
> > +		    REG_IN_RANGE(reg, 0x25000, 0x2ffff) ||
> > +		    REG_IN_RANGE(reg, 0x40000, 0xfffff) ||
> > +		    REG_IN_RANGE(reg, 0x108000, 0x13ffff))
> > +			return 1;
> > +
> > +		if (reg > 0x17ffff)
> > +			return 1;
> > +	}
> 
> I think this would be more maintainable as a set of tables with the
> ranges. And a r/w flag could be stored in the table for the individual
> read/write blacklists.

That's fine with me. When I started it was much less complicated than by
the time I submitted the patch.

> > +	if (ret)
> > +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, reading anyway\n");
> > +
> > +	args->value = (u32)i915_gt_read(dev_priv, args->offset);
> > +
> > +	mutex_unlock(&dev->struct_mutex);
> 
> Don't even think about unlocking an unlocked mutex. We either apply
> strict locking or we don't bother. I think we can assume that in the
> scenario where the mutex is blocked (from experience that is only from an
> OOPS within i915.ko) we can play with the registers to our hearts contents
> directly from userspace.

This was a bug in my patch... you mean?
ret = mutex_lock_interruptible();
if (ret)
	reg_read

if (!ret)
	mutex_unlock()

> > +static int
> > +i915_write_register_ioctl(struct drm_device *dev, void *data,
> > +			  struct drm_file *file_priv)
> > +{
> > +	struct drm_intel_read_reg *args = data;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret;
> > +
> > +	if (!reg_write_allowed(dev, args->offset))
> > +		return -EINVAL;
> > +
> > +	ret = i915_mutex_lock_interruptible(dev);
> > +	if (ret)
> > +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
> > +
> > +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> > +	dev_priv->user_tainted = true;
> 
> Nice touch. Let's try add_taint(TAINT_USER) instead.

Cool! I didn't know about that interface. I'd like to be able to query
this at runtime as opposed to an OOPS... I'm too tired to look if it
supports that right now, but if it does, I'll switch.

> >  
> > +struct drm_intel_read_reg {
> > +	/* register offset to read */
> > +	__u32 offset;
> > +
> > +	/* register size, RFU */
> > +	__u8 size;
> > +
> > +	/* return value, high 4 bytes RFU */
> > +	__u64 value;
> > +};
> 
> Haha! I was going to ask for this to handle arbitrary register sizes.
> What I think is also necessary is to be able to read/write a block of
> registers in a single call - thinking of some of the more complex
> procedures we may want to try from userspace (the pcode messages for
> example) or for snapshotting a set of registers.

I brought this question up at our meeting on Thursday. The consensus to
was just have the 1 register at a time because the overhead of the
syscall was small enough to make it not matter. I think I prefer the
single read/write a little bit just because it keeps the kernel code
simpler, but I don't care enough to argue, and I most certainly don't
care enough to set up some kind of performance test for it. Anyone else
care enough to argue?

> 
> My dream would be a small domain specific language that would allow me to
> write a set of registers and poll for a result and then return that
> result. In the meantime I'd settle for passing an array of registers to be
> written and read in a single ioctl.
> 

GFX User mode driver? :)

> -- 
> Chris Wilson, Intel Open Source Technology Centre

Thanks for the feedback. I'll try to fix it up when I wake up.

Ben

Re: [PATCH] drm/i915: read/write IOCTLs

[Chris Wilson]

On Fri, 1 Apr 2011 00:06:37 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Apr 01, 2011 at 07:36:56AM +0100, Chris Wilson wrote:
> > Nice. The consensus is that this ioctl is required, just a few comments
> > inline.
> 
> Nobody likes it, but it could pay off, especially if we end up with
> weird non-mappable registers we wish to allow user space to read. I had
> actually decided to add another field (something like a bus) to the
> interface just in case.

I think most of that can and should be done in userspace. Eventually we
should have the list of registers and be able to use the spec names and
have those translated into the correct offset for the chipset.

Has anyone succeeded in getting a machine-readable description of the
registers? Or am I just dreaming that we have real documentation
somewhere?

> This was a bug in my patch... you mean?
> ret = mutex_lock_interruptible();
> if (ret)
> 	reg_read
> 
> if (!ret)
> 	mutex_unlock()

Slightly better in avoiding the kernel killer. ;-)

I'm just not happy about haphazard locking. Can we do simple and safe
locking and revisit it if a real use-case for brute-forcing the read/write
is found?

> > > +static int
> > > +i915_write_register_ioctl(struct drm_device *dev, void *data,
> > > +			  struct drm_file *file_priv)
> > > +{
> > > +	struct drm_intel_read_reg *args = data;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int ret;
> > > +
> > > +	if (!reg_write_allowed(dev, args->offset))
> > > +		return -EINVAL;
> > > +
> > > +	ret = i915_mutex_lock_interruptible(dev);
> > > +	if (ret)
> > > +		DRM_DEBUG_DRIVER("Couldn't acquire mutex, writing anyway\n");
> > > +
> > > +	DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> > > +	dev_priv->user_tainted = true;
> > 
> > Nice touch. Let's try add_taint(TAINT_USER) instead.
> 
> Cool! I didn't know about that interface. I'd like to be able to query
> this at runtime as opposed to an OOPS... I'm too tired to look if it
> supports that right now, but if it does, I'll switch.

There's an equivalent get_taint(), but we have no way of knowing if that
was us or not. We do need to taint the oops (as having the hardware change
behind our backs is evil personified). When were you thinking of using
dev_priv->user_tainted?

> > > +struct drm_intel_read_reg {
> > > +	/* register offset to read */
> > > +	__u32 offset;
> > > +
> > > +	/* register size, RFU */
> > > +	__u8 size;
> > > +
> > > +	/* return value, high 4 bytes RFU */
> > > +	__u64 value;
> > > +};
> > 
> > Haha! I was going to ask for this to handle arbitrary register sizes.
> > What I think is also necessary is to be able to read/write a block of
> > registers in a single call - thinking of some of the more complex
> > procedures we may want to try from userspace (the pcode messages for
> > example) or for snapshotting a set of registers.
> 
> I brought this question up at our meeting on Thursday. The consensus to
> was just have the 1 register at a time because the overhead of the
> syscall was small enough to make it not matter. I think I prefer the
> single read/write a little bit just because it keeps the kernel code
> simpler, but I don't care enough to argue, and I most certainly don't
> care enough to set up some kind of performance test for it. Anyone else
> care enough to argue?

It's not the performance I care about, but the atomicity. There seem to be
a growing abundance of messaging systems within the chip driven by
combinations of registers. I'd feel happier if we could send a message
without fouling or being fouled by the kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: read/write IOCTLs

[Eric Anholt]

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

On Fri, 01 Apr 2011 08:32:09 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, 1 Apr 2011 00:06:37 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Fri, Apr 01, 2011 at 07:36:56AM +0100, Chris Wilson wrote:
> > > Nice. The consensus is that this ioctl is required, just a few comments
> > > inline.
> > 
> > Nobody likes it, but it could pay off, especially if we end up with
> > weird non-mappable registers we wish to allow user space to read. I had
> > actually decided to add another field (something like a bus) to the
> > interface just in case.
> 
> I think most of that can and should be done in userspace. Eventually we
> should have the list of registers and be able to use the spec names and
> have those translated into the correct offset for the chipset.
> 
> Has anyone succeeded in getting a machine-readable description of the
> registers? Or am I just dreaming that we have real documentation
> somewhere?
> 
> > This was a bug in my patch... you mean?
> > ret = mutex_lock_interruptible();
> > if (ret)
> > 	reg_read
> > 
> > if (!ret)
> > 	mutex_unlock()
> 
> Slightly better in avoiding the kernel killer. ;-)
> 
> I'm just not happy about haphazard locking. Can we do simple and safe
> locking and revisit it if a real use-case for brute-forcing the read/write
> is found?

The concern we had was "I want to be sure that when the GPU is hung we
can still reg_dump so people can write bug reports.  The mutex lock try
should have a timeout, and we should go ahead and just try forcewaking
and reading the reg anyway after a while."

> > > > +struct drm_intel_read_reg {
> > > > +	/* register offset to read */
> > > > +	__u32 offset;
> > > > +
> > > > +	/* register size, RFU */
> > > > +	__u8 size;
> > > > +
> > > > +	/* return value, high 4 bytes RFU */
> > > > +	__u64 value;
> > > > +};
> > > 
> > > Haha! I was going to ask for this to handle arbitrary register sizes.
> > > What I think is also necessary is to be able to read/write a block of
> > > registers in a single call - thinking of some of the more complex
> > > procedures we may want to try from userspace (the pcode messages for
> > > example) or for snapshotting a set of registers.
> > 
> > I brought this question up at our meeting on Thursday. The consensus to
> > was just have the 1 register at a time because the overhead of the
> > syscall was small enough to make it not matter. I think I prefer the
> > single read/write a little bit just because it keeps the kernel code
> > simpler, but I don't care enough to argue, and I most certainly don't
> > care enough to set up some kind of performance test for it. Anyone else
> > care enough to argue?
> 
> It's not the performance I care about, but the atomicity. There seem to be
> a growing abundance of messaging systems within the chip driven by
> combinations of registers. I'd feel happier if we could send a message
> without fouling or being fouled by the kernel.

Generally for those things, you end up needing to poll in the middle.
Let's not build a more complicated interface than required for current
use-cases in userland (reading many registers, or writing an arbitrary
one), without solving a specific problem.

[-- 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: read/write IOCTLs

[Chris Wilson]

On Fri, 01 Apr 2011 11:51:23 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Fri, 01 Apr 2011 08:32:09 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > I'm just not happy about haphazard locking. Can we do simple and safe
> > locking and revisit it if a real use-case for brute-forcing the read/write
> > is found?
> 
> The concern we had was "I want to be sure that when the GPU is hung we
> can still reg_dump so people can write bug reports.  The mutex lock try
> should have a timeout, and we should go ahead and just try forcewaking
> and reading the reg anyway after a while."

If the GPU has hung (and more importantly in such a way as to block the
mutex - nowadays that is it an OOPS with the lock held), we can simply do
the reg read/write from the userspace without worry. But for bug reports,
I'd much rather we automatically capture any register that we might need than
rely on the frustrated user doing the right thing.

So I don't see having the haphazard locking in the kernel, for everyone to
criticise us for, justified.

> > It's not the performance I care about, but the atomicity. There seem to be
> > a growing abundance of messaging systems within the chip driven by
> > combinations of registers. I'd feel happier if we could send a message
> > without fouling or being fouled by the kernel.
> 
> Generally for those things, you end up needing to poll in the middle.
> Let's not build a more complicated interface than required for current
> use-cases in userland (reading many registers, or writing an arbitrary
> one), without solving a specific problem.

What I guess I was trying to express was that we need to be very clear
what the interface is for and the limitations about its use.

For the more complicated set of registers, we can and should expose knobs
in the debugfs to read and write them. For instance, to control the render
clock frequencies and thresholds.

But perhaps we do need to reconsider the performance aspect. intel_gpu_top
samples the ring HEAD and TAIL at around 10KHz and forcing gt-wake is
about 50 microseconds... I hope I'm mistaken, because even batched that is
doomed. Ben, do you mind checking that thought experiment with a little
hard fact?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: read/write IOCTLs

[Ben Widawsky]

On Sat, Apr 02, 2011 at 07:46:31AM +0100, Chris Wilson wrote:
> 
> But perhaps we do need to reconsider the performance aspect. intel_gpu_top
> samples the ring HEAD and TAIL at around 10KHz and forcing gt-wake is
> about 50 microseconds... I hope I'm mistaken, because even batched that is
> doomed. Ben, do you mind checking that thought experiment with a little
> hard fact?

I can get some numbers for it... but I'd like to further the discussion
a bit since I have to go to the office to get a SNB, and typing is
easier than doing that right now :).

I too think we might be doomed. At the very least we have the
POSTING_READ, which is expensive. The udelays may or may not actually
occur (I'll find out). Let's not forget too that we do fix other tools,
not just intel_gpu_top, and those tools don't poll, and don't care about
timing (granted they're mostly less interesting too).

I think what we really need to try to defer the forcewake_put, as well
as something like the last patch I sent
<1301105269-23970-2-git-send-email-ben@bwidawsk.net> to remove the need
to protect force_wake_get with struct_mutex, and keep a refcount.  I'd
rather get the current stuff accepted, port the tools, and then make
improvements to the performance. However if you feel the order must be
another way, I can work with that.

> -Chris
> 

Thanks.
Ben

Re: [PATCH] drm/i915: read/write IOCTLs

[Ben Widawsky]

On Sat, Apr 02, 2011 at 07:46:31AM +0100, Chris Wilson wrote:

> What I guess I was trying to express was that we need to be very clear
> what the interface is for and the limitations about its use.
> 
> For the more complicated set of registers, we can and should expose knobs
> in the debugfs to read and write them. For instance, to control the render
> clock frequencies and thresholds.
> 
> But perhaps we do need to reconsider the performance aspect. intel_gpu_top
> samples the ring HEAD and TAIL at around 10KHz and forcing gt-wake is
> about 50 microseconds... I hope I'm mistaken, because even batched that is
> doomed. Ben, do you mind checking that thought experiment with a little
> hard fact?

Here is the data from ~100 samples while playing playing Armacycles Advanced
measured off of d-i-f 7f58aabc369014fda3a4a33604ba0a1b63b941ac.

min	02.775us
max	19.402us
avg	07.057us
stddev	02.819us

When I do a cat /sys/kernel/debug/dri/0/i915_gem_interrupt, I always get
3 reads, in a similar pattern to this:

6) ! 285.852 us  |  __gen6_gt_force_wake_get();
6)   1.944 us    |  __gen6_gt_force_wake_get();
6)   1.854 us    |  __gen6_gt_force_wake_get();

Not sure why that case is so different.

> -Chris

Ben

Re: [PATCH] drm/i915: read/write IOCTLs

[Chris Wilson]

On Sun, 3 Apr 2011 18:35:04 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Here is the data from ~100 samples while playing playing Armacycles Advanced
> measured off of d-i-f 7f58aabc369014fda3a4a33604ba0a1b63b941ac.
> 
> min	02.775us
> max	19.402us
> avg	07.057us
> stddev	02.819us
> 
> When I do a cat /sys/kernel/debug/dri/0/i915_gem_interrupt, I always get
> 3 reads, in a similar pattern to this:
> 
> 6) ! 285.852 us  |  __gen6_gt_force_wake_get();
> 6)   1.944 us    |  __gen6_gt_force_wake_get();
> 6)   1.854 us    |  __gen6_gt_force_wake_get();
> 
> Not sure why that case is so different.

Presumably the high cost is when we need to wait for the GT to power up
and the hardware has its own hysteresis and will delay before powering
down again. [It also looks like we always have to wait at least for one
loop. Perhaps a posting read is in order?] So at least we don't have to
worry about doing that ourselves - adding a spinlock just for performance
optimisation on a seldom used debugging ioctl is painful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre