[RFC,v2,02/12] drm/i915/svm: Runtime (RT) allocator support
diff mbox series

Message ID 20191213215614.24558-3-niranjana.vishwanathapura@intel.com
State New
Headers show
Series
  • drm/i915/svm: Add SVM support
Related show

Commit Message

Niranjana Vishwanathapura Dec. 13, 2019, 9:56 p.m. UTC
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@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@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

Comments

Jason Ekstrand Dec. 13, 2019, 10:58 p.m. UTC | #1
On Fri, Dec 13, 2019 at 4:07 PM Niranjana Vishwanathapura <
niranjana.vishwanathapura@intel.com> wrote:

> 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@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Signed-off-by: Niranjana Vishwanathapura <
> niranjana.vishwanathapura@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;
> +
> +       rcu_read_lock();
> +       vm = i915_vm_get(ctx->vm);
> +       rcu_read_unlock();
> +
> +alloc_again:
> +       svm_count = vm->svm_count;
>         /* Allocate an extra slot for use by the command parser */
> -       exec2_list = kvmalloc_array(count + 1, eb_element_size(),
> +       exec2_list = kvmalloc_array(count + svm_count + 1,
> eb_element_size(),
>                                     __GFP_NOWARN | GFP_KERNEL);
>         if (exec2_list == NULL) {
>                 DRM_DEBUG("Failed to allocate exec list for %zd buffers\n",
> -                         count);
> +                         count + svm_count);
>                 return -ENOMEM;
>         }
> -       if (copy_from_user(exec2_list,
> +       mutex_lock(&vm->mutex);
> +       if (svm_count != vm->svm_count) {
> +               mutex_unlock(&vm->mutex);
> +               kvfree(exec2_list);
> +               goto alloc_again;
> +       }
> +
> +       list_for_each_entry(svm_obj, &vm->svm_list, link) {
> +               memset(&exec2_list[i], 0, sizeof(*exec2_list));
> +               exec2_list[i].handle = svm_obj->handle;
> +               exec2_list[i].offset = svm_obj->offset;
> +               exec2_list[i].flags = EXEC_OBJECT_PINNED |
> +                                     EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +               i++;
> +       }
> +       exec2_list_user = &exec2_list[i];
> +       args->buffer_count += svm_count;
> +       mutex_unlock(&vm->mutex);
> +       i915_vm_put(vm);
> +       i915_gem_context_put(ctx);
> +
> +       if (copy_from_user(exec2_list_user,
>                            u64_to_user_ptr(args->buffers_ptr),
>                            sizeof(*exec2_list) * count)) {
>                 DRM_DEBUG("copy %zd exec entries failed\n", count);
> @@ -2876,6 +2911,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev,
> void *data,
>         }
>
>         err = i915_gem_do_execbuffer(dev, file, args, exec2_list, fences);
> +       args->buffer_count -= svm_count;
>
>         /*
>          * Now that we have begun execution of the batchbuffer, we ignore
> @@ -2886,7 +2922,6 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev,
> void *data,
>         if (args->flags & __EXEC_HAS_RELOC) {
>                 struct drm_i915_gem_exec_object2 __user *user_exec_list =
>                         u64_to_user_ptr(args->buffers_ptr);
> -               unsigned int i;
>
>                 /* Copy the new buffer offsets back to the user's exec
> list. */
>                 /*
> @@ -2900,13 +2935,14 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev,
> void *data,
>                         goto end;
>
>                 for (i = 0; i < args->buffer_count; i++) {
> -                       if (!(exec2_list[i].offset & UPDATE))
> +                       u64 *offset = &exec2_list_user[i].offset;
> +
> +                       if (!(*offset & UPDATE))
>                                 continue;
>
> -                       exec2_list[i].offset =
> -                               gen8_canonical_addr(exec2_list[i].offset &
> PIN_OFFSET_MASK);
> -                       unsafe_put_user(exec2_list[i].offset,
> -                                       &user_exec_list[i].offset,
> +                       *offset = gen8_canonical_addr(*offset &
> +                                                     PIN_OFFSET_MASK);
> +                       unsafe_put_user(*offset, &user_exec_list[i].offset,
>                                         end_user);
>                 }
>  end_user:
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_svm.c
> b/drivers/gpu/drm/i915/gem/i915_gem_svm.c
> new file mode 100644
> index 000000000000..882fe56138e2
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_svm.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "i915_gem_gtt.h"
> +#include "i915_gem_lmem.h"
> +
> +int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
> +                            struct drm_i915_gem_vm_bind *args,
> +                            struct drm_file *file)
> +{
> +       struct i915_svm_obj *svm_obj, *tmp;
> +       struct drm_i915_gem_object *obj;
> +       int ret = 0;
> +
> +       obj = i915_gem_object_lookup(file, args->handle);
> +       if (!obj)
> +               return -ENOENT;
> +
> +       /* For dgfx, ensure the obj is in device local memory only */
> +       if (IS_DGFX(vm->i915) && !i915_gem_object_is_lmem(obj))
> +               return -EINVAL;
> +
> +       /* FIXME: Need to handle case with unending batch buffers */
> +       if (!(args->flags & I915_GEM_VM_BIND_UNBIND)) {
> +               svm_obj = kmalloc(sizeof(*svm_obj), GFP_KERNEL);
> +               if (!svm_obj) {
> +                       ret = -ENOMEM;
> +                       goto put_obj;
> +               }
> +               svm_obj->handle = args->handle;
> +               svm_obj->offset = args->start;
> +       }
> +
> +       mutex_lock(&vm->mutex);
> +       if (!(args->flags & I915_GEM_VM_BIND_UNBIND)) {
> +               list_add(&svm_obj->link, &vm->svm_list);
> +               vm->svm_count++;
> +       } else {
> +               /*
> +                * FIXME: Need to handle case where object is
> migrated/closed
> +                * without unbinding first.
> +                */
> +               list_for_each_entry_safe(svm_obj, tmp, &vm->svm_list,
> link) {
> +                       if (svm_obj->handle != args->handle)
> +                               continue;
> +
> +                       list_del_init(&svm_obj->link);
> +                       vm->svm_count--;
> +                       kfree(svm_obj);
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&vm->mutex);
> +put_obj:
> +       i915_gem_object_put(obj);
> +       return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_svm.h
> b/drivers/gpu/drm/i915/gem/i915_gem_svm.h
> new file mode 100644
> index 000000000000..d60b35c7d21a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_svm.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __I915_GEM_SVM_H
> +#define __I915_GEM_SVM_H
> +
> +#include "i915_drv.h"
> +
> +#if defined(CONFIG_DRM_I915_SVM)
> +int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
> +                            struct drm_i915_gem_vm_bind *args,
> +                            struct drm_file *file);
> +#else
> +static inline int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
> +                                          struct drm_i915_gem_vm_bind
> *args,
> +                                          struct drm_file *file)
> +{ return -ENOTSUPP; }
> +#endif
> +
> +#endif /* __I915_GEM_SVM_H */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 2a11f60c4fd2..d452ea8e40b3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2680,6 +2680,26 @@ i915_gem_reject_pin_ioctl(struct drm_device *dev,
> void *data,
>         return -ENODEV;
>  }
>
> +static int i915_gem_vm_bind_ioctl(struct drm_device *dev, void *data,
> +                                 struct drm_file *file)
> +{
> +       struct drm_i915_gem_vm_bind *args = data;
> +       struct i915_address_space *vm;
> +       int ret = -EINVAL;
> +
> +       vm = i915_gem_address_space_lookup(file->driver_priv, args->vm_id);
> +       if (unlikely(!vm))
> +               return -ENOENT;
> +
> +       switch (args->type) {
> +       case I915_GEM_VM_BIND_SVM_OBJ:
> +               ret = i915_gem_vm_bind_svm_obj(vm, args, file);
> +       }
> +
> +       i915_vm_put(vm);
> +       return ret;
> +}
> +
>  static const struct drm_ioctl_desc i915_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(I915_INIT, drm_noop,
> DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>         DRM_IOCTL_DEF_DRV(I915_FLUSH, drm_noop, DRM_AUTH),
> @@ -2739,6 +2759,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl,
> DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
> DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(I915_GEM_VM_BIND, i915_gem_vm_bind_ioctl,
> DRM_RENDER_ALLOW),
>  };
>
>  static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index ce130e1f1e47..2d0a7cd2dc44 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1909,6 +1909,28 @@ i915_gem_context_lookup(struct
> drm_i915_file_private *file_priv, u32 id)
>         return ctx;
>  }
>
> +static inline struct i915_address_space *
> +__i915_gem_address_space_lookup_rcu(struct drm_i915_file_private
> *file_priv,
> +                                   u32 id)
> +{
> +       return idr_find(&file_priv->vm_idr, id);
> +}
> +
> +static inline struct i915_address_space *
> +i915_gem_address_space_lookup(struct drm_i915_file_private *file_priv,
> +                             u32 id)
> +{
> +       struct i915_address_space *vm;
> +
> +       rcu_read_lock();
> +       vm = __i915_gem_address_space_lookup_rcu(file_priv, id);
> +       if (vm)
> +               vm = i915_vm_get(vm);
> +       rcu_read_unlock();
> +
> +       return vm;
> +}
> +
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>                                           u64 min_size, u64 alignment,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index be36719e7987..7d4f5fa84b02 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -586,6 +586,7 @@ static void i915_address_space_init(struct
> i915_address_space *vm, int subclass)
>         stash_init(&vm->free_pages);
>
>         INIT_LIST_HEAD(&vm->bound_list);
> +       INIT_LIST_HEAD(&vm->svm_list);
>  }
>
>  static int __setup_page_dma(struct i915_address_space *vm,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 31a4a96ddd0d..7c1b54c9677d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -285,6 +285,13 @@ struct pagestash {
>         struct pagevec pvec;
>  };
>
> +struct i915_svm_obj {
> +       /** This obj's place in the SVM object list */
> +       struct list_head link;
> +       u32 handle;
> +       u64 offset;
> +};
> +
>  struct i915_address_space {
>         struct kref ref;
>         struct rcu_work rcu;
> @@ -329,6 +336,12 @@ struct i915_address_space {
>          */
>         struct list_head bound_list;
>
> +       /**
> +        * List of SVM bind objects.
> +        */
> +       struct list_head svm_list;
> +       unsigned int svm_count;
> +
>         struct pagestash free_pages;
>
>         /* Global GTT */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 20314eea632a..e10d7bf2cf9f 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -360,6 +360,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_GEM_VM_CREATE         0x3a
>  #define DRM_I915_GEM_VM_DESTROY                0x3b
>  #define DRM_I915_GEM_OBJECT_SETPARAM   DRM_I915_GEM_CONTEXT_SETPARAM
> +#define DRM_I915_GEM_VM_BIND           0x3c
>  /* Must be kept compact -- no holes */
>
>  #define DRM_IOCTL_I915_INIT            DRM_IOW( DRM_COMMAND_BASE +
> DRM_I915_INIT, drm_i915_init_t)
> @@ -424,6 +425,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_VM_CREATE   DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control)
>  #define DRM_IOCTL_I915_GEM_VM_DESTROY  DRM_IOW (DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control)
>  #define DRM_IOCTL_I915_GEM_OBJECT_SETPARAM     DRM_IOWR(DRM_COMMAND_BASE
> + DRM_I915_GEM_OBJECT_SETPARAM, struct drm_i915_gem_object_param)
> +#define DRM_IOCTL_I915_GEM_VM_BIND             DRM_IOWR(DRM_COMMAND_BASE
> + DRM_I915_GEM_VM_BIND, struct drm_i915_gem_vm_bind)
>
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -2300,6 +2302,31 @@ struct drm_i915_query_perf_config {
>         __u8 data[];
>  };
>
> +/**
> + * struct drm_i915_gem_vm_bind
> + *
> + * Bind an object in a vm's page table.
>

First off, this is something I've wanted for a while for Vulkan, it's just
never made its way high enough up the priority list.  However, it's going
to have to come one way or another soon.  I'm glad to see kernel API for
this being proposed.

I do, however, have a few high-level comments/questions about the API:

 1. In order to be useful for sparse memory support, the API has to go the
other way around so that it binds a VA range to a range within the BO.  It
also needs to be able to handle overlapping where two different VA ranges
may map to the same underlying bytes in the BO.  This likely means that
unbind needs to also take a VA range and only unbind that range.

 2. If this is going to be useful for managing GL's address space where we
have lots of BOs, we probably want it to take a list of ranges so we aren't
making one ioctl for each thing we want to bind.

 3. Why are there no ways to synchronize this with anything?  For binding,
this probably isn't really needed as long as the VA range you're binding is
empty.  However, if you want to move bindings around or unbind something,
the only option is to block in userspace and then call bind/unbind.  This
can be done but it means even more threads in the UMD which is unpleasant.
One could argue that that's more or less what the kernel is going to have
to do so we may as well do it in userspace.  However, I'm not 100%
convinced that's true.

--Jason



> + */
> +struct drm_i915_gem_vm_bind {
> +       /** VA start to bind **/
> +       __u64 start;
> +
> +       /** 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)
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> --
> 2.21.0.rc0.32.g243a4c7e27
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Niranjana Vishwanathapura Dec. 13, 2019, 11:13 p.m. UTC | #2
On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
>
>     +/**
>     + * struct drm_i915_gem_vm_bind
>     + *
>     + * Bind an object in a vm's page table.
>
>   First off, this is something I've wanted for a while for Vulkan, it's just
>   never made its way high enough up the priority list.  However, it's going
>   to have to come one way or another soon.  I'm glad to see kernel API for
>   this being proposed.
>   I do, however, have a few high-level comments/questions about the API:
>    1. In order to be useful for sparse memory support, the API has to go the
>   other way around so that it binds a VA range to a range within the BO.  It
>   also needs to be able to handle overlapping where two different VA ranges
>   may map to the same underlying bytes in the BO.  This likely means that
>   unbind needs to also take a VA range and only unbind that range.
>    2. If this is going to be useful for managing GL's address space where we
>   have lots of BOs, we probably want it to take a list of ranges so we
>   aren't making one ioctl for each thing we want to bind.

Hi Jason,

Yah, some of these requirements came up.
They are not being done here due to time and effort involved in defining
those requirements, implementing and validating.

However, this ioctl can be extended in a backward compatible way to handle
those requirements if required.

>    3. Why are there no ways to synchronize this with anything?  For binding,
>   this probably isn't really needed as long as the VA range you're binding
>   is empty.  However, if you want to move bindings around or unbind
>   something, the only option is to block in userspace and then call
>   bind/unbind.  This can be done but it means even more threads in the UMD
>   which is unpleasant.  One could argue that that's more or less what the
>   kernel is going to have to do so we may as well do it in userspace. 
>   However, I'm not 100% convinced that's true.
>   --Jason
>

Yah, that is the thought.
But as SVM feature evolves, I think we can consider handling some such cases
if hadling those in driver does make whole lot sense. 

Thanks,
Niranjana

>
>     + */
>     +struct drm_i915_gem_vm_bind {
>     +       /** VA start to bind **/
>     +       __u64 start;
>     +
>     +       /** 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)
>     +};
>     +
>      #if defined(__cplusplus)
>      }
>      #endif
>     --
>     2.21.0.rc0.32.g243a4c7e27
>
>     _______________________________________________
>     Intel-gfx mailing list
>     Intel-gfx@lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jason Ekstrand Dec. 14, 2019, 12:36 a.m. UTC | #3
On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
niranjana.vishwanathapura@intel.com> wrote:

> On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
> >
> >     +/**
> >     + * struct drm_i915_gem_vm_bind
> >     + *
> >     + * Bind an object in a vm's page table.
> >
> >   First off, this is something I've wanted for a while for Vulkan, it's
> just
> >   never made its way high enough up the priority list.  However, it's
> going
> >   to have to come one way or another soon.  I'm glad to see kernel API
> for
> >   this being proposed.
> >   I do, however, have a few high-level comments/questions about the API:
> >    1. In order to be useful for sparse memory support, the API has to go
> the
> >   other way around so that it binds a VA range to a range within the
> BO.  It
> >   also needs to be able to handle overlapping where two different VA
> ranges
> >   may map to the same underlying bytes in the BO.  This likely means that
> >   unbind needs to also take a VA range and only unbind that range.
> >    2. If this is going to be useful for managing GL's address space
> where we
> >   have lots of BOs, we probably want it to take a list of ranges so we
> >   aren't making one ioctl for each thing we want to bind.
>
> Hi Jason,
>
> Yah, some of these requirements came up.
>

Yes, I have raised them every single time an API like this has come across
my e-mail inbox for years and they continue to get ignored.  Why are we
landing an API that we know isn't the API we want especially when it's
pretty obvious roughly what the API we want is?  It may be less time in the
short term, but long-term it means two ioctls and two implementations in
i915, IGT tests for both code paths, and code in all UMDs to call one or
the other depending on what kernel you're running on, and we have to
maintain all that code going forward forever.  Sure, that's a price we pay
today for a variety of things but that's because they all seemed like the
right thing at the time.  Landing the wrong API when we know it's the wrong
API seems foolish.

They are not being done here due to time and effort involved in defining
> those requirements, implementing and validating.
>

For #1, yes, it would require more effort but for #2, it really doesn't
take any extra effort to make it take an array...


> However, this ioctl can be extended in a backward compatible way to handle
> those requirements if required.
>
> >    3. Why are there no ways to synchronize this with anything?  For
> binding,
> >   this probably isn't really needed as long as the VA range you're
> binding
> >   is empty.  However, if you want to move bindings around or unbind
> >   something, the only option is to block in userspace and then call
> >   bind/unbind.  This can be done but it means even more threads in the
> UMD
> >   which is unpleasant.  One could argue that that's more or less what the
> >   kernel is going to have to do so we may as well do it in userspace.
> >   However, I'm not 100% convinced that's true.
> >   --Jason
> >
>
> Yah, that is the thought.
> But as SVM feature evolves, I think we can consider handling some such
> cases
> if hadling those in driver does make whole lot sense.
>

Sparse binding exists as a feature.  It's been in D3D for some time and
it's in Vulkan.  We pretty much know what the requirements are.  If you go
look at how it's supposed to work in Vulkan, you have a binding queue and
it waits on semaphores before [un]binding and signals semaphores after
[un]binding.  The biggest problem from an API (as opposed to
implementation) POV with doing that in i915 is that we have too many
synchronization primitives to choose from. :-(

--Jason



> Thanks,
> Niranjana
>
> >
> >     + */
> >     +struct drm_i915_gem_vm_bind {
> >     +       /** VA start to bind **/
> >     +       __u64 start;
> >     +
> >     +       /** 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)
> >     +};
> >     +
> >      #if defined(__cplusplus)
> >      }
> >      #endif
> >     --
> >     2.21.0.rc0.32.g243a4c7e27
> >
> >     _______________________________________________
> >     Intel-gfx mailing list
> >     Intel-gfx@lists.freedesktop.org
> >     https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Chris Wilson Dec. 14, 2019, 10:31 a.m. UTC | #4
Quoting Jason Ekstrand (2019-12-14 00:36:19)
> On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
> niranjana.vishwanathapura@intel.com> wrote:
> 
>     On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
>     >
>     >     +/**
>     >     + * struct drm_i915_gem_vm_bind
>     >     + *
>     >     + * Bind an object in a vm's page table.
>     >
>     >   First off, this is something I've wanted for a while for Vulkan, it's
>     just
>     >   never made its way high enough up the priority list.  However, it's
>     going
>     >   to have to come one way or another soon.  I'm glad to see kernel API
>     for
>     >   this being proposed.
>     >   I do, however, have a few high-level comments/questions about the API:
>     >    1. In order to be useful for sparse memory support, the API has to go
>     the
>     >   other way around so that it binds a VA range to a range within the BO. 
>     It
>     >   also needs to be able to handle overlapping where two different VA
>     ranges
>     >   may map to the same underlying bytes in the BO.  This likely means that
>     >   unbind needs to also take a VA range and only unbind that range.
>     >    2. If this is going to be useful for managing GL's address space where
>     we
>     >   have lots of BOs, we probably want it to take a list of ranges so we
>     >   aren't making one ioctl for each thing we want to bind.
> 
>     Hi Jason,
> 
>     Yah, some of these requirements came up.
> 
>  
> Yes, I have raised them every single time an API like this has come across my
> e-mail inbox for years and they continue to get ignored.  Why are we landing an
> API that we know isn't the API we want especially when it's pretty obvious
> roughly what the API we want is?  It may be less time in the short term, but
> long-term it means two ioctls and two implementations in i915, IGT tests for
> both code paths, and code in all UMDs to call one or the other depending on
> what kernel you're running on, and we have to maintain all that code going
> forward forever.  Sure, that's a price we pay today for a variety of things but
> that's because they all seemed like the right thing at the time.  Landing the
> wrong API when we know it's the wrong API seems foolish.

Exactly. This is not even close to the uAPI we need. Reposting an RFC
without taking in the concerns last time (or the time before that, or
the time before that...) suggests that you aren't really requesting for
comments at all.
-Chris
Chris Wilson Dec. 14, 2019, 10:56 a.m. UTC | #5
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@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@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[]

> +struct drm_i915_gem_vm_bind {
> +       /** VA start to bind **/
> +       __u64 start;

iova;
offset; /* into handle */
length; /* from offset */

> +
> +       /** 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.
-Chris
Niranjana Vishwanathapura Dec. 16, 2019, 4:13 a.m. UTC | #6
On Sat, Dec 14, 2019 at 10:31:37AM +0000, Chris Wilson wrote:
>Quoting Jason Ekstrand (2019-12-14 00:36:19)
>> On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
>> niranjana.vishwanathapura@intel.com> wrote:
>>
>>     On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
>>     >
>>     >     +/**
>>     >     + * struct drm_i915_gem_vm_bind
>>     >     + *
>>     >     + * Bind an object in a vm's page table.
>>     >
>>     >   First off, this is something I've wanted for a while for Vulkan, it's
>>     just
>>     >   never made its way high enough up the priority list.  However, it's
>>     going
>>     >   to have to come one way or another soon.  I'm glad to see kernel API
>>     for
>>     >   this being proposed.
>>     >   I do, however, have a few high-level comments/questions about the API:
>>     >    1. In order to be useful for sparse memory support, the API has to go
>>     the
>>     >   other way around so that it binds a VA range to a range within the BO. 
>>     It
>>     >   also needs to be able to handle overlapping where two different VA
>>     ranges
>>     >   may map to the same underlying bytes in the BO.  This likely means that
>>     >   unbind needs to also take a VA range and only unbind that range.
>>     >    2. If this is going to be useful for managing GL's address space where
>>     we
>>     >   have lots of BOs, we probably want it to take a list of ranges so we
>>     >   aren't making one ioctl for each thing we want to bind.
>>
>>     Hi Jason,
>>
>>     Yah, some of these requirements came up.
>>
>>  
>> Yes, I have raised them every single time an API like this has come across my
>> e-mail inbox for years and they continue to get ignored.  Why are we landing an
>> API that we know isn't the API we want especially when it's pretty obvious
>> roughly what the API we want is?  It may be less time in the short term, but
>> long-term it means two ioctls and two implementations in i915, IGT tests for
>> both code paths, and code in all UMDs to call one or the other depending on
>> what kernel you're running on, and we have to maintain all that code going
>> forward forever.  Sure, that's a price we pay today for a variety of things but
>> that's because they all seemed like the right thing at the time.  Landing the
>> wrong API when we know it's the wrong API seems foolish.
>
>Exactly. This is not even close to the uAPI we need. Reposting an RFC
>without taking in the concerns last time (or the time before that, or
>the time before that...) suggests that you aren't really requesting for
>comments at all.

Thanks Jason for detailed exlanation.
Chris, all comments and guidance are much appreciated :)

I haven't looked in detail, but my concern is that implementing
partial object binding (offset, lenght) from vma down to [un]binding
in ppgtt might be a lot of work to include in this SVM patch series.
I believe we need the partial object binding in non-SVM scenario
as well?

Ok, let me change the interface as below.

struct drm_i915_gem_vm_bind_va
{
        /** VA start to bind **/
        __u64 start;

        /** Offset in Object to bind for I915_GEM_VM_BIND_SVM_OBJ type **/
        __u64 offset;

        /** VA length to [un]bind **/
        __u64 length;

        /** Type of memory to [un]bind **/
        __u32 type;
#define I915_GEM_VM_BIND_SVM_OBJ      0
#define I915_GEM_VM_BIND_SVM_BUFFER   1

        /** Object handle to [un]bind for I915_GEM_VM_BIND_SVM_OBJ type **/
        __u32 handle;

        /** Flags **/
        __u32 flags;
#define I915_GEM_VM_BIND_UNBIND      (1 << 0)
#define I915_GEM_VM_BIND_READONLY    (1 << 1)
}

struct drm_i915_gem_vm_bind {
        /** vm to [un]bind **/
        __u32 vm_id;

	/** number of VAs to bind **/
	__u32 num_vas;

	/** Array of VAs to bind **/
	struct drm_i915_gem_vm_bind_va *bind_vas;

	/** User extensions **/
        __u64 extensions;
};

When synchronization control is added as extension, it applies to all VAs in the array.
Does this looks good?

Niranjana

>-Chris
Niranjana Vishwanathapura Dec. 16, 2019, 4:15 a.m. UTC | #7
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@linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@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
Jason Ekstrand Dec. 17, 2019, 6:01 p.m. UTC | #8
On Sun, Dec 15, 2019 at 10:24 PM Niranjan Vishwanathapura <
niranjana.vishwanathapura@intel.com> wrote:

> On Sat, Dec 14, 2019 at 10:31:37AM +0000, Chris Wilson wrote:
> >Quoting Jason Ekstrand (2019-12-14 00:36:19)
> >> On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
> >> niranjana.vishwanathapura@intel.com> wrote:
> >>
> >>     On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
> >>     >
> >>     >     +/**
> >>     >     + * struct drm_i915_gem_vm_bind
> >>     >     + *
> >>     >     + * Bind an object in a vm's page table.
> >>     >
> >>     >   First off, this is something I've wanted for a while for
> Vulkan, it's
> >>     just
> >>     >   never made its way high enough up the priority list.  However,
> it's
> >>     going
> >>     >   to have to come one way or another soon.  I'm glad to see
> kernel API
> >>     for
> >>     >   this being proposed.
> >>     >   I do, however, have a few high-level comments/questions about
> the API:
> >>     >    1. In order to be useful for sparse memory support, the API
> has to go
> >>     the
> >>     >   other way around so that it binds a VA range to a range within
> the BO.
> >>     It
> >>     >   also needs to be able to handle overlapping where two different
> VA
> >>     ranges
> >>     >   may map to the same underlying bytes in the BO.  This likely
> means that
> >>     >   unbind needs to also take a VA range and only unbind that range.
> >>     >    2. If this is going to be useful for managing GL's address
> space where
> >>     we
> >>     >   have lots of BOs, we probably want it to take a list of ranges
> so we
> >>     >   aren't making one ioctl for each thing we want to bind.
> >>
> >>     Hi Jason,
> >>
> >>     Yah, some of these requirements came up.
> >>
> >>
> >> Yes, I have raised them every single time an API like this has come
> across my
> >> e-mail inbox for years and they continue to get ignored.  Why are we
> landing an
> >> API that we know isn't the API we want especially when it's pretty
> obvious
> >> roughly what the API we want is?  It may be less time in the short
> term, but
> >> long-term it means two ioctls and two implementations in i915, IGT
> tests for
> >> both code paths, and code in all UMDs to call one or the other
> depending on
> >> what kernel you're running on, and we have to maintain all that code
> going
> >> forward forever.  Sure, that's a price we pay today for a variety of
> things but
> >> that's because they all seemed like the right thing at the time.
> Landing the
> >> wrong API when we know it's the wrong API seems foolish.
> >
> >Exactly. This is not even close to the uAPI we need. Reposting an RFC
> >without taking in the concerns last time (or the time before that, or
> >the time before that...) suggests that you aren't really requesting for
> >comments at all.
>
> Thanks Jason for detailed exlanation.
> Chris, all comments and guidance are much appreciated :)
>
> I haven't looked in detail, but my concern is that implementing
> partial object binding (offset, lenght) from vma down to [un]binding
> in ppgtt might be a lot of work to include in this SVM patch series.
> I believe we need the partial object binding in non-SVM scenario
> as well?
>

Yes, the Vulkan APIs require both partial binding and aliasing.

It may be worth pointing out that we're already doing some of this stuff
today, although in a rather backwards way.  Our non-softpin model for
Vulkan uses a memfd which we then map in userspace and turn into a BO via
userptr.  Due to the way we handle threading in the driver, we end up with
multiple BOs pointing to the same overlapping range in the memfd and hence
the same pages.  That doesn't mean that everything in the path is already
set up for what you need but the VA -> page mappings should be.  Also,
avoiding these kinds of shinanigans is exactly why we want a "real" kernel
API for this. :-)


> Ok, let me change the interface as below.
>
> struct drm_i915_gem_vm_bind_va
> {
>         /** VA start to bind **/
>         __u64 start;
>
>         /** Offset in Object to bind for I915_GEM_VM_BIND_SVM_OBJ type **/
>         __u64 offset;
>
>         /** VA length to [un]bind **/
>         __u64 length;
>
>         /** Type of memory to [un]bind **/
>         __u32 type;
> #define I915_GEM_VM_BIND_SVM_OBJ      0
> #define I915_GEM_VM_BIND_SVM_BUFFER   1
>
>         /** Object handle to [un]bind for I915_GEM_VM_BIND_SVM_OBJ type **/
>         __u32 handle;
>
>         /** Flags **/
>         __u32 flags;
> #define I915_GEM_VM_BIND_UNBIND      (1 << 0)
> #define I915_GEM_VM_BIND_READONLY    (1 << 1)
> }
>
> struct drm_i915_gem_vm_bind {
>         /** vm to [un]bind **/
>         __u32 vm_id;
>
>         /** number of VAs to bind **/
>         __u32 num_vas;
>
>         /** Array of VAs to bind **/
>         struct drm_i915_gem_vm_bind_va *bind_vas;
>
>         /** User extensions **/
>         __u64 extensions;
> };
>
> When synchronization control is added as extension, it applies to all VAs
> in the array.
> Does this looks good?
>

Yes, I think that header looks good.  It's probably fine if synchronization
comes later.

I have two more questions (more for my own education than anything):

 1. What is the difference between a SVM object and a buffer?

 2. I see a vm_id but there is no context.  What (if any) are the
synchronization guarantees between the VM_BIND ioctl and EXECBUF?  If I
VM_BIND followed by EXECBUF is it guaranteed to happen in that order?  What
if I EXECBUF and then VM_BIND to unbind something?  If I VM_BIND while an
execbuf is happening but I have some way of delaying the GPU work from the
CPU and I unblock it once the VM_BIND comes back, is that ok?

If those questions are answered by other patches, feel free to just point
me at them instead of answering in detail here.

--Jason
Jason Gunthorpe Dec. 17, 2019, 8:18 p.m. UTC | #9
On Fri, Dec 13, 2019 at 01:56:04PM -0800, Niranjana Vishwanathapura wrote:
  
> +	ctx = i915_gem_context_lookup(file->driver_priv, args->rsvd1);
> +	if (!ctx || !rcu_access_pointer(ctx->vm))
> +		return -ENOENT;
> +
> +	rcu_read_lock();
> +	vm = i915_vm_get(ctx->vm);
> +	rcu_read_unlock();

This looks like wrong use of RCU to me.

Getting a rcu lock and not calling rcu_dereference under it is
basically guarenteed to be wrong..

Jason
Niranjana Vishwanathapura Dec. 18, 2019, 10:51 p.m. UTC | #10
On Sun, Dec 15, 2019 at 08:15:24PM -0800, Niranjan Vishwanathapura wrote:
>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@linux.intel.com>
>>>Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>>>Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>>>Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@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.
>

Ok, we discussed it offline.
I will look into some of the requirement/usecases above to capture change
required to uapi (if any) including around synchronization between
VM_BIND and execbuff paths.

Thanks,
Niranjana

>>>+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
Niranjana Vishwanathapura Dec. 18, 2019, 11:25 p.m. UTC | #11
On Tue, Dec 17, 2019 at 12:01:26PM -0600, Jason Ekstrand wrote:
>   On Sun, Dec 15, 2019 at 10:24 PM Niranjan Vishwanathapura
>   <niranjana.vishwanathapura@intel.com> wrote:
>
>     On Sat, Dec 14, 2019 at 10:31:37AM +0000, Chris Wilson wrote:
>     >Quoting Jason Ekstrand (2019-12-14 00:36:19)
>     >> On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
>     >> niranjana.vishwanathapura@intel.com> wrote:
>     >>
>     >>     On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
>     >>     >
>     >>     >     +/**
>     >>     >     + * struct drm_i915_gem_vm_bind
>     >>     >     + *
>     >>     >     + * Bind an object in a vm's page table.
>     >>     >
>     >>     >   First off, this is something I've wanted for a while for
>     Vulkan, it's
>     >>     just
>     >>     >   never made its way high enough up the priority list. 
>     However, it's
>     >>     going
>     >>     >   to have to come one way or another soon.  I'm glad to see
>     kernel API
>     >>     for
>     >>     >   this being proposed.
>     >>     >   I do, however, have a few high-level comments/questions about
>     the API:
>     >>     >    1. In order to be useful for sparse memory support, the API
>     has to go
>     >>     the
>     >>     >   other way around so that it binds a VA range to a range
>     within the BO. 
>     >>     It
>     >>     >   also needs to be able to handle overlapping where two
>     different VA
>     >>     ranges
>     >>     >   may map to the same underlying bytes in the BO.  This likely
>     means that
>     >>     >   unbind needs to also take a VA range and only unbind that
>     range.
>     >>     >    2. If this is going to be useful for managing GL's address
>     space where
>     >>     we
>     >>     >   have lots of BOs, we probably want it to take a list of
>     ranges so we
>     >>     >   aren't making one ioctl for each thing we want to bind.
>     >>
>     >>     Hi Jason,
>     >>
>     >>     Yah, some of these requirements came up.
>     >>
>     >>  
>     >> Yes, I have raised them every single time an API like this has come
>     across my
>     >> e-mail inbox for years and they continue to get ignored.  Why are we
>     landing an
>     >> API that we know isn't the API we want especially when it's pretty
>     obvious
>     >> roughly what the API we want is?  It may be less time in the short
>     term, but
>     >> long-term it means two ioctls and two implementations in i915, IGT
>     tests for
>     >> both code paths, and code in all UMDs to call one or the other
>     depending on
>     >> what kernel you're running on, and we have to maintain all that code
>     going
>     >> forward forever.  Sure, that's a price we pay today for a variety of
>     things but
>     >> that's because they all seemed like the right thing at the time. 
>     Landing the
>     >> wrong API when we know it's the wrong API seems foolish.
>     >
>     >Exactly. This is not even close to the uAPI we need. Reposting an RFC
>     >without taking in the concerns last time (or the time before that, or
>     >the time before that...) suggests that you aren't really requesting for
>     >comments at all.
>
>     Thanks Jason for detailed exlanation.
>     Chris, all comments and guidance are much appreciated :)
>
>     I haven't looked in detail, but my concern is that implementing
>     partial object binding (offset, lenght) from vma down to [un]binding
>     in ppgtt might be a lot of work to include in this SVM patch series.
>     I believe we need the partial object binding in non-SVM scenario
>     as well?
>
>   Yes, the Vulkan APIs require both partial binding and aliasing.
>   It may be worth pointing out that we're already doing some of this stuff
>   today, although in a rather backwards way.  Our non-softpin model for
>   Vulkan uses a memfd which we then map in userspace and turn into a BO via
>   userptr.  Due to the way we handle threading in the driver, we end up with
>   multiple BOs pointing to the same overlapping range in the memfd and hence
>   the same pages.  That doesn't mean that everything in the path is already
>   set up for what you need but the VA -> page mappings should be.  Also,
>   avoiding these kinds of shinanigans is exactly why we want a "real" kernel
>   API for this. :-)
>    

Ok thanks Jason for the explantion.
Will look into supporting this here.

>
>     Ok, let me change the interface as below.
>
>     struct drm_i915_gem_vm_bind_va
>     {
>             /** VA start to bind **/
>             __u64 start;
>
>             /** Offset in Object to bind for I915_GEM_VM_BIND_SVM_OBJ type
>     **/
>             __u64 offset;
>
>             /** VA length to [un]bind **/
>             __u64 length;
>
>             /** Type of memory to [un]bind **/
>             __u32 type;
>     #define I915_GEM_VM_BIND_SVM_OBJ      0
>     #define I915_GEM_VM_BIND_SVM_BUFFER   1
>
>             /** Object handle to [un]bind for I915_GEM_VM_BIND_SVM_OBJ type
>     **/
>             __u32 handle;
>
>             /** Flags **/
>             __u32 flags;
>     #define I915_GEM_VM_BIND_UNBIND      (1 << 0)
>     #define I915_GEM_VM_BIND_READONLY    (1 << 1)
>     }
>
>     struct drm_i915_gem_vm_bind {
>             /** vm to [un]bind **/
>             __u32 vm_id;
>
>             /** number of VAs to bind **/
>             __u32 num_vas;
>
>             /** Array of VAs to bind **/
>             struct drm_i915_gem_vm_bind_va *bind_vas;
>
>             /** User extensions **/
>             __u64 extensions;
>     };
>
>     When synchronization control is added as extension, it applies to all
>     VAs in the array.
>     Does this looks good?
>
>   Yes, I think that header looks good.  It's probably fine if
>   synchronization comes later.
>   I have two more questions (more for my own education than anything):
>    1. What is the difference between a SVM object and a buffer?

SVM object is the GEM BO. By buffer I mean system allocated memory (say malloc()).
I have some documentation in patch 01.
https://lists.freedesktop.org/archives/intel-gfx/2019-December/223481.html

>    2. I see a vm_id but there is no context.  What (if any) are the
>   synchronization guarantees between the VM_BIND ioctl and EXECBUF?  If I
>   VM_BIND followed by EXECBUF is it guaranteed to happen in that order? 
>   What if I EXECBUF and then VM_BIND to unbind something?  If I VM_BIND
>   while an execbuf is happening but I have some way of delaying the GPU work
>   from the CPU and I unblock it once the VM_BIND comes back, is that ok?
>   If those questions are answered by other patches, feel free to just point
>   me at them instead of answering in detail here.

Binding/unbinding is w.r.t to a device page table (hence the vm_id).
With current implementation, Yes, EXECBUF after the VM_BIND will see those binds
and ensure that those VA ranges are bound in device page table.
VM_BIND to bind/unbind after issuing EXECBUF which is alredy running (eg., endless
batch buffer), is not currently supported by this patch. But yes, I expect your
above scenario should be allowed and supported eventually.
I agree we need to set right expectation here for some of the use cases.
Let me look into this synchronization b/w two paths a bit more and sync with you.

Thanks,
Niranjana

>   --Jason
Niranjana Vishwanathapura Dec. 18, 2019, 11:34 p.m. UTC | #12
On Tue, Dec 17, 2019 at 08:18:21PM +0000, Jason Gunthorpe wrote:
>On Fri, Dec 13, 2019 at 01:56:04PM -0800, Niranjana Vishwanathapura wrote:
>
>> +	ctx = i915_gem_context_lookup(file->driver_priv, args->rsvd1);
>> +	if (!ctx || !rcu_access_pointer(ctx->vm))
>> +		return -ENOENT;
>> +
>> +	rcu_read_lock();
>> +	vm = i915_vm_get(ctx->vm);
>> +	rcu_read_unlock();
>
>This looks like wrong use of RCU to me.
>
>Getting a rcu lock and not calling rcu_dereference under it is
>basically guarenteed to be wrong..
>

Oops, yah, I should be just calling i915_gem_context_get_vm_rcu().

Thanks,
Niranjana

>Jason

Patch
diff mbox series

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;
+
+	rcu_read_lock();
+	vm = i915_vm_get(ctx->vm);
+	rcu_read_unlock();
+
+alloc_again:
+	svm_count = vm->svm_count;
 	/* Allocate an extra slot for use by the command parser */
-	exec2_list = kvmalloc_array(count + 1, eb_element_size(),
+	exec2_list = kvmalloc_array(count + svm_count + 1, eb_element_size(),
 				    __GFP_NOWARN | GFP_KERNEL);
 	if (exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %zd buffers\n",
-			  count);
+			  count + svm_count);
 		return -ENOMEM;
 	}
-	if (copy_from_user(exec2_list,
+	mutex_lock(&vm->mutex);
+	if (svm_count != vm->svm_count) {
+		mutex_unlock(&vm->mutex);
+		kvfree(exec2_list);
+		goto alloc_again;
+	}
+
+	list_for_each_entry(svm_obj, &vm->svm_list, link) {
+		memset(&exec2_list[i], 0, sizeof(*exec2_list));
+		exec2_list[i].handle = svm_obj->handle;
+		exec2_list[i].offset = svm_obj->offset;
+		exec2_list[i].flags = EXEC_OBJECT_PINNED |
+				      EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+		i++;
+	}
+	exec2_list_user = &exec2_list[i];
+	args->buffer_count += svm_count;
+	mutex_unlock(&vm->mutex);
+	i915_vm_put(vm);
+	i915_gem_context_put(ctx);
+
+	if (copy_from_user(exec2_list_user,
 			   u64_to_user_ptr(args->buffers_ptr),
 			   sizeof(*exec2_list) * count)) {
 		DRM_DEBUG("copy %zd exec entries failed\n", count);
@@ -2876,6 +2911,7 @@  i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
 	}
 
 	err = i915_gem_do_execbuffer(dev, file, args, exec2_list, fences);
+	args->buffer_count -= svm_count;
 
 	/*
 	 * Now that we have begun execution of the batchbuffer, we ignore
@@ -2886,7 +2922,6 @@  i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & __EXEC_HAS_RELOC) {
 		struct drm_i915_gem_exec_object2 __user *user_exec_list =
 			u64_to_user_ptr(args->buffers_ptr);
-		unsigned int i;
 
 		/* Copy the new buffer offsets back to the user's exec list. */
 		/*
@@ -2900,13 +2935,14 @@  i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
 			goto end;
 
 		for (i = 0; i < args->buffer_count; i++) {
-			if (!(exec2_list[i].offset & UPDATE))
+			u64 *offset = &exec2_list_user[i].offset;
+
+			if (!(*offset & UPDATE))
 				continue;
 
-			exec2_list[i].offset =
-				gen8_canonical_addr(exec2_list[i].offset & PIN_OFFSET_MASK);
-			unsafe_put_user(exec2_list[i].offset,
-					&user_exec_list[i].offset,
+			*offset = gen8_canonical_addr(*offset &
+						      PIN_OFFSET_MASK);
+			unsafe_put_user(*offset, &user_exec_list[i].offset,
 					end_user);
 		}
 end_user:
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_svm.c b/drivers/gpu/drm/i915/gem/i915_gem_svm.c
new file mode 100644
index 000000000000..882fe56138e2
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_svm.c
@@ -0,0 +1,60 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include "i915_drv.h"
+#include "i915_gem_gtt.h"
+#include "i915_gem_lmem.h"
+
+int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
+			     struct drm_i915_gem_vm_bind *args,
+			     struct drm_file *file)
+{
+	struct i915_svm_obj *svm_obj, *tmp;
+	struct drm_i915_gem_object *obj;
+	int ret = 0;
+
+	obj = i915_gem_object_lookup(file, args->handle);
+	if (!obj)
+		return -ENOENT;
+
+	/* For dgfx, ensure the obj is in device local memory only */
+	if (IS_DGFX(vm->i915) && !i915_gem_object_is_lmem(obj))
+		return -EINVAL;
+
+	/* FIXME: Need to handle case with unending batch buffers */
+	if (!(args->flags & I915_GEM_VM_BIND_UNBIND)) {
+		svm_obj = kmalloc(sizeof(*svm_obj), GFP_KERNEL);
+		if (!svm_obj) {
+			ret = -ENOMEM;
+			goto put_obj;
+		}
+		svm_obj->handle = args->handle;
+		svm_obj->offset = args->start;
+	}
+
+	mutex_lock(&vm->mutex);
+	if (!(args->flags & I915_GEM_VM_BIND_UNBIND)) {
+		list_add(&svm_obj->link, &vm->svm_list);
+		vm->svm_count++;
+	} else {
+		/*
+		 * FIXME: Need to handle case where object is migrated/closed
+		 * without unbinding first.
+		 */
+		list_for_each_entry_safe(svm_obj, tmp, &vm->svm_list, link) {
+			if (svm_obj->handle != args->handle)
+				continue;
+
+			list_del_init(&svm_obj->link);
+			vm->svm_count--;
+			kfree(svm_obj);
+			break;
+		}
+	}
+	mutex_unlock(&vm->mutex);
+put_obj:
+	i915_gem_object_put(obj);
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_svm.h b/drivers/gpu/drm/i915/gem/i915_gem_svm.h
new file mode 100644
index 000000000000..d60b35c7d21a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_svm.h
@@ -0,0 +1,22 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __I915_GEM_SVM_H
+#define __I915_GEM_SVM_H
+
+#include "i915_drv.h"
+
+#if defined(CONFIG_DRM_I915_SVM)
+int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
+			     struct drm_i915_gem_vm_bind *args,
+			     struct drm_file *file);
+#else
+static inline int i915_gem_vm_bind_svm_obj(struct i915_address_space *vm,
+					   struct drm_i915_gem_vm_bind *args,
+					   struct drm_file *file)
+{ return -ENOTSUPP; }
+#endif
+
+#endif /* __I915_GEM_SVM_H */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2a11f60c4fd2..d452ea8e40b3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2680,6 +2680,26 @@  i915_gem_reject_pin_ioctl(struct drm_device *dev, void *data,
 	return -ENODEV;
 }
 
+static int i915_gem_vm_bind_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file)
+{
+	struct drm_i915_gem_vm_bind *args = data;
+	struct i915_address_space *vm;
+	int ret = -EINVAL;
+
+	vm = i915_gem_address_space_lookup(file->driver_priv, args->vm_id);
+	if (unlikely(!vm))
+		return -ENOENT;
+
+	switch (args->type) {
+	case I915_GEM_VM_BIND_SVM_OBJ:
+		ret = i915_gem_vm_bind_svm_obj(vm, args, file);
+	}
+
+	i915_vm_put(vm);
+	return ret;
+}
+
 static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_INIT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_FLUSH, drm_noop, DRM_AUTH),
@@ -2739,6 +2759,7 @@  static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_VM_BIND, i915_gem_vm_bind_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ce130e1f1e47..2d0a7cd2dc44 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1909,6 +1909,28 @@  i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
 	return ctx;
 }
 
+static inline struct i915_address_space *
+__i915_gem_address_space_lookup_rcu(struct drm_i915_file_private *file_priv,
+				    u32 id)
+{
+	return idr_find(&file_priv->vm_idr, id);
+}
+
+static inline struct i915_address_space *
+i915_gem_address_space_lookup(struct drm_i915_file_private *file_priv,
+			      u32 id)
+{
+	struct i915_address_space *vm;
+
+	rcu_read_lock();
+	vm = __i915_gem_address_space_lookup_rcu(file_priv, id);
+	if (vm)
+		vm = i915_vm_get(vm);
+	rcu_read_unlock();
+
+	return vm;
+}
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index be36719e7987..7d4f5fa84b02 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -586,6 +586,7 @@  static void i915_address_space_init(struct i915_address_space *vm, int subclass)
 	stash_init(&vm->free_pages);
 
 	INIT_LIST_HEAD(&vm->bound_list);
+	INIT_LIST_HEAD(&vm->svm_list);
 }
 
 static int __setup_page_dma(struct i915_address_space *vm,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 31a4a96ddd0d..7c1b54c9677d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -285,6 +285,13 @@  struct pagestash {
 	struct pagevec pvec;
 };
 
+struct i915_svm_obj {
+	/** This obj's place in the SVM object list */
+	struct list_head link;
+	u32 handle;
+	u64 offset;
+};
+
 struct i915_address_space {
 	struct kref ref;
 	struct rcu_work rcu;
@@ -329,6 +336,12 @@  struct i915_address_space {
 	 */
 	struct list_head bound_list;
 
+	/**
+	 * List of SVM bind objects.
+	 */
+	struct list_head svm_list;
+	unsigned int svm_count;
+
 	struct pagestash free_pages;
 
 	/* Global GTT */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 20314eea632a..e10d7bf2cf9f 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -360,6 +360,7 @@  typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_VM_CREATE		0x3a
 #define DRM_I915_GEM_VM_DESTROY		0x3b
 #define DRM_I915_GEM_OBJECT_SETPARAM	DRM_I915_GEM_CONTEXT_SETPARAM
+#define DRM_I915_GEM_VM_BIND		0x3c
 /* Must be kept compact -- no holes */
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
@@ -424,6 +425,7 @@  typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_VM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control)
 #define DRM_IOCTL_I915_GEM_VM_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control)
 #define DRM_IOCTL_I915_GEM_OBJECT_SETPARAM	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_OBJECT_SETPARAM, struct drm_i915_gem_object_param)
+#define DRM_IOCTL_I915_GEM_VM_BIND		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_BIND, struct drm_i915_gem_vm_bind)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -2300,6 +2302,31 @@  struct drm_i915_query_perf_config {
 	__u8 data[];
 };
 
+/**
+ * struct drm_i915_gem_vm_bind
+ *
+ * Bind an object in a vm's page table.
+ */
+struct drm_i915_gem_vm_bind {
+	/** VA start to bind **/
+	__u64 start;
+
+	/** 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)
+};
+
 #if defined(__cplusplus)
 }
 #endif