Hi Ben,
Please refer to comments below.
On 02/22/2012 08:29 PM, Ben Widawsky wrote:
> dma-buf export implementation. Heavily influenced by Dave Airlie's proof
> of concept work.
>
> Cc: Daniel Vetter<daniel.vetter at ffwll.ch>
> Cc: Dave Airlie<airlied at redhat.com>
> Signed-off-by: Ben Widawsky<ben at bwidawsk.net>
> ---
> drivers/gpu/drm/vgem/Makefile | 2 +-
> drivers/gpu/drm/vgem/vgem_dma_buf.c | 128
> +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/vgem/vgem_drv.c | 6 ++
> drivers/gpu/drm/vgem/vgem_drv.h | 7 ++
> 4 files changed, 142 insertions(+), 1 deletions(-)
> create mode 100644 drivers/gpu/drm/vgem/vgem_dma_buf.c
>
[snip]
> +
> +int vgem_prime_to_fd(struct drm_device *dev, struct drm_file *file,
> + uint32_t handle, int *prime_fd)
> +{
> + struct drm_vgem_file_private *file_priv = file->driver_priv;
> + struct drm_vgem_gem_object *obj;
> + int ret;
> +
> + DRM_DEBUG_PRIME("Request fd for handle %d\n", handle);
> +
> + obj = to_vgem_bo(drm_gem_object_lookup(dev, file, handle));
> + if (!obj)
> + return -EBADF;
> +
> + /* This means a user has already called get_fd on this */
> + if (obj->base.prime_fd != -1) {
> + DRM_DEBUG_PRIME("User requested a previously exported buffer "
> + "%d %d\n", handle, obj->base.prime_fd);
> + drm_gem_object_unreference(&obj->base);
> + goto out_fd;
IMO, it is not safe to cache a number of file descriptor. This number
may unexpectedly become invalid introducing a bug. Please refer to the
scenario:
- application possess a GEM buffer called A
- call DRM_IOCTL_PRIME_HANDLE_TO_FD on A to obtain a file descriptor fd_A
- application calls fd_B = dup(fd_A). Now both fd_A and fd_B point to
the same DMABUF instance.
- application calls close(fd_A), now fd_A is no longer a valid file
descriptor
- application tries to export the buffer again and calls ioctl
DRM_IOCTL_PRIME_HANDLE_TO_FD on GEM buffer. The field obj->base.prime_fd
is already set to fd_A so value fd_A is returned to userspace. Notice
that this is a bug because fd_A is no longer valid.
I think that field prime_fd should be removed because it is too
unreliable. The driver should call dma_buf_fd every time when the buffer
is exported.
> + }
> +
> + /* Make a dma buf out of our vgem object */
> + obj->base.export_dma_buf = dma_buf_export(obj,&vgem_dmabuf_ops,
> + obj->base.size,
> + VGEM_FD_PERMS);
> + if (IS_ERR(obj->base.export_dma_buf)) {
> + DRM_DEBUG_PRIME("export fail\n");
> + return PTR_ERR(obj->base.export_dma_buf);
> + } else
> + obj->base.prime_fd = dma_buf_fd(obj->base.export_dma_buf);
> +
> + mutex_lock(&dev->prime_mutex);
> + ret = drm_prime_insert_fd_handle_mapping(&file_priv->prime,
> + obj->base.export_dma_buf,
> + handle);
> + WARN_ON(ret);
> + ret = drm_prime_add_dma_buf(dev,&obj->base);
> + mutex_unlock(&dev->prime_mutex);
> + if (ret)
> + return ret;
> +
> +out_fd:
> + *prime_fd = obj->base.prime_fd;
> +
> + return 0;
> +}
> +
Regards,
Tomasz Stanislawski
