[Intel-gfx] [PATCH v5 3/6] drm/i915: Add object locking to i915_gem_evict_for_node and i915_gem_evict_something, v2.

Thomas Hellström thomas.hellstrom at linux.intel.com
Thu Jan 13 14:43:22 UTC 2022


On 1/13/22 12:44, Maarten Lankhorst wrote:
> Because we will start to require the obj->resv lock for unbinding,
> ensure these shrinker functions also take the lock.
Perhaps "vma eviction utilities" rather than "shrinker functions"?
>
> This requires some function signature changes, to ensure that the
> ww context is passed around, but is mostly straightforward.
>
> Previously this was split up into several patches, but reworking
> should allow for easier bisection.
>
> Changes since v1:
> - Handle dead objects better.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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_gem_evict.c         | 67 +++++++++++++++++--
>   drivers/gpu/drm/i915/i915_gem_evict.h         |  2 +
>   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 |  6 +-
>   11 files changed, 91 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index a1b2761bc16e..da7f54b6fa38 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -506,7 +506,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 4a20ba63446c..c078d3f55815 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -1383,7 +1383,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_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 370eb7238d1c..2367902f6ac1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -38,6 +38,11 @@ I915_SELFTEST_DECLARE(static struct igt_evict_ctl {
>   	bool fail_if_busy:1;
>   } igt_evict_ctl;)
>   
> +static bool dying_vma(struct i915_vma *vma)
> +{
> +	return !kref_read(&vma->obj->base.refcount);
> +}
> +
>   static int ggtt_flush(struct intel_gt *gt)
>   {
>   	/*
> @@ -50,8 +55,37 @@ static int ggtt_flush(struct intel_gt *gt)
>   	return intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
>   }
>   
> +static bool grab_vma(struct i915_vma *vma, struct i915_gem_ww_ctx *ww)
> +{
> +	/*
> +	 * We add the extra refcount so the object doesn't drop to zero until
> +	 * after ungrab_vma(), this way trylock is always paired with unlock.
> +	 */
> +	if (i915_gem_object_get_rcu(vma->obj)) {
> +		if (!i915_gem_object_trylock(vma->obj, ww)) {
> +			i915_gem_object_put(vma->obj);
> +			return false;
> +		}
> +	} else {
> +		/* Dead objects don't need pins */
> +		atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
> +	}
> +
> +	return true;
> +}
> +
> +static void ungrab_vma(struct i915_vma *vma)
> +{
> +	if (dying_vma(vma))
> +		return;
> +
> +	i915_gem_object_unlock(vma->obj);
> +	i915_gem_object_put(vma->obj);
> +}
> +
>   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)
> @@ -59,6 +93,9 @@ mark_free(struct drm_mm_scan *scan,
>   	if (i915_vma_is_pinned(vma))
>   		return false;
>   
> +	if (!grab_vma(vma, ww))
> +		return false;
> +
>   	list_add(&vma->evict_link, unwind);
>   	return drm_mm_scan_add_block(scan, &vma->node);
>   }
> @@ -105,6 +142,7 @@ static int evict_dead(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,
> @@ -177,7 +215,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;
>   	}
>   
> @@ -185,6 +223,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);
> +		ungrab_vma(vma);
>   	}
>   
>   	/*
> @@ -229,10 +268,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);
> +			ungrab_vma(vma);
> +		}
>   	}
>   
>   	/* Unbinding will emit any required flushes */
> @@ -241,16 +282,20 @@ i915_gem_evict_something(struct i915_address_space *vm,
>   		__i915_vma_unpin(vma);
>   		if (ret == 0)
>   			ret = __i915_vma_unbind(vma);
> +		ungrab_vma(vma);
>   	}
>   
>   	while (ret == 0 && (node = drm_mm_scan_color_evict(&scan))) {
>   		vma = container_of(node, struct i915_vma, node);
>   
>   		/* 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 &&
> +		    grab_vma(vma, ww)) {
>   			ret = __i915_vma_unbind(vma);
> -		else
> -			ret = -ENOSPC; /* XXX search failed, try again? */
> +			ungrab_vma(vma);
> +		} else {
> +			ret = -ENOSPC;
> +		}
>   	}
>   
>   	return ret;
> @@ -268,6 +313,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)
>   {
> @@ -340,6 +386,11 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>   			break;
>   		}
>   
> +		if (!grab_vma(vma, ww)) {
> +			ret = -ENOSPC;
> +			break;
> +		}
> +
>   		/*
>   		 * Never show fear in the face of dragons!
>   		 *
> @@ -357,6 +408,8 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>   		__i915_vma_unpin(vma);
>   		if (ret == 0)
>   			ret = __i915_vma_unbind(vma);
> +
> +		ungrab_vma(vma);
>   	}
>   
>   	return ret;
> @@ -398,7 +451,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
>   		LIST_HEAD(locked_eviction_list);
>   
>   		list_for_each_entry(vma, &vm->bound_list, vm_link) {
> -			if (!kref_read(&vma->obj->base.refcount)) {
> +			if (dying_vma(vma)) {
>   				ret = evict_dead(vma);
>   				if (ret)
>   					break;
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.h b/drivers/gpu/drm/i915/i915_gem_evict.h
> index f5b7a9100609..e593c530f9bd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.h
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.h
> @@ -13,11 +13,13 @@ struct i915_address_space;
>   struct i915_gem_ww_ctx;
>   
>   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_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b7094ca48047..022543f9bf32 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -94,6 +94,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)
> @@ -118,7 +119,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);
>   
> @@ -185,6 +186,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)
> @@ -270,7 +272,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;
>   
> @@ -278,7 +280,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 8477cae5f877..900cd4d9ebde 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -713,7 +713,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;
> @@ -765,7 +766,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)
> @@ -804,7 +805,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)
> @@ -1452,7 +1453,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 7c075c16a573..15b4e5631070 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);
> +

Stray newline?

Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>




More information about the dri-devel mailing list