Re: [PATCH 2/7] v4l: videobuf2: add generic memory handling routines
Hi Marek,
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig|3 +
drivers/media/video/Makefile |1 +
drivers/media/video/videobuf2-memops.c | 199
include/media/videobuf2-memops.h |
31 +
4 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-memops.c
create mode 100644 include/media/videobuf2-memops.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index fef6a14..83ce858 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -52,6 +52,9 @@ config V4L2_MEM2MEM_DEV
config VIDEOBUF2_CORE
tristate
+config VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 77c4f85..a97a2a0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
+obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-memops.c
b/drivers/media/video/videobuf2-memops.c new file mode 100644
index 000..67ebdff
--- /dev/null
+++ b/drivers/media/video/videobuf2-memops.c
@@ -0,0 +1,199 @@
+/*
+ * videobuf2-memops.c - generic memory handling routines for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ * Marek Szyprowski m.szyprow...@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/slab.h
+#include linux/module.h
+#include linux/dma-mapping.h
+#include linux/vmalloc.h
+#include linux/cma.h
+#include linux/mm.h
+#include linux/sched.h
+#include linux/file.h
+
+#include media/videobuf2-core.h
+
+/**
+ * vb2_contig_verify_userptr() - verify contiguity of a userspace-mapped
memory + * @vma: virtual memory region which maps the physical memory
+ * to be verified
+ * @vaddr: starting virtual address of the area to be verified
+ * @size:size of the area to be verified
+ * @paddr: will return physical address for the given vaddr
+ *
+ * This function will go through memory area of size size mapped at vaddr
and + * verify that the underlying physical pages are contiguous.
+ *
+ * Returns 0 on success and a physical address to the memory pointed
+ * to by vaddr in paddr.
+ */
+int vb2_contig_verify_userptr(struct vm_area_struct *vma,
+ unsigned long vaddr, unsigned long size,
+ unsigned long *paddr)
+{
+ struct mm_struct *mm = current-mm;
+ unsigned long offset;
+ unsigned long vma_size;
+ unsigned long curr_pfn, prev_pfn;
+ unsigned long num_pages;
+ int ret = -EINVAL;
+ unsigned int i;
+
+ offset = vaddr ~PAGE_MASK;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
You're overwriting the vma argument, why do you need it ?
+ if (!vma) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ vma_size = vma-vm_end - vma-vm_start;
+
+ if (size vma_size - offset) {
+ printk(KERN_ERR Region too small\n);
+ goto done;
+ }
+ num_pages = (size + offset) PAGE_SHIFT;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
Maybe you could put this call in the loop to avoid code duplication ? Feel
free to steal code from
http://git.linuxtv.org/pinchartl/media.git?a=blob;f=drivers/media/video/isp/ispqueue.c;h=6750f68a6476fc168d44563b332f6b23b2700504;hb=630004a88986b1f8b42c5fddb9a5e5c0d575b464#l358
:-)
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
+
RE: [PATCH 2/7] v4l: videobuf2: add generic memory handling routines
Hello,
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
Hi Marek,
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig|3 +
drivers/media/video/Makefile |1 +
drivers/media/video/videobuf2-memops.c | 199
include/media/videobuf2-memops.h |
31 +
4 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-memops.c
create mode 100644 include/media/videobuf2-memops.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index fef6a14..83ce858 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -52,6 +52,9 @@ config V4L2_MEM2MEM_DEV
config VIDEOBUF2_CORE
tristate
+config VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 77c4f85..a97a2a0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
+obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-memops.c
b/drivers/media/video/videobuf2-memops.c new file mode 100644
index 000..67ebdff
--- /dev/null
+++ b/drivers/media/video/videobuf2-memops.c
@@ -0,0 +1,199 @@
+/*
+ * videobuf2-memops.c - generic memory handling routines for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *Marek Szyprowski m.szyprow...@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/slab.h
+#include linux/module.h
+#include linux/dma-mapping.h
+#include linux/vmalloc.h
+#include linux/cma.h
+#include linux/mm.h
+#include linux/sched.h
+#include linux/file.h
+
+#include media/videobuf2-core.h
+
+/**
+ * vb2_contig_verify_userptr() - verify contiguity of a userspace-mapped
memory + * @vma:virtual memory region which maps the physical memory
+ * to be verified
+ * @vaddr: starting virtual address of the area to be verified
+ * @size: size of the area to be verified
+ * @paddr: will return physical address for the given vaddr
+ *
+ * This function will go through memory area of size size mapped at vaddr
and + * verify that the underlying physical pages are contiguous.
+ *
+ * Returns 0 on success and a physical address to the memory pointed
+ * to by vaddr in paddr.
+ */
+int vb2_contig_verify_userptr(struct vm_area_struct *vma,
+ unsigned long vaddr, unsigned long size,
+ unsigned long *paddr)
+{
+ struct mm_struct *mm = current-mm;
+ unsigned long offset;
+ unsigned long vma_size;
+ unsigned long curr_pfn, prev_pfn;
+ unsigned long num_pages;
+ int ret = -EINVAL;
+ unsigned int i;
+
+ offset = vaddr ~PAGE_MASK;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
You're overwriting the vma argument, why do you need it ?
Well, I'm sorry for this. Looks I've missed something here. I will fix this in
the next version.
+ if (!vma) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ vma_size = vma-vm_end - vma-vm_start;
+
+ if (size vma_size - offset) {
+ printk(KERN_ERR Region too small\n);
+ goto done;
+ }
+ num_pages = (size + offset) PAGE_SHIFT;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
Maybe you could put this call in the loop to avoid code duplication ? Feel
free to steal code from
http://git.linuxtv.org/pinchartl/media.git?a=blob;f=drivers/media/video/isp/ispqueue.c;h=6750f68a6476f
c168d44563b332f6b23b2700504;hb=630004a88986b1f8b42c5fddb9a5e5c0d575b464#l358
:-)
Ok, Thx :)
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma,
Re: [PATCH 2/7] v4l: videobuf2: add generic memory handling routines
Hi Marek,
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
I don't think that this situation desires for a EFAULT error code. EINVAL
is imho enough to let application know that this memory cannot be used for
the userptr buffer.
-EINVAL is used quite a lot already to report many different error conditions.
-EFAULT would let application know that the issue is about the pointer address
as opposed to the buffer index for instance.
[snip]
+ * Returns 0 on success.
+ */
+int vb2_mmap_pfn_range(struct vm_area_struct *vma, unsigned long
paddr, + unsigned long size,
+ const struct vm_operations_struct *vm_ops,
+ void *priv)
+{
+ int ret;
+
+ size = min_t(unsigned long, vma-vm_end - vma-vm_start, size);
+
+ vma-vm_page_prot = pgprot_noncached(vma-vm_page_prot);
Now we're getting to the dirty part :-) Do we always want to map the
memory uncached ?
That's how it is handled by the videobuf1 and probably most of drivers that
use physically contiguous memory. I don't want to change it right now.
This will be a way too much for a single step. Once drivers will settle
down on vb2 we may start thinking of adding support for CACHED vs.
UNCACHED mappings.
OK.
I'm not too sure about what the use cases for this function are. It seems
to be used by the DMA coherent and CMA allocators. Are you sure they
will always use physically contiguous memory ?
Yes, both of them are designed for physically contiguous memory. I just
noticed that the videobuf1 used the dma-contig name which is a bit better
for this case. ;)
OK.
+ ret = remap_pfn_range(vma, vma-vm_start, paddr PAGE_SHIFT,
+ size, vma-vm_page_prot);
+ if (ret) {
+ printk(KERN_ERR Remapping memory failed, error: %d\n, ret);
+ return ret;
+ }
+
+ vma-vm_flags |= VM_DONTEXPAND | VM_RESERVED;
+ vma-vm_private_data= priv;
+ vma-vm_ops = vm_ops;
+
+ vm_ops-open(vma);
+
+ printk(KERN_DEBUG %s: mapped paddr 0x%08lx at 0x%08lx, size %ld\n,
+ __func__, paddr, vma-vm_start, size);
+
+ return 0;
+}
+
+/**
+ * vb2_get_userptr() - acquire an area pointed to by userspace addres
vaddr + * @vaddr: virtual userspace address to the given area
+ *
+ * This function attempts to acquire an area mapped in the userspace
for + * the duration of a hardware operation.
+ *
+ * Returns a virtual memory region associated with the given vaddr on
success + * or NULL.
+ */
+struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
+{
+ struct mm_struct *mm = current-mm;
+ struct vm_area_struct *vma;
+ struct vm_area_struct *vma_copy;
+
+ vma_copy = kmalloc(sizeof(struct vm_area_struct), GFP_KERNEL);
+ if (vma_copy == NULL)
+ return NULL;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma)
+ goto done;
+
+ if (vma-vm_ops vma-vm_ops-open)
+ vma-vm_ops-open(vma);
+
+ if (vma-vm_file)
+ get_file(vma-vm_file);
Could you explain what this is for ?
This is called when you access physically contiguous memory from different
device which has been earlier mmaped by the application. This way
applications uses userptr mode with dmacontig. When doing this, you must
ensure somehow that this memory won't be released/freed before you finish.
This is achieved by calling open() callback and increasing the use count
of the mmaped file. Same things happen when your application calls fork()
and a new process gets access to your mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages() ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message
RE: [PATCH 2/7] v4l: videobuf2: add generic memory handling routines
Hello,
On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
Hi Marek,
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
I don't think that this situation desires for a EFAULT error code. EINVAL
is imho enough to let application know that this memory cannot be used for
the userptr buffer.
-EINVAL is used quite a lot already to report many different error conditions.
-EFAULT would let application know that the issue is about the pointer address
as opposed to the buffer index for instance.
The application will not notice it probably in this case, because a default
EFAULT handler will kill it in most of the cases ;)
snip
+/**
+ * vb2_get_userptr() - acquire an area pointed to by userspace addres
vaddr + * @vaddr: virtual userspace address to the given area
+ *
+ * This function attempts to acquire an area mapped in the userspace
for + * the duration of a hardware operation.
+ *
+ * Returns a virtual memory region associated with the given vaddr on
success + * or NULL.
+ */
+struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
+{
+ struct mm_struct *mm = current-mm;
+ struct vm_area_struct *vma;
+ struct vm_area_struct *vma_copy;
+
+ vma_copy = kmalloc(sizeof(struct vm_area_struct), GFP_KERNEL);
+ if (vma_copy == NULL)
+ return NULL;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma)
+ goto done;
+
+ if (vma-vm_ops vma-vm_ops-open)
+ vma-vm_ops-open(vma);
+
+ if (vma-vm_file)
+ get_file(vma-vm_file);
Could you explain what this is for ?
This is called when you access physically contiguous memory from different
device which has been earlier mmaped by the application. This way
applications uses userptr mode with dmacontig. When doing this, you must
ensure somehow that this memory won't be released/freed before you finish.
This is achieved by calling open() callback and increasing the use count
of the mmaped file. Same things happen when your application calls fork()
and a new process gets access to your mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages() ?
Nope. get_user_pages() doesn't support pfn-type memory at all (it just fails
this that case). PFN-type memory cannot be swapped out anyway so it not a
problem here.
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 2/7] v4l: videobuf2: add generic memory handling routines
Hi Marek,
On Monday 29 November 2010 15:27:00 Marek Szyprowski wrote:
Hello,
On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
Hi Marek,
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer
handling, contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
I don't think that this situation desires for a EFAULT error code.
EINVAL is imho enough to let application know that this memory cannot
be used for the userptr buffer.
-EINVAL is used quite a lot already to report many different error
conditions. -EFAULT would let application know that the issue is about
the pointer address as opposed to the buffer index for instance.
The application will not notice it probably in this case, because a default
EFAULT handler will kill it in most of the cases ;)
Will it ? Where ? ioctls can return -EFAULT.
+/**
+ * vb2_get_userptr() - acquire an area pointed to by userspace
addres vaddr + * @vaddr: virtual userspace address to the given
area + *
+ * This function attempts to acquire an area mapped in the
userspace for + * the duration of a hardware operation.
+ *
+ * Returns a virtual memory region associated with the given vaddr
on success + * or NULL.
+ */
+struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
+{
+ struct mm_struct *mm = current-mm;
+ struct vm_area_struct *vma;
+ struct vm_area_struct *vma_copy;
+
+ vma_copy = kmalloc(sizeof(struct vm_area_struct), GFP_KERNEL);
+ if (vma_copy == NULL)
+ return NULL;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma)
+ goto done;
+
+ if (vma-vm_ops vma-vm_ops-open)
+ vma-vm_ops-open(vma);
+
+ if (vma-vm_file)
+ get_file(vma-vm_file);
Could you explain what this is for ?
This is called when you access physically contiguous memory from
different device which has been earlier mmaped by the application.
This way applications uses userptr mode with dmacontig. When doing
this, you must ensure somehow that this memory won't be released/freed
before you finish. This is achieved by calling open() callback and
increasing the use count of the mmaped file. Same things happen when
your application calls fork() and a new process gets access to your
mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages() ?
Nope. get_user_pages() doesn't support pfn-type memory at all (it just
fails this that case). PFN-type memory cannot be swapped out anyway so it
not a problem here.
But vb2_get_userptr() can be used on non-PFN memory, can't it ?
--
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 2/7] v4l: videobuf2: add generic memory handling routines
Hello,
On Monday, November 29, 2010 3:37 PM Laurent Pinchart wrote:
On Monday 29 November 2010 15:27:00 Marek Szyprowski wrote:
Hello,
On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
Hi Marek,
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer
handling, contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
I don't think that this situation desires for a EFAULT error code.
EINVAL is imho enough to let application know that this memory cannot
be used for the userptr buffer.
-EINVAL is used quite a lot already to report many different error
conditions. -EFAULT would let application know that the issue is about
the pointer address as opposed to the buffer index for instance.
The application will not notice it probably in this case, because a default
EFAULT handler will kill it in most of the cases ;)
Will it ? Where ? ioctls can return -EFAULT.
Ok. You are right. I mixed something. Please ignore my previous comment.
+/**
+ * vb2_get_userptr() - acquire an area pointed to by userspace
addres vaddr + * @vaddr:virtual userspace address to the given
area + *
+ * This function attempts to acquire an area mapped in the
userspace for + * the duration of a hardware operation.
+ *
+ * Returns a virtual memory region associated with the given vaddr
on success + * or NULL.
+ */
+struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
+{
+ struct mm_struct *mm = current-mm;
+ struct vm_area_struct *vma;
+ struct vm_area_struct *vma_copy;
+
+ vma_copy = kmalloc(sizeof(struct vm_area_struct), GFP_KERNEL);
+ if (vma_copy == NULL)
+ return NULL;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma)
+ goto done;
+
+ if (vma-vm_ops vma-vm_ops-open)
+ vma-vm_ops-open(vma);
+
+ if (vma-vm_file)
+ get_file(vma-vm_file);
Could you explain what this is for ?
This is called when you access physically contiguous memory from
different device which has been earlier mmaped by the application.
This way applications uses userptr mode with dmacontig. When doing
this, you must ensure somehow that this memory won't be released/freed
before you finish. This is achieved by calling open() callback and
increasing the use count of the mmaped file. Same things happen when
your application calls fork() and a new process gets access to your
mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages() ?
Nope. get_user_pages() doesn't support pfn-type memory at all (it just
fails this that case). PFN-type memory cannot be swapped out anyway so it
not a problem here.
But vb2_get_userptr() can be used on non-PFN memory, can't it ?
I thought that this function is used only by dma-coherent and cma allocators.
dma-sg and iommu allocators will definitely call get_user_pages().
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 2/7] v4l: videobuf2: add generic memory handling routines
Hi Marek,
On Monday 29 November 2010 16:13:17 Marek Szyprowski wrote:
On Monday, November 29, 2010 3:37 PM Laurent Pinchart wrote:
On Monday 29 November 2010 15:27:00 Marek Szyprowski wrote:
On Monday, November 29, 2010 2:41 PM Laurent Pinchart wrote:
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer
handling, contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
[snip]
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
I would use -EFAULT when curr_pfn != prev_pfn + 1.
I don't think that this situation desires for a EFAULT error code.
EINVAL is imho enough to let application know that this memory
cannot be used for the userptr buffer.
-EINVAL is used quite a lot already to report many different error
conditions. -EFAULT would let application know that the issue is
about the pointer address as opposed to the buffer index for
instance.
The application will not notice it probably in this case, because a
default EFAULT handler will kill it in most of the cases ;)
Will it ? Where ? ioctls can return -EFAULT.
Ok. You are right. I mixed something. Please ignore my previous comment.
+/**
+ * vb2_get_userptr() - acquire an area pointed to by userspace
addres vaddr + * @vaddr: virtual userspace address to the given
area + *
+ * This function attempts to acquire an area mapped in the
userspace for + * the duration of a hardware operation.
+ *
+ * Returns a virtual memory region associated with the given
vaddr on success + * or NULL.
+ */
+struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
+{
+ struct mm_struct *mm = current-mm;
+ struct vm_area_struct *vma;
+ struct vm_area_struct *vma_copy;
+
+ vma_copy = kmalloc(sizeof(struct vm_area_struct),
GFP_KERNEL); +if (vma_copy == NULL)
+ return NULL;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma)
+ goto done;
+
+ if (vma-vm_ops vma-vm_ops-open)
+ vma-vm_ops-open(vma);
+
+ if (vma-vm_file)
+ get_file(vma-vm_file);
Could you explain what this is for ?
This is called when you access physically contiguous memory from
different device which has been earlier mmaped by the application.
This way applications uses userptr mode with dmacontig. When doing
this, you must ensure somehow that this memory won't be
released/freed before you finish. This is achieved by calling
open() callback and increasing the use count of the mmaped file.
Same things happen when your application calls fork() and a new
process gets access to your mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages()
?
Nope. get_user_pages() doesn't support pfn-type memory at all (it just
fails this that case). PFN-type memory cannot be swapped out anyway so
it not a problem here.
But vb2_get_userptr() can be used on non-PFN memory, can't it ?
I thought that this function is used only by dma-coherent and cma
allocators. dma-sg and iommu allocators will definitely call
get_user_pages().
Then it should probably be renamed to vb2_get_pfn_user_pages().
--
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
[PATCH 2/7] v4l: videobuf2: add generic memory handling routines
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig|3 +
drivers/media/video/Makefile |1 +
drivers/media/video/videobuf2-memops.c | 199
include/media/videobuf2-memops.h | 31 +
4 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-memops.c
create mode 100644 include/media/videobuf2-memops.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index fef6a14..83ce858 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -52,6 +52,9 @@ config V4L2_MEM2MEM_DEV
config VIDEOBUF2_CORE
tristate
+config VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 77c4f85..a97a2a0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
+obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-memops.c
b/drivers/media/video/videobuf2-memops.c
new file mode 100644
index 000..67ebdff
--- /dev/null
+++ b/drivers/media/video/videobuf2-memops.c
@@ -0,0 +1,199 @@
+/*
+ * videobuf2-memops.c - generic memory handling routines for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *Marek Szyprowski m.szyprow...@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/slab.h
+#include linux/module.h
+#include linux/dma-mapping.h
+#include linux/vmalloc.h
+#include linux/cma.h
+#include linux/mm.h
+#include linux/sched.h
+#include linux/file.h
+
+#include media/videobuf2-core.h
+
+/**
+ * vb2_contig_verify_userptr() - verify contiguity of a userspace-mapped memory
+ * @vma: virtual memory region which maps the physical memory
+ * to be verified
+ * @vaddr: starting virtual address of the area to be verified
+ * @size: size of the area to be verified
+ * @paddr: will return physical address for the given vaddr
+ *
+ * This function will go through memory area of size size mapped at vaddr and
+ * verify that the underlying physical pages are contiguous.
+ *
+ * Returns 0 on success and a physical address to the memory pointed
+ * to by vaddr in paddr.
+ */
+int vb2_contig_verify_userptr(struct vm_area_struct *vma,
+ unsigned long vaddr, unsigned long size,
+ unsigned long *paddr)
+{
+ struct mm_struct *mm = current-mm;
+ unsigned long offset;
+ unsigned long vma_size;
+ unsigned long curr_pfn, prev_pfn;
+ unsigned long num_pages;
+ int ret = -EINVAL;
+ unsigned int i;
+
+ offset = vaddr ~PAGE_MASK;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ vma_size = vma-vm_end - vma-vm_start;
+
+ if (size vma_size - offset) {
+ printk(KERN_ERR Region too small\n);
+ goto done;
+ }
+ num_pages = (size + offset) PAGE_SHIFT;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
+ break;
+ }
+ }
+
+done:
+ up_read(mm-mmap_sem);
+ return ret;
+}
+
+/**
+ * vb2_mmap_pfn_range() - map physical pages to userspace
+ * @vma: virtual memory region for the mapping
+ * @paddr: starting physical address of the memory to be mapped
+ * @size: size of the memory to be mapped
+ * @vm_ops:vm operations to be assigned to the created area
+ * @priv: private data to be associated with the area
+ *
+ * Returns 0 on success.
+ */
+int
[PATCH 2/7] v4l: videobuf2: add generic memory handling routines
From: Pawel Osciak p.osc...@samsung.com
Add generic memory handling routines for userspace pointer handling,
contiguous memory verification and mapping.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
---
drivers/media/video/Kconfig|3 +
drivers/media/video/Makefile |1 +
drivers/media/video/videobuf2-memops.c | 199
include/media/videobuf2-memops.h | 31 +
4 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/videobuf2-memops.c
create mode 100644 include/media/videobuf2-memops.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index fef6a14..83ce858 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -52,6 +52,9 @@ config V4L2_MEM2MEM_DEV
config VIDEOBUF2_CORE
tristate
+config VIDEOBUF2_MEMOPS
+ tristate
+
#
# Multimedia Video device configuration
#
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 77c4f85..a97a2a0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
+obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/video/videobuf2-memops.c
b/drivers/media/video/videobuf2-memops.c
new file mode 100644
index 000..67ebdff
--- /dev/null
+++ b/drivers/media/video/videobuf2-memops.c
@@ -0,0 +1,199 @@
+/*
+ * videobuf2-memops.c - generic memory handling routines for videobuf2
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * Author: Pawel Osciak p.osc...@samsung.com
+ *Marek Szyprowski m.szyprow...@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/slab.h
+#include linux/module.h
+#include linux/dma-mapping.h
+#include linux/vmalloc.h
+#include linux/cma.h
+#include linux/mm.h
+#include linux/sched.h
+#include linux/file.h
+
+#include media/videobuf2-core.h
+
+/**
+ * vb2_contig_verify_userptr() - verify contiguity of a userspace-mapped memory
+ * @vma: virtual memory region which maps the physical memory
+ * to be verified
+ * @vaddr: starting virtual address of the area to be verified
+ * @size: size of the area to be verified
+ * @paddr: will return physical address for the given vaddr
+ *
+ * This function will go through memory area of size size mapped at vaddr and
+ * verify that the underlying physical pages are contiguous.
+ *
+ * Returns 0 on success and a physical address to the memory pointed
+ * to by vaddr in paddr.
+ */
+int vb2_contig_verify_userptr(struct vm_area_struct *vma,
+ unsigned long vaddr, unsigned long size,
+ unsigned long *paddr)
+{
+ struct mm_struct *mm = current-mm;
+ unsigned long offset;
+ unsigned long vma_size;
+ unsigned long curr_pfn, prev_pfn;
+ unsigned long num_pages;
+ int ret = -EINVAL;
+ unsigned int i;
+
+ offset = vaddr ~PAGE_MASK;
+
+ down_read(mm-mmap_sem);
+
+ vma = find_vma(mm, vaddr);
+ if (!vma) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ vma_size = vma-vm_end - vma-vm_start;
+
+ if (size vma_size - offset) {
+ printk(KERN_ERR Region too small\n);
+ goto done;
+ }
+ num_pages = (size + offset) PAGE_SHIFT;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret) {
+ printk(KERN_ERR Invalid userspace address\n);
+ goto done;
+ }
+
+ *paddr = (curr_pfn PAGE_SHIFT) + offset;
+
+ for (i = 1; i num_pages; ++i) {
+ prev_pfn = curr_pfn;
+ vaddr += PAGE_SIZE;
+
+ ret = follow_pfn(vma, vaddr, curr_pfn);
+ if (ret || curr_pfn != prev_pfn + 1) {
+ printk(KERN_ERR Invalid userspace address\n);
+ ret = -EINVAL;
+ break;
+ }
+ }
+
+done:
+ up_read(mm-mmap_sem);
+ return ret;
+}
+
+/**
+ * vb2_mmap_pfn_range() - map physical pages to userspace
+ * @vma: virtual memory region for the mapping
+ * @paddr: starting physical address of the memory to be mapped
+ * @size: size of the memory to be mapped
+ * @vm_ops:vm operations to be assigned to the created area
+ * @priv: private data to be associated with the area
+ *
+ * Returns 0 on success.
+ */
+int
