From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Stanislawski Subject: Re: [PATCH 5/7] drm/vgem: prime export support Date: Mon, 27 Feb 2012 14:37:04 +0100 Message-ID: <4F4B8700.1090101@samsung.com> References: <1329938960-4903-1-git-send-email-ben@bwidawsk.net> <1329938960-4903-7-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from plane.gmane.org (plane.gmane.org [80.91.229.3]) by gabe.freedesktop.org (Postfix) with ESMTP id 8C00D9E793 for ; Mon, 27 Feb 2012 05:40:10 -0800 (PST) Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1S20o7-0001rj-JX for dri-devel@lists.freedesktop.org; Mon, 27 Feb 2012 14:40:03 +0100 Received: from 217-67-201-162.itsa.net.pl ([217.67.201.162]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 27 Feb 2012 14:40:03 +0100 Received: from t.stanislaws by 217-67-201-162.itsa.net.pl with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 27 Feb 2012 14:40:03 +0100 In-Reply-To: <1329938960-4903-7-git-send-email-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org, Daniel Vetter , dri-devel@lists.freedesktop.org, Dave Airlie List-Id: dri-devel@lists.freedesktop.org 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 > Cc: Dave Airlie > Signed-off-by: Ben Widawsky > --- > 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