Hi Laurent, Thank you for your review. Please refer to the comments below.
On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
This patch adds support for exporting a dma-contig buffer using DMABUF interface.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
[snip]
+static struct sg_table *vb2_dc_dmabuf_ops_map(
- struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
- struct dma_buf *dbuf = db_attach->dmabuf;
- struct vb2_dc_buf *buf = dbuf->priv;
- struct vb2_dc_attachment *attach = db_attach->priv;
- struct sg_table *sgt;
- struct scatterlist *rd, *wr;
- int i, ret;
You can make i an unsigned int :-)
Right.. splitting declaration may be also a good idea :)
- /* return previously mapped sg table */
- if (attach)
return &attach->sgt;This effectively keeps the mapping around as long as the attachment exists. We don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it might not be too much of an issue, but the behaviour of the implementation will change if we later decide to map/unmap the buffers in the map/unmap handlers. Do you think that could be a problem ?
I don't that it is a problem. If an importer calls dma_map_sg then caching sgt on an exporter side reduces a cost of an allocating and an initialization of sgt.
- attach = kzalloc(sizeof *attach, GFP_KERNEL);
- if (!attach)
return ERR_PTR(-ENOMEM);Why don't you allocate the vb2_dc_attachment here instead of vb2_dc_dmabuf_ops_attach() ?
Good point. The attachment could be allocated at vb2_dc_attachment but all its fields would be uninitialized. I mean an empty sgt and an undefined dma direction. I decided to allocate the attachment in vb2_dc_dmabuf_ops_map because only than all information needed to create a valid attachment object are available.
The other solution might be the allocation at vb2_dc_attachment. The field dir would be set to DMA_NONE. If this filed is equal to DMA_NONE at vb2_dc_dmabuf_ops_map then sgt is allocated and mapped and direction field is updated. If value is not DMA_NONE then the sgt is reused.
Do you think that it is a good idea?
- sgt = &attach->sgt;
- attach->dir = dir;
- /* copying the buf->base_sgt to attachment */
I would add an explanation regarding why you need to copy the SG list. Something like.
"Copy the buf->base_sgt scatter list to the attachment, as we can't map the same scatter list to multiple devices at the same time."
ok
ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
if (ret) {
kfree(attach);return ERR_PTR(-ENOMEM);}
rd = buf->sgt_base->sgl;
wr = sgt->sgl;
for (i = 0; i < sgt->orig_nents; ++i) {
sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}
/* mapping new sglist to the client */
ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
if (ret <= 0) {
printk(KERN_ERR "failed to map scatterlist\n");sg_free_table(sgt);kfree(attach);return ERR_PTR(-EIO);}
db_attach->priv = attach;
return sgt;
+}
+static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
- struct sg_table *sgt, enum dma_data_direction dir)
+{
- /* nothing to be done here */
+}
+static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) +{
- /* drop reference obtained in vb2_dc_get_dmabuf */
- vb2_dc_put(dbuf->priv);
Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
No.
The buffer object is destroyed at vb2_dc_put when reference count drops to 0. It happens could happen after only REQBUF(count=0) or on last close(). The DMABUF object is created only for MMAP buffers. The DMABUF object is based only on results of dma_alloc_coherent and dma_get_pages (or its future equivalent). Therefore the DMABUF object is valid as long as the buffer is valid.
Notice that dmabuf object could be created in vb2_dc_alloc. I moved it to vb2_dc_get_dmabuf to avoid a creation of an object that may not be used.
+}
+static struct dma_buf_ops vb2_dc_dmabuf_ops = {
- .attach = vb2_dc_dmabuf_ops_attach,
- .detach = vb2_dc_dmabuf_ops_detach,
- .map_dma_buf = vb2_dc_dmabuf_ops_map,
- .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
- .release = vb2_dc_dmabuf_ops_release,
+};
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{
- struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf *dbuf;
- if (buf->dma_buf)
return buf->dma_buf;Can't there be a race condition here if the user closes the DMABUF file handle before vb2 core calls dma_buf_fd() ?
The user cannot access the file until it is associated with a file descriptor. How can the user close it? Could you give me a more detailed description of this potential race condition?
- /* dmabuf keeps reference to vb2 buffer */
- atomic_inc(&buf->refcount);
- dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
- if (IS_ERR(dbuf)) {
atomic_dec(&buf->refcount);return NULL;- }
- buf->dma_buf = dbuf;
- return dbuf;
+}
+/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/
@@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = { .alloc = vb2_dc_alloc, .put = vb2_dc_put,
- .get_dmabuf = vb2_dc_get_dmabuf, .cookie = vb2_dc_cookie, .vaddr = vb2_dc_vaddr, .mmap = vb2_dc_mmap,
Regards, Tomasz Stanislawski