Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

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

Hi Tomasz,

Sorry for the late reply, this one slipped through the cracks.

On Thursday 19 April 2012 12:42:12 Tomasz Stanislawski wrote:
> On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
> > 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?

I think I would prefer that. It sounds more logical to allocate the attachment 
in the attach operation handler.

> >> +	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.

OK.

> 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?

Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is 
set to a non-NULL value, and the application has an open file handle for the 
buffer. The application then tries to export the buffer a second time. 
vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's 
non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the 
struct dma_buf, the application closes the file handle to the exported buffer. 
The struct dma_buf object gets freed, as the reference count drops to 0. The 
vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been 
freed.

> >> +	/* 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;
> >> +}

-- 
Regards,

Laurent Pinchart