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.stanisl...@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.p...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html