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@xxxxxxxxxxx>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> >> ---
> >>
> >> 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;
> >> +}
--
Regards,
Laurent Pinchart
--
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
