[RFC PATCH] drm/gem: Warn on illegal use of the dumb buffer interface
Thomas Hellstrom
thellstrom at vmware.com
Thu Nov 13 11:31:39 PST 2014
Hi!
Could we perhaps get an ack from Radeon / Nouveau as well?
Thanks,
Thomas
On 11/12/2014 12:55 PM, Thomas Hellstrom wrote:
> It happens on occasion that developers of generic user-space applications
> abuse the dumb buffer API to get hold of drm buffers that they can both
> mmap() and use for GPU acceleration, using the assumptions that dumb buffers
> and buffers available for GPU are
> a) The same type and can be aribtrarily type-casted.
> b) fully coherent.
>
> This patch makes the most widely used drivers warn nicely when that happens,
> the next step will be to fail.
>
> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
> ---
> Patch is only compile-tested.
> FWIW vmware should typically fail on these errors.
>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 5 +++--
> drivers/gpu/drm/i915/i915_gem.c | 28 +++++++++++++++++++++++-----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++
> drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++++++++
> drivers/gpu/drm/nouveau/nouveau_gem.c | 3 +++
> drivers/gpu/drm/radeon/radeon_gem.c | 29 ++++++++++++++++++++++++++++-
> drivers/gpu/drm/radeon/radeon_object.c | 3 +++
> include/drm/drmP.h | 7 +++++++
> 9 files changed, 80 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e27cdbe..956b154 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1593,7 +1593,7 @@ static struct drm_driver driver = {
> .gem_prime_import = i915_gem_prime_import,
>
> .dumb_create = i915_gem_dumb_create,
> - .dumb_map_offset = i915_gem_mmap_gtt,
> + .dumb_map_offset = i915_gem_dumb_map_offset,
> .dumb_destroy = drm_gem_dumb_destroy,
> .ioctls = i915_ioctls,
> .fops = &i915_driver_fops,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a830ea..669537c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2321,8 +2321,9 @@ void i915_vma_move_to_active(struct i915_vma *vma,
> int i915_gem_dumb_create(struct drm_file *file_priv,
> struct drm_device *dev,
> struct drm_mode_create_dumb *args);
> -int i915_gem_mmap_gtt(struct drm_file *file_priv, struct drm_device *dev,
> - uint32_t handle, uint64_t *offset);
> +int i915_gem_dumb_map_offset(struct drm_file *file_priv,
> + struct drm_device *dev, uint32_t handle,
> + uint64_t *offset);
> /**
> * Returns true if seq1 is later than seq2.
> */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ba7f5c6..3d97a43 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -347,6 +347,7 @@ static int
> i915_gem_create(struct drm_file *file,
> struct drm_device *dev,
> uint64_t size,
> + bool dumb,
> uint32_t *handle_p)
> {
> struct drm_i915_gem_object *obj;
> @@ -362,6 +363,7 @@ i915_gem_create(struct drm_file *file,
> if (obj == NULL)
> return -ENOMEM;
>
> + obj->base.dumb = dumb;
> ret = drm_gem_handle_create(file, &obj->base, &handle);
> /* drop reference from allocate - handle holds it now */
> drm_gem_object_unreference_unlocked(&obj->base);
> @@ -381,7 +383,7 @@ i915_gem_dumb_create(struct drm_file *file,
> args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 64);
> args->size = args->pitch * args->height;
> return i915_gem_create(file, dev,
> - args->size, &args->handle);
> + args->size, true, &args->handle);
> }
>
> /**
> @@ -394,7 +396,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
> struct drm_i915_gem_create *args = data;
>
> return i915_gem_create(file, dev,
> - args->size, &args->handle);
> + args->size, false, &args->handle);
> }
>
> static inline int
> @@ -1750,10 +1752,10 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
> drm_gem_free_mmap_offset(&obj->base);
> }
>
> -int
> +static int
> i915_gem_mmap_gtt(struct drm_file *file,
> struct drm_device *dev,
> - uint32_t handle,
> + uint32_t handle, bool dumb,
> uint64_t *offset)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1770,6 +1772,13 @@ i915_gem_mmap_gtt(struct drm_file *file,
> goto unlock;
> }
>
> + /*
> + * We don't allow dumb mmaps on objects created using another
> + * interface.
> + */
> + WARN_ONCE(dumb && !(obj->base.dumb || obj->base.import_attach),
> + "Illegal dumb map of accelerated buffer.\n");
> +
> if (obj->base.size > dev_priv->gtt.mappable_end) {
> ret = -E2BIG;
> goto out;
> @@ -1794,6 +1803,15 @@ unlock:
> return ret;
> }
>
> +int
> +i915_gem_dumb_map_offset(struct drm_file *file,
> + struct drm_device *dev,
> + uint32_t handle,
> + uint64_t *offset)
> +{
> + return i915_gem_mmap_gtt(file, dev, handle, true, offset);
> +}
> +
> /**
> * i915_gem_mmap_gtt_ioctl - prepare an object for GTT mmap'ing
> * @dev: DRM device
> @@ -1815,7 +1833,7 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_i915_gem_mmap_gtt *args = data;
>
> - return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
> + return i915_gem_mmap_gtt(file, dev, args->handle, false, &args->offset);
> }
>
> static inline int
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 60998fc..ba78ea0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -120,6 +120,9 @@ eb_lookup_vmas(struct eb_vmas *eb,
> ret = -EINVAL;
> goto err;
> }
> +
> + WARN_ONCE(obj->base.dumb,
> + "GPU use of dumb buffer is illegal.\n");
>
> drm_gem_object_reference(&obj->base);
> list_add_tail(&obj->obj_exec_link, &objects);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 65b4fd5..63f746a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -867,6 +867,7 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> if (ret)
> return ret;
>
> + bo->gem.dumb = true;
> ret = drm_gem_handle_create(file_priv, &bo->gem, &args->handle);
> drm_gem_object_unreference_unlocked(&bo->gem);
> return ret;
> @@ -882,6 +883,14 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv,
> gem = drm_gem_object_lookup(dev, file_priv, handle);
> if (gem) {
> struct nouveau_bo *bo = nouveau_gem_object(gem);
> +
> + /*
> + * We don't allow dumb mmaps on objects created using another
> + * interface.
> + */
> + WARN_ONCE(!(gem->dumb || gem->import_attach),
> + "Illegal dumb map of accelerated buffer.\n");
> +
> *poffset = drm_vma_node_offset_addr(&bo->bo.vma_node);
> drm_gem_object_unreference_unlocked(gem);
> return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 292a677..92ba48e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -459,6 +459,9 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli,
> list_for_each_entry(nvbo, list, entry) {
> struct drm_nouveau_gem_pushbuf_bo *b = &pbbo[nvbo->pbbo_index];
>
> + WARN_ONCE(nvbo->gem.dumb,
> + "GPU use of dumb buffer is illegal.\n");
> +
> ret = nouveau_gem_set_domain(&nvbo->gem, b->read_domains,
> b->write_domains,
> b->valid_domains);
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index bfd7e1b..cbfba17 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -303,6 +303,24 @@ int radeon_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> return r;
> }
>
> +static int radeon_mode_mmap(struct drm_file *filp,
> + struct drm_device *dev,
> + uint32_t handle, uint64_t *offset_p)
> +{
> + struct drm_gem_object *gobj;
> + struct radeon_bo *robj;
> +
> + gobj = drm_gem_object_lookup(dev, filp, handle);
> + if (gobj == NULL) {
> + return -ENOENT;
> + }
> +
> + robj = gem_to_radeon_bo(gobj);
> + *offset_p = radeon_bo_mmap_offset(robj);
> + drm_gem_object_unreference_unlocked(gobj);
> + return 0;
> +}
> +
> int radeon_mode_dumb_mmap(struct drm_file *filp,
> struct drm_device *dev,
> uint32_t handle, uint64_t *offset_p)
> @@ -314,6 +332,14 @@ int radeon_mode_dumb_mmap(struct drm_file *filp,
> if (gobj == NULL) {
> return -ENOENT;
> }
> +
> + /*
> + * We don't allow dumb mmaps on objects created using another
> + * interface.
> + */
> + WARN_ONCE(!(gobj->dumb || gobj->import_attach),
> + "Illegal dumb map of GPU buffer.\n");
> +
> robj = gem_to_radeon_bo(gobj);
> *offset_p = radeon_bo_mmap_offset(robj);
> drm_gem_object_unreference_unlocked(gobj);
> @@ -325,7 +351,7 @@ int radeon_gem_mmap_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_radeon_gem_mmap *args = data;
>
> - return radeon_mode_dumb_mmap(filp, dev, args->handle, &args->addr_ptr);
> + return radeon_mode_mmap(filp, dev, args->handle, &args->addr_ptr);
> }
>
> int radeon_gem_busy_ioctl(struct drm_device *dev, void *data,
> @@ -575,6 +601,7 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
> return -ENOMEM;
>
> r = drm_gem_handle_create(file_priv, gobj, &handle);
> + gobj->dumb = true;
> /* drop reference from allocate - handle holds it now */
> drm_gem_object_unreference_unlocked(gobj);
> if (r) {
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 480c87d..1450ea0 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -471,6 +471,9 @@ int radeon_bo_list_validate(struct radeon_device *rdev,
> u32 current_domain =
> radeon_mem_type_to_domain(bo->tbo.mem.mem_type);
>
> + WARN_ONCE(bo->gem_base.dumb,
> + "GPU use of dumb buffer is illegal.\n");
> +
> /* Check if this buffer will be moved and don't move it
> * if we have moved too many buffers for this IB already.
> *
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 1968907..3d7908e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -640,6 +640,13 @@ struct drm_gem_object {
> * simply leave it as NULL.
> */
> struct dma_buf_attachment *import_attach;
> +
> + /**
> + * dumb - created as dumb buffer
> + * Whether the gem object was created using the dumb buffer interface
> + * as such it may not be used for GPU rendering.
> + */
> + bool dumb;
> };
>
> #include <drm/drm_crtc.h>
More information about the dri-devel
mailing list