From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [RFCv2 PATCH 3/9] v4l: vb2: Add dma-contig allocator as dma_buf user Date: Wed, 28 Mar 2012 17:50:18 +0200 Message-ID: <1882491.JVmAQqEWZC@avalon> References: <1331633827-503-1-git-send-email-t.stanislaws@samsung.com> <2700146.cfupX1bb0I@avalon> <4F7090E5.2000707@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 67C279F30B for ; Wed, 28 Mar 2012 12:07:50 -0700 (PDT) In-Reply-To: <4F7090E5.2000707@samsung.com> 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 To: Tomasz Stanislawski Cc: ben@bwidawsk.net, daniel.vetter@ffwll.ch, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, kyungmin.park@samsung.com, sakari.ailus@iki.fi, airlied@redhat.com, Sumit Semwal , m.szyprowski@samsung.com List-Id: dri-devel@lists.freedesktop.org Hi Tomasz, On Monday 26 March 2012 17:53:09 Tomasz Stanislawski wrote: > On 03/22/2012 12:04 PM, Laurent Pinchart wrote: > > On Tuesday 13 March 2012 11:17:01 Tomasz Stanislawski wrote: [snip] > >> +static void vb2_dc_detach_dmabuf(void *mem_priv) > >> +{ > >> + struct vb2_dc_buf *buf = mem_priv; > >> + > >> + if (buf->dma_addr) > >> + vb2_dc_unmap_dmabuf(buf); > > > > Can detach() be called with the buffer still mapped() ? Wouldn't that be a > > bug in the caller ? > > No, it is not. The functions from vb2_dc_*_dmabuf are called by vb2-core. > It is not a part of DMABUF API itself. Therefore usage can be a bit > different. Please refer to the function __qbuf_dmabuf function vb2-core. If > new DMABUF is passed to in the QBUF then old DMABUF is released by calling > detach_dmabuf without unmap. However, this part of code is probably not > reachable if the buffer was not dequeued. > > Detach without unmap may happen in a case of closing a video fd > without prior dequeuing of all buffers. > > Do you think it would be a good idea to move a call map_dmabuf to > __enqueue_in_driver and add calling unmap_dmabuf to vb2_buffer_done? That's hard to tell. From the DRM point of view, I expect that you will be asked to keep the buffer mapped for as little time as possible. This is a sane behaviour, but will have a performance impact as we will have to constantly map/unmap buffers. From a V4L2 point of view I would prefer keeping the mappins when possible. This topic has already been discussed, and we haven't reached any consensus yet as far as I know :-/ > >> + > >> + /* detach this attachment */ > >> + dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach); > >> + kfree(buf); > >> +} > > > > There's nothing allocator-specific in the attach/detach operations. > > Shouldn't they be moved to the vb2 core ? > > Calling dma_buf_attach could be moved to vb2-core. But it gives little gain. > First, the pointer of dma_buf_attachment would have to be added to struct > vb2_plane. Second, the allocator would have to keep in the copy of this > pointer in its context structure for use of vb2_dc_(un)map_dmabuf > functions. Right. Would it make sense to pass the vb2_plane pointer, or possibly the dma_buf_attachment pointer, to the mmap_dmabuf and unmap_dmabuf operations ? > Third, dma_buf_attach requires a pointer to 'struct device' which is not > available in the vb2-core layer. OK, that's a problem. > Because of the mentioned reasons I decided to keep attach_dmabuf in > allocator-specific code. Maybe it would make sense to create a vb2_mem_buf structure from which vb2_dc_buf (and other allocator-specific buffer descriptors) would inherit ? That structure would store the dma_buf_attach pointer, and common dma-buf code could be put in videobuf2-memops.c and shared between allocators. Just an idea. -- Regards, Laurent Pinchart