RE: [PATCH 2/7] v4l: videobuf2: add generic memory handling routines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.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 ?

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, 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.

> > +			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 ?

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.

> 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. ;)
 
> > +	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.

<snip>

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux