[PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
Hi Laurent, On 08/15/2012 10:04 PM, Laurent Pinchart wrote: > Hi Tomasz, > > Thanks for the patch. > > On Tuesday 14 August 2012 17:34:51 Tomasz Stanislawski wrote: >> This patch adds taking reference to the device for MMAP buffers. >> >> Such buffers, may be exported using DMABUF mechanism. If the driver that >> created a queue is unloaded then the queue is released, the device might be >> released too. However, buffers cannot be released if they are referenced by >> DMABUF descriptor(s). The device pointer kept in a buffer must be valid for >> the whole buffer's lifetime. Therefore MMAP buffers should take a reference >> to the device to avoid risk of dangling pointers. > > That patch looks good, but that approach won't scale if we ever need a more > complex context that can't be copied. We can ignore that for now, but if we > decide to support "orphan" vb2 buffers, that's an issue we need to be aware > of > as it might come back to bite us in the future. > Good news is that vb2-dma-contig knows the structure of the context. Therefore it will always be able to implement a proper 'copy constructor'. One could always add reference counting for contexts but I consider it little over-engineering. > I assume that this patch requires your dmabuf owner patch to fix the orphan > buffer case properly. > The 'dmabuf owner' patch is not required for this patchset. It will only protect 'videobuf2-dma-contig' module from being unloaded while its dmabuf is in use. Regards, Tomasz Stanislawski
Re: [PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
Hi Laurent, On 08/15/2012 10:04 PM, Laurent Pinchart wrote: Hi Tomasz, Thanks for the patch. On Tuesday 14 August 2012 17:34:51 Tomasz Stanislawski wrote: This patch adds taking reference to the device for MMAP buffers. Such buffers, may be exported using DMABUF mechanism. If the driver that created a queue is unloaded then the queue is released, the device might be released too. However, buffers cannot be released if they are referenced by DMABUF descriptor(s). The device pointer kept in a buffer must be valid for the whole buffer's lifetime. Therefore MMAP buffers should take a reference to the device to avoid risk of dangling pointers. That patch looks good, but that approach won't scale if we ever need a more complex context that can't be copied. We can ignore that for now, but if we decide to support orphan vb2 buffers, that's an issue we need to be aware of as it might come back to bite us in the future. Good news is that vb2-dma-contig knows the structure of the context. Therefore it will always be able to implement a proper 'copy constructor'. One could always add reference counting for contexts but I consider it little over-engineering. I assume that this patch requires your dmabuf owner patch to fix the orphan buffer case properly. The 'dmabuf owner' patch is not required for this patchset. It will only protect 'videobuf2-dma-contig' module from being unloaded while its dmabuf is in use. Regards, Tomasz Stanislawski ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
Hi Tomasz,
Thanks for the patch.
On Tuesday 14 August 2012 17:34:51 Tomasz Stanislawski wrote:
> This patch adds taking reference to the device for MMAP buffers.
>
> Such buffers, may be exported using DMABUF mechanism. If the driver that
> created a queue is unloaded then the queue is released, the device might be
> released too. However, buffers cannot be released if they are referenced by
> DMABUF descriptor(s). The device pointer kept in a buffer must be valid for
> the whole buffer's lifetime. Therefore MMAP buffers should take a reference
> to the device to avoid risk of dangling pointers.
That patch looks good, but that approach won't scale if we ever need a more
complex context that can't be copied. We can ignore that for now, but if we
decide to support "orphan" vb2 buffers, that's an issue we need to be aware of
as it might come back to bite us in the future.
I assume that this patch requires your dmabuf owner patch to fix the orphan
buffer case properly.
> Signed-off-by: Tomasz Stanislawski
> ---
> drivers/media/video/videobuf2-dma-contig.c |5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index bb2b4ac8..d44766e 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c
> @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv)
> kfree(buf->sgt_base);
> }
> dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> + put_device(buf->dev);
> kfree(buf);
> }
>
> @@ -161,9 +162,13 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) if (!buf)
> return ERR_PTR(-ENOMEM);
>
> + /* prevent the device from release while the buffer is exported */
> + get_device(dev);
> +
> buf->vaddr = dma_alloc_coherent(dev, size, >dma_addr, GFP_KERNEL);
> if (!buf->vaddr) {
> dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
> + put_device(dev);
> kfree(buf);
> return ERR_PTR(-ENOMEM);
> }
--
Regards,
Laurent Pinchart
Re: [PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
Hi Tomasz,
Thanks for the patch.
On Tuesday 14 August 2012 17:34:51 Tomasz Stanislawski wrote:
This patch adds taking reference to the device for MMAP buffers.
Such buffers, may be exported using DMABUF mechanism. If the driver that
created a queue is unloaded then the queue is released, the device might be
released too. However, buffers cannot be released if they are referenced by
DMABUF descriptor(s). The device pointer kept in a buffer must be valid for
the whole buffer's lifetime. Therefore MMAP buffers should take a reference
to the device to avoid risk of dangling pointers.
That patch looks good, but that approach won't scale if we ever need a more
complex context that can't be copied. We can ignore that for now, but if we
decide to support orphan vb2 buffers, that's an issue we need to be aware of
as it might come back to bite us in the future.
I assume that this patch requires your dmabuf owner patch to fix the orphan
buffer case properly.
Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
---
drivers/media/video/videobuf2-dma-contig.c |5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/media/video/videobuf2-dma-contig.c
b/drivers/media/video/videobuf2-dma-contig.c index bb2b4ac8..d44766e 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv)
kfree(buf-sgt_base);
}
dma_free_coherent(buf-dev, buf-size, buf-vaddr, buf-dma_addr);
+ put_device(buf-dev);
kfree(buf);
}
@@ -161,9 +162,13 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
long size) if (!buf)
return ERR_PTR(-ENOMEM);
+ /* prevent the device from release while the buffer is exported */
+ get_device(dev);
+
buf-vaddr = dma_alloc_coherent(dev, size, buf-dma_addr, GFP_KERNEL);
if (!buf-vaddr) {
dev_err(dev, dma_alloc_coherent of size %ld failed\n, size);
+ put_device(dev);
kfree(buf);
return ERR_PTR(-ENOMEM);
}
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
This patch adds taking reference to the device for MMAP buffers.
Such buffers, may be exported using DMABUF mechanism. If the driver that
created a queue is unloaded then the queue is released, the device might be
released too. However, buffers cannot be released if they are referenced by
DMABUF descriptor(s). The device pointer kept in a buffer must be valid for the
whole buffer's lifetime. Therefore MMAP buffers should take a reference to the
device to avoid risk of dangling pointers.
Signed-off-by: Tomasz Stanislawski
---
drivers/media/video/videobuf2-dma-contig.c |5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/media/video/videobuf2-dma-contig.c
b/drivers/media/video/videobuf2-dma-contig.c
index bb2b4ac8..d44766e 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv)
kfree(buf->sgt_base);
}
dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
+ put_device(buf->dev);
kfree(buf);
}
@@ -161,9 +162,13 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
size)
if (!buf)
return ERR_PTR(-ENOMEM);
+ /* prevent the device from release while the buffer is exported */
+ get_device(dev);
+
buf->vaddr = dma_alloc_coherent(dev, size, >dma_addr, GFP_KERNEL);
if (!buf->vaddr) {
dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
+ put_device(dev);
kfree(buf);
return ERR_PTR(-ENOMEM);
}
--
1.7.9.5
[PATCHv8 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context
This patch adds taking reference to the device for MMAP buffers.
Such buffers, may be exported using DMABUF mechanism. If the driver that
created a queue is unloaded then the queue is released, the device might be
released too. However, buffers cannot be released if they are referenced by
DMABUF descriptor(s). The device pointer kept in a buffer must be valid for the
whole buffer's lifetime. Therefore MMAP buffers should take a reference to the
device to avoid risk of dangling pointers.
Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
---
drivers/media/video/videobuf2-dma-contig.c |5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/media/video/videobuf2-dma-contig.c
b/drivers/media/video/videobuf2-dma-contig.c
index bb2b4ac8..d44766e 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv)
kfree(buf-sgt_base);
}
dma_free_coherent(buf-dev, buf-size, buf-vaddr, buf-dma_addr);
+ put_device(buf-dev);
kfree(buf);
}
@@ -161,9 +162,13 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
size)
if (!buf)
return ERR_PTR(-ENOMEM);
+ /* prevent the device from release while the buffer is exported */
+ get_device(dev);
+
buf-vaddr = dma_alloc_coherent(dev, size, buf-dma_addr, GFP_KERNEL);
if (!buf-vaddr) {
dev_err(dev, dma_alloc_coherent of size %ld failed\n, size);
+ put_device(dev);
kfree(buf);
return ERR_PTR(-ENOMEM);
}
--
1.7.9.5
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
