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 16:43:02 +0100 Message-ID: <4F4BA486.2030400@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 106B39E988 for ; Mon, 27 Feb 2012 07:43:17 -0800 (PST) Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1S22jL-0005WU-Tk for dri-devel@lists.freedesktop.org; Mon, 27 Feb 2012 16:43:15 +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 16:43:15 +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 16:43:15 +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: arnd@arndb.de, Daniel Vetter , dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, =?UTF-8?B?J+uwleqyveuvvCc=?= , Rob Clark , Dave Airlie , Marek Szyprowski List-Id: dri-devel@lists.freedesktop.org Hi Ben and Sumit, 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] > +struct sg_table *vgem_gem_map_dma_buf(struct dma_buf_attachment *attachment, > + enum dma_data_direction dir) > +{ > + struct drm_vgem_gem_object *obj = attachment->dmabuf->priv; > + struct sg_table *sg; > + int ret; > + > + ret = vgem_gem_get_pages(obj); > + if (ret) { > + vgem_gem_put_pages(obj); is it safe to call vgem_gem_put_pages if vgem_gem_get_pages failed (I mean ret < 0 was returned) ? > + return NULL; > + } > + > + BUG_ON(obj->pages == NULL); > + > + sg = drm_prime_pages_to_sg(obj->pages, obj->base.size / PAGE_SIZE); if drm_prime_pages_to_sg fails then you should call vgem_gem_put_pages for cleanup. > + return sg; > +} The documentation of DMABUF says fallowing words about map_dma_buf callback. "It is one of the buffer operations that must be implemented by the exporter. It should return the sg_table containing scatterlist for this buffer, mapped into caller's address space." The scatterlist returned by drm_prime_pages_to_sg is not mapped to any device. The map_dma_buf callback should call dma_map_sg for caller device, which is attachment->dev. Otherwise the implementation is not consistent with the DMABUF spec. I think that it should be chosen to change implementation in map_dma_buf in DRM drivers or to drop mentioned sentence from the DMABUF spec. I bear to the second solution because IMO there is no reliable way of mapping a scatterlist to importer device by an exporter. The dma_map_sg could be used but not all drivers makes use of DMA framework. Sumit, what is your opinion about it? > + > +void vgem_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > + struct sg_table *sg) > +{ > + sg_free_table(sg); > + kfree(sg); > +} > + > +void vgem_gem_release(struct dma_buf *buf) > +{ > + struct drm_vgem_gem_object *obj = buf->priv; > + > + BUG_ON(buf != obj->base.export_dma_buf); > + > + obj->base.prime_fd = -1; > + obj->base.export_dma_buf = NULL; > + drm_gem_object_unreference_unlocked(&obj->base); > +} > + > +struct dma_buf_ops vgem_dmabuf_ops = { > + .map_dma_buf = vgem_gem_map_dma_buf, > + .unmap_dma_buf = vgem_gem_unmap_dma_buf, > + .release = vgem_gem_release > +}; > + Regards, Tomasz Stanislawski