Hi Marek,
On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
> From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
>
> Add generic memory handling routines for userspace pointer handling,
> contiguous memory verification and mapping.
>
> Signed-off-by: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> CC: Pawel Osciak <pawel@xxxxxxxxxx>
> ---
> 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 0000000..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.osciak@xxxxxxxxxxx>
> + * Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> + *
> + * 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.
> + 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
> + *
A more detailed explanation (cache handling, use cases, ...) is needed.
> + * 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 ?
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 ?
> + 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 ?
> + memcpy(vma_copy, vma, sizeof(*vma));
> +done:
> + up_read(&mm->mmap_sem);
> +
> + vma_copy->vm_mm = NULL;
> + vma_copy->vm_next = NULL;
> + vma_copy->vm_prev = NULL;
> +
> + return vma_copy;
> +}
> +
> +/**
> + * vb2_put_userptr() - release a userspace memory area
> + * @vma: virtual memory region associated with the area to be released
> + *
> + * This function releases the previously acquired memory area after a
> hardware + * operation.
> + */
> +void vb2_put_userptr(struct vm_area_struct *vma)
> +{
> + if (!vma)
> + return;
> +
> + if (vma->vm_file)
> + fput(vma->vm_file);
> +
> + if (vma->vm_ops && vma->vm_ops->close)
> + vma->vm_ops->close(vma);
> +
> + kfree(vma);
> +}
> +
> +MODULE_DESCRIPTION("common memory handling routines for videobuf2");
> +MODULE_AUTHOR("Pawel Osciak");
> +MODULE_LICENSE("GPL");
> diff --git a/include/media/videobuf2-memops.h
> b/include/media/videobuf2-memops.h new file mode 100644
> index 0000000..3257411
> --- /dev/null
> +++ b/include/media/videobuf2-memops.h
> @@ -0,0 +1,31 @@
> +/*
> + * videobuf2-memops.h - generic memory handling routines for videobuf2
> + *
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * Author: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#ifndef _MEDIA_VIDEOBUF2_MEMOPS_H
> +#define _MEDIA_VIDEOBUF2_MEMOPS_H
> +
> +#include <media/videobuf2-core.h>
> +
> +int vb2_contig_verify_userptr(struct vm_area_struct *vma,
> + unsigned long vaddr, unsigned long size,
> + unsigned long *paddr);
> +
> +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);
> +
> +struct vm_area_struct *vb2_get_userptr(unsigned long vaddr);
> +
> +void vb2_put_userptr(struct vm_area_struct *vma);
> +
> +#endif
--
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
