Re: [RFC PATCH] drm/gem: Warn on illegal use of the dumb buffer interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 13, 2014 at 2:31 PM, Thomas Hellstrom <thellstrom@xxxxxxxxxx> 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@xxxxxxx>

>
> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux