Hi Tomasz,
On Tuesday 21 August 2012 15:47:13 Tomasz Stanislawski wrote:
On 08/21/2012 12:03 PM, Laurent Pinchart wrote:
On Tuesday 14 August 2012 17:34:50 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
drivers/media/video/videobuf2-dma-contig.c | 204 ++++++++++++++++++++++ 1 file changed, 204 insertions(+)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 7fc71a0..bb2b4ac8 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c
[snip]
+static struct sg_table *vb2_dc_dmabuf_ops_map(
- struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
- struct vb2_dc_attachment *attach = db_attach->priv;
- /* stealing dmabuf mutex to serialize map/unmap operations */
Why isn't this operation serialized by the dma-buf core itself ?
Indeed, it is a very good question. The lock was introduced in RFCv3 of DMABUF patches. It was dedicated to serialize attach/detach calls. No requirements for map/unmap serialization were stated so serialization was delegated to an exporter.
A deadlock could occur if dma_map_attachment is called from inside of attach ops. IMO, such an operation is invalid because an attachment list is not in a valid state while attach ops is being processed.
Do you think that stealing a lock from dma-buf internals is too hacky?
No, I would be OK with that, but I'd like to make sure that it won't bite us back later. If there's a specific reason why the lock is not taken by the dmabuf core around map/unmap calls, stealing the same lock might cause unforeseen problems. That's why I would like to understand why the core doesn't perform locking on its own.
I prefer not to introduce any extra locks in dma-contig allocator
Agreed.
but it is not a big deal to add it.
- struct mutex *lock = &db_attach->dmabuf->lock;
- struct sg_table *sgt;
- int ret;
- mutex_lock(lock);
- sgt = &attach->sgt;
- /* return previously mapped sg table */
- if (attach->dir == dir) {
mutex_unlock(lock);return sgt;- }
- /* release any previous cache */
- if (attach->dir != DMA_NONE) {
dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,attach->dir);attach->dir = DMA_NONE;- }
- /* mapping to the client with new direction */
- ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
- if (ret <= 0) {
pr_err("failed to map scatterlist\n");mutex_unlock(lock);return ERR_PTR(-EIO);- }
- attach->dir = dir;
- mutex_unlock(lock);
- return sgt;
+}