[PATCH 0/6] Page faults to help user space debug

[PATCH 0/6] Page faults to help user space debug

[Ben Widawsky]

This series originated from the request from Paul, "can you enable page
faults"?  After some though and discussion, we came up with 3 debug features to
implement:

1. echo 1 > /sys/kernel/debug/dri/0/i915_debug_flags
Make all scratch pages fault on access

2. echo 2 > /sys/kernel/debug/dri/0/i915_debug_flags
Unbind all buffers synchronously, so any batch which has bad code will fault
every time and not get lucky by hitting a buffer it bound. This will not catch
cases where it accidentally reads/writes someone else's buffer.

3. echo 4 > /sys/kernel/debug/dri/0/i915_debug_flags
Add an extra page to all user allocated objects which will fault when accessed.
This is meant to catch the case where mesa overruns one of it's local buffers.
It's meant to be a finer grained version of #1, and was a specific request.

Personally, I don't know how useful page faults are. The one time I tried to
turn it on previously, I remember getting a hard hang, but my memory if very
fuzzy.

Putt this out there for review, and hopefully for Paul to test as soon
as he has something read.


Ben Widawsky (6):
  drm/i915: Faults for scratch PTEs
  drm/i915: Debugfs for setting debug_flags
  drm/i915: Reset scratch pages when using debug_flags
  drm/i915: Synchronous execbuf for debug
  drm/i915: Let userspace create a faultable pad page
  drm/i915: distinguish pad and fault pages

 drivers/gpu/drm/i915/i915_debugfs.c        | 53 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c            |  3 ++
 drivers/gpu/drm/i915/i915_drv.h            | 17 +++++--
 drivers/gpu/drm/i915/i915_gem.c            |  7 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  7 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 81 +++++++++++++++++++++++++-----
 6 files changed, 151 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] drm/i915: Faults for scratch PTEs

[Ben Widawsky]

Using a new "debug_flags" we'll be able to configure some options which
make it easier for user space to quickly identify when there is a bug in
their code.

There is no way to yet turn it on.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 1 +
 drivers/gpu/drm/i915/i915_drv.c     | 3 +++
 drivers/gpu/drm/i915/i915_drv.h     | 4 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 9 +++++++++
 4 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b3a2dbe..4d9d8a1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -812,6 +812,7 @@ static int i915_error_state(struct i915_error_state_file_priv *error_priv,
 	err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake);
 	err_printf(m, "DERRMR: 0x%08x\n", error->derrmr);
 	err_printf(m, "CCID: 0x%08x\n", error->ccid);
+	err_printf(m, "debug_flags: 0x%llx\n", dev_priv->debug_flags);
 
 	for (i = 0; i < dev_priv->num_fence_regs; i++)
 		err_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 062cbda..5718f45 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -940,6 +940,9 @@ int i915_reset(struct drm_device *dev)
 		return ret;
 	}
 
+	/* Assume debug flags may have been the cause of the problem */
+	dev_priv->debug_flags = 0;
+
 	/* Ok, now get things going again... */
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56bd82b..7a98f7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1176,6 +1176,10 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+
+#define I915_DEBUG_NONE 0
+#define I915_SCRATCH_FAULTS (1<<0)
+	u64 debug_flags;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5101ab6..982c791 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -41,6 +41,8 @@
 #define GEN6_PTE_CACHE_LLC		(2 << 1)
 #define GEN6_PTE_CACHE_LLC_MLC		(3 << 1)
 #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
+/* Use a pattern to make debug a bit easier */
+#define GEN6_PTE_FAULT			0xbaddc0de
 
 static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
 				      dma_addr_t addr,
@@ -185,6 +187,7 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 				   unsigned first_entry,
 				   unsigned num_entries)
 {
+	struct drm_i915_private *dev_priv = ppgtt->dev->dev_private;
 	gen6_gtt_pte_t *pt_vaddr, scratch_pte;
 	unsigned act_pt = first_entry / I915_PPGTT_PT_ENTRIES;
 	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
@@ -194,6 +197,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 					ppgtt->scratch_page_dma_addr,
 					I915_CACHE_LLC);
 
+	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
+		scratch_pte = GEN6_PTE_FAULT;
+
 	while (num_entries) {
 		last_pte = first_pte + num_entries;
 		if (last_pte > I915_PPGTT_PT_ENTRIES)
@@ -521,6 +527,8 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 	scratch_pte = dev_priv->gtt.pte_encode(dev,
 					       dev_priv->gtt.scratch_page_dma,
 					       I915_CACHE_LLC);
+	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
+		scratch_pte = GEN6_PTE_FAULT;
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
@@ -623,6 +631,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 	struct drm_i915_gem_object *obj;
 	unsigned long hole_start, hole_end;
 
+	BUILD_BUG_ON(GEN6_PTE_FAULT & GEN6_PTE_VALID);
 	BUG_ON(mappable_end > end);
 
 	/* Subtract the guard page ... */
-- 
1.8.3.1

[PATCH 2/6] drm/i915: Debugfs for setting debug_flags

[Ben Widawsky]

The lock in the setter isn't really necessary - however we're doing some
more stuff there presently, so just leave it in...

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4d9d8a1..5d6a020 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2182,6 +2182,42 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
 			i915_cache_sharing_get, i915_cache_sharing_set,
 			"%llu\n");
 
+static int
+i915_debug_flags_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	*val = dev_priv->debug_flags;
+
+	return 0;
+}
+
+static int
+i915_debug_flags_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	/* FIXME?: Nothing prevents this from working pre GEN6; but don't want
+	 * to touch AGP code.
+	 */
+	if (INTEL_INFO(dev)->gen < 6 && val & I915_SCRATCH_FAULTS)
+		DRM_DEBUG_DRIVER("Tried to set unsupported SCRATCH_FAULTS\n");
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	dev_priv->debug_flags = val;
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_debug_flag_fops,
+			i915_debug_flags_get, i915_debug_flags_set,
+			"%llu\n");
+
 /* As the drm_debugfs_init() routines are called before dev->dev_private is
  * allocated we need to hook into the minor for release. */
 static int
@@ -2366,6 +2402,13 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				 "i915_debug_flags",
+				 &i915_debug_flag_fops);
+	if (ret)
+		return ret;
+
+
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
@@ -2393,6 +2436,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_next_seqno_fops,
 				 1, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_debug_flag_fops,
+				 1, minor);
 }
 
 #endif /* CONFIG_DEBUG_FS */
-- 
1.8.3.1

[PATCH 3/6] drm/i915: Reset scratch pages when using debug_flags

[Ben Widawsky]

Instead of waiting an indeterminate amount of time for ranges to get
cleared with the PTEs that will fault, do it immediately.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  7 +++++++
 drivers/gpu/drm/i915/i915_drv.h     |  4 +++-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 20 ++++++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5d6a020..86e750c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2198,6 +2198,7 @@ i915_debug_flags_set(void *data, u64 val)
 {
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 old_flags = dev_priv->debug_flags;
 	int ret;
 
 	/* FIXME?: Nothing prevents this from working pre GEN6; but don't want
@@ -2209,7 +2210,13 @@ i915_debug_flags_set(void *data, u64 val)
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
+
 	dev_priv->debug_flags = val;
+
+	/* If setting or unsetting scratch faults, we need special handling */
+	if ((old_flags ^ val) & I915_SCRATCH_FAULTS)
+		i915_gem_rewrite_scratch_ptes(dev);
+
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a98f7e..b99087f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1865,7 +1865,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen < 6)
 		intel_gtt_chipset_flush();
 }
-
+#ifdef CONFIG_DEBUG_FS
+void i915_gem_rewrite_scratch_ptes(struct drm_device *dev);
+#endif
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 982c791..f71636e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -889,3 +889,23 @@ int i915_gem_gtt_init(struct drm_device *dev)
 
 	return 0;
 }
+
+#ifdef CONFIG_DEBUG_FS
+void i915_gem_rewrite_scratch_ptes(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mm_node *n;
+	unsigned long start, end;
+	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
+
+	drm_mm_for_each_hole(n, &dev_priv->mm.gtt_space, start, end) {
+		dev_priv->gtt.gtt_clear_range(dev,
+					      start >> PAGE_SHIFT,
+					      (end - start) >> PAGE_SHIFT);
+		if (ppgtt)
+			ppgtt->clear_range(ppgtt,
+					   start >> PAGE_SHIFT,
+					   (end - start) >> PAGE_SHIFT);
+	}
+}
+#endif
-- 
1.8.3.1

[PATCH 4/6] drm/i915: Synchronous execbuf for debug

[Ben Widawsky]

In order to be able to hit the previously introduced page faults,
userspace may also want to make sure it's buffers are evicted, so that
it doesn't get lucky on accident.

This debug feature will have a horrible performance implication (I
measure roughly 3x in the negative direction for some basic tests).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h            | 1 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b99087f8..c9e38a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1179,6 +1179,7 @@ typedef struct drm_i915_private {
 
 #define I915_DEBUG_NONE 0
 #define I915_SCRATCH_FAULTS (1<<0)
+#define I915_SYNC_EXECBUF (1<<1)
 	u64 debug_flags;
 } drm_i915_private_t;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 87a3227..fe3bb5f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -150,7 +150,8 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
 }
 
 static void
-eb_destroy(struct eb_objects *eb)
+eb_destroy(struct drm_i915_private *dev_priv,
+	   struct eb_objects *eb)
 {
 	while (!list_empty(&eb->objects)) {
 		struct drm_i915_gem_object *obj;
@@ -158,6 +159,8 @@ eb_destroy(struct eb_objects *eb)
 		obj = list_first_entry(&eb->objects,
 				       struct drm_i915_gem_object,
 				       exec_list);
+		if (unlikely(dev_priv->debug_flags & I915_SYNC_EXECBUF))
+			i915_gem_object_unbind(obj);
 		list_del_init(&obj->exec_list);
 		drm_gem_object_unreference(&obj->base);
 	}
@@ -1087,7 +1090,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
 
 err:
-	eb_destroy(eb);
+	eb_destroy(dev_priv, eb);
 
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.8.3.1

[PATCH 5/6] drm/i915: Let userspace create a faultable pad page

[Ben Widawsky]

Whenever userspace allocates a BO, add one more page. On maps of this
BO, make sure that last page will fault if accessed. We don't need to do
this for the kernel, since our bugs should be fairly easy to catch
already.

The code could be optimized to allocate one less page for this, but it
requires a lot of work to make that happen. Instead, do everything as we
normally would, and clear out the last page after it's all done.

NOTE: This does not convert previously allocated objects.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/i915_gem.c     |  7 +++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +++++++++++++++++++---
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9e38a6..3b2046b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1180,6 +1180,7 @@ typedef struct drm_i915_private {
 #define I915_DEBUG_NONE 0
 #define I915_SCRATCH_FAULTS (1<<0)
 #define I915_SYNC_EXECBUF (1<<1)
+#define I915_PAD_PAGE (1<<2)
 	u64 debug_flags;
 } drm_i915_private_t;
 
@@ -1307,6 +1308,7 @@ struct drm_i915_gem_object {
 	unsigned int has_aliasing_ppgtt_mapping:1;
 	unsigned int has_global_gtt_mapping:1;
 	unsigned int has_dma_mapping:1;
+	unsigned int has_pad_page:1;
 
 	struct sg_table *pages;
 	int pages_pin_count;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c68b90f..f84aada 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -205,6 +205,7 @@ i915_gem_create(struct drm_file *file,
 		uint64_t size,
 		uint32_t *handle_p)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	int ret;
 	u32 handle;
@@ -213,11 +214,17 @@ i915_gem_create(struct drm_file *file,
 	if (size == 0)
 		return -EINVAL;
 
+	if (dev_priv->debug_flags & I915_PAD_PAGE)
+		size += PAGE_SIZE;
+
 	/* Allocate the new object */
 	obj = i915_gem_alloc_object(dev, size);
 	if (obj == NULL)
 		return -ENOMEM;
 
+	if (dev_priv->debug_flags & I915_PAD_PAGE)
+		obj->has_pad_page = 1;
+
 	ret = drm_gem_handle_create(file, &obj->base, &handle);
 	if (ret) {
 		drm_gem_object_release(&obj->base);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f71636e..187738f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -43,6 +43,7 @@
 #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
 /* Use a pattern to make debug a bit easier */
 #define GEN6_PTE_FAULT			0xbaddc0de
+#define GEN6_PAD_PTE_FAULT		0x0c0ffee
 
 static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
 				      dma_addr_t addr,
@@ -200,6 +201,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
 		scratch_pte = GEN6_PTE_FAULT;
 
+	if (unlikely(dev_priv->debug_flags & I915_PAD_PAGE))
+		scratch_pte = GEN6_PAD_PTE_FAULT;
+
 	while (num_entries) {
 		last_pte = first_pte + num_entries;
 		if (last_pte > I915_PPGTT_PT_ENTRIES)
@@ -385,9 +389,13 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
 			    struct drm_i915_gem_object *obj,
 			    enum i915_cache_level cache_level)
 {
-	ppgtt->insert_entries(ppgtt, obj->pages,
-			      obj->gtt_space->start >> PAGE_SHIFT,
-			      cache_level);
+	unsigned int first = obj->gtt_space->start >> PAGE_SHIFT;
+
+	ppgtt->insert_entries(ppgtt, obj->pages, first, cache_level);
+	if (unlikely(obj->has_pad_page)) {
+		first += (obj->gtt_space->size >> PAGE_SHIFT) - 1;
+		ppgtt->clear_range(ppgtt, first, 1);
+	}
 }
 
 void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
@@ -529,6 +537,8 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 					       I915_CACHE_LLC);
 	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
 		scratch_pte = GEN6_PTE_FAULT;
+	if (unlikely(dev_priv->debug_flags & I915_PAD_PAGE))
+		scratch_pte = GEN6_PAD_PTE_FAULT;
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
@@ -560,10 +570,15 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned int first = obj->gtt_space->start >> PAGE_SHIFT;
 
 	dev_priv->gtt.gtt_insert_entries(dev, obj->pages,
 					 obj->gtt_space->start >> PAGE_SHIFT,
 					 cache_level);
+	if (unlikely(obj->has_pad_page)) {
+		first += (obj->gtt_space->size >> PAGE_SHIFT) - 1;
+		dev_priv->gtt.gtt_clear_range(dev, first, 1);
+	}
 
 	obj->has_global_gtt_mapping = 1;
 }
@@ -632,6 +647,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 	unsigned long hole_start, hole_end;
 
 	BUILD_BUG_ON(GEN6_PTE_FAULT & GEN6_PTE_VALID);
+	BUILD_BUG_ON(GEN6_PAD_PTE_FAULT & GEN6_PTE_VALID);
 	BUG_ON(mappable_end > end);
 
 	/* Subtract the guard page ... */
-- 
1.8.3.1

[PATCH 6/6] drm/i915: distinguish pad and fault pages

[Ben Widawsky]

For finer grained debug, make it possible to have both magic numbers be
present for the PTEs if both faulting and pad pages options are enabled.
Previously the padding magic number took precedence.

This change might not seem worthwhile to some. It helped me verify my
code was correct, and I like it. It can be dropped without too much
impact to the series though.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     |  6 ++++--
 drivers/gpu/drm/i915/i915_gem_gtt.c | 42 ++++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3b2046b..aaf9554 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -475,7 +475,8 @@ struct i915_gtt {
 	void (*gtt_remove)(struct drm_device *dev);
 	void (*gtt_clear_range)(struct drm_device *dev,
 				unsigned int first_entry,
-				unsigned int num_entries);
+				unsigned int num_entries,
+				bool pad);
 	void (*gtt_insert_entries)(struct drm_device *dev,
 				   struct sg_table *st,
 				   unsigned int pg_start,
@@ -499,7 +500,8 @@ struct i915_hw_ppgtt {
 	/* pte functions, mirroring the interface of the global gtt. */
 	void (*clear_range)(struct i915_hw_ppgtt *ppgtt,
 			    unsigned int first_entry,
-			    unsigned int num_entries);
+			    unsigned int num_entries,
+			    bool pad);
 	void (*insert_entries)(struct i915_hw_ppgtt *ppgtt,
 			       struct sg_table *st,
 			       unsigned int pg_start,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 187738f..5192f45 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -186,7 +186,8 @@ static int gen6_ppgtt_enable(struct drm_device *dev)
 /* PPGTT support for Sandybdrige/Gen6 and later */
 static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 				   unsigned first_entry,
-				   unsigned num_entries)
+				   unsigned num_entries,
+				   bool pad)
 {
 	struct drm_i915_private *dev_priv = ppgtt->dev->dev_private;
 	gen6_gtt_pte_t *pt_vaddr, scratch_pte;
@@ -194,6 +195,7 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
 	unsigned last_pte, i;
 
+	BUG_ON(pad && num_entries != 1);
 	scratch_pte = ppgtt->pte_encode(ppgtt->dev,
 					ppgtt->scratch_page_dma_addr,
 					I915_CACHE_LLC);
@@ -201,7 +203,7 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
 		scratch_pte = GEN6_PTE_FAULT;
 
-	if (unlikely(dev_priv->debug_flags & I915_PAD_PAGE))
+	if (unlikely(pad))
 		scratch_pte = GEN6_PAD_PTE_FAULT;
 
 	while (num_entries) {
@@ -324,7 +326,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	}
 
 	ppgtt->clear_range(ppgtt, 0,
-			   ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES);
+			   ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES,
+			   false);
 
 	ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gen6_gtt_pte_t);
 
@@ -394,7 +397,7 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
 	ppgtt->insert_entries(ppgtt, obj->pages, first, cache_level);
 	if (unlikely(obj->has_pad_page)) {
 		first += (obj->gtt_space->size >> PAGE_SHIFT) - 1;
-		ppgtt->clear_range(ppgtt, first, 1);
+		ppgtt->clear_range(ppgtt, first, 1, true);
 	}
 }
 
@@ -403,7 +406,8 @@ void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
 {
 	ppgtt->clear_range(ppgtt,
 			   obj->gtt_space->start >> PAGE_SHIFT,
-			   obj->base.size >> PAGE_SHIFT);
+			   obj->base.size >> PAGE_SHIFT,
+			   false);
 }
 
 extern int intel_iommu_gfx_mapped;
@@ -451,7 +455,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 
 	/* First fill our portion of the GTT with scratch pages */
 	dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE,
-				      dev_priv->gtt.total / PAGE_SIZE);
+				      dev_priv->gtt.total / PAGE_SIZE,
+				      false);
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		i915_gem_clflush_object(obj);
@@ -519,7 +524,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
 
 static void gen6_ggtt_clear_range(struct drm_device *dev,
 				  unsigned int first_entry,
-				  unsigned int num_entries)
+				  unsigned int num_entries,
+				  bool pad)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	gen6_gtt_pte_t scratch_pte, __iomem *gtt_base =
@@ -527,6 +533,7 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 	const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
 	int i;
 
+	BUG_ON(pad && num_entries != 1);
 	if (WARN(num_entries > max_entries,
 		 "First entry = %d; Num entries = %d (max=%d)\n",
 		 first_entry, num_entries, max_entries))
@@ -537,7 +544,7 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 					       I915_CACHE_LLC);
 	if (unlikely(dev_priv->debug_flags & I915_SCRATCH_FAULTS))
 		scratch_pte = GEN6_PTE_FAULT;
-	if (unlikely(dev_priv->debug_flags & I915_PAD_PAGE))
+	if (unlikely(pad))
 		scratch_pte = GEN6_PAD_PTE_FAULT;
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
@@ -559,7 +566,8 @@ static void i915_ggtt_insert_entries(struct drm_device *dev,
 
 static void i915_ggtt_clear_range(struct drm_device *dev,
 				  unsigned int first_entry,
-				  unsigned int num_entries)
+				  unsigned int num_entries,
+				  bool pad)
 {
 	intel_gtt_clear_range(first_entry, num_entries);
 }
@@ -577,7 +585,7 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
 					 cache_level);
 	if (unlikely(obj->has_pad_page)) {
 		first += (obj->gtt_space->size >> PAGE_SHIFT) - 1;
-		dev_priv->gtt.gtt_clear_range(dev, first, 1);
+		dev_priv->gtt.gtt_clear_range(dev, first, 1, true);
 	}
 
 	obj->has_global_gtt_mapping = 1;
@@ -590,7 +598,8 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 
 	dev_priv->gtt.gtt_clear_range(obj->base.dev,
 				      obj->gtt_space->start >> PAGE_SHIFT,
-				      obj->base.size >> PAGE_SHIFT);
+				      obj->base.size >> PAGE_SHIFT,
+				      false);
 
 	obj->has_global_gtt_mapping = 0;
 }
@@ -677,11 +686,12 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
 			      hole_start, hole_end);
 		dev_priv->gtt.gtt_clear_range(dev, hole_start / PAGE_SIZE,
-					      (hole_end-hole_start) / PAGE_SIZE);
+					      (hole_end-hole_start) / PAGE_SIZE,
+					      false);
 	}
 
 	/* And finally clear the reserved guard page */
-	dev_priv->gtt.gtt_clear_range(dev, end / PAGE_SIZE - 1, 1);
+	dev_priv->gtt.gtt_clear_range(dev, end / PAGE_SIZE - 1, 1, false);
 }
 
 static bool
@@ -917,11 +927,13 @@ void i915_gem_rewrite_scratch_ptes(struct drm_device *dev)
 	drm_mm_for_each_hole(n, &dev_priv->mm.gtt_space, start, end) {
 		dev_priv->gtt.gtt_clear_range(dev,
 					      start >> PAGE_SHIFT,
-					      (end - start) >> PAGE_SHIFT);
+					      (end - start) >> PAGE_SHIFT,
+					      false);
 		if (ppgtt)
 			ppgtt->clear_range(ppgtt,
 					   start >> PAGE_SHIFT,
-					   (end - start) >> PAGE_SHIFT);
+					   (end - start) >> PAGE_SHIFT,
+					   false);
 	}
 }
 #endif
-- 
1.8.3.1

Re: [PATCH 0/6] Page faults to help user space debug

[Chris Wilson]

On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
> This series originated from the request from Paul, "can you enable page
> faults"?  After some though and discussion, we came up with 3 debug features to
> implement:

The issue lies in that the CS and EU units like to prefetch 128 bytes
and will cross page boundaries. Userspace is rather lax in providing the
extra page (or preventing the read past the end of its bo) and so
without adding a sentinel page behind every bo you quickly generate
false positives. (Unless you also run a fixed userspace).

If you are prepared to fix userspace, tweaking the kernel not to install
scratch pages everywhere is trivial.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/6] Page faults to help user space debug

[Ben Widawsky]

On Fri, Jun 28, 2013 at 11:39:53PM +0100, Chris Wilson wrote:
> On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
> > This series originated from the request from Paul, "can you enable page
> > faults"?  After some though and discussion, we came up with 3 debug features to
> > implement:
> 
> The issue lies in that the CS and EU units like to prefetch 128 bytes
> and will cross page boundaries. Userspace is rather lax in providing the
> extra page (or preventing the read past the end of its bo) and so
> without adding a sentinel page behind every bo you quickly generate
> false positives. (Unless you also run a fixed userspace).
> 
> If you are prepared to fix userspace, tweaking the kernel not to install
> scratch pages everywhere is trivial.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I'll leave the further discussion of whether or not they still want all
the features to Paul (I think the synchronous unbinding execbuf is
useful regardless).

As for the trivial, "remove the scratch page" I didn't think that was
the plan as generally we'd like to carry on as long as possible, and we
know faults are fatal. I think keeping the scratch page, even if we
didn't have to worry about prefetching is what we want.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/6] Page faults to help user space debug

[Chris Wilson]

On Fri, Jun 28, 2013 at 04:30:44PM -0700, Ben Widawsky wrote:
> On Fri, Jun 28, 2013 at 11:39:53PM +0100, Chris Wilson wrote:
> > On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
> > > This series originated from the request from Paul, "can you enable page
> > > faults"?  After some though and discussion, we came up with 3 debug features to
> > > implement:
> > 
> > The issue lies in that the CS and EU units like to prefetch 128 bytes
> > and will cross page boundaries. Userspace is rather lax in providing the
> > extra page (or preventing the read past the end of its bo) and so
> > without adding a sentinel page behind every bo you quickly generate
> > false positives. (Unless you also run a fixed userspace).
> > 
> > If you are prepared to fix userspace, tweaking the kernel not to install
> > scratch pages everywhere is trivial.
> 
> I'll leave the further discussion of whether or not they still want all
> the features to Paul (I think the synchronous unbinding execbuf is
> useful regardless).
> 
> As for the trivial, "remove the scratch page" I didn't think that was
> the plan as generally we'd like to carry on as long as possible, and we
> know faults are fatal. I think keeping the scratch page, even if we
> didn't have to worry about prefetching is what we want.

No plans to remove the scratch page as it hides too many bugs, which
generally are fatal enough when run for long enough. I was just offering
up my experience on using page faults.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/6] Page faults to help user space debug

[Paul Berry]

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

On 28 June 2013 15:39, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
> > This series originated from the request from Paul, "can you enable page
> > faults"?  After some though and discussion, we came up with 3 debug
> features to
> > implement:
>
> The issue lies in that the CS and EU units like to prefetch 128 bytes
> and will cross page boundaries. Userspace is rather lax in providing the
> extra page (or preventing the read past the end of its bo) and so
> without adding a sentinel page behind every bo you quickly generate
> false positives. (Unless you also run a fixed userspace).
>
> If you are prepared to fix userspace, tweaking the kernel not to install
> scratch pages everywhere is trivial.
>

Mesa already adds the necessary padding to EU programs to ensure that
prefetch won't cause a page fault, and because of its "stack and heap"
model for batch buffers, CS prefetching shouldn't cause a page fault
either.  I don't know whether the 2D drivers do something similar, so they
might potentially need fixing.

Having tracked down a number of painful Mesa bugs over the last two years
where we supplied the GPU with an incorrect pointer, or we sized a buffer
incorrectly causing other buffers to be corrupted, I am fairly certain that
being able to turn off scratch pages will be a benefit in debugging--these
debug options should turn confusing, sporadic failures (and failures that
don't get noticed for months) into easily reproducible failures that get
noticed quickly.  So these debug flags will definitely be useful in
development.

If the 2D driver is lax about providing the necessary padding for prefetch,
then that will make things more difficult, but the debug flags should still
be useful.  It just means that when using the debug flags, I'll have to run
my piglit tests under gbm.

I'll try to give the patches a shot soon to make sure they work.

[-- Attachment #1.2: Type: text/html, Size: 2418 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 0/6] Page faults to help user space debug

[Chris Wilson]

On Tue, Jul 02, 2013 at 12:00:48PM -0700, Paul Berry wrote:
>    On 28 June 2013 15:39, Chris Wilson <[1]chris@chris-wilson.co.uk> wrote:
> 
>      On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
>      > This series originated from the request from Paul, "can you enable
>      page
>      > faults"? �After some though and discussion, we came up with 3 debug
>      features to
>      > implement:
> 
>      The issue lies in that the CS and EU units like to prefetch 128 bytes
>      and will cross page boundaries. Userspace is rather lax in providing the
>      extra page (or preventing the read past the end of its bo) and so
>      without adding a sentinel page behind every bo you quickly generate
>      false positives. (Unless you also run a fixed userspace).
> 
>      If you are prepared to fix userspace, tweaking the kernel not to install
>      scratch pages everywhere is trivial.
> 
>    Mesa already adds the necessary padding to EU programs to ensure that
>    prefetch won't cause a page fault, and because of its "stack and heap"
>    model for batch buffers, CS prefetching shouldn't cause a page fault
>    either.� I don't know whether the 2D drivers do something similar, so they
>    might potentially need fixing.

You do not on the BLT ring, and have not done historically. The EU is no
longer padded. We have been purposely lax in this area.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Page faults to help user space debug

[Paul Berry]

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

On 2 July 2013 12:03, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, Jul 02, 2013 at 12:00:48PM -0700, Paul Berry wrote:
> >    On 28 June 2013 15:39, Chris Wilson <[1]chris@chris-wilson.co.uk>
> wrote:
> >
> >      On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
> >      > This series originated from the request from Paul, "can you enable
> >      page
> >      > faults"? �After some though and discussion, we came up with 3
> debug
> >      features to
> >      > implement:
> >
> >      The issue lies in that the CS and EU units like to prefetch 128
> bytes
> >      and will cross page boundaries. Userspace is rather lax in
> providing the
> >      extra page (or preventing the read past the end of its bo) and so
> >      without adding a sentinel page behind every bo you quickly generate
> >      false positives. (Unless you also run a fixed userspace).
> >
> >      If you are prepared to fix userspace, tweaking the kernel not to
> install
> >      scratch pages everywhere is trivial.
> >
> >    Mesa already adds the necessary padding to EU programs to ensure that
> >    prefetch won't cause a page fault, and because of its "stack and heap"
> >    model for batch buffers, CS prefetching shouldn't cause a page fault
> >    either.� I don't know whether the 2D drivers do something similar, so
> they
> >    might potentially need fixing.
>
> You do not on the BLT ring, and have not done historically. The EU is no
> longer padded. We have been purposely lax in this area.
> -Chris
>

You're right--I didn't realize that we removed the EU padding, and I wasn't
thinking about the BLT code.

[-- Attachment #1.2: Type: text/html, Size: 2329 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 0/6] Page faults to help user space debug

[Ben Widawsky]

Can we get a collective status update? I'm touching some code at the
moment which will have some uncomfortable conflicts with this series,
and I'd like to have an idea on the plan.

On Tue, Jul 02, 2013 at 12:18:55PM -0700, Paul Berry wrote:
>    On 2 July 2013 12:03, Chris Wilson <[1]chris@chris-wilson.co.uk> wrote:
> 
>    On Tue, Jul 02, 2013 at 12:00:48PM -0700, Paul Berry wrote:
>    > Â  Â On 28 June 2013 15:39, Chris Wilson
>    <[1][2]chris@chris-wilson.co.uk> wrote:
>    >
>    > Â  Â  Â On Fri, Jun 28, 2013 at 03:23:31PM -0700, Ben Widawsky wrote:
>    > Â  Â  Â > This series originated from the request from Paul, "can you
>    enable
>    > Â  Â  Â page
>    >      > faults"? �After some though and discussion, we came up
>    with 3 debug
>    > Â  Â  Â features to
>    > Â  Â  Â > implement:
>    >
>    > Â  Â  Â The issue lies in that the CS and EU units like to prefetch
>    128 bytes
>    > Â  Â  Â and will cross page boundaries. Userspace is rather lax in
>    providing the
>    > Â  Â  Â extra page (or preventing the read past the end of its bo)
>    and so
>    > Â  Â  Â without adding a sentinel page behind every bo you quickly
>    generate
>    > Â  Â  Â false positives. (Unless you also run a fixed userspace).
>    >
>    > Â  Â  Â If you are prepared to fix userspace, tweaking the kernel not
>    to install
>    > Â  Â  Â scratch pages everywhere is trivial.
>    >
>    > Â  Â Mesa already adds the necessary padding to EU programs to ensure
>    that
>    > Â  Â prefetch won't cause a page fault, and because of its "stack and
>    heap"
>    > Â  Â model for batch buffers, CS prefetching shouldn't cause a page
>    fault
>    >    either.� I don't know whether the 2D drivers do something
>    similar, so they
>    > Â  Â might potentially need fixing.
> 
>      You do not on the BLT ring, and have not done historically. The EU
>      is no
>      longer padded. We have been purposely lax in this area.
> 
>    -Chris
> 
>    You're right--I didn't realize that we removed the EU padding, and I
>    wasn't thinking about the BLT code.
> 
> References
> 
>    1. mailto:chris@chris-wilson.co.uk
>    2. mailto:chris@chris-wilson.co.uk

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Page faults to help user space debug

[Ben Widawsky]

On Tue, Jul 02, 2013 at 12:00:48PM -0700, Paul Berry wrote:
>    On 28 June 2013 15:39, Chris Wilson <[1]chris@chris-wilson.co.uk>
>    wrote:
> 
[snip]
>    I'll try to give the patches a shot soon to make sure they work.

By the way, if it wasn't clear, any combinations of the debug flags
should work, ie. echo 7 > ....

> 
> References
> 
>    1. mailto:chris@chris-wilson.co.uk

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/6] Page faults to help user space debug

[Daniel Vetter]

On Sat, Jun 29, 2013 at 12:23 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> 2. echo 2 > /sys/kernel/debug/dri/0/i915_debug_flags
> Unbind all buffers synchronously, so any batch which has bad code will fault
> every time and not get lucky by hitting a buffer it bound. This will not catch
> cases where it accidentally reads/writes someone else's buffer.

We already have this as the drop_caches debugfs interface. Chris used
it to reproduce the 3.7 reloc-tree regression, the only downside is
that no one ever got around to write the respective i-g-t ... :(
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/6] Page faults to help user space debug

[Ben Widawsky]

On Sat, Jun 29, 2013 at 04:38:00PM +0200, Daniel Vetter wrote:
> On Sat, Jun 29, 2013 at 12:23 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > 2. echo 2 > /sys/kernel/debug/dri/0/i915_debug_flags
> > Unbind all buffers synchronously, so any batch which has bad code will fault
> > every time and not get lucky by hitting a buffer it bound. This will not catch
> > cases where it accidentally reads/writes someone else's buffer.
> 
> We already have this as the drop_caches debugfs interface. Chris used
> it to reproduce the 3.7 reloc-tree regression, the only downside is
> that no one ever got around to write the respective i-g-t ... :(
> -Daniel

In order to achieve the same behavior, one must instrument every GPU
client to use that interface before, and after every batch (you can skip
one if you're positive all clients have opted in). Also, every client
must be run as root.

This mechanism I've introduced is much simpler to achieve the intended
result.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/6] Page faults to help user space debug

[Daniel Vetter]

On Sat, Jun 29, 2013 at 02:20:40PM -0700, Ben Widawsky wrote:
> On Sat, Jun 29, 2013 at 04:38:00PM +0200, Daniel Vetter wrote:
> > On Sat, Jun 29, 2013 at 12:23 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > 2. echo 2 > /sys/kernel/debug/dri/0/i915_debug_flags
> > > Unbind all buffers synchronously, so any batch which has bad code will fault
> > > every time and not get lucky by hitting a buffer it bound. This will not catch
> > > cases where it accidentally reads/writes someone else's buffer.
> > 
> > We already have this as the drop_caches debugfs interface. Chris used
> > it to reproduce the 3.7 reloc-tree regression, the only downside is
> > that no one ever got around to write the respective i-g-t ... :(
> > -Daniel
> 
> In order to achieve the same behavior, one must instrument every GPU
> client to use that interface before, and after every batch (you can skip
> one if you're positive all clients have opted in). Also, every client
> must be run as root.
> 
> This mechanism I've introduced is much simpler to achieve the intended
> result.

Oh right, your patch is doing something else completely. One thing which
would be neat on top of this is to randomize the locations of each bo a
bit (varibale padding in front should be good enough). That way we can
check whether the relocations are all handled properly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch