[RFC 03/10] drm/i915/vm_bind: Support private and shared BOs

Niranjana Vishwanathapura niranjana.vishwanathapura at intel.com
Fri Jul 8 13:23:09 UTC 2022


On Thu, Jul 07, 2022 at 03:27:43PM +0200, Christian König wrote:
>Am 02.07.22 um 00:50 schrieb Niranjana Vishwanathapura:
>>Add uapi allowing user to specify a BO as private to a specified VM
>>during the BO creation.
>>VM private BOs can only be mapped on the specified VM and can't be
>>dma_buf exported. VM private BOs share a single common dma_resv object,
>>hence has a performance advantage requiring a single dma_resv object
>>update in the execbuf path compared to non-private (shared) BOs.
>
>Sounds like you picked up the per VM BO idea from amdgpu here :)
>
>Of hand looks like a good idea, but shouldn't we add a few comments in 
>the common documentation about that?
>
>E.g. something like "Multiple buffer objects sometimes share the same 
>dma_resv object....." to the dma_resv documentation.
>
>Probably best as a separate patch after this here has landed.

:)
Sounds good. Probably we need to update documentation of
drm_gem_object.resv and drm_gem_object._resv here, right?

Doing it in a separate patch after this series lands sounds good to me.

Thanks,
Niranjana

>
>Regards,
>Christian.
>
>>
>>Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
>>---
>>  drivers/gpu/drm/i915/gem/i915_gem_create.c    | 41 ++++++++++++++++++-
>>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  6 +++
>>  .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 ++
>>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |  3 ++
>>  drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h   | 11 +++++
>>  .../drm/i915/gem/i915_gem_vm_bind_object.c    |  9 ++++
>>  drivers/gpu/drm/i915/gt/intel_gtt.c           |  4 ++
>>  drivers/gpu/drm/i915/gt/intel_gtt.h           |  2 +
>>  drivers/gpu/drm/i915/i915_vma.c               |  1 +
>>  drivers/gpu/drm/i915/i915_vma_types.h         |  2 +
>>  include/uapi/drm/i915_drm.h                   | 30 ++++++++++++++
>>  11 files changed, 110 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>index 927a87e5ec59..7e264566b51f 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>@@ -11,6 +11,7 @@
>>  #include "pxp/intel_pxp.h"
>>  #include "i915_drv.h"
>>+#include "i915_gem_context.h"
>>  #include "i915_gem_create.h"
>>  #include "i915_trace.h"
>>  #include "i915_user_extensions.h"
>>@@ -243,6 +244,7 @@ struct create_ext {
>>  	unsigned int n_placements;
>>  	unsigned int placement_mask;
>>  	unsigned long flags;
>>+	u32 vm_id;
>>  };
>>  static void repr_placements(char *buf, size_t size,
>>@@ -392,9 +394,24 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>>  	return 0;
>>  }
>>+static int ext_set_vm_private(struct i915_user_extension __user *base,
>>+			      void *data)
>>+{
>>+	struct drm_i915_gem_create_ext_vm_private ext;
>>+	struct create_ext *ext_data = data;
>>+
>>+	if (copy_from_user(&ext, base, sizeof(ext)))
>>+		return -EFAULT;
>>+
>>+	ext_data->vm_id = ext.vm_id;
>>+
>>+	return 0;
>>+}
>>+
>>  static const i915_user_extension_fn create_extensions[] = {
>>  	[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>>  	[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
>>+	[I915_GEM_CREATE_EXT_VM_PRIVATE] = ext_set_vm_private,
>>  };
>>  /**
>>@@ -410,6 +427,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>  	struct drm_i915_private *i915 = to_i915(dev);
>>  	struct drm_i915_gem_create_ext *args = data;
>>  	struct create_ext ext_data = { .i915 = i915 };
>>+	struct i915_address_space *vm = NULL;
>>  	struct drm_i915_gem_object *obj;
>>  	int ret;
>>@@ -423,6 +441,12 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>  	if (ret)
>>  		return ret;
>>+	if (ext_data.vm_id) {
>>+		vm = i915_gem_vm_lookup(file->driver_priv, ext_data.vm_id);
>>+		if (unlikely(!vm))
>>+			return -ENOENT;
>>+	}
>>+
>>  	if (!ext_data.n_placements) {
>>  		ext_data.placements[0] =
>>  			intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
>>@@ -449,8 +473,21 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>  						ext_data.placements,
>>  						ext_data.n_placements,
>>  						ext_data.flags);
>>-	if (IS_ERR(obj))
>>-		return PTR_ERR(obj);
>>+	if (IS_ERR(obj)) {
>>+		ret = PTR_ERR(obj);
>>+		goto vm_put;
>>+	}
>>+
>>+	if (vm) {
>>+		obj->base.resv = vm->root_obj->base.resv;
>>+		obj->priv_root = i915_gem_object_get(vm->root_obj);
>>+		i915_vm_put(vm);
>>+	}
>>  	return i915_gem_publish(obj, file, &args->size, &args->handle);
>>+vm_put:
>>+	if (vm)
>>+		i915_vm_put(vm);
>>+
>>+	return ret;
>>  }
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>>index f5062d0c6333..6433173c3e84 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>>@@ -218,6 +218,12 @@ struct dma_buf *i915_gem_prime_export(struct drm_gem_object *gem_obj, int flags)
>>  	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
>>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>>+	if (obj->priv_root) {
>>+		drm_dbg(obj->base.dev,
>>+			"Exporting VM private objects is not allowed\n");
>>+		return ERR_PTR(-EINVAL);
>>+	}
>>+
>>  	exp_info.ops = &i915_dmabuf_ops;
>>  	exp_info.size = gem_obj->size;
>>  	exp_info.flags = flags;
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>index 5cf36a130061..9fe3395ad4d9 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>@@ -241,6 +241,9 @@ struct drm_i915_gem_object {
>>  	const struct drm_i915_gem_object_ops *ops;
>>+	/* Shared root is object private to a VM; NULL otherwise */
>>+	struct drm_i915_gem_object *priv_root;
>>+
>>  	struct {
>>  		/**
>>  		 * @vma.lock: protect the list/tree of vmas
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>index 7e1f8b83077f..f1912b12db00 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>@@ -1152,6 +1152,9 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
>>  	i915_gem_object_release_memory_region(obj);
>>  	mutex_destroy(&obj->ttm.get_io_page.lock);
>>+	if (obj->priv_root)
>>+		i915_gem_object_put(obj->priv_root);
>>+
>>  	if (obj->ttm.created) {
>>  		/*
>>  		 * We freely manage the shrinker LRU outide of the mm.pages life
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
>>index 642cdb559f17..ee6e4c52e80e 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
>>@@ -26,6 +26,17 @@ static inline void i915_gem_vm_bind_unlock(struct i915_address_space *vm)
>>  	mutex_unlock(&vm->vm_bind_lock);
>>  }
>>+static inline int i915_gem_vm_priv_lock(struct i915_address_space *vm,
>>+					struct i915_gem_ww_ctx *ww)
>>+{
>>+	return i915_gem_object_lock(vm->root_obj, ww);
>>+}
>>+
>>+static inline void i915_gem_vm_priv_unlock(struct i915_address_space *vm)
>>+{
>>+	i915_gem_object_unlock(vm->root_obj);
>>+}
>>+
>>  struct i915_vma *
>>  i915_gem_vm_bind_lookup_vma(struct i915_address_space *vm, u64 va);
>>  void i915_gem_vm_bind_remove(struct i915_vma *vma, bool release_obj);
>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
>>index 43ceb4dcca6c..3201204c8e74 100644
>>--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
>>@@ -85,6 +85,7 @@ void i915_gem_vm_bind_remove(struct i915_vma *vma, bool release_obj)
>>  	if (!list_empty(&vma->vm_bind_link)) {
>>  		list_del_init(&vma->vm_bind_link);
>>+		list_del_init(&vma->non_priv_vm_bind_link);
>>  		i915_vm_bind_it_remove(vma, &vma->vm->va);
>>  		/* Release object */
>>@@ -185,6 +186,11 @@ int i915_gem_vm_bind_obj(struct i915_address_space *vm,
>>  		goto put_obj;
>>  	}
>>+	if (obj->priv_root && obj->priv_root != vm->root_obj) {
>>+		ret = -EINVAL;
>>+		goto put_obj;
>>+	}
>>+
>>  	ret = i915_gem_vm_bind_lock_interruptible(vm);
>>  	if (ret)
>>  		goto put_obj;
>>@@ -211,6 +217,9 @@ int i915_gem_vm_bind_obj(struct i915_address_space *vm,
>>  	list_add_tail(&vma->vm_bind_link, &vm->vm_bound_list);
>>  	i915_vm_bind_it_insert(vma, &vm->va);
>>+	if (!obj->priv_root)
>>+		list_add_tail(&vma->non_priv_vm_bind_link,
>>+			      &vm->non_priv_vm_bind_list);
>>  	/* Hold object reference until vm_unbind */
>>  	i915_gem_object_get(vma->obj);
>>diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
>>index 135dc4a76724..df0a8459c3c6 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
>>+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
>>@@ -176,6 +176,7 @@ int i915_vm_lock_objects(struct i915_address_space *vm,
>>  void i915_address_space_fini(struct i915_address_space *vm)
>>  {
>>  	drm_mm_takedown(&vm->mm);
>>+	i915_gem_object_put(vm->root_obj);
>>  	GEM_BUG_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
>>  	mutex_destroy(&vm->vm_bind_lock);
>>  }
>>@@ -289,6 +290,9 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
>>  	INIT_LIST_HEAD(&vm->vm_bind_list);
>>  	INIT_LIST_HEAD(&vm->vm_bound_list);
>>  	mutex_init(&vm->vm_bind_lock);
>>+	INIT_LIST_HEAD(&vm->non_priv_vm_bind_list);
>>+	vm->root_obj = i915_gem_object_create_internal(vm->i915, PAGE_SIZE);
>>+	GEM_BUG_ON(IS_ERR(vm->root_obj));
>>  }
>>  void *__px_vaddr(struct drm_i915_gem_object *p)
>>diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
>>index d4a6ce65251d..f538ce9115c9 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
>>+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
>>@@ -267,6 +267,8 @@ struct i915_address_space {
>>  	struct list_head vm_bound_list;
>>  	/* va tree of persistent vmas */
>>  	struct rb_root_cached va;
>>+	struct list_head non_priv_vm_bind_list;
>>+	struct drm_i915_gem_object *root_obj;
>>  	/* Global GTT */
>>  	bool is_ggtt:1;
>>diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>>index d324e29cef0a..f0226581d342 100644
>>--- a/drivers/gpu/drm/i915/i915_vma.c
>>+++ b/drivers/gpu/drm/i915/i915_vma.c
>>@@ -236,6 +236,7 @@ vma_create(struct drm_i915_gem_object *obj,
>>  	mutex_unlock(&vm->mutex);
>>  	INIT_LIST_HEAD(&vma->vm_bind_link);
>>+	INIT_LIST_HEAD(&vma->non_priv_vm_bind_link);
>>  	return vma;
>>  err_unlock:
>>diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
>>index b6d179bdbfa0..2298b3d6b7c4 100644
>>--- a/drivers/gpu/drm/i915/i915_vma_types.h
>>+++ b/drivers/gpu/drm/i915/i915_vma_types.h
>>@@ -290,6 +290,8 @@ struct i915_vma {
>>  	struct list_head vm_link;
>>  	struct list_head vm_bind_link; /* Link in persistent VMA list */
>>+	/* Link in non-private persistent VMA list */
>>+	struct list_head non_priv_vm_bind_link;
>>  	/** Interval tree structures for persistent vma */
>>  	struct rb_node rb;
>>diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>index 26cca49717f8..ce1c6592b0d7 100644
>>--- a/include/uapi/drm/i915_drm.h
>>+++ b/include/uapi/drm/i915_drm.h
>>@@ -3542,9 +3542,13 @@ struct drm_i915_gem_create_ext {
>>  	 *
>>  	 * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>>  	 * struct drm_i915_gem_create_ext_protected_content.
>>+	 *
>>+	 * For I915_GEM_CREATE_EXT_VM_PRIVATE usage see
>>+	 * struct drm_i915_gem_create_ext_vm_private.
>>  	 */
>>  #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>>  #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
>>+#define I915_GEM_CREATE_EXT_VM_PRIVATE 2
>>  	__u64 extensions;
>>  };
>>@@ -3662,6 +3666,32 @@ struct drm_i915_gem_create_ext_protected_content {
>>  /* ID of the protected content session managed by i915 when PXP is active */
>>  #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>>+/**
>>+ * struct drm_i915_gem_create_ext_vm_private - Extension to make the object
>>+ * private to the specified VM.
>>+ *
>>+ * See struct drm_i915_gem_create_ext.
>>+ *
>>+ * By default, BOs can be mapped on multiple VMs and can also be dma-buf
>>+ * exported. Hence these BOs are referred to as Shared BOs.
>>+ * During each execbuf3 submission, the request fence must be added to the
>>+ * dma-resv fence list of all shared BOs mapped on the VM.
>>+ *
>>+ * Unlike Shared BOs, these VM private BOs can only be mapped on the VM they
>>+ * are private to and can't be dma-buf exported. All private BOs of a VM share
>>+ * the dma-resv object. Hence during each execbuf3 submission, they need only
>>+ * one dma-resv fence list updated. Thus, the fast path (where required
>>+ * mappings are already bound) submission latency is O(1) w.r.t the number of
>>+ * VM private BOs.
>>+ */
>>+struct drm_i915_gem_create_ext_vm_private {
>>+	/** @base: Extension link. See struct i915_user_extension. */
>>+	struct i915_user_extension base;
>>+
>>+	/** @vm_id: Id of the VM to which the object is private */
>>+	__u32 vm_id;
>>+};
>>+
>>  /**
>>   * struct drm_i915_gem_vm_bind - VA to object mapping to bind.
>>   *
>


More information about the dri-devel mailing list