Re: [Intel-gfx] [PATCH v3 35/37] drm/i915: Introduce GEM_OBJECT_SETPARAM with I915_PARAM_MEMORY_REGION

Quoting Matthew Auld (2019-08-09 23:26:41)

From: Abdiel Janulgue abdiel.janulgue@linux.intel.com

This call will specify which memory region an object should be placed.

Note that changing the object's backing storage should be immediately done after an object is created or if it's not yet in use, otherwise this will fail on a busy object.

Signed-off-by: Abdiel Janulgue abdiel.janulgue@linux.intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 17 +++ drivers/gpu/drm/i915/gem/i915_gem_context.h | 2 + drivers/gpu/drm/i915/gem/i915_gem_ioctls.h | 2 + drivers/gpu/drm/i915/gem/i915_gem_object.c | 115 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.c | 2 +- include/uapi/drm/i915_drm.h | 23 ++++ 6 files changed, 160 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index b407baaf0014..572033ac6e3b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -76,6 +76,7 @@ #include "i915_globals.h" #include "i915_trace.h" #include "i915_user_extensions.h" +#include "i915_gem_ioctls.h"

#define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1

@@ -2308,6 +2309,22 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, return ret; }

+int i915_gem_setparam_ioctl(struct drm_device *dev, void *data,

•                       struct drm_file *file)
  

This does not need to be in i915_gem_context.c

+{

•   struct drm_i915_gem_context_param *args = data;
  

switch(upper_32_bits(args->param)) {

•   case 0:
  

•           return i915_gem_context_setparam_ioctl(dev, data, file);
  

•   case 1:
  

•           return i915_gem_object_setparam_ioctl(dev, data, file);
  

•   }
  

•   return -EINVAL;
  

+}

int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h index 106e2ccf7a4c..1cfcf1e6bbb9 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h @@ -157,6 +157,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int i915_gem_setparam_ioctl(struct drm_device *dev, void *data,

•                       struct drm_file *file);
  

int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data, struct drm_file *file);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h b/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h index 5abd5b2172f2..af7465bceebd 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h @@ -32,6 +32,8 @@ int i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data, struct drm_file *file); int i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,

•                              struct drm_file *file_priv);
  

int i915_gem_pread_ioctl(struct drm_device *dev, void *data, struct drm_file *file); int i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 5982aeaaa2e3..52ea65f203a1 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -506,6 +506,121 @@ int __init i915_global_objects_init(void) return 0; }

+static enum intel_region_id +__region_id(u32 region) +{

•   enum intel_region_id id;
  

•   for (id = 0; id < INTEL_MEMORY_UKNOWN; ++id) {
  

•           if (intel_region_map[id] == region)
  

•                   return id;
  

•   }
  

•   return INTEL_MEMORY_UKNOWN;
  

+}

+static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,

•                                    struct drm_i915_gem_object_param *args,
  

•                                    struct drm_file *file,
  

•                                    struct drm_i915_gem_object *obj)
  

+{

•   struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
  

•   u32 __user *uregions = u64_to_user_ptr(args->data);
  

•   u32 uregions_copy[INTEL_MEMORY_UKNOWN];
  

•   int i, ret;
  

•   if (args->size > INTEL_MEMORY_UKNOWN)
  

•           return -EINVAL;
  

args->size should be in bytes (as returned by the GETPARAM for this).

•   memset(uregions_copy, 0, sizeof(uregions_copy));
  

•   for (i = 0; i < args->size; i++) {
  

•           u32 region;
  

•           ret = get_user(region, uregions);
  

•           if (ret)
  

•                   return ret;
  

•           uregions_copy[i] = region;
  

•           ++uregions;
  

•   }
  

•   mutex_lock(&dev_priv->drm.struct_mutex);
  

Pardon?

•   ret = i915_gem_object_prepare_move(obj);
  

•   if (ret) {
  

•           DRM_ERROR("Cannot set memory region, object in use\n");
  

•           goto err;
  

•   }
  

•   for (i = 0; i < args->size; i++) {
  

•           u32 region = uregions_copy[i];
  

•           enum intel_region_id id = __region_id(region);
  

•           if (id == INTEL_MEMORY_UKNOWN) {
  

•                   ret = -EINVAL;
  

•                   goto err;
  

•           }
  

•           ret = i915_gem_object_migrate(obj, ce, id);
  

•           if (!ret) {
  

•                   if (!i915_gem_object_has_pages(obj) &&
  

•                       MEMORY_TYPE_FROM_REGION(region) ==
  

•                       INTEL_LMEM) {
  

•                           /*
  

•                            * TODO: this should be part of get_pages(),
  

•                            * when async get_pages arrives
  

•                            */
  

•                           ret = i915_gem_object_fill_blt(obj, ce, 0);
  

•                           if (ret) {
  

•                                   DRM_ERROR("Failed clearing the object\n");
  

•                                   goto err;
  

•                           }
  

•                           i915_gem_object_lock(obj);
  

•                           ret = i915_gem_object_set_to_cpu_domain(obj, false);
  

•                           i915_gem_object_unlock(obj);
  

Pardon?

•                           if (ret)
  

•                                   goto err;
  

•                   }
  

•                   break;
  

•           }
  

•   }
  

+err:

•   mutex_unlock(&dev_priv->drm.struct_mutex);
  

•   return ret;
  

+}

+int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,

•                              struct drm_file *file)
  

+{

•   struct drm_i915_gem_object_param *args = data;
  

•   struct drm_i915_private *dev_priv = to_i915(dev);
  

•   struct drm_i915_gem_object *obj;
  

•   int ret;
  

•   obj = i915_gem_object_lookup(file, args->handle);
  

•   if (!obj)
  

•           return -ENOENT;
  

•   switch (lower_32_bits(args->param)) {
  

•   case I915_PARAM_MEMORY_REGION:
  

•           ret = i915_gem_object_region_select(dev_priv, args, file, obj);
  

•           if (ret) {
  

•                   DRM_ERROR("Cannot set memory region, migration failed\n");
  

•                   goto err;
  

•           }
  

•           break;
  

•   default:
  

•           ret = -EINVAL;
  

•           break;
  

•   }
  

+err:

•   i915_gem_object_put(obj);
  

•   return ret;
  

+}

#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/huge_gem_object.c" #include "selftests/huge_pages.c" diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f6a3daf696f6..845e80c2acc0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2729,7 +2729,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_gem_context_reset_stats_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),

•   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
  

•   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_setparam_ioctl, DRM_RENDER_ALLOW),
    DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
    DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_RENDER_ALLOW),
    DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_RENDER_ALLOW),
  

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index fb84aed10825..75d79c17e91b 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_MMAP_OFFSET DRM_I915_GEM_MMAP_GTT +#define DRM_I915_GEM_OBJECT_SETPARAM DRM_I915_GEM_CONTEXT_SETPARAM

#define DRM_I915_GEM_SETPARAM 0x35 #define DRM_I915_GEM_GETPARAM 0x36 #define DRM_I915_GEM_CONTEXT_SETPARAM DRM_I915_GEM_SETPARAM #define DRM_I915_GEM_CONTEXT_GETPARAM DRM_I915_GEM_GETPARAM

Make sure you do do both.

/* Must be kept compact -- no holes */

#define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t) @@ -423,6 +424,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_MMAP_OFFSET DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_OFFSET, struct drm_i915_gem_mmap_offset) +#define DRM_IOCTL_I915_GEM_OBJECT_SETPARAM DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_OBJECT_SETPARAM, struct drm_i915_gem_object_param)

/* Allow drivers to submit batchbuffers directly to hardware, relying

• on the security mechanisms provided by hardware.

@@ -1601,6 +1603,27 @@ struct drm_i915_gem_context_param { __u64 value; };

+struct drm_i915_gem_object_param {

•   /** Handle for the object */
  

•   __u32 handle;
  

•   __u32 size;
  

•   /** Set the memory region for the object listed in preference order
  

/** * Set

This is not kerneldoc anyway.

And the comment is not abutting the right variable.

•    *  as an array of region ids within data. To force an object
  

•    *  to a particular memory region, set the region as the sole entry.
  

•    *
  

•    *  Valid region ids are derived from the id field of
  

•    *  struct drm_i915_memory_region_info.
  

•    *  See struct drm_i915_query_memory_region_info.
  

•    */
  

+#define I915_OBJECT_PARAM (1ull<<32)

#define I915_OBJECT_PARAM_REGION (I915_OBJECT_PARAM | 0x1)

•   __u64 param;
  

•   __u64 data;
  

Any particular reason for the divergent with the identical struct? -Chris