[PATCH 1/5] drm: Define drm_mm_for_each_node_in_range()

[PATCH 1/5] drm: Define drm_mm_for_each_node_in_range()

[Chris Wilson]

Some clients would like to iterate over every node within a certain
range. Make a nice little macro for them to hide the mixing of the
rbtree search and linear walk.

v2: Blurb

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/drm_mm.c | 11 ++---------
 include/drm/drm_mm.h     | 22 +++++++++++++++++++---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 632473beb40c..f8eebbde376e 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -174,19 +174,12 @@ INTERVAL_TREE_DEFINE(struct drm_mm_node, rb,
 		     START, LAST, static inline, drm_mm_interval_tree)
 
 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
 {
 	return drm_mm_interval_tree_iter_first(&mm->interval_tree,
 					       start, last);
 }
-EXPORT_SYMBOL(drm_mm_interval_first);
-
-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last)
-{
-	return drm_mm_interval_tree_iter_next(node, start, last);
-}
-EXPORT_SYMBOL(drm_mm_interval_next);
+EXPORT_SYMBOL(__drm_mm_interval_first);
 
 static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
 					  struct drm_mm_node *node)
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 41ddafe92b2f..6add455c651b 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -308,10 +308,26 @@ void drm_mm_takedown(struct drm_mm *mm);
 bool drm_mm_clean(struct drm_mm *mm);
 
 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);
 
-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last);
+/**
+ * drm_mm_for_each_node_in_range - iterator to walk over a range of
+ * allocated nodes
+ * @node: drm_mm_node structure to assign to in each iteration step
+ * @mm: drm_mm allocator to walk
+ * @start: starting offset, the first node will overlap this
+ * @end: ending offset, the last node will start before this (but may overlap)
+ *
+ * This iterator walks over all nodes in the range allocator that lie
+ * between @start and @end. It is implemented similarly to list_for_each(),
+ * but using the internal interval tree to accelerate the search for the
+ * starting node, and so not safe against removal of elements. It assumes
+ * that @end is within (or is the upper limit of) the drm_mm allocator.
+ */
+#define drm_mm_for_each_node_in_range(node, mm, start, end)		\
+	for (node = __drm_mm_interval_first((mm), (start), (end)-1);	\
+	     node && node->start < (end);				\
+	     node = list_next_entry(node, node_list))			\
 
 void drm_mm_init_scan(struct drm_mm *mm,
 		      u64 size,
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm: Check against color expansion in drm_mm_reserve_node()

[Chris Wilson]

Use the color_adjust callback when reserving a node to check if
inserting a node into this hole requires any additional space, and so if
that space then conflicts with an existing allocation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/drm_mm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index f8eebbde376e..025dcd8cadcb 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -306,6 +306,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
 	u64 end = node->start + node->size;
 	struct drm_mm_node *hole;
 	u64 hole_start, hole_end;
+	u64 adj_start, adj_end;
 
 	if (WARN_ON(node->size == 0))
 		return -EINVAL;
@@ -327,9 +328,13 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
 	if (!hole->hole_follows)
 		return -ENOSPC;
 
-	hole_start = __drm_mm_hole_node_start(hole);
-	hole_end = __drm_mm_hole_node_end(hole);
-	if (hole_start > node->start || hole_end < end)
+	adj_start = hole_start = __drm_mm_hole_node_start(hole);
+	adj_end = hole_end = __drm_mm_hole_node_end(hole);
+
+	if (mm->color_adjust)
+		mm->color_adjust(hole, node->color, &adj_start, &adj_end);
+
+	if (adj_start > node->start || adj_end < end)
 		return -ENOSPC;
 
 	node->mm = mm;
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: Check against color expansion in drm_mm_reserve_node()

[Daniel Vetter]

On Wed, Nov 23, 2016 at 02:11:15PM +0000, Chris Wilson wrote:
> Use the color_adjust callback when reserving a node to check if
> inserting a node into this hole requires any additional space, and so if
> that space then conflicts with an existing allocation.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Both applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_mm.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index f8eebbde376e..025dcd8cadcb 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -306,6 +306,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  	u64 end = node->start + node->size;
>  	struct drm_mm_node *hole;
>  	u64 hole_start, hole_end;
> +	u64 adj_start, adj_end;
>  
>  	if (WARN_ON(node->size == 0))
>  		return -EINVAL;
> @@ -327,9 +328,13 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  	if (!hole->hole_follows)
>  		return -ENOSPC;
>  
> -	hole_start = __drm_mm_hole_node_start(hole);
> -	hole_end = __drm_mm_hole_node_end(hole);
> -	if (hole_start > node->start || hole_end < end)
> +	adj_start = hole_start = __drm_mm_hole_node_start(hole);
> +	adj_end = hole_end = __drm_mm_hole_node_end(hole);
> +
> +	if (mm->color_adjust)
> +		mm->color_adjust(hole, node->color, &adj_start, &adj_end);
> +
> +	if (adj_start > node->start || adj_end < end)
>  		return -ENOSPC;
>  
>  	node->mm = mm;
> -- 
> 2.10.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: Check against color expansion in drm_mm_reserve_node()

[Daniel Vetter]

On Wed, Nov 23, 2016 at 02:11:15PM +0000, Chris Wilson wrote:
> Use the color_adjust callback when reserving a node to check if
> inserting a node into this hole requires any additional space, and so if
> that space then conflicts with an existing allocation.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Both drm_mm patches merged to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_mm.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index f8eebbde376e..025dcd8cadcb 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -306,6 +306,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  	u64 end = node->start + node->size;
>  	struct drm_mm_node *hole;
>  	u64 hole_start, hole_end;
> +	u64 adj_start, adj_end;
>  
>  	if (WARN_ON(node->size == 0))
>  		return -EINVAL;
> @@ -327,9 +328,13 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  	if (!hole->hole_follows)
>  		return -ENOSPC;
>  
> -	hole_start = __drm_mm_hole_node_start(hole);
> -	hole_end = __drm_mm_hole_node_end(hole);
> -	if (hole_start > node->start || hole_end < end)
> +	adj_start = hole_start = __drm_mm_hole_node_start(hole);
> +	adj_end = hole_end = __drm_mm_hole_node_end(hole);
> +
> +	if (mm->color_adjust)
> +		mm->color_adjust(hole, node->color, &adj_start, &adj_end);
> +
> +	if (adj_start > node->start || adj_end < end)
>  		return -ENOSPC;
>  
>  	node->mm = mm;
> -- 
> 2.10.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Mark all non-vma being inserted into the address spaces

[Chris Wilson]

We need to distinguish between full i915_vma structs and simple
drm_mm_nodes when considering eviction (i.e. we must be careful not to
treat a mere drm_mm_node as a much larger i915_vma causing memory
corruption, if we are lucky). To do this, color these not-a-vma with -1
(I915_COLOR_UNEVICTABLE).

v2...v200: New name for -1.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gvt/aperture_gm.c     |  7 +++++--
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem.c            |  3 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 11 ++++++-----
 5 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c b/drivers/gpu/drm/i915/gvt/aperture_gm.c
index 0d41ebc4aea6..7d33b607bc89 100644
--- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
+++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
@@ -73,12 +73,15 @@ static int alloc_gm(struct intel_vgpu *vgpu, bool high_gm)
 	mutex_lock(&dev_priv->drm.struct_mutex);
 search_again:
 	ret = drm_mm_insert_node_in_range_generic(&dev_priv->ggtt.base.mm,
-						  node, size, 4096, 0,
+						  node, size, 4096,
+						  I915_COLOR_UNEVICTABLE,
 						  start, end, search_flag,
 						  alloc_flag);
 	if (ret) {
 		ret = i915_gem_evict_something(&dev_priv->ggtt.base,
-					       size, 4096, 0, start, end, 0);
+					       size, 4096,
+					       I915_COLOR_UNEVICTABLE,
+					       start, end, 0);
 		if (ret == 0 && ++retried < 3)
 			goto search_again;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 970e50bf9884..b8e65b189635 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -912,6 +912,7 @@ enum i915_cache_level {
 			      the CPU, but L3 is only visible to the GPU. */
 	I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */
 };
+#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */
 
 #define DEFAULT_CONTEXT_HANDLE 0
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3d4e07e9734f..097b3154b0ef 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -70,7 +70,8 @@ insert_mappable_node(struct i915_ggtt *ggtt,
 {
 	memset(node, 0, sizeof(*node));
 	return drm_mm_insert_node_in_range_generic(&ggtt->base.mm, node,
-						   size, 0, -1,
+						   size, 0,
+						   I915_COLOR_UNEVICTABLE,
 						   0, ggtt->mappable_end,
 						   DRM_MM_SEARCH_DEFAULT,
 						   DRM_MM_CREATE_DEFAULT);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 522ecfb4dc9d..8ad6a64c9e01 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -437,7 +437,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
 			memset(&cache->node, 0, sizeof(cache->node));
 			ret = drm_mm_insert_node_in_range_generic
 				(&ggtt->base.mm, &cache->node,
-				 4096, 0, 0,
+				 4096, 0, I915_COLOR_UNEVICTABLE,
 				 0, ggtt->mappable_end,
 				 DRM_MM_SEARCH_DEFAULT,
 				 DRM_MM_CREATE_DEFAULT);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b4bde1452f2a..bc210c5ecc13 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2076,15 +2076,15 @@ static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt)
 		return ret;
 
 alloc:
-	ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm,
-						  &ppgtt->node, GEN6_PD_SIZE,
-						  GEN6_PD_ALIGN, 0,
+	ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm, &ppgtt->node,
+						  GEN6_PD_SIZE, GEN6_PD_ALIGN,
+						  I915_COLOR_UNEVICTABLE,
 						  0, ggtt->base.total,
 						  DRM_MM_TOPDOWN);
 	if (ret == -ENOSPC && !retried) {
 		ret = i915_gem_evict_something(&ggtt->base,
 					       GEN6_PD_SIZE, GEN6_PD_ALIGN,
-					       I915_CACHE_NONE,
+					       I915_COLOR_UNEVICTABLE,
 					       0, ggtt->base.total,
 					       0);
 		if (ret)
@@ -2760,7 +2760,8 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	/* Reserve a mappable slot for our lockless error capture */
 	ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm,
 						  &ggtt->error_capture,
-						  4096, 0, -1,
+						  4096, 0,
+						  I915_COLOR_UNEVICTABLE,
 						  0, ggtt->mappable_end,
 						  0, 0);
 	if (ret)
-- 
2.10.2

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

Re: [PATCH 3/5] drm/i915: Mark all non-vma being inserted into the address spaces

[Joonas Lahtinen]

On ke, 2016-11-23 at 14:11 +0000, Chris Wilson wrote:
> We need to distinguish between full i915_vma structs and simple
> drm_mm_nodes when considering eviction (i.e. we must be careful not to
> treat a mere drm_mm_node as a much larger i915_vma causing memory
> corruption, if we are lucky). To do this, color these not-a-vma with -1
> (I915_COLOR_UNEVICTABLE).
> 
> v2...v200: New name for -1.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Chris Wilson]

Soft-pinning depends upon being able to check for availabilty of an
interval and evict overlapping object from a drm_mm range manager very
quickly. Currently it uses a linear list, and so performance is dire and
not suitable as a general replacement. Worse, the current code will oops
if it tries to evict an active buffer.

It also helps if the routine reports the correct error codes as expected
by its callers and emits a tracepoint upon use.

For posterity since the wrong patch was pushed (i.e. that missed these
key points and had known bugs), this is the changelog that should have
been on commit 506a8e87d8d2 ("drm/i915: Add soft-pinning API for
execbuffer"):

Userspace can pass in an offset that it presumes the object is located
at. The kernel will then do its utmost to fit the object into that
location. The assumption is that userspace is handling its own object
locations (for example along with full-ppgtt) and that the kernel will
rarely have to make space for the user's requests.

This extends the DRM_IOCTL_I915_GEM_EXECBUFFER2 to do the following:
* if the user supplies a virtual address via the execobject->offset
  *and* sets the EXEC_OBJECT_PINNED flag in execobject->flags, then
  that object is placed at that offset in the address space selected
  by the context specifier in execbuffer.
* the location must be aligned to the GTT page size, 4096 bytes
* as the object is placed exactly as specified, it may be used by this
  execbuffer call without relocations pointing to it

It may fail to do so if:
* EINVAL is returned if the object does not have a 4096 byte aligned
  address
* the object conflicts with another pinned object (either pinned by
  hardware in that address space, e.g. scanouts in the aliasing ppgtt)
  or within the same batch.
  EBUSY is returned if the location is pinned by hardware
  EINVAL is returned if the location is already in use by the batch
* EINVAL is returned if the object conflicts with its own alignment (as meets
  the hardware requirements) or if the placement of the object does not fit
  within the address space

All other execbuffer errors apply.

Presence of this execbuf extension may be queried by passing
I915_PARAM_HAS_EXEC_SOFTPIN to DRM_IOCTL_I915_GETPARAM and checking for
a reported value of 1 (or greater).

v2: Combine the hole/adjusted-hole ENOSPC checks
v3: More color, more splitting, more blurb.

Fixes: 506a8e87d8d2 ("drm/i915: Add soft-pinning API for execbuffer")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h            |   3 +-
 drivers/gpu/drm/i915/i915_gem_evict.c      | 104 ++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   1 +
 drivers/gpu/drm/i915/i915_trace.h          |  23 +++++++
 drivers/gpu/drm/i915/i915_vma.c            |   2 +-
 5 files changed, 106 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b8e65b189635..423665904e0d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3410,7 +3410,8 @@ int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  unsigned cache_level,
 					  u64 start, u64 end,
 					  unsigned flags);
-int __must_check i915_gem_evict_for_vma(struct i915_vma *target);
+int __must_check i915_gem_evict_for_vma(struct i915_vma *vma,
+					unsigned int flags);
 int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
 
 /* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index bd08814b015c..e6104c659e3e 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -212,45 +212,99 @@ i915_gem_evict_something(struct i915_address_space *vm,
 	return ret;
 }
 
-int
-i915_gem_evict_for_vma(struct i915_vma *target)
+/**
+ * i915_gem_evict_for_vma - Evict vmas to make room for binding a new one
+ * @target: address space and range to evict for
+ * @flags: additional flags to control the eviction algorithm
+ *
+ * This function will try to evict vmas that overlap the target node.
+ *
+ * To clarify: This is for freeing up virtual address space, not for freeing
+ * memory in e.g. the shrinker.
+ */
+int i915_gem_evict_for_vma(struct i915_vma *target, unsigned int flags)
 {
-	struct drm_mm_node *node, *next;
+	LIST_HEAD(eviction_list);
+	struct drm_mm_node *node;
+	u64 start = target->node.start;
+	u64 end = start + target->node.size;
+	struct i915_vma *vma, *next;
+	bool check_color;
+	int ret = 0;
 
 	lockdep_assert_held(&target->vm->dev->struct_mutex);
+	trace_i915_gem_evict_vma(target, flags);
+
+	check_color = target->vm->mm.color_adjust;
+	if (check_color) {
+		/* Expand search to cover neighbouring guard pages (or lack!) */
+		if (start > target->vm->start)
+			start -= 4096;
+		if (end < target->vm->start + target->vm->total)
+			end += 4096;
+	}
 
-	list_for_each_entry_safe(node, next,
-			&target->vm->mm.head_node.node_list,
-			node_list) {
-		struct i915_vma *vma;
-		int ret;
-
-		if (node->start + node->size <= target->node.start)
-			continue;
-		if (node->start >= target->node.start + target->node.size)
+	drm_mm_for_each_node_in_range(node, &target->vm->mm, start, end) {
+		/* If we find any non-objects (!vma), we cannot evict them */
+		if (node->color == I915_COLOR_UNEVICTABLE) {
+			ret = -ENOSPC;
 			break;
+		}
 
 		vma = container_of(node, typeof(*vma), node);
 
-		if (i915_vma_is_pinned(vma)) {
-			if (!vma->exec_entry || i915_vma_pin_count(vma) > 1)
-				/* Object is pinned for some other use */
-				return -EBUSY;
+		/* If we are using coloring to insert guard pages between
+		 * different cache domains within the address space, we have
+		 * to check whether the objects on either side of our range
+		 * abutt and conflict. If they are in conflict, then we evict
+		 * those as well to make room for our guard pages.
+		 */
+		if (check_color) {
+			if (vma->node.start + vma->node.size == target->node.start) {
+				if (vma->node.color == target->node.color)
+					continue;
+			}
+			if (vma->node.start == target->node.start + target->node.size) {
+				if (vma->node.color == target->node.color)
+					continue;
+			}
+		}
 
-			/* We need to evict a buffer in the same batch */
-			if (vma->exec_entry->flags & EXEC_OBJECT_PINNED)
-				/* Overlapping fixed objects in the same batch */
-				return -EINVAL;
+		if (flags & PIN_NONBLOCK &&
+		    (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))) {
+			ret = -ENOSPC;
+			break;
+		}
 
-			return -ENOSPC;
+		/* Overlap of objects in the same batch? */
+		if (i915_vma_is_pinned(vma)) {
+			ret = -ENOSPC;
+			if (vma->exec_entry &&
+			    vma->exec_entry->flags & EXEC_OBJECT_PINNED)
+				ret = -EINVAL;
+			break;
 		}
 
-		ret = i915_vma_unbind(vma);
-		if (ret)
-			return ret;
+		/* Never show fear in the face of dragons!
+		 *
+		 * We cannot directly remove this node from within this
+		 * iterator and as with i915_gem_evict_something() we employ
+		 * the vma pin_count in order to prevent the action of
+		 * unbinding one vma from freeing (by dropping its active
+		 * reference) another in our eviction list.
+		 */
+		__i915_vma_pin(vma);
+		list_add(&vma->exec_list, &eviction_list);
 	}
 
-	return 0;
+	list_for_each_entry_safe(vma, next, &eviction_list, exec_list) {
+		list_del_init(&vma->exec_list);
+		__i915_vma_unpin(vma);
+		if (ret == 0)
+			ret = i915_vma_unbind(vma);
+	}
+
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 8ad6a64c9e01..35f31fb778c8 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -274,6 +274,7 @@ static void eb_destroy(struct eb_vmas *eb)
 				       exec_list);
 		list_del_init(&vma->exec_list);
 		i915_gem_execbuffer_unreserve_vma(vma);
+		vma->exec_entry = NULL;
 		i915_vma_put(vma);
 	}
 	kfree(eb);
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index c5d210ebaa9a..2ed60ed70fe1 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -450,6 +450,29 @@ TRACE_EVENT(i915_gem_evict_vm,
 	    TP_printk("dev=%d, vm=%p", __entry->dev, __entry->vm)
 );
 
+TRACE_EVENT(i915_gem_evict_vma,
+	    TP_PROTO(struct i915_vma *vma, unsigned int flags),
+	    TP_ARGS(vma, flags),
+
+	    TP_STRUCT__entry(
+			     __field(u32, dev)
+			     __field(struct i915_address_space *, vm)
+			     __field(u64, start)
+			     __field(u64, size)
+			     __field(unsigned int, flags)
+			    ),
+
+	    TP_fast_assign(
+			   __entry->dev = vma->vm->dev->primary->index;
+			   __entry->vm = vma->vm;
+			   __entry->start = vma->node.start;
+			   __entry->size = vma->node.size;
+			   __entry->flags = flags;
+			  ),
+
+	    TP_printk("dev=%d, vm=%p, start=%llx size=%llx, flags=%x", __entry->dev, __entry->vm, (long long)__entry->start, (long long)__entry->size, __entry->flags)
+);
+
 TRACE_EVENT(i915_gem_ring_sync_to,
 	    TP_PROTO(struct drm_i915_gem_request *to,
 		     struct drm_i915_gem_request *from),
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index a792dcb902b5..19f17b02f4a0 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -401,7 +401,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 		vma->node.color = obj->cache_level;
 		ret = drm_mm_reserve_node(&vma->vm->mm, &vma->node);
 		if (ret) {
-			ret = i915_gem_evict_for_vma(vma);
+			ret = i915_gem_evict_for_vma(vma, flags);
 			if (ret == 0)
 				ret = drm_mm_reserve_node(&vma->vm->mm, &vma->node);
 			if (ret)
-- 
2.10.2

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

Re: [PATCH 4/5] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Joonas Lahtinen]

On ke, 2016-11-23 at 14:11 +0000, Chris Wilson wrote:
> Soft-pinning depends upon being able to check for availabilty of an
> interval and evict overlapping object from a drm_mm range manager very
> quickly. Currently it uses a linear list, and so performance is dire and
> not suitable as a general replacement. Worse, the current code will oops
> if it tries to evict an active buffer.
> 
> It also helps if the routine reports the correct error codes as expected
> by its callers and emits a tracepoint upon use.
> 
> For posterity since the wrong patch was pushed (i.e. that missed these
> key points and had known bugs), this is the changelog that should have
> been on commit 506a8e87d8d2 ("drm/i915: Add soft-pinning API for
> execbuffer"):
> 
> Userspace can pass in an offset that it presumes the object is located
> at. The kernel will then do its utmost to fit the object into that
> location. The assumption is that userspace is handling its own object
> locations (for example along with full-ppgtt) and that the kernel will
> rarely have to make space for the user's requests.
> 
> This extends the DRM_IOCTL_I915_GEM_EXECBUFFER2 to do the following:
> * if the user supplies a virtual address via the execobject->offset
>   *and* sets the EXEC_OBJECT_PINNED flag in execobject->flags, then
>   that object is placed at that offset in the address space selected
>   by the context specifier in execbuffer.
> * the location must be aligned to the GTT page size, 4096 bytes
> * as the object is placed exactly as specified, it may be used by this
>   execbuffer call without relocations pointing to it
> 
> It may fail to do so if:
> * EINVAL is returned if the object does not have a 4096 byte aligned
>   address
> * the object conflicts with another pinned object (either pinned by
>   hardware in that address space, e.g. scanouts in the aliasing ppgtt)
>   or within the same batch.
>   EBUSY is returned if the location is pinned by hardware
>   EINVAL is returned if the location is already in use by the batch
> * EINVAL is returned if the object conflicts with its own alignment (as meets
>   the hardware requirements) or if the placement of the object does not fit
>   within the address space
> 
> All other execbuffer errors apply.
> 
> Presence of this execbuf extension may be queried by passing
> I915_PARAM_HAS_EXEC_SOFTPIN to DRM_IOCTL_I915_GETPARAM and checking for
> a reported value of 1 (or greater).
> 
> v2: Combine the hole/adjusted-hole ENOSPC checks
> v3: More color, more splitting, more blurb.
> 
> Fixes: 506a8e87d8d2 ("drm/i915: Add soft-pinning API for execbuffer")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +TRACE_EVENT(i915_gem_evict_vma,
> +	    TP_PROTO(struct i915_vma *vma, unsigned int flags),
> +	    TP_ARGS(vma, flags),
> +
> +	    TP_STRUCT__entry(
> +			     __field(u32, dev)
> +			     __field(struct i915_address_space *, vm)
> +			     __field(u64, start)
> +			     __field(u64, size)
> +			     __field(unsigned int, flags)
> +			    ),
> +
> +	    TP_fast_assign(
> +			   __entry->dev = vma->vm->dev->primary->index;
> +			   __entry->vm = vma->vm;
> +			   __entry->start = vma->node.start;
> +			   __entry->size = vma->node.size;
> +			   __entry->flags = flags;
> +			  ),
> +
> +	    TP_printk("dev=%d, vm=%p, start=%llx size=%llx, flags=%x", __entry->dev, __entry->vm, (long long)__entry->start, (long long)__entry->size, __entry->flags)

A few newlines could be useful. (long long) explicit conversion is not
done elsewhere in the func but should not hurt. Can't complain on lack
of consistency as it's wild already :P

With the newlines, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Chris Wilson]

On Thu, Nov 24, 2016 at 01:45:12PM +0200, Joonas Lahtinen wrote:
> On ke, 2016-11-23 at 14:11 +0000, Chris Wilson wrote:
> > Soft-pinning depends upon being able to check for availabilty of an
> > interval and evict overlapping object from a drm_mm range manager very
> > quickly. Currently it uses a linear list, and so performance is dire and
> > not suitable as a general replacement. Worse, the current code will oops
> > if it tries to evict an active buffer.
> > 
> > It also helps if the routine reports the correct error codes as expected
> > by its callers and emits a tracepoint upon use.
> > 
> > For posterity since the wrong patch was pushed (i.e. that missed these
> > key points and had known bugs), this is the changelog that should have
> > been on commit 506a8e87d8d2 ("drm/i915: Add soft-pinning API for
> > execbuffer"):
> > 
> > Userspace can pass in an offset that it presumes the object is located
> > at. The kernel will then do its utmost to fit the object into that
> > location. The assumption is that userspace is handling its own object
> > locations (for example along with full-ppgtt) and that the kernel will
> > rarely have to make space for the user's requests.
> > 
> > This extends the DRM_IOCTL_I915_GEM_EXECBUFFER2 to do the following:
> > * if the user supplies a virtual address via the execobject->offset
> >   *and* sets the EXEC_OBJECT_PINNED flag in execobject->flags, then
> >   that object is placed at that offset in the address space selected
> >   by the context specifier in execbuffer.
> > * the location must be aligned to the GTT page size, 4096 bytes
> > * as the object is placed exactly as specified, it may be used by this
> >   execbuffer call without relocations pointing to it
> > 
> > It may fail to do so if:
> > * EINVAL is returned if the object does not have a 4096 byte aligned
> >   address
> > * the object conflicts with another pinned object (either pinned by
> >   hardware in that address space, e.g. scanouts in the aliasing ppgtt)
> >   or within the same batch.
> >   EBUSY is returned if the location is pinned by hardware
> >   EINVAL is returned if the location is already in use by the batch
> > * EINVAL is returned if the object conflicts with its own alignment (as meets
> >   the hardware requirements) or if the placement of the object does not fit
> >   within the address space
> > 
> > All other execbuffer errors apply.
> > 
> > Presence of this execbuf extension may be queried by passing
> > I915_PARAM_HAS_EXEC_SOFTPIN to DRM_IOCTL_I915_GETPARAM and checking for
> > a reported value of 1 (or greater).
> > 
> > v2: Combine the hole/adjusted-hole ENOSPC checks
> > v3: More color, more splitting, more blurb.
> > 
> > Fixes: 506a8e87d8d2 ("drm/i915: Add soft-pinning API for execbuffer")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +TRACE_EVENT(i915_gem_evict_vma,
> > +	    TP_PROTO(struct i915_vma *vma, unsigned int flags),
> > +	    TP_ARGS(vma, flags),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(u32, dev)
> > +			     __field(struct i915_address_space *, vm)
> > +			     __field(u64, start)
> > +			     __field(u64, size)
> > +			     __field(unsigned int, flags)
> > +			    ),
> > +
> > +	    TP_fast_assign(
> > +			   __entry->dev = vma->vm->dev->primary->index;
> > +			   __entry->vm = vma->vm;
> > +			   __entry->start = vma->node.start;
> > +			   __entry->size = vma->node.size;
> > +			   __entry->flags = flags;
> > +			  ),
> > +
> > +	    TP_printk("dev=%d, vm=%p, start=%llx size=%llx, flags=%x", __entry->dev, __entry->vm, (long long)__entry->start, (long long)__entry->size, __entry->flags)
> 
> A few newlines could be useful. (long long) explicit conversion is not
> done elsewhere in the func but should not hurt. Can't complain on lack
> of consistency as it's wild already :P

The (long long) is wrong. u64 should be %llx. Can't remember if there is
a good excuse or not (it was almost certainly copied from elsewhere ;)
-Chris

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

[PATCH 5/5] drm/i915: Tidy i915_gem_valid_gtt_space()

[Chris Wilson]

We can replace a couple of tests with an assertion that the passed in
node is already allocated (as matches the existing call convention) and
by a small bit of refactoring we can bring the line lengths to under
80cols.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 19f17b02f4a0..da50be488861 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -297,10 +297,15 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
 		vma->flags &= ~I915_VMA_CAN_FENCE;
 }
 
+static bool color_differs(struct drm_mm_node *node, unsigned long color)
+{
+	return node->allocated && node->color != color;
+}
+
 bool i915_gem_valid_gtt_space(struct i915_vma *vma,
 			      unsigned long cache_level)
 {
-	struct drm_mm_node *gtt_space = &vma->node;
+	struct drm_mm_node *node = &vma->node;
 	struct drm_mm_node *other;
 
 	/*
@@ -313,18 +318,16 @@ bool i915_gem_valid_gtt_space(struct i915_vma *vma,
 	if (vma->vm->mm.color_adjust == NULL)
 		return true;
 
-	if (!drm_mm_node_allocated(gtt_space))
-		return true;
-
-	if (list_empty(&gtt_space->node_list))
-		return true;
+	/* Only valid to be called on an already inserted vma */
+	GEM_BUG_ON(!drm_mm_node_allocated(node));
+	GEM_BUG_ON(list_empty(&node->node_list));
 
-	other = list_entry(gtt_space->node_list.prev, struct drm_mm_node, node_list);
-	if (other->allocated && !other->hole_follows && other->color != cache_level)
+	other = list_prev_entry(node, node_list);
+	if (color_differs(other, cache_level) && !other->hole_follows)
 		return false;
 
-	other = list_entry(gtt_space->node_list.next, struct drm_mm_node, node_list);
-	if (other->allocated && !gtt_space->hole_follows && other->color != cache_level)
+	other = list_next_entry(node, node_list);
+	if (color_differs(other, cache_level) && !node->hole_follows)
 		return false;
 
 	return true;
-- 
2.10.2

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

Re: [PATCH 5/5] drm/i915: Tidy i915_gem_valid_gtt_space()

[Joonas Lahtinen]

On ke, 2016-11-23 at 14:11 +0000, Chris Wilson wrote:
> We can replace a couple of tests with an assertion that the passed in
> node is already allocated (as matches the existing call convention) and
> by a small bit of refactoring we can bring the line lengths to under
> 80cols.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/5] drm: Define drm_mm_for_each_node_in_range()

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm: Define drm_mm_for_each_node_in_range()
URL   : https://patchwork.freedesktop.org/series/15825/
State : warning

== Summary ==

Series 15825v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/15825/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (fi-ilk-650)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:190  dwarn:1   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

01896e61d9cc0cad08e19990cd095cdf679f6142 drm-intel-nightly: 2016y-11m-23d-14h-45m-53s UTC integration manifest
753bc0e drm/i915: Tidy i915_gem_valid_gtt_space()
f85a474 drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)
9d2cdd7 drm/i915: Mark all non-vma being inserted into the address spaces
f81a48c drm: Check against color expansion in drm_mm_reserve_node()
46d7eb3 drm: Define drm_mm_for_each_node_in_range()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3093/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx