Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
On Tue, 2010-11-30 at 21:57 +0100, Laurent Pinchart wrote:
Hi Marek,
On Tuesday 30 November 2010 11:39:41 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:51 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and
handling routines for videobuf2, implemented on top of
vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
Some drivers might need vmalloc_32_user instead.
Which driver might require this? I'm quite surprised.
I don't think this a driver issue, but a hardware issue.
Legacy PCI address space is 32 bits. DMA engines on Legacy PCI cards
can't access addresses larger than 2^32. A hardware IOMMU or the
SWIOTLB software IOMMU in the kernel is needed for these devices to
write to physical memory addresses above 2^32.
( http://en.wikipedia.org/wiki/IOMMU has a nice picture)
The SWIOTLB software IOMMU in the kernel uses bounce buffers with
physical addresses less than 2^32 and then uses the CPU copy them to the
final buffer location. That's a waste of memory and a performance hit
on systems without a hardware IOMMU that is worth avoiding. Also the
SWIOTLB has a bug in it, last I checked, that panics the system when one
requests too much memory.
I thought that
vmalloced memory buffers are used only by the drivers that need to copy
video data with CPU anyway.
The CX23418 has a DMA engine that handles scatter-gather lists. The
cx18 driver calls these Memory Descriptor Lists or MDLs. Right now the
driver kmalloc()'s contiguous buffers at module load, so it only uses
MDLs trivially (except for raw YUV video streams)
http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/media/video/cx18/cx18-queue.c;h=8884537bd62f9bf4685ea1a48c89f970cbb6f8b7;hb=linuxtv-v2.6.38#l347
I'd like to change that in the future to have the cx18 driver buffers
vmalloc'ed and have the CX23418 DMA engine directly fill the physical
chunks of the vmalloc'ed buffers. This is nicer on the user's system
memory, and also avoids manually reassembling the buffers pointed to by
the MDL for YUV video.
To do this, the CX23418 DMA engine needs to be given addresses less than
2^32.
The OMAP3 ISP driver uses vmalloc_32_user to allocate memory for the MMAP
method and then maps it to the device address space through the IOMMU. I'm
not
sure that's the best solution, but that's how it works now.
That seems unnecessary, since you have a hardware IOMMU. Addresses
larger than 2^32 can be remapped by the IOMMU to addresses lower than
2^32 in the address space on the IO bus.
Am I missing something?
Regards,
Andy
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
Hello,
On Monday, November 29, 2010 10:51 AM Laurent Pinchart wrote:
Hi Marek,
On Friday 19 November 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++ include/media/videobuf2-vmalloc.h |
21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 83ce858..9351423 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
config VIDEOBUF2_MEMOPS
tristate
+config VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a97a2a0..538bee9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
+obj-$(CONFIG_VIDEOBUF2_VMALLOC)+= videobuf2-vmalloc.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c new file mode 100644
index 000..3310900
--- /dev/null
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -0,0 +1,177 @@
+/*
+ * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/mm.h
+#include linux/slab.h
+#include linux/vmalloc.h
+
+#include media/videobuf2-core.h
+#include media/videobuf2-memops.h
+
+struct vb2_vmalloc_conf {
+ struct vb2_alloc_ctxalloc_ctx;
+};
+
+struct vb2_vmalloc_buf {
+ void*vaddr;
+ unsigned long size;
+ unsigned intrefcount;
+};
+
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
Some drivers might need vmalloc_32_user instead.
Which driver might require this? I'm quite surprised. I thought that vmalloced
memory buffers are used only by the drivers that need to copy video data with
CPU
anyway.
snip
+static int vb2_vmalloc_mmap(void *buf_priv, struct vm_area_struct *vma)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+ int ret;
+
+ if (!buf) {
+ printk(KERN_ERR No memory to map\n);
+ return -EINVAL;
+ }
I think you should test that the VMA size is equal to (or smaller than) the
buffer size. That could be done in the core.
Right.
+ ret = remap_vmalloc_range(vma, buf-vaddr, 0);
+ if (ret) {
+ printk(KERN_ERR Remapping vmalloc memory, error: %d\n, ret);
+ return ret;
+ }
+
+ vma-vm_flags |= VM_DONTEXPAND | VM_RESERVED;
VM_RESERVED is set by remap_vmalloc_range(). What is VM_DONTEXPAND for ?
It is used to prevent merging vm_areas for 2 consecutive buffers, because
otherwise one might cause a lot of troubles by calling mremap() function from
userspace with size larger than the size of the single buffer.
snip
Best regards
--
Marek Szyprowski
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
Hi Marek,
On Friday 19 November 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++ include/media/videobuf2-vmalloc.h |
21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 83ce858..9351423 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
config VIDEOBUF2_MEMOPS
tristate
+config VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a97a2a0..538bee9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
+obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c new file mode 100644
index 000..3310900
--- /dev/null
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -0,0 +1,177 @@
+/*
+ * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/mm.h
+#include linux/slab.h
+#include linux/vmalloc.h
+
+#include media/videobuf2-core.h
+#include media/videobuf2-memops.h
+
+struct vb2_vmalloc_conf {
+ struct vb2_alloc_ctxalloc_ctx;
+};
+
+struct vb2_vmalloc_buf {
+ void*vaddr;
+ unsigned long size;
+ unsigned intrefcount;
+};
+
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
Some drivers might need vmalloc_32_user instead.
+ if (!buf-vaddr) {
+ printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ kfree(buf);
+ return NULL;
+ }
+
+ buf-refcount++;
+ printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
+ buf-size, buf-vaddr);
+
+ return buf;
+}
+
+static void vb2_vmalloc_put(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ buf-refcount--;
+
+ if (0 == buf-refcount) {
+ printk(KERN_DEBUG %s: Freeing vmalloc mem at vaddr=%p\n,
+ __func__, buf-vaddr);
+ vfree(buf-vaddr);
+ kfree(buf);
+ }
+}
+
+static void *vb2_vmalloc_vaddr(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ BUG_ON(!buf);
+
+ if (!buf-vaddr) {
+ printk(KERN_ERR Address of an unallocated
+ plane requested\n);
Can this happen under normal circumstances, or is it a driver bug ? In the
later case a WARN_ON might be better.
+ return NULL;
+ }
+
+ return buf-vaddr;
+}
+
+static unsigned int vb2_vmalloc_num_users(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ return buf-refcount;
+}
+
+/* TODO generalize and extract to core as much as possible */
I agree with the TODO :-) This part doesn't seem very clean, and
vm_open/vm_close handling certainly needs documentation.
+static void vb2_vmalloc_vm_open(struct vm_area_struct *vma)
+{
+ struct vb2_vmalloc_buf *buf = vma-vm_private_data;
+
+ printk(KERN_DEBUG %s vmalloc_priv: %p, refcount: %d,
+ vma: %08lx-%08lx\n, __func__, buf, buf-refcount,
+ vma-vm_start, vma-vm_end);
+
+ buf-refcount++;
+}
+
+static void
Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
Hi Marek,
On Friday 19 November 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++ include/media/videobuf2-vmalloc.h |
21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
[snip]
+static const struct vb2_mem_ops vb2_vmalloc_ops = {
+ .alloc = vb2_vmalloc_alloc,
+ .put= vb2_vmalloc_put,
+ .vaddr = vb2_vmalloc_vaddr,
+ .mmap = vb2_vmalloc_mmap,
+ .num_users = vb2_vmalloc_num_users,
+};
No USERPTR support ? :-)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++
include/media/videobuf2-vmalloc.h | 21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 83ce858..9351423 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
config VIDEOBUF2_MEMOPS
tristate
+config VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a97a2a0..538bee9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
+obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c
new file mode 100644
index 000..3310900
--- /dev/null
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -0,0 +1,177 @@
+/*
+ * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/mm.h
+#include linux/slab.h
+#include linux/vmalloc.h
+
+#include media/videobuf2-core.h
+#include media/videobuf2-memops.h
+
+struct vb2_vmalloc_conf {
+ struct vb2_alloc_ctxalloc_ctx;
+};
+
+struct vb2_vmalloc_buf {
+ void*vaddr;
+ unsigned long size;
+ unsigned intrefcount;
+};
+
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
+ if (!buf-vaddr) {
+ printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ kfree(buf);
+ return NULL;
+ }
+
+ buf-refcount++;
+ printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
+ buf-size, buf-vaddr);
+
+ return buf;
+}
+
+static void vb2_vmalloc_put(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ buf-refcount--;
+
+ if (0 == buf-refcount) {
I think I would feel happier if refcount was of type atomic_t and you would use
the atomic decrease-and-test operation here. I know that this is almost
certainly
called with some lock, but still...
+ printk(KERN_DEBUG %s: Freeing vmalloc mem at vaddr=%p\n,
+ __func__, buf-vaddr);
+ vfree(buf-vaddr);
+ kfree(buf);
+ }
+}
+
+static void *vb2_vmalloc_vaddr(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ BUG_ON(!buf);
+
+ if (!buf-vaddr) {
+ printk(KERN_ERR Address of an unallocated
+ plane requested\n);
+ return NULL;
+ }
+
+ return buf-vaddr;
+}
+
+static unsigned int vb2_vmalloc_num_users(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ return buf-refcount;
+}
+
+/* TODO generalize and extract to core as much as possible */
+static void vb2_vmalloc_vm_open(struct vm_area_struct *vma)
+{
+ struct vb2_vmalloc_buf *buf = vma-vm_private_data;
+
+ printk(KERN_DEBUG %s vmalloc_priv: %p, refcount: %d,
+ vma: %08lx-%08lx\n, __func__, buf, buf-refcount,
+ vma-vm_start, vma-vm_end);
+
+ buf-refcount++;
+}
+
+static void vb2_vmalloc_vm_close(struct vm_area_struct *vma)
+{
+ struct vb2_vmalloc_buf *buf =
RE: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
Hello,
On Thursday, November 25, 2010 11:07 AM Hans Verkuil wrote:
On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++
include/media/videobuf2-vmalloc.h | 21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 83ce858..9351423 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
config VIDEOBUF2_MEMOPS
tristate
+config VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a97a2a0..538bee9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
+obj-$(CONFIG_VIDEOBUF2_VMALLOC)+= videobuf2-vmalloc.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c
new file mode 100644
index 000..3310900
--- /dev/null
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -0,0 +1,177 @@
+/*
+ * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/mm.h
+#include linux/slab.h
+#include linux/vmalloc.h
+
+#include media/videobuf2-core.h
+#include media/videobuf2-memops.h
+
+struct vb2_vmalloc_conf {
+ struct vb2_alloc_ctxalloc_ctx;
+};
+
+struct vb2_vmalloc_buf {
+ void*vaddr;
+ unsigned long size;
+ unsigned intrefcount;
+};
+
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
+ if (!buf-vaddr) {
+ printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ kfree(buf);
+ return NULL;
+ }
+
+ buf-refcount++;
+ printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
+ buf-size, buf-vaddr);
+
+ return buf;
+}
+
+static void vb2_vmalloc_put(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ buf-refcount--;
+
+ if (0 == buf-refcount) {
I think I would feel happier if refcount was of type atomic_t and you would
use
the atomic decrease-and-test operation here. I know that this is almost
certainly
called with some lock, but still...
Yes, it is called with mm_sem taken in all cases. Maybe a comment in the source
will
be enough to indicate that this variable does not need to be atomic_t type?
I can change it to atomic_t if this is really required anyway.
snip
Best regards
--
Marek Szyprowski
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator
On Thursday, November 25, 2010 11:26:56 Marek Szyprowski wrote:
Hello,
On Thursday, November 25, 2010 11:07 AM Hans Verkuil wrote:
On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add an implementation of contiguous virtual memory allocator and handling
routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig |5 +
drivers/media/video/Makefile|1 +
drivers/media/video/videobuf2-vmalloc.c | 177
+++
include/media/videobuf2-vmalloc.h | 21
4 files changed, 204 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-vmalloc.c
create mode 100644 include/media/videobuf2-vmalloc.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 83ce858..9351423 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
config VIDEOBUF2_MEMOPS
tristate
+config VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index a97a2a0..538bee9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
+obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c
new file mode 100644
index 000..3310900
--- /dev/null
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -0,0 +1,177 @@
+/*
+ * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/mm.h
+#include linux/slab.h
+#include linux/vmalloc.h
+
+#include media/videobuf2-core.h
+#include media/videobuf2-memops.h
+
+struct vb2_vmalloc_conf {
+ struct vb2_alloc_ctxalloc_ctx;
+};
+
+struct vb2_vmalloc_buf {
+ void*vaddr;
+ unsigned long size;
+ unsigned intrefcount;
+};
+
+static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
+ unsigned long size)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-size = size;
+ buf-vaddr = vmalloc_user(buf-size);
+ if (!buf-vaddr) {
+ printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ kfree(buf);
+ return NULL;
+ }
+
+ buf-refcount++;
+ printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
+ buf-size, buf-vaddr);
+
+ return buf;
+}
+
+static void vb2_vmalloc_put(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ buf-refcount--;
+
+ if (0 == buf-refcount) {
I think I would feel happier if refcount was of type atomic_t and you would
use
the atomic decrease-and-test operation here. I know that this is almost
certainly
called with some lock, but still...
Yes, it is called with mm_sem taken in all cases. Maybe a comment in the
source will
be enough to indicate that this variable does not need to be atomic_t type?
I can change it to atomic_t if this is really required anyway.
It definitely needs to be documented. But I wonder if what you say is true
anyway:
vb2_mmap is called without mm_sem taken as far as I can tell, this calls the
mmap
callback, vb2_vmalloc_mmap calls vb2_vmalloc_vm_open directly which does
buf_refcount++.
Making refcount atomic would make it easier to prove correctness IMHO.
Anyway, this leads me to a second question: the documentation (either separate
or in the
header) should document which ops need to be called with some lock set. That
way driver
writers have some guidelines regarding locking. videobuf used to lock
internally, but
that's now moved to the driver (and rightly so), but that does mean that the
driver writer
needs pointers
