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.