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

From: "Christian König" <christian.koenig@amd.com>
To: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Cc: matthew.brost@intel.com, paulo.r.zanoni@intel.com,
	tvrtko.ursulin@intel.com, intel-gfx@lists.freedesktop.org,
	lionel.g.landwerlin@intel.com, thomas.hellstrom@intel.com,
	dri-devel@lists.freedesktop.org, jason@jlekstrand.net,
	daniel.vetter@intel.com, matthew.auld@intel.com
Subject: Re: [RFC 03/10] drm/i915/vm_bind: Support private and shared BOs
Date: Fri, 8 Jul 2022 19:32:54 +0200	[thread overview]
Message-ID: <0fe0fc02-1aed-242a-97be-8ec494a6f97b@amd.com> (raw)
In-Reply-To: <20220708132307.GV14039@nvishwa1-DESK>

Am 08.07.22 um 15:23 schrieb Niranjana Vishwanathapura:
> 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?

Yes, I would also add a word or two to the dma_resv object. Something 
like "Multiple buffers are sometimes using a single dma_resv object..."

Christian.

>
> 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@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.
>>>   *
>>