On Thu, Jul 07, 2022 at 03:31:42AM -0700, Hellstrom, Thomas wrote:
On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote: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. Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@xxxxxxxxx> --- 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);This only works for ttm-based objects. For non-TTM objects on integrated, we'll need to mimic the dma-resv individualization from TTM.
Ah, earlier I was doing this during VM destruction, but ran into problem as vma resources lives longer than VM in TTM case. So, I moved it here. Ok, yah, we probably need to mimic the dma-resv individualization from TTM, or, we can release priv_root during VM destruction for non-TTM objects?
+ 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); +}Please make a pass on this patch making sure we provide kerneldoc where supposed to.+ +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);I guess I'll find more details in the execbuf patches, but would it work to keep the non-private objects on the vm_bind_list, and just never move them to the vm_bound_list, rather than having a separate list for them?
The vm_bind/bound_list and the non_priv_vm_bind_list are there for very different reasons. The reason for having separate vm_bind_list and vm_bound_list is that during the execbuf path, we can rebind the unbound mappings by scooping all unbound vmas back from bound list into the bind list and binding them. In fact, this probably can be done with a single vm_bind_list and a 'eb.bind_list' (local to execbuf3 ioctl) for rebinding. The non_priv_vm_bind_list is just an optimization to loop only through non-priv objects while taking the locks in eb_lock_persistent_vmas() as only non-priv objects needs that (private objects are locked in a single shot with vm_priv_lock). A non-priv mapping will also be in the vm_bind/bound_list. I think, we need to add this as documentation to be more clear. Niranjana
/* 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. *Thanks, Thomas
