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@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>> ---
[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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
