drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

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>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161205142941.21965-2-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0c67916ec20634816c6ecf0640605a306dee5a6..605247baa7d1afb4762a0ca3aa67420ad6e611d2 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3341,7 +3341,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 739ede7c89ed4d9681202a8d468f98450888d1b1..fa40100146ea8de65e5a2619d2860e10be301fc3 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -212,45 +212,99 @@ found:
        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->i915->drm.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 5f164adc837cd11937e12003ae931405b6c787d7..d665a33229bd93575363277503d4eac3005e7f98 100644 (file)
--- 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 240067705e1c53e311bc26d3f82be829ac3d8f2e..18ae37c411fd4226b13bb2192323543d77b76c65 100644 (file)
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -450,6 +450,34 @@ 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 long, color)
+                            __field(unsigned int, flags)
+                           ),
+
+           TP_fast_assign(
+                          __entry->dev = vma->vm->i915->drm.primary->index;
+                          __entry->vm = vma->vm;
+                          __entry->start = vma->node.start;
+                          __entry->size = vma->node.size;
+                          __entry->color = vma->node.color;
+                          __entry->flags = flags;
+                         ),
+
+           TP_printk("dev=%d, vm=%p, start=%llx size=%llx, color=%lx, flags=%x",
+                     __entry->dev, __entry->vm,
+                     __entry->start, __entry->size,
+                     __entry->color, __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 4c91a68ecb6d57eb20f04c978f1e21d06b93fe8a..bc077e51b3e70048f11c48f942fb43844e33e453 100644 (file)
--- 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)