Re: [RFCv2 PATCH 3/9] v4l: vb2: Add dma-contig allocator as dma_buf user

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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