[PATCH v2 1/4] drm: Define drm_mm_for_each_node_in_range()

[PATCH v2 1/4] 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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_mm.c | 11 ++---------
 include/drm/drm_mm.h     |  8 +++++---
 2 files changed, 7 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..fca5f313cf18 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -308,10 +308,12 @@ 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);
+#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

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

[PATCH v2 2/4] 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@list.freedesktop.org
---
 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

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

[PATCH v2 3/4] 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
(UNCOLORED).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gvt/aperture_gm.c     |  5 +++--
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem.c            |  2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 10 +++++-----
 5 files changed, 11 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..e7b8674dea76 100644
--- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
+++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
@@ -73,12 +73,13 @@ 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, UNCOLORED,
 						  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, UNCOLORED,
+					       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 5192206c62e2..32a77ee489c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -895,6 +895,7 @@ enum i915_cache_level {
 			      the CPU, but L3 is only visible to the GPU. */
 	I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */
 };
+#define UNCOLORED (-1)
 
 struct i915_ctx_hang_stats {
 	/* This context had batch pending when hang was declared */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3fb5e66e4d65..ca89751c0c22 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -69,7 +69,7 @@ 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, UNCOLORED,
 						   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 e804cb2fa57e..109f99f22e7c 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, UNCOLORED,
 				 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 01f238adfb67..65d1845d2bd8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2072,15 +2072,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,
+						  UNCOLORED,
 						  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,
+					       UNCOLORED,
 					       0, ggtt->base.total,
 					       0);
 		if (ret)
@@ -2757,7 +2757,7 @@ 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, UNCOLORED,
 						  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

[PATCH v2 4/4] 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

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      | 85 +++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  1 +
 drivers/gpu/drm/i915/i915_trace.h          | 23 ++++++++
 drivers/gpu/drm/i915/i915_vma.c            |  8 +--
 5 files changed, 87 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 32a77ee489c8..714990be2e16 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3259,7 +3259,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..c0664490a424 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -212,45 +212,80 @@ i915_gem_evict_something(struct i915_address_space *vm,
 	return ret;
 }
 
-int
-i915_gem_evict_for_vma(struct i915_vma *target)
+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) {
+		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 == UNCOLORED) {
+			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;
+		__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 109f99f22e7c..f219f2894a34 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 738ff3a5cd6e..827eaeb75524 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -325,12 +325,6 @@ 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;
-
 	other = list_entry(gtt_space->node_list.prev, struct drm_mm_node, node_list);
 	if (other->allocated && !other->hole_follows && other->color != cache_level)
 		return false;
@@ -413,7 +407,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

✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm: Define drm_mm_for_each_node_in_range()

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm: Define drm_mm_for_each_node_in_range()
URL   : https://patchwork.freedesktop.org/series/15489/
State : success

== Summary ==

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


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:191  dwarn:0   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 

ccd01198820ab7286f0b98f7b28dbf6ad29fa861 drm-intel-nightly: 2016y-11m-17d-10h-54m-57s UTC integration manifest
b597a03 drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)
7ce666c drm/i915: Mark all non-vma being inserted into the address spaces
b911b14 drm: Check against color expansion in drm_mm_reserve_node()
c7cd905 drm: Define drm_mm_for_each_node_in_range()

== Logs ==

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

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

[Joonas Lahtinen]

On to, 2016-11-17 at 12:08 +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@list.freedesktop.org

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 v2 3/4] drm/i915: Mark all non-vma being inserted into the address spaces

[Joonas Lahtinen]

On to, 2016-11-17 at 12:08 +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
> (UNCOLORED).
> 

I might opt for I915_MM_UNCOLORED, just to stay on the safe side as
this is in a .h file.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

You may or may not want to have it in i915_gem_stolen.c for
completeness.

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 v2 1/4] drm: Define drm_mm_for_each_node_in_range()

[Joonas Lahtinen]

On to, 2016-11-17 at 12:08 +0000, Chris Wilson wrote:
> 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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> 

<SNIP>

> 
> @@ -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);

These were introduced by you so I guess the reason for them ceased to
exist, so the removal should not hurt anybody.

> +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..fca5f313cf18 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -308,10 +308,12 @@ 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);
> +#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))			\

Drop a quick kerneldoc for this. It's almost ready in the commit message.

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 v2 3/4] drm/i915: Mark all non-vma being inserted into the address spaces

[Chris Wilson]

On Thu, Nov 17, 2016 at 03:31:54PM +0200, Joonas Lahtinen wrote:
> On to, 2016-11-17 at 12:08 +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
> > (UNCOLORED).
> > 
> 
> I might opt for I915_MM_UNCOLORED, just to stay on the safe side as
> this is in a .h file.

I've used i915_mm elsewhere as a subclass of struct mm, so a little wary
of I915_MM. The other users are enum i915_cache_level, but I didn't want
to imply that this is a cache level and I wanted something that could
clearly demarque the I-am-not-vma.

Maybe COLOR_ME_PINNED?

Definitely run out of paint today.
-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] 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
---
 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..6aab836c5338 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 start
+ * between @start and @end. It is implemented similar 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

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

✗ Fi.CI.BAT: failure for series starting with drm: Define drm_mm_for_each_node_in_range() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with drm: Define drm_mm_for_each_node_in_range() (rev2)
URL   : https://patchwork.freedesktop.org/series/15489/
State : failure

== Summary ==

Series 15489v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/15489/revisions/2/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-skl-6770hq)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-skl-6260u)

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:191  dwarn:0   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:206  pass:193  dwarn:0   dfail:0   fail:0   skip:12 
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:229  dwarn:1   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 

4f5493e90cc50f1c6391c4f389f7a79c8fe52355 drm-intel-nightly: 2016y-11m-17d-15h-37m-59s UTC integration manifest
371dcd0 drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)
94abd0b drm/i915: Mark all non-vma being inserted into the address spaces
d8da7bf drm: Check against color expansion in drm_mm_reserve_node()
92a81c3 drm: Define drm_mm_for_each_node_in_range()

== Logs ==

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

Re: [PATCH] drm: Define drm_mm_for_each_node_in_range()

[Joonas Lahtinen]

On to, 2016-11-17 at 14:49 +0000, Chris Wilson wrote:
> 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>

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 v2 3/4] drm/i915: Mark all non-vma being inserted into the address spaces

[Chris Wilson]

On Thu, Nov 17, 2016 at 02:12:19PM +0000, Chris Wilson wrote:
> On Thu, Nov 17, 2016 at 03:31:54PM +0200, Joonas Lahtinen wrote:
> > On to, 2016-11-17 at 12:08 +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
> > > (UNCOLORED).
> > > 
> > 
> > I might opt for I915_MM_UNCOLORED, just to stay on the safe side as
> > this is in a .h file.
> 
> I've used i915_mm elsewhere as a subclass of struct mm, so a little wary
> of I915_MM. The other users are enum i915_cache_level, but I didn't want
> to imply that this is a cache level and I wanted something that could
> clearly demarque the I-am-not-vma.
> 
> Maybe COLOR_ME_PINNED?

This morning I feel more like
#define COLOR_UNEVICTABLE (-1)
-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

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

[Joonas Lahtinen]

On to, 2016-11-17 at 12:08 +0000, Chris Wilson wrote: 
> -int
> -i915_gem_evict_for_vma(struct i915_vma *target)
> +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);
> +

Daniel already misread this, so I think it might be worth commenting;

> +	check_color = target->vm->mm.color_adjust;
> +	if (check_color) {

		/* Extend the search area to cover guard pages. */

> +		if (start > target->vm->start)
> +			start -= 4096;
> +		if (end < target->vm->start + target->vm->total)
> +			end += 4096;
> +	}

<SNIP>

> +		if (flags & PIN_NONBLOCK &&
> +		    (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))) {
> +			ret = -ENOSPC;
> +			break;
> +		}

i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
or without NOBLOCK, just skipping the exec_entry test without it. I
would clarify that. Now it's bit odd.

>  
> -			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;
> +		__i915_vma_pin(vma);

I don't quite see why? Are you expecting the iteration to hit same vma
twice? Or somebody moving it while we iterate.

> +		list_add(&vma->exec_list, &eviction_list);

I'd prefer an union instead of brutally reusing member for other
purposes.

>  	}
>  
> -	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;
>  }
>  

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 v2 3/4] drm/i915: Mark all non-vma being inserted into the address spaces

[Joonas Lahtinen]

On pe, 2016-11-18 at 09:05 +0000, Chris Wilson wrote:
> On Thu, Nov 17, 2016 at 02:12:19PM +0000, Chris Wilson wrote:
> > I've used i915_mm elsewhere as a subclass of struct mm, so a little wary
> > of I915_MM. The other users are enum i915_cache_level, but I didn't want
> > to imply that this is a cache level and I wanted something that could
> > clearly demarque the I-am-not-vma.
> > 
> > Maybe COLOR_ME_PINNED?
> 
> This morning I feel more like
> #define COLOR_UNEVICTABLE (-1)

I'd still slap at least I915_ in the front.

Regards, Joonas

> -Chris
> 
-- 
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 v2 4/4] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Chris Wilson]

On Fri, Nov 18, 2016 at 11:18:09AM +0200, Joonas Lahtinen wrote:
> On to, 2016-11-17 at 12:08 +0000, Chris Wilson wrote: 
> > +		if (flags & PIN_NONBLOCK &&
> > +		    (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))) {
> > +			ret = -ENOSPC;
> > +			break;
> > +		}
> 
> i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
> or without NOBLOCK, just skipping the exec_entry test without it. I
> would clarify that. Now it's bit odd.

It's a necessary test for use by execbuf. The interface is that it tests
a location first with NONBLOCK before deciding on whether it is a good
final location. (With various other hints as to whether any eviction is
a good idea, vs whether it mandatory to use this location.)

> > -			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;
> > +		__i915_vma_pin(vma);
> 
> I don't quite see why? Are you expecting the iteration to hit same vma
> twice? Or somebody moving it while we iterate.

The unbind may causes a free of any member on this list, so the pinning
prevents other vma from being unbound whilst waiting on this one. It
used to be a big deal, but since the various reworking the deferred free
hides the oops.
 
> > +		list_add(&vma->exec_list, &eviction_list);
> 
> I'd prefer an union instead of brutally reusing member for other
> purposes.

There have been patches to add evict_link :-p
-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

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

[Joonas Lahtinen]

On pe, 2016-11-18 at 10:14 +0000, Chris Wilson wrote:
> On Fri, Nov 18, 2016 at 11:18:09AM +0200, Joonas Lahtinen wrote:
> > i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
> > or without NOBLOCK, just skipping the exec_entry test without it. I
> > would clarify that. Now it's bit odd.
> 
> It's a necessary test for use by execbuf. The interface is that it tests
> a location first with NONBLOCK before deciding on whether it is a good
> final location. (With various other hints as to whether any eviction is
> a good idea, vs whether it mandatory to use this location.)
> 

Well, I do not object the new way of formatting it, it's more explicit.
But does GCC do equally good job still?

> > > -		ret = i915_vma_unbind(vma);
> > > -		if (ret)
> > > -			return ret;
> > > +		__i915_vma_pin(vma);
> > 
> > I don't quite see why? Are you expecting the iteration to hit same vma
> > twice? Or somebody moving it while we iterate.
> 
> The unbind may causes a free of any member on this list, so the pinning
> prevents other vma from being unbound whilst waiting on this one. It
> used to be a big deal, but since the various reworking the deferred free
> hides the oops.

Drop a comment.

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 v2 4/4] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Chris Wilson]

On Fri, Nov 18, 2016 at 01:31:57PM +0200, Joonas Lahtinen wrote:
> On pe, 2016-11-18 at 10:14 +0000, Chris Wilson wrote:
> > On Fri, Nov 18, 2016 at 11:18:09AM +0200, Joonas Lahtinen wrote:
> > > i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
> > > or without NOBLOCK, just skipping the exec_entry test without it. I
> > > would clarify that. Now it's bit odd.
> > 
> > It's a necessary test for use by execbuf. The interface is that it tests
> > a location first with NONBLOCK before deciding on whether it is a good
> > final location. (With various other hints as to whether any eviction is
> > a good idea, vs whether it mandatory to use this location.)
> > 
> 
> Well, I do not object the new way of formatting it, it's more explicit.
> But does GCC do equally good job still?

Honestly, haven't looked as eviction itself is quite rare that I haven't
stared at it in perf. The regression that came from preferring softpin
was from drm_mm_reserve_node() doing a linear search. Similarly, the
biggest cost for evict would also have been the linear search. The cost
of checking each vma is going to be underwhelming (vs the cost of
changing the pagetables for the evict). But yes, it is something we can
look at if we have such a workload.

When we do see eviction, perf is dominated by insert_ppgtt. (With some
caveats about drm_mm_search on some paths that will be fixed!)
-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

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

[Chris Wilson]

On Fri, Nov 18, 2016 at 01:31:57PM +0200, Joonas Lahtinen wrote:
> On pe, 2016-11-18 at 10:14 +0000, Chris Wilson wrote:
> > On Fri, Nov 18, 2016 at 11:18:09AM +0200, Joonas Lahtinen wrote:
> > > I don't quite see why? Are you expecting the iteration to hit same vma
> > > twice? Or somebody moving it while we iterate.
> > 
> > The unbind may causes a free of any member on this list, so the pinning
> > prevents other vma from being unbound whilst waiting on this one. It
> > used to be a big deal, but since the various reworking the deferred free
> > hides the oops.
> 
> Drop a comment.

                /* 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. (Actually this
                 * *should* be safe now due to the independent retirement of
                 * vma and deferred free preventing other nodes from
                 * disappearing, but for consistency and that extra layer of
                 * warm protection, let it be!)
                 */


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