[Intel-gfx] [PATCH 28/38] drm/i915: Create/destroy VM (ppGTT) for use with contexts
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Jan 23 11:30:39 UTC 2019
On 18/01/2019 14:00, Chris Wilson wrote:
> In preparation to making the ppGTT binding for a context explicit (to
> facilitate reusing the same ppGTT between different contexts), allow the
> user to create and destroy named ppGTT.
Needs some more text to explain uAPI usage, or even better kerneldoc in
i915_drm.h.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> drivers/gpu/drm/i915/i915_gem_context.c | 83 +++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_context.h | 5 ++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +
> include/uapi/drm/i915_drm.h | 10 +++
> 6 files changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f462a4d28af4..56bd087f1c4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3003,6 +3003,8 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_UNLOCKED|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),
> };
>
> static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 74bccb153359..cb11c23823c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -217,6 +217,9 @@ struct drm_i915_file_private {
> } mm;
> struct idr context_idr;
>
> + struct mutex vm_lock;
> + struct idr vm_idr;
> +
> struct intel_rps_client {
> atomic_t boosts;
> } rps_client;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e8334c4bc130..7c90981704bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -598,19 +598,29 @@ static int context_idr_cleanup(int id, void *p, void *data)
> return 0;
> }
>
> +static int vm_idr_cleanup(int id, void *p, void *data)
> +{
> + i915_ppgtt_put(p);
> + return 0;
> +}
> +
> int i915_gem_context_open(struct drm_i915_private *i915,
> struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct i915_gem_context *ctx;
>
> + mutex_init(&file_priv->vm_lock);
> +
> idr_init(&file_priv->context_idr);
> + idr_init_base(&file_priv->vm_idr, 1);
>
> mutex_lock(&i915->drm.struct_mutex);
> ctx = i915_gem_create_context(i915, file_priv);
> mutex_unlock(&i915->drm.struct_mutex);
> if (IS_ERR(ctx)) {
> idr_destroy(&file_priv->context_idr);
> + idr_destroy(&file_priv->vm_idr);
> return PTR_ERR(ctx);
> }
>
> @@ -627,6 +637,79 @@ void i915_gem_context_close(struct drm_file *file)
>
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> idr_destroy(&file_priv->context_idr);
> +
> + idr_for_each(&file_priv->vm_idr, vm_idr_cleanup, NULL);
> + idr_destroy(&file_priv->vm_idr);
> +
> + mutex_destroy(&file_priv->vm_lock);
> +}
> +
> +int i915_gem_vm_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> + struct drm_i915_private *i915 = to_i915(dev);
> + struct drm_i915_gem_vm_create *args = data;
> + struct drm_i915_file_private *file_priv = file->driver_priv;
> + struct i915_hw_ppgtt *ppgtt;
> + int err;
> +
> + if (!HAS_FULL_PPGTT(i915))
> + return -ENODEV;
> +
> + if (args->flags)
> + return -EINVAL;
> +
> + if (args->extensions)
> + return -EINVAL;
> +
> + ppgtt = i915_ppgtt_create(i915, file_priv);
> + if (IS_ERR(ppgtt))
> + return PTR_ERR(ppgtt);
> +
> + err = mutex_lock_interruptible(&file_priv->vm_lock);
> + if (err)
> + goto err_put;
> +
> + err = idr_alloc(&file_priv->vm_idr, ppgtt, 0, 0, GFP_KERNEL);
> + mutex_unlock(&file_priv->vm_lock);
> + if (err < 0)
> + goto err_put;
else if (GEM_WARN_ON(err == 0) {
err = -EINVAL;
goto err_put;
}
?
Or a GEM_BUG_ON(err == 0) if you must. :)
> +
> + ppgtt->user_handle = err;
> + args->id = err;
> + return 0;
> +
> +err_put:
> + i915_ppgtt_put(ppgtt);
> + return err;
> +}
> +
> +int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void *data,
Can we cheat and declare data as u32 * here and so avoid need for the
local, since there are only two dereferences?
> + struct drm_file *file)
> +{
> + struct drm_i915_file_private *file_priv = file->driver_priv;
> + struct i915_hw_ppgtt *ppgtt;
> + int err;
> + u32 id;
> +
> + id = *(u32 *)data;
> + if (!id)
> + return -ENOENT;
> +
> + err = mutex_lock_interruptible(&file_priv->vm_lock);
> + if (err)
> + return err;
> +
> + ppgtt = idr_remove(&file_priv->vm_idr, id);
> + if (ppgtt)
GEM_WARN_ON(id != ppgtt->user_handle) too much paranoia?
> + ppgtt->user_handle = 0;
> +
> + mutex_unlock(&file_priv->vm_lock);
> + if (!ppgtt)
> + return -ENOENT;
> +
> + i915_ppgtt_put(ppgtt);
> + return 0;
Or end with simply:
i915_ppgtt_put(ppgtt);
return ppgtt ? 0 : -ENOENT;
?
> }
>
> static struct i915_request *
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 1e41a97b8007..9786f86b659d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -348,6 +348,11 @@ void i915_gem_context_release(struct kref *ctx_ref);
> struct i915_gem_context *
> i915_gem_context_create_gvt(struct drm_device *dev);
>
> +int i915_gem_vm_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
> +int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
> +
> int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 03ade71b8d9a..61698609a62c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -396,6 +396,8 @@ struct i915_hw_ppgtt {
> struct i915_page_directory_pointer pdp; /* GEN8+ */
> struct i915_page_directory pd; /* GEN6-7 */
> };
> +
> + u32 user_handle;
> };
>
> struct gen6_hw_ppgtt {
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 6ee2221838da..c3336c931a95 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -339,6 +339,8 @@ typedef struct _drm_i915_sarea {
> #define DRM_I915_PERF_ADD_CONFIG 0x37
> #define DRM_I915_PERF_REMOVE_CONFIG 0x38
> #define DRM_I915_QUERY 0x39
> +#define DRM_I915_GEM_VM_CREATE 0x3a
> +#define DRM_I915_GEM_VM_DESTROY 0x3b
>
> #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -397,6 +399,8 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
> #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
> #define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
> +#define DRM_IOCTL_I915_GEM_VM_CREATE DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_create)
> +#define DRM_IOCTL_I915_GEM_VM_DESTROY DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_VM_DESTROY, uint32_t)
>
> /* Allow drivers to submit batchbuffers directly to hardware, relying
> * on the security mechanisms provided by hardware.
> @@ -1442,6 +1446,12 @@ struct drm_i915_gem_context_destroy {
> __u32 pad;
> };
Some kernel doc overview of create and destroy usage, especially since
destroy does not have it's own data structure.
> +struct drm_i915_gem_vm_create {
> + __u64 extensions;
> + __u32 flags;
> + __u32 id; /* output: id of new vm */
> +};
> +
> struct drm_i915_reg_read {
> /*
> * Register offset.
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list