| Message ID | 1415793332-57236-1-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 12, 2014 at 12:55:32PM +0100, 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. Yeah, makes sense to enforce a bit of sanity here. Not sure whether the armsoc folks have already terminally screwed this up though :( > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > Patch is only compile-tested. > FWIW vmware should typically fail on these errors. At first I wanted to drop a bikeshed about the new bool arguments (since they tend to make the code harder to read). But looking closer we can't remove it from create because that would cause races (by setting it afterwards) and for the mmap offset handling the code would end up rather more ugly I think. So Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the patch and i915 parts. -Daniel
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@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>
On Thu, Nov 13, 2014 at 2:31 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: > Hi! > > Could we perhaps get an ack from Radeon / Nouveau as well? This needs to be rebased on a newer kernel to take into account the userptr changes in radeon_mode_dumb_mmap() so they don't get lost. With that fixed: Acked-by: Alex Deucher <alexander.deucher@amd.com> > > 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@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> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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>
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@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(-)