Re: [Intel-gfx] [PATCH v3 13/17] drm/i915: Add object locking to i915_gem_evict_for_node and i915_gem_evict_something

On 17-12-2021 14:55, Matthew Auld wrote:

On Thu, 16 Dec 2021 at 14:28, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:

...

Because we will start to require the obj->resv lock for unbinding, ensure these shrinker functions also take the lock.

This requires some function signature changes, to ensure that the ww context is passed around, but is mostly straightforward.

Do we even need this? We aren't handling the shared dma-resv for these ones, right? Or more just to have a consistent-ish interface?

Yeah, we don't currently use it, but it will allow for easier eviction in the future, with -EDEADLK handling

when required. It will currently only annotate the objects as being held by the ww context, which can cause -EDEADLK when other threads try to lock.

Ideally we'd take a ref to the object and perform a blocking lock, but our locking and eviction code can't handle that right now.

...

Previously this was split up into several patches, but reworking should allow for easier bisection.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +- drivers/gpu/drm/i915/gvt/aperture_gm.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem_evict.c | 34 +++++++++++++++---- drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +++-- drivers/gpu/drm/i915/i915_gem_gtt.h | 3 ++ drivers/gpu/drm/i915/i915_vgpu.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 9 ++--- .../gpu/drm/i915/selftests/i915_gem_evict.c | 17 +++++----- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 14 ++++---- 11 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index a287c9186ec9..ed43e9c80aaa 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -504,7 +504,7 @@ static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt) GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP); size = ggtt->vm.total - GUC_GGTT_TOP;

•   ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
  

•   ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, size,
                               GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
                               PIN_NOEVICT);
    if (ret)
  

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index e5ad4d5a91c0..a3597a6bb805 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -1382,7 +1382,7 @@ static int evict_vma(void *data) complete(&arg->completion);

    mutex_lock(&vm->mutex);

•   err = i915_gem_evict_for_node(vm, &evict, 0);
  

•   err = i915_gem_evict_for_node(vm, NULL, &evict, 0);
    mutex_unlock(&vm->mutex);
  
    return err;
  

diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c b/drivers/gpu/drm/i915/gvt/aperture_gm.c index 0d6d59871308..c08098a167e9 100644 --- a/drivers/gpu/drm/i915/gvt/aperture_gm.c +++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c @@ -63,7 +63,7 @@ static int alloc_gm(struct intel_vgpu *vgpu, bool high_gm)

    mutex_lock(&gt->ggtt->vm.mutex);
    mmio_hw_access_pre(gt);

•   ret = i915_gem_gtt_insert(&gt->ggtt->vm, node,
  

•   ret = i915_gem_gtt_insert(&gt->ggtt->vm, NULL, node,
                              size, I915_GTT_PAGE_SIZE,
                              I915_COLOR_UNEVICTABLE,
                              start, end, flags);
  

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c180019c607f..2a98192a89c1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1718,11 +1718,13 @@ i915_gem_vm_lookup(struct drm_i915_file_private *file_priv, u32 id)

/* i915_gem_evict.c */ int __must_check i915_gem_evict_something(struct i915_address_space *vm,

•                                     struct i915_gem_ww_ctx *ww,
                                      u64 min_size, u64 alignment,
                                      unsigned long color,
                                      u64 start, u64 end,
                                      unsigned flags);
  

int __must_check i915_gem_evict_for_node(struct i915_address_space *vm,

•                                    struct i915_gem_ww_ctx *ww,
                                     struct drm_mm_node *node,
                                     unsigned int flags);
  

int i915_gem_evict_vm(struct i915_address_space *vm, diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index bfd66f539fc1..f502a617b35c 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -51,6 +51,7 @@ static int ggtt_flush(struct intel_gt *gt)

static bool mark_free(struct drm_mm_scan *scan,

•     struct i915_gem_ww_ctx *ww,
      struct i915_vma *vma,
      unsigned int flags,
      struct list_head *unwind)
  

@@ -58,6 +59,9 @@ mark_free(struct drm_mm_scan *scan, if (i915_vma_is_pinned(vma)) return false;

•   if (!i915_gem_object_trylock(vma->obj, ww))
  

•           return false;
  

•   list_add(&vma->evict_link, unwind);
    return drm_mm_scan_add_block(scan, &vma->node);
  

} @@ -98,6 +102,7 @@ static bool defer_evict(struct i915_vma *vma) */ int i915_gem_evict_something(struct i915_address_space *vm,

•                    struct i915_gem_ww_ctx *ww,
                     u64 min_size, u64 alignment,
                     unsigned long color,
                     u64 start, u64 end,
  

@@ -170,7 +175,7 @@ i915_gem_evict_something(struct i915_address_space *vm, continue; }

•           if (mark_free(&scan, vma, flags, &eviction_list))
  

•           if (mark_free(&scan, ww, vma, flags, &eviction_list))
                    goto found;
    }
  
  

@@ -178,6 +183,7 @@ i915_gem_evict_something(struct i915_address_space *vm, list_for_each_entry_safe(vma, next, &eviction_list, evict_link) { ret = drm_mm_scan_remove_block(&scan, &vma->node); BUG_ON(ret);

•           i915_gem_object_unlock(vma->obj);
    }
  
    /*
  

@@ -222,10 +228,12 @@ i915_gem_evict_something(struct i915_address_space *vm, * of any of our objects, thus corrupting the list). */ list_for_each_entry_safe(vma, next, &eviction_list, evict_link) {

•           if (drm_mm_scan_remove_block(&scan, &vma->node))
  

•           if (drm_mm_scan_remove_block(&scan, &vma->node)) {
                    __i915_vma_pin(vma);
  

•           else
  

•           } else {
                    list_del(&vma->evict_link);
  

•                   i915_gem_object_unlock(vma->obj);
  

•           }
    }
  
    /* Unbinding will emit any required flushes */
  

@@ -234,16 +242,22 @@ i915_gem_evict_something(struct i915_address_space *vm, __i915_vma_unpin(vma); if (ret == 0) ret = __i915_vma_unbind(vma);

•           i915_gem_object_unlock(vma->obj);
    }
  
    while (ret == 0 && (node = drm_mm_scan_color_evict(&scan))) {
            vma = container_of(node, struct i915_vma, node);
  
  

Spurious new line.

Otherwise, Reviewed-by: Matthew Auld matthew.auld@intel.com

...

            /* If we find any non-objects (!vma), we cannot evict them */

•           if (vma->node.color != I915_COLOR_UNEVICTABLE)
  

•           if (vma->node.color != I915_COLOR_UNEVICTABLE &&
  

•               i915_gem_object_trylock(vma->obj, ww)) {
                    ret = __i915_vma_unbind(vma);
  

•           else
  

•                   ret = -ENOSPC; /* XXX search failed, try again? */
  

•                   i915_gem_object_unlock(vma->obj);
  

•           } else {
  

•                   ret = -ENOSPC;
  

•           }
    }
  
    return ret;
  

@@ -261,6 +275,7 @@ i915_gem_evict_something(struct i915_address_space *vm,

• memory in e.g. the shrinker.

*/ int i915_gem_evict_for_node(struct i915_address_space *vm,

•                       struct i915_gem_ww_ctx *ww,
                        struct drm_mm_node *target,
                        unsigned int flags)
  

{ @@ -333,6 +348,11 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, break; }

•           if (!i915_gem_object_trylock(vma->obj, ww)) {
  

•                   ret = -ENOSPC;
  

•                   break;
  

•           }
  

•           /*
             * Never show fear in the face of dragons!
             *
  

@@ -350,6 +370,8 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, __i915_vma_unpin(vma); if (ret == 0) ret = __i915_vma_unbind(vma);

•           i915_gem_object_unlock(vma->obj);
    }
  
    return ret;
  

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index cd5f2348a187..c810da476c2b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -93,6 +93,7 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj,

• asked to wait for eviction and interrupted.

*/ int i915_gem_gtt_reserve(struct i915_address_space *vm,

•                    struct i915_gem_ww_ctx *ww,
                     struct drm_mm_node *node,
                     u64 size, u64 offset, unsigned long color,
                     unsigned int flags)
  

@@ -117,7 +118,7 @@ int i915_gem_gtt_reserve(struct i915_address_space *vm, if (flags & PIN_NOEVICT) return -ENOSPC;

•   err = i915_gem_evict_for_node(vm, node, flags);
  

•   err = i915_gem_evict_for_node(vm, ww, node, flags);
    if (err == 0)
            err = drm_mm_reserve_node(&vm->mm, node);
  
  

@@ -184,6 +185,7 @@ static u64 random_offset(u64 start, u64 end, u64 len, u64 align)

• asked to wait for eviction and interrupted.

*/ int i915_gem_gtt_insert(struct i915_address_space *vm,

•                   struct i915_gem_ww_ctx *ww,
                    struct drm_mm_node *node,
                    u64 size, u64 alignment, unsigned long color,
                    u64 start, u64 end, unsigned int flags)
  

@@ -269,7 +271,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm, */ offset = random_offset(start, end, size, alignment ?: I915_GTT_MIN_ALIGNMENT);

•   err = i915_gem_gtt_reserve(vm, node, size, offset, color, flags);
  

•   err = i915_gem_gtt_reserve(vm, ww, node, size, offset, color, flags);
    if (err != -ENOSPC)
            return err;
  
  

@@ -277,7 +279,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm, return -ENOSPC;

    /* Randomly selected placement is pinned, do a search */

•   err = i915_gem_evict_something(vm, size, alignment, color,
  

•   err = i915_gem_evict_something(vm, ww, size, alignment, color,
                                   start, end, flags);
    if (err)
            return err;
  

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index c9b0ee5e1d23..e4938aba3fe9 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -16,6 +16,7 @@

struct drm_i915_gem_object; struct i915_address_space; +struct i915_gem_ww_ctx;

int __must_check i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj, struct sg_table *pages); @@ -23,11 +24,13 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj, struct sg_table *pages);

int i915_gem_gtt_reserve(struct i915_address_space *vm,

•                    struct i915_gem_ww_ctx *ww,
                     struct drm_mm_node *node,
                     u64 size, u64 offset, unsigned long color,
                     unsigned int flags);
  
  

int i915_gem_gtt_insert(struct i915_address_space *vm,

•                   struct i915_gem_ww_ctx *ww,
                    struct drm_mm_node *node,
                    u64 size, u64 alignment, unsigned long color,
                    u64 start, u64 end, unsigned int flags);
  

diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c index 31a105bc1792..c97323973f9b 100644 --- a/drivers/gpu/drm/i915/i915_vgpu.c +++ b/drivers/gpu/drm/i915/i915_vgpu.c @@ -197,7 +197,7 @@ static int vgt_balloon_space(struct i915_ggtt *ggtt, drm_info(&dev_priv->drm, "balloon space: range [ 0x%lx - 0x%lx ] %lu KiB.\n", start, end, size / 1024);

•   ret = i915_gem_gtt_reserve(&ggtt->vm, node,
  

•   ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, node,
                               size, start, I915_COLOR_UNEVICTABLE,
                               0);
    if (!ret)
  

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index d24e90eac948..83df1290df5e 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -650,7 +650,8 @@ bool i915_gem_valid_gtt_space(struct i915_vma *vma, unsigned long color)

• 0 on success, negative error code otherwise.

*/ static int -i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags) +i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

•           u64 size, u64 alignment, u64 flags)
  

{ unsigned long color; u64 start, end; @@ -702,7 +703,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags) range_overflows(offset, size, end)) return -EINVAL;

•           ret = i915_gem_gtt_reserve(vma->vm, &vma->node,
  

•           ret = i915_gem_gtt_reserve(vma->vm, ww, &vma->node,
                                       size, offset, color,
                                       flags);
            if (ret)
  

@@ -741,7 +742,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags) size = round_up(size, I915_GTT_PAGE_SIZE_2M); }

•           ret = i915_gem_gtt_insert(vma->vm, &vma->node,
  

•           ret = i915_gem_gtt_insert(vma->vm, ww, &vma->node,
                                      size, alignment, color,
                                      start, end, flags);
            if (ret)
  

@@ -1382,7 +1383,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, goto err_unlock;

    if (!(bound & I915_VMA_BIND_MASK)) {

•           err = i915_vma_insert(vma, size, alignment, flags);
  

•           err = i915_vma_insert(vma, ww, size, alignment, flags);
            if (err)
                    goto err_active;
  
  

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c index 7178811366af..c80e1483d603 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c @@ -117,7 +117,7 @@ static int igt_evict_something(void *arg)

    /* Everything is pinned, nothing should happen */
    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_something(&ggtt->vm,
  

•   err = i915_gem_evict_something(&ggtt->vm, NULL,
                                   I915_GTT_PAGE_SIZE, 0, 0,
                                   0, U64_MAX,
                                   0);
  

@@ -132,11 +132,12 @@ static int igt_evict_something(void *arg)

    /* Everything is unpinned, we should be able to evict something */
    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_something(&ggtt->vm,
  

•   err = i915_gem_evict_something(&ggtt->vm, NULL,
                                   I915_GTT_PAGE_SIZE, 0, 0,
                                   0, U64_MAX,
                                   0);
    mutex_unlock(&ggtt->vm.mutex);
  

•   if (err) {
            pr_err("i915_gem_evict_something failed on a full GGTT with err=%d\n",
                   err);
  

@@ -204,7 +205,7 @@ static int igt_evict_for_vma(void *arg)

    /* Everything is pinned, nothing should happen */
    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_for_node(&ggtt->vm, &target, 0);
  

•   err = i915_gem_evict_for_node(&ggtt->vm, NULL, &target, 0);
    mutex_unlock(&ggtt->vm.mutex);
    if (err != -ENOSPC) {
            pr_err("i915_gem_evict_for_node on a full GGTT returned err=%d\n",
  

@@ -216,7 +217,7 @@ static int igt_evict_for_vma(void *arg)

    /* Everything is unpinned, we should be able to evict the node */
    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_for_node(&ggtt->vm, &target, 0);
  

•   err = i915_gem_evict_for_node(&ggtt->vm, NULL, &target, 0);
    mutex_unlock(&ggtt->vm.mutex);
    if (err) {
            pr_err("i915_gem_evict_for_node returned err=%d\n",
  

@@ -297,7 +298,7 @@ static int igt_evict_for_cache_color(void *arg)

    /* Remove just the second vma */
    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_for_node(&ggtt->vm, &target, 0);
  

•   err = i915_gem_evict_for_node(&ggtt->vm, NULL, &target, 0);
    mutex_unlock(&ggtt->vm.mutex);
    if (err) {
            pr_err("[0]i915_gem_evict_for_node returned err=%d\n", err);
  

@@ -310,7 +311,7 @@ static int igt_evict_for_cache_color(void *arg) target.color = I915_CACHE_L3_LLC;

    mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_evict_for_node(&ggtt->vm, &target, 0);
  

•   err = i915_gem_evict_for_node(&ggtt->vm, NULL, &target, 0);
    mutex_unlock(&ggtt->vm.mutex);
    if (!err) {
            pr_err("[1]i915_gem_evict_for_node returned err=%d\n", err);
  

@@ -408,7 +409,7 @@ static int igt_evict_contexts(void *arg) /* Reserve a block so that we know we have enough to fit a few rq */ memset(&hole, 0, sizeof(hole)); mutex_lock(&ggtt->vm.mutex);

•   err = i915_gem_gtt_insert(&ggtt->vm, &hole,
  

•   err = i915_gem_gtt_insert(&ggtt->vm, NULL, &hole,
                              PRETEND_GGTT_SIZE, 0, I915_COLOR_UNEVICTABLE,
                              0, ggtt->vm.total,
                              PIN_NOEVICT);
  

@@ -428,7 +429,7 @@ static int igt_evict_contexts(void *arg) goto out_locked; }

•           if (i915_gem_gtt_insert(&ggtt->vm, &r->node,
  

•           if (i915_gem_gtt_insert(&ggtt->vm, NULL, &r->node,
                                    1ul << 20, 0, I915_COLOR_UNEVICTABLE,
                                    0, ggtt->vm.total,
                                    PIN_NOEVICT)) {
  

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c index 88b3a4d9e27e..169e2330b386 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -1378,7 +1378,7 @@ static int igt_gtt_reserve(void *arg) }

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_reserve(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_reserve(&ggtt->vm, NULL, &vma->node,
                                       obj->base.size,
                                       total,
                                       obj->cache_level,
  

@@ -1430,7 +1430,7 @@ static int igt_gtt_reserve(void *arg) }

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_reserve(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_reserve(&ggtt->vm, NULL, &vma->node,
                                       obj->base.size,
                                       total,
                                       obj->cache_level,
  

@@ -1477,7 +1477,7 @@ static int igt_gtt_reserve(void *arg) I915_GTT_MIN_ALIGNMENT);

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_reserve(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_reserve(&ggtt->vm, NULL, &vma->node,
                                       obj->base.size,
                                       offset,
                                       obj->cache_level,
  

@@ -1552,7 +1552,7 @@ static int igt_gtt_insert(void *arg) /* Check a couple of obviously invalid requests */ for (ii = invalid_insert; ii->size; ii++) { mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_insert(&ggtt->vm, &tmp,
  

•           err = i915_gem_gtt_insert(&ggtt->vm, NULL, &tmp,
                                      ii->size, ii->alignment,
                                      I915_COLOR_UNEVICTABLE,
                                      ii->start, ii->end,
  

@@ -1594,7 +1594,7 @@ static int igt_gtt_insert(void *arg) }

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_insert(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_insert(&ggtt->vm, NULL, &vma->node,
                                      obj->base.size, 0, obj->cache_level,
                                      0, ggtt->vm.total,
                                      0);
  

@@ -1654,7 +1654,7 @@ static int igt_gtt_insert(void *arg) }

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_insert(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_insert(&ggtt->vm, NULL, &vma->node,
                                      obj->base.size, 0, obj->cache_level,
                                      0, ggtt->vm.total,
                                      0);
  

@@ -1703,7 +1703,7 @@ static int igt_gtt_insert(void *arg) }

            mutex_lock(&ggtt->vm.mutex);

•           err = i915_gem_gtt_insert(&ggtt->vm, &vma->node,
  

•           err = i915_gem_gtt_insert(&ggtt->vm, NULL, &vma->node,
                                      obj->base.size, 0, obj->cache_level,
                                      0, ggtt->vm.total,
                                      0);
  

-- 2.34.1