[Intel-gfx] [RFC v2 02/12] drm/i915/svm: Runtime (RT) allocator support

Niranjan Vishwanathapura niranjana.vishwanathapura at intel.com
Mon Dec 16 04:15:24 UTC 2019


On Sat, Dec 14, 2019 at 10:56:54AM +0000, Chris Wilson wrote:
>Quoting Niranjana Vishwanathapura (2019-12-13 21:56:04)
>> Shared Virtual Memory (SVM) runtime allocator support allows
>> binding a shared virtual address to a buffer object (BO) in the
>> device page table through an ioctl call.
>>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>> Cc: Sudeep Dutt <sudeep.dutt at intel.com>
>> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
>> ---
>>  drivers/gpu/drm/i915/Kconfig                  | 11 ++++
>>  drivers/gpu/drm/i915/Makefile                 |  3 +
>>  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 58 ++++++++++++++----
>>  drivers/gpu/drm/i915/gem/i915_gem_svm.c       | 60 +++++++++++++++++++
>>  drivers/gpu/drm/i915/gem/i915_gem_svm.h       | 22 +++++++
>>  drivers/gpu/drm/i915/i915_drv.c               | 21 +++++++
>>  drivers/gpu/drm/i915/i915_drv.h               | 22 +++++++
>>  drivers/gpu/drm/i915/i915_gem_gtt.c           |  1 +
>>  drivers/gpu/drm/i915/i915_gem_gtt.h           | 13 ++++
>>  include/uapi/drm/i915_drm.h                   | 27 +++++++++
>>  10 files changed, 227 insertions(+), 11 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_svm.c
>>  create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_svm.h
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index ba9595960bbe..c2e48710eec8 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -137,6 +137,16 @@ config DRM_I915_GVT_KVMGT
>>           Choose this option if you want to enable KVMGT support for
>>           Intel GVT-g.
>>
>> +config DRM_I915_SVM
>> +       bool "Enable Shared Virtual Memory support in i915"
>> +       depends on STAGING
>> +       depends on DRM_I915
>> +       default n
>> +       help
>> +         Choose this option if you want Shared Virtual Memory (SVM)
>> +         support in i915. With SVM support, one can share the virtual
>> +         address space between a process and the GPU.
>> +
>>  menu "drm/i915 Debugging"
>>  depends on DRM_I915
>>  depends on EXPERT
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e0fd10c0cfb8..75fe45633779 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -153,6 +153,9 @@ i915-y += \
>>           intel_region_lmem.o \
>>           intel_wopcm.o
>>
>> +# SVM code
>> +i915-$(CONFIG_DRM_I915_SVM) += gem/i915_gem_svm.o
>> +
>>  # general-purpose microcontroller (GuC) support
>>  obj-y += gt/uc/
>>  i915-y += gt/uc/intel_uc.o \
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 5003e616a1ad..af360238a392 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -2836,10 +2836,14 @@ int
>>  i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
>>                            struct drm_file *file)
>>  {
>> +       struct drm_i915_gem_exec_object2 *exec2_list, *exec2_list_user;
>>         struct drm_i915_gem_execbuffer2 *args = data;
>> -       struct drm_i915_gem_exec_object2 *exec2_list;
>> -       struct drm_syncobj **fences = NULL;
>>         const size_t count = args->buffer_count;
>> +       struct drm_syncobj **fences = NULL;
>> +       unsigned int i = 0, svm_count = 0;
>> +       struct i915_address_space *vm;
>> +       struct i915_gem_context *ctx;
>> +       struct i915_svm_obj *svm_obj;
>>         int err;
>>
>>         if (!check_buffer_count(count)) {
>> @@ -2851,15 +2855,46 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
>>         if (err)
>>                 return err;
>>
>> +       ctx = i915_gem_context_lookup(file->driver_priv, args->rsvd1);
>> +       if (!ctx || !rcu_access_pointer(ctx->vm))
>> +               return -ENOENT;
>
>This is just hopelessly wrong.
>
>For persistence, the _ce_->vm will have a list of must-be-present
>vma, with a flag for whether they need prefaulting (!svm everything must
>be prefaulted obviously). Then during reservation we ensure that all those
>persistent vma are in place (so we probably use an eviction list to keep
>track of those we need to instantiate on this execbuf). We don't even
>want to individually track activity on those vma, preferring to assume
>they are used by every request and so on change they need serialising
>[for explicit uAPI unbind, where possible we strive to do it async for
>endless, or at least sync against iova semaphore] against the last request
>in the vm (so we need a vm->active). However, we do need an EXT_EXTENSION
>to mark writes for implicit fencing (e.g.  exported dmabuf) to replace
>the information lost from execobject[]
>

I did not understand some points above.
I am no expert here, and appreciate the feedback.
My understanding is that [excluding endless batch buffer scenario which
is not supported in this patch series,] VM_BIND is no different than the
soft-pinning of objects we have today in the execbuf path. Hence the idea
here is to add those VM_BIND objects to the execobject[] and let the
execbuffer path to take care of the rest. Persistence of bindings across
multiple requests is something not considered. Do we need this flag in
execobject[] as well in execbuff path (with & without soft-pinning)?
Other than that, we do have a list of VM_BIND objects in a per 'vm' list
as you are suggesting above.
Let me sync with you to better understand this.

>> +struct drm_i915_gem_vm_bind {
>> +       /** VA start to bind **/
>> +       __u64 start;
>
>iova;
>offset; /* into handle */
>length; /* from offset */
>

Here iova is same as 'start' above?

>> +
>> +       /** Type of memory to [un]bind **/
>> +       __u32 type;
>> +#define I915_GEM_VM_BIND_SVM_OBJ      0
>> +
>> +       /** Object handle to [un]bind for I915_GEM_VM_BIND_SVM_OBJ type **/
>> +       __u32 handle;
>> +
>> +       /** vm to [un]bind **/
>> +       __u32 vm_id;
>> +
>> +       /** Flags **/
>> +       __u32 flags;
>> +#define I915_GEM_VM_BIND_UNBIND      (1 << 0)
>> +#define I915_GEM_VM_BIND_READONLY    (1 << 1)
>
>And don't forget extensions so that we can define the synchronisation
>controls.

OK.

Niranjana
>-Chris


More information about the Intel-gfx mailing list