[PATCH 0/5] untested DRM node alloc cleanups

[PATCH 0/5] untested DRM node alloc cleanups

[Ben Widawsky]

Just some stuff I noticed which I should have done in the original PPGTT
series (minus the first patch, which I *did* do in the original series.

Primarily, I want to extract as much of the node allocation as possible
to be able to do something like what the stolen allocations do with
preallocated nodes. With 64b PPGTT, and something like a userptr
interface, doing this becomes a really desirable thing to do.

In any event, I think the patches stand as a nice cleanup on their own,
provided they don't blow anything up.  I haven't had a chance to do
anything but compile these yet.

Ben Widawsky (5):
  drm/i915: Use topdown allocation for PPGTT PDEs on gen6/7
  drm/i915: Extract node allocation from bind
  drm/i915: WARN on unexpected return from drm_mm
  drm/i915: Limit the number of node allocation retries
  drm/i915: Use new drm node allocator for PPGTT

 drivers/gpu/drm/i915/i915_drv.h     | 16 ++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     | 52 +++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_gtt.c | 24 ++++-------------
 3 files changed, 60 insertions(+), 32 deletions(-)

-- 
1.9.2

[PATCH 1/5] drm/i915: Use topdown allocation for PPGTT PDEs on gen6/7

[Ben Widawsky]

It was always the intention to do the topdown allocation for context
objects (Chris' idea originally). Unfortunately, I never managed to land
the patch, but someone else did, so now we can use it.

As a reminder, hardware contexts never need to be in the precious GTT
aperture space - which is what is what happens with the normal bottom up
allocation we do today. Doing a top down allocation increases the odds
that the HW contexts can get out of the way, especially with per FD
contexts as is done in full PPGTT

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9e19735..4083eab 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1849,8 +1849,7 @@ alloc:
 						  &ppgtt->node, GEN6_PD_SIZE,
 						  GEN6_PD_ALIGN, 0,
 						  0, dev_priv->gtt.base.total,
-						  DRM_MM_SEARCH_DEFAULT,
-						  DRM_MM_CREATE_DEFAULT);
+						  DRM_MM_TOPDOWN);
 	if (ret == -ENOSPC && !retried) {
 		ret = i915_gem_evict_something(dev, &dev_priv->gtt.base,
 					       GEN6_PD_SIZE, GEN6_PD_ALIGN,
-- 
1.9.2

[PATCH 2/5] drm/i915: Extract node allocation from bind

[Ben Widawsky]

The DRM node allocation code was already a bit of an ugly bit of code
within a complex function. Removing it serves the purpose of cleaning
the function up. More importantly, it provides a way to have a
preallocated (address space) VMA to easily skip this code. Something
we're very likely to need.

This is essentially a wrapper for DRM node allocation with an eviction +
retry. It is something which could be moved to core DRM eventually, if
other drivers had similar eviction semantics.

This removes a goto used for something other than error unwinding, a
generally reviled (I am neutral) practice, and replaces it with a
function call to itself that should have the same effect. Note that it's
not a recursive function as all the problem set reduction is done in the
eviction code.

Some might say this change is worse than before because we are using
stack for each subsequent call. Frankly, I'd rather overflow the stack
and blow it up than loop forever. In either case, this is addressed in
the next patch.

I believe, and intend, that other than the stack usage, there is no
functional change here.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ce9ad17..d03702ac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3216,6 +3216,34 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
 #endif
 }
 
+static int
+i915_gem_find_vm_space(struct i915_address_space *vm,
+		       struct drm_mm_node *node,
+		       unsigned long size,
+		       unsigned long align,
+		       unsigned long color,
+		       unsigned long start,
+		       unsigned long end,
+		       uint32_t flags)
+{
+	int ret;
+	ret = drm_mm_insert_node_in_range_generic(&vm->mm, node,
+						  size, align, color,
+						  start, end,
+						  DRM_MM_SEARCH_DEFAULT,
+						  DRM_MM_CREATE_DEFAULT);
+	if (ret) {
+		ret = i915_gem_evict_something(vm->dev, vm, size, align, color,
+					       flags);
+		if (ret == 0)
+			return i915_gem_find_vm_space(vm, node,
+						      size, align, color,
+						      start, end, flags);
+	}
+
+	return ret;
+}
+
 /**
  * Finds free space in the GTT aperture and binds the object there.
  */
@@ -3275,20 +3303,11 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	if (IS_ERR(vma))
 		goto err_unpin;
 
-search_free:
-	ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node,
-						  size, alignment,
-						  obj->cache_level, 0, gtt_max,
-						  DRM_MM_SEARCH_DEFAULT,
-						  DRM_MM_CREATE_DEFAULT);
-	if (ret) {
-		ret = i915_gem_evict_something(dev, vm, size, alignment,
-					       obj->cache_level, flags);
-		if (ret == 0)
-			goto search_free;
-
+	ret = i915_gem_find_vm_space(vm, &vma->node, size, alignment,
+				     obj->cache_level, 0, gtt_max, flags);
+	if (ret)
 		goto err_free_vma;
-	}
+
 	if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
 					      obj->cache_level))) {
 		ret = -EINVAL;
-- 
1.9.2

[PATCH 3/5] drm/i915: WARN on unexpected return from drm_mm

[Ben Widawsky]

We only actually want to retry if the failure mode was not enough space,
and so we'll evict. This will help us realize quickly in case we missed
a change in the common drm code.

NOTE: A similar check is already in place for the GEN7 PPGTT code.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d03702ac..1f85ec5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3233,6 +3233,9 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
 						  DRM_MM_SEARCH_DEFAULT,
 						  DRM_MM_CREATE_DEFAULT);
 	if (ret) {
+		if (WARN_ON(ret != -ENOSPC))
+			return ret;
+
 		ret = i915_gem_evict_something(vm->dev, vm, size, align, color,
 					       flags);
 		if (ret == 0)
-- 
1.9.2

[PATCH 4/5] drm/i915: Limit the number of node allocation retries

[Ben Widawsky]

AFAICT, it's impossible to actually infinitely retry the allocation in
our current code. However, a small oversight on my part, slight bug, or
future bug, could easily change that.

To avoid a potential breakage, a simple retry variable of 16 bits will
help us prevent infinitely running.

Retry is limited to 100 as a mostly random number. Some consideration
about stack usage was taken into account. As an example, if we allowed
256 retries on a 32b arch (and my memory serves that all arguments are
passed on the stack for such architectures), thats 33 bytes * 256
retries + (fudge for return address and such)... it's way more than we
want to use already. 64b architectures might be slightly better, since
6? of the first args will get passed through registers, but it's still
bad.

If anything, we might want to do way less than 100, like 3.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f85ec5..c60365a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3216,6 +3216,7 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
 #endif
 }
 
+#define MAX_VMA_FIND_RETRY 100
 static int
 i915_gem_find_vm_space(struct i915_address_space *vm,
 		       struct drm_mm_node *node,
@@ -3224,7 +3225,8 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
 		       unsigned long color,
 		       unsigned long start,
 		       unsigned long end,
-		       uint32_t flags)
+		       uint32_t flags,
+		       uint8_t retry)
 {
 	int ret;
 	ret = drm_mm_insert_node_in_range_generic(&vm->mm, node,
@@ -3232,7 +3234,7 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
 						  start, end,
 						  DRM_MM_SEARCH_DEFAULT,
 						  DRM_MM_CREATE_DEFAULT);
-	if (ret) {
+	if (ret && (retry < MAX_VMA_FIND_RETRY)) {
 		if (WARN_ON(ret != -ENOSPC))
 			return ret;
 
@@ -3241,7 +3243,8 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
 		if (ret == 0)
 			return i915_gem_find_vm_space(vm, node,
 						      size, align, color,
-						      start, end, flags);
+						      start, end, flags,
+						      retry++);
 	}
 
 	return ret;
@@ -3306,8 +3309,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	if (IS_ERR(vma))
 		goto err_unpin;
 
-	ret = i915_gem_find_vm_space(vm, &vma->node, size, alignment,
-				     obj->cache_level, 0, gtt_max, flags);
+	ret = i915_gem_find_vm_space(vm, &vma->node,
+				     size, alignment, obj->cache_level,
+				     0, gtt_max, flags, 1);
 	if (ret)
 		goto err_free_vma;
 
-- 
1.9.2

[PATCH 5/5] drm/i915: Use new drm node allocator for PPGTT

[Ben Widawsky]

The two users were already really similar. By adding the flags (I hope
you like a lot of arguments in your functions), we can satisfy both
callers quite well.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     | 16 ++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     | 34 +++++++++++++++++-----------------
 drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++------------------
 3 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a4762c0..ebaf570 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2094,6 +2094,22 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
+#define MAX_VMA_FIND_RETRY 100
+int i915_gem_find_vm_space_generic(struct i915_address_space *vm,
+				   struct drm_mm_node *node,
+				   unsigned long size,
+				   unsigned long align,
+				   unsigned long color,
+				   unsigned long start,
+				   unsigned long end,
+				   uint32_t flags,
+				   enum drm_mm_search_flags sflags,
+				   enum drm_mm_allocator_flags aflags,
+				   uint8_t retry);
+#define i915_gem_find_vm_space(vm, node, size, align, color, start, end, flags, retry) \
+	i915_gem_find_vm_space_generic(vm, node, size, align, color, \
+				       start, end, flags, DRM_MM_BOTTOMUP, retry)
+
 #define PIN_MAPPABLE 0x1
 #define PIN_NONBLOCK 0x2
 #define PIN_GLOBAL 0x4
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c60365a..72f0300 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3216,24 +3216,23 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
 #endif
 }
 
-#define MAX_VMA_FIND_RETRY 100
-static int
-i915_gem_find_vm_space(struct i915_address_space *vm,
-		       struct drm_mm_node *node,
-		       unsigned long size,
-		       unsigned long align,
-		       unsigned long color,
-		       unsigned long start,
-		       unsigned long end,
-		       uint32_t flags,
-		       uint8_t retry)
+int i915_gem_find_vm_space_generic(struct i915_address_space *vm,
+				    struct drm_mm_node *node,
+				    unsigned long size,
+				    unsigned long align,
+				    unsigned long color,
+				    unsigned long start,
+				    unsigned long end,
+				    uint32_t flags,
+				    enum drm_mm_search_flags sflags,
+				    enum drm_mm_allocator_flags aflags,
+				    uint8_t retry)
 {
 	int ret;
 	ret = drm_mm_insert_node_in_range_generic(&vm->mm, node,
 						  size, align, color,
 						  start, end,
-						  DRM_MM_SEARCH_DEFAULT,
-						  DRM_MM_CREATE_DEFAULT);
+						  sflags, aflags);
 	if (ret && (retry < MAX_VMA_FIND_RETRY)) {
 		if (WARN_ON(ret != -ENOSPC))
 			return ret;
@@ -3241,10 +3240,11 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
 		ret = i915_gem_evict_something(vm->dev, vm, size, align, color,
 					       flags);
 		if (ret == 0)
-			return i915_gem_find_vm_space(vm, node,
-						      size, align, color,
-						      start, end, flags,
-						      retry++);
+			return i915_gem_find_vm_space_generic(vm, node,
+							      size, align, color,
+							      start, end,
+							      sflags, aflags, flags,
+							      retry++);
 	}
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4083eab..62f6aee 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1831,7 +1831,6 @@ static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_device *dev = ppgtt->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool retried = false;
 	int ret;
 
 	/* PPGTT PDEs reside in the GGTT and consists of 512 entries. The
@@ -1844,23 +1843,11 @@ static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt)
 	if (IS_ERR(ppgtt->scratch_pt))
 		return PTR_ERR(ppgtt->scratch_pt);
 
-alloc:
-	ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
-						  &ppgtt->node, GEN6_PD_SIZE,
-						  GEN6_PD_ALIGN, 0,
-						  0, dev_priv->gtt.base.total,
-						  DRM_MM_TOPDOWN);
-	if (ret == -ENOSPC && !retried) {
-		ret = i915_gem_evict_something(dev, &dev_priv->gtt.base,
-					       GEN6_PD_SIZE, GEN6_PD_ALIGN,
-					       I915_CACHE_NONE, 0);
-		if (ret)
-			goto err_out;
-
-		retried = true;
-		goto alloc;
-	}
-
+	ret = i915_gem_find_vm_space_generic(&dev_priv->gtt.base, &ppgtt->node,
+					     GEN6_PD_SIZE, GEN6_PD_ALIGN, 0,
+					     0, dev_priv->gtt.base.total,
+					     DRM_MM_TOPDOWN, 0,
+					     MAX_VMA_FIND_RETRY-1);
 	if (ret)
 		goto err_out;
 
-- 
1.9.2