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

Chris Wilson chris at chris-wilson.co.uk
Sat Aug 10 11:54:54 UTC 2019


Quoting Matthew Auld (2019-08-09 23:26:41)
> From: Abdiel Janulgue <abdiel.janulgue at 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 at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at 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


More information about the Intel-gfx mailing list