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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
