Re: [PATCH v2 34/37] drm/i915: Introduce GEM_OBJECT_SETPARAM with I915_PARAM_MEMORY_REGION

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/06/2019 21:56, Matthew Auld wrote:
> 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 |  12 ++
>   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  | 117 ++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_drv.c             |   2 +-
>   include/uapi/drm/i915_drm.h                 |  27 +++++
>   6 files changed, 161 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 8a9787cf0cd0..157ca8247752 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -75,6 +75,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
>   
> @@ -2357,6 +2358,17 @@ 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)
> +{
> +	struct drm_i915_gem_context_param *args = data;
> +
> +	if (args->param <= I915_CONTEXT_PARAM_MAX)
> +		return i915_gem_context_setparam_ioctl(dev, data, file);
> +
> +	return i915_gem_object_setparam_ioctl(dev, data, 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_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index 9691dd062f72..d5a9a63bb34c 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 691af388e4e7..bc95f449de50 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -551,6 +551,123 @@ 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 < ARRAY_SIZE(intel_region_map); ++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 > ARRAY_SIZE(intel_region_map))
> +		return -EINVAL;
> +
> +	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);
> +	ret = i915_gem_object_prepare_move(obj);
> +	if (ret) {
> +		DRM_ERROR("Cannot set memory region, object in use\n");
> +	        goto err;
> +	}
> +
> +	if (args->size > ARRAY_SIZE(intel_region_map))
> +		return -EINVAL;
> +
> +	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 (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);
> +				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 (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 1c3d5cb2893c..3d6fe993f26e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3196,7 +3196,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 729e729e2282..5cf976e7608a 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
>   /* 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.
> @@ -1595,11 +1597,36 @@ struct drm_i915_gem_context_param {
>    *   i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
>    */
>   #define I915_CONTEXT_PARAM_ENGINES	0xa
> +
> +#define I915_CONTEXT_PARAM_MAX	        0xffffffff
>   /* Must be kept compact -- no holes and well documented */
>   
>   	__u64 value;
>   };
>   
> +struct drm_i915_gem_object_param {
> +	/** Handle for the object */
> +	__u32 handle;
> +
> +	__u32 size;
> +
> +	/* Must be 1 */
> +	__u32 object_class;

What is this for? It's not used in the patch.

> +
> +	/** Set the memory region for the object listed in preference order
> +	 *  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.
> +	 */

These two structs only come in the next patch so I suspect the order of 
the two needs swapping.

Regards,

Tvrtko

> +#define I915_PARAM_MEMORY_REGION 0x1
> +	__u32 param;
> +
> +	__u64 data;
> +};
> +
>   /**
>    * Context SSEU programming
>    *
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx