Re: [PATCH v2] media: vb2: vmalloc-based allocator user pointer handling
Hi Sylwester,
On Thursday 08 December 2011 22:43:00 Sylwester Nawrocki wrote:
Hi Laurent,
On 12/08/2011 11:56 AM, Laurent Pinchart wrote:
On Wednesday 07 December 2011 17:29:06 Marek Szyprowski wrote:
From: Andrzej Pietrasiewicz andrze...@samsung.com
[...]
- printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at
vaddr=%p\n, - buf-size, buf-vaddr);
+ pr_err(Allocated vmalloc buffer of size %ld at vaddr=%p\n,
buf-size, + buf-vaddr);
Turning KERN_DEBUG into pr_err() is a bit harsh :-) In my opinion even
KERN_DEBUG is too much here, I don't want to get messages printed to the
kernel log every time I allocate buffers.
Indeed, pr_err looks like an overkill:) I think pr_debug() would be fine
here.
return buf;
}
@@ -59,13 +63,87 @@ static void vb2_vmalloc_put(void *buf_priv)
struct vb2_vmalloc_buf *buf = buf_priv;
if (atomic_dec_and_test(buf-refcount)) {
- printk(KERN_DEBUG %s: Freeing vmalloc mem at vaddr=%p\n,
- __func__, buf-vaddr);
+ pr_debug(%s: Freeing vmalloc mem at vaddr=%p\n, __func__,
+ buf-vaddr);
Same here. Should we get rid of those two messages, or at least
conditionally- compile them out of the kernel by default ?
During compilation pr_debug() will most likely be optimized away if DEBUG
and CONFIG_DYNAMIC_DEBUG isn't defined, as it is then defined as:
static inline __printf(1, 2)
int no_printk(const char *fmt, ...)
{
return 0;
}
Plus it's easy with pr_debug() to enable debug trace while dynamic printk()
is enabled in the kernel configuration.
My bad. pr_debug() is fine.
--
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 v2] media: vb2: vmalloc-based allocator user pointer handling
Hi Marek and Andrzej,
Thanks for the patch.
On Wednesday 07 December 2011 17:29:06 Marek Szyprowski wrote:
From: Andrzej Pietrasiewicz andrze...@samsung.com
This patch adds support for user pointer memory buffers to vmalloc
videobuf2 allocator.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/media/video/videobuf2-vmalloc.c | 97
--- 1 files changed, 89 insertions(+), 8
deletions(-)
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c index a3a8842..8843ad0 100644
--- a/drivers/media/video/videobuf2-vmalloc.c
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -12,6 +12,7 @@
#include linux/module.h
#include linux/mm.h
+#include linux/sched.h
#include linux/slab.h
#include linux/vmalloc.h
@@ -20,7 +21,10 @@
struct vb2_vmalloc_buf {
void*vaddr;
+ struct page **pages;
+ int write;
unsigned long size;
+ unsigned intn_pages;
atomic_trefcount;
struct vb2_vmarea_handler handler;
};
@@ -42,14 +46,14 @@ static void *vb2_vmalloc_alloc(void *alloc_ctx,
unsigned long size) buf-handler.arg = buf;
if (!buf-vaddr) {
- printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ pr_err(vmalloc of size %ld failed\n, buf-size);
kfree(buf);
return NULL;
}
atomic_inc(buf-refcount);
- printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
- buf-size, buf-vaddr);
+ pr_err(Allocated vmalloc buffer of size %ld at vaddr=%p\n, buf-size,
+buf-vaddr);
Turning KERN_DEBUG into pr_err() is a bit harsh :-) In my opinion even
KERN_DEBUG is too much here, I don't want to get messages printed to the
kernel log every time I allocate buffers.
return buf;
}
@@ -59,13 +63,87 @@ static void vb2_vmalloc_put(void *buf_priv)
struct vb2_vmalloc_buf *buf = buf_priv;
if (atomic_dec_and_test(buf-refcount)) {
- printk(KERN_DEBUG %s: Freeing vmalloc mem at vaddr=%p\n,
- __func__, buf-vaddr);
+ pr_debug(%s: Freeing vmalloc mem at vaddr=%p\n, __func__,
+ buf-vaddr);
Same here. Should we get rid of those two messages, or at least conditionally-
compile them out of the kernel by default ?
vfree(buf-vaddr);
kfree(buf);
}
}
+static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
+ unsigned long size, int write)
+{
+ struct vb2_vmalloc_buf *buf;
+
No need for a blank line.
+ unsigned long first, last;
+ int n_pages_from_user, offset;
You seem to like long names :-) I'd use n_pages, and I would also shorten the
labels below, but that's just me.
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
The kernel coding style encourages parenthesis after the sizeof operator:
sizeof(*buf).
+ if (!buf)
+ return NULL;
+
+ buf-write = write;
+ offset = vaddr ~PAGE_MASK;
+ buf-size = size;
+
+ first = (vaddr PAGE_MASK) PAGE_SHIFT;
+ last = ((vaddr + size - 1) PAGE_MASK) PAGE_SHIFT;
If you shift right anyway is there a need to PAGE_MASK first ?
+ buf-n_pages = last - first + 1;
+ buf-pages = kzalloc(buf-n_pages * sizeof(struct page *), GFP_KERNEL);
It's a common practice in the kernel to use variables instead of types when
possible with the sizeof operator: sizeof(buf-pages). That's up to you.
+ if (!buf-pages)
+ goto userptr_fail_pages_array_alloc;
+
+ /* current-mm-mmap_sem is taken by videobuf core */
+ n_pages_from_user = get_user_pages(current, current-mm,
+ vaddr PAGE_MASK,
+ buf-n_pages,
+ write,
+ 1, /* force */
+ buf-pages,
+ NULL);
+ if (n_pages_from_user != buf-n_pages)
+ goto userptr_fail_get_user_pages;
+
+ buf-vaddr = vm_map_ram(buf-pages, buf-n_pages, -1, PAGE_KERNEL);
+
No need for a blank line.
+ if (!buf-vaddr)
+ goto userptr_fail_get_user_pages;
+
+ buf-vaddr += offset;
+ return buf;
+
+userptr_fail_get_user_pages:
+ pr_debug(get_user_pages requested/got: %d/%d]\n, n_pages_from_user,
+ buf-n_pages);
+ while (--n_pages_from_user = 0)
+ put_page(buf-pages[n_pages_from_user]);
+ kfree(buf-pages);
+
+userptr_fail_pages_array_alloc:
+
[PATCH v2] media: vb2: vmalloc-based allocator user pointer handling
From: Andrzej Pietrasiewicz andrze...@samsung.com
This patch adds support for user pointer memory buffers to vmalloc
videobuf2 allocator.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/media/video/videobuf2-vmalloc.c | 97 ---
1 files changed, 89 insertions(+), 8 deletions(-)
diff --git a/drivers/media/video/videobuf2-vmalloc.c
b/drivers/media/video/videobuf2-vmalloc.c
index a3a8842..8843ad0 100644
--- a/drivers/media/video/videobuf2-vmalloc.c
+++ b/drivers/media/video/videobuf2-vmalloc.c
@@ -12,6 +12,7 @@
#include linux/module.h
#include linux/mm.h
+#include linux/sched.h
#include linux/slab.h
#include linux/vmalloc.h
@@ -20,7 +21,10 @@
struct vb2_vmalloc_buf {
void*vaddr;
+ struct page **pages;
+ int write;
unsigned long size;
+ unsigned intn_pages;
atomic_trefcount;
struct vb2_vmarea_handler handler;
};
@@ -42,14 +46,14 @@ static void *vb2_vmalloc_alloc(void *alloc_ctx, unsigned
long size)
buf-handler.arg = buf;
if (!buf-vaddr) {
- printk(KERN_ERR vmalloc of size %ld failed\n, buf-size);
+ pr_err(vmalloc of size %ld failed\n, buf-size);
kfree(buf);
return NULL;
}
atomic_inc(buf-refcount);
- printk(KERN_DEBUG Allocated vmalloc buffer of size %ld at vaddr=%p\n,
- buf-size, buf-vaddr);
+ pr_err(Allocated vmalloc buffer of size %ld at vaddr=%p\n, buf-size,
+ buf-vaddr);
return buf;
}
@@ -59,13 +63,87 @@ static void vb2_vmalloc_put(void *buf_priv)
struct vb2_vmalloc_buf *buf = buf_priv;
if (atomic_dec_and_test(buf-refcount)) {
- printk(KERN_DEBUG %s: Freeing vmalloc mem at vaddr=%p\n,
- __func__, buf-vaddr);
+ pr_debug(%s: Freeing vmalloc mem at vaddr=%p\n, __func__,
+buf-vaddr);
vfree(buf-vaddr);
kfree(buf);
}
}
+static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
+unsigned long size, int write)
+{
+ struct vb2_vmalloc_buf *buf;
+
+ unsigned long first, last;
+ int n_pages_from_user, offset;
+
+ buf = kzalloc(sizeof *buf, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ buf-write = write;
+ offset = vaddr ~PAGE_MASK;
+ buf-size = size;
+
+ first = (vaddr PAGE_MASK) PAGE_SHIFT;
+ last = ((vaddr + size - 1) PAGE_MASK) PAGE_SHIFT;
+ buf-n_pages = last - first + 1;
+ buf-pages = kzalloc(buf-n_pages * sizeof(struct page *), GFP_KERNEL);
+ if (!buf-pages)
+ goto userptr_fail_pages_array_alloc;
+
+ /* current-mm-mmap_sem is taken by videobuf core */
+ n_pages_from_user = get_user_pages(current, current-mm,
+vaddr PAGE_MASK,
+buf-n_pages,
+write,
+1, /* force */
+buf-pages,
+NULL);
+ if (n_pages_from_user != buf-n_pages)
+ goto userptr_fail_get_user_pages;
+
+ buf-vaddr = vm_map_ram(buf-pages, buf-n_pages, -1, PAGE_KERNEL);
+
+ if (!buf-vaddr)
+ goto userptr_fail_get_user_pages;
+
+ buf-vaddr += offset;
+ return buf;
+
+userptr_fail_get_user_pages:
+ pr_debug(get_user_pages requested/got: %d/%d]\n, n_pages_from_user,
+buf-n_pages);
+ while (--n_pages_from_user = 0)
+ put_page(buf-pages[n_pages_from_user]);
+ kfree(buf-pages);
+
+userptr_fail_pages_array_alloc:
+ kfree(buf);
+
+ return NULL;
+}
+
+static void vb2_vmalloc_put_userptr(void *buf_priv)
+{
+ struct vb2_vmalloc_buf *buf = buf_priv;
+
+ unsigned int i;
+ int offset = (unsigned long)buf-vaddr ~PAGE_MASK;
+
+ if (buf-vaddr)
+ vm_unmap_ram((const void *)((unsigned long)buf-vaddr - offset),
+buf-n_pages);
+ for (i = 0; i buf-n_pages; ++i) {
+ if (buf-write)
+ set_page_dirty_lock(buf-pages[i]);
+ put_page(buf-pages[i]);
+ }
+ kfree(buf-pages);
+ kfree(buf);
+}
+
static void *vb2_vmalloc_vaddr(void *buf_priv)
{
struct vb2_vmalloc_buf *buf = buf_priv;
@@ -73,7 +151,8 @@ static void *vb2_vmalloc_vaddr(void *buf_priv)
BUG_ON(!buf);
if (!buf-vaddr) {
- printk(KERN_ERR Address of
