videbuf-vmalloc: Corrects mmap code

From: Mauro Carvalho Chehab <mchehab@infradead.org>

There were some bugs on videobuf-vmalloc.

Basically, remap were called with a wrong parameter. Due to that, a later remap
were needed, generating the need of some hacks on videobuf-vmalloc and
videobuf-core.

This patch fixes the remap and removes the hacks.

TODO:

- V4L2_MEMORY_USERPTR is not implemented yet. This method should be
  properly implemented, in order to work with a few userspace applications.

- The driver also doesn't implement V4L2_MEMORY_OVERLAY. This method is used
  only by a few applications, and are becaming obsolete, due to the increment
  of cpu performance. So, most apps prefer to retrieve data to an internal
  buffer, doing some processing like de-interlacing.

Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>

2 files changed, 110 insertions, 72 deletions

diff --git a/linux/drivers/media/video/videobuf-core.c b/linux/drivers/media/video/videobuf-core.c
index 52d9a766c..660d28972 100644
--- a/linux/drivers/media/video/videobuf-core.c
+++ b/linux/drivers/media/video/videobuf-core.c
@@ -92,20 +92,6 @@ int videobuf_iolock(struct videobuf_queue *q, struct videobuf_buffer *vb,
 	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
 	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
 
-	/* This is required to avoid OOPS on some cases,
-	   since mmap_mapper() method should be called before _iolock.
-	   On some cases, the mmap_mapper() is called only after scheduling.
-	 */
-	if (vb->memory == V4L2_MEMORY_MMAP) {
-		wait_event_timeout(vb->done, q->is_mmapped,
-				   msecs_to_jiffies(100));
-		if (!q->is_mmapped) {
-			printk(KERN_ERR
-			       "Error: mmap_mapper() never called!\n");
-			return -EINVAL;
-		}
-	}
-
 	return CALL(q, iolock, q, vb, fbuf);
 }
 
diff --git a/linux/drivers/media/video/videobuf-vmalloc.c b/linux/drivers/media/video/videobuf-vmalloc.c
index 6cdd27d55..340337eda 100644
--- a/linux/drivers/media/video/videobuf-vmalloc.c
+++ b/linux/drivers/media/video/videobuf-vmalloc.c
@@ -125,45 +125,75 @@ static int __videobuf_iolock (struct videobuf_queue* q,
 			      struct videobuf_buffer *vb,
 			      struct v4l2_framebuffer *fbuf)
 {
-	int pages;
-	struct videobuf_vmalloc_memory *mem=vb->priv;
+	struct videobuf_vmalloc_memory *mem = vb->priv;
 
 	BUG_ON(!mem);
 
-	MAGIC_CHECK(mem->magic,MAGIC_VMAL_MEM);
+	MAGIC_CHECK(mem->magic, MAGIC_VMAL_MEM);
 
-	pages = PAGE_ALIGN(vb->size) >> PAGE_SHIFT;
+	switch (vb->memory) {
+	case V4L2_MEMORY_MMAP:
+		dprintk(1, "%s memory method MMAP\n", __func__);
 
-	/* Currently, doesn't support V4L2_MEMORY_OVERLAY */
-	if ((vb->memory != V4L2_MEMORY_MMAP) &&
-				(vb->memory != V4L2_MEMORY_USERPTR) ) {
-		printk(KERN_ERR "Method currently unsupported.\n");
-		return -EINVAL;
-	}
+		/* All handling should be done by __videobuf_mmap_mapper() */
+		if (!mem->vmalloc) {
+			printk(KERN_ERR "memory is not alloced/mmapped.\n");
+			return -EINVAL;
+		}
+		break;
+	case V4L2_MEMORY_USERPTR:
+	{
+		int pages = PAGE_ALIGN(vb->size);
 
-	/* FIXME: should be tested with kernel mmap mem */
-	mem->vmalloc=vmalloc_user (PAGE_ALIGN(vb->size));
-	if (NULL == mem->vmalloc) {
-		printk(KERN_ERR "vmalloc (%d pages) failed\n",pages);
-		return -ENOMEM;
-	}
+		dprintk(1, "%s memory method USERPTR\n", __func__);
+
+#if 1 /* keep */
+		if (vb->baddr) {
+			printk(KERN_ERR "USERPTR is currently not supported\n");
+			return -EINVAL;
+		}
+#endif
 
-	dprintk(1,"vmalloc is at addr 0x%08lx, size=%d\n",
-				(unsigned long)mem->vmalloc,
-				pages << PAGE_SHIFT);
-
-	/* It seems that some kernel versions need to do remap *after*
-	   the mmap() call
-	 */
-	if (mem->vma) {
-		int retval=remap_vmalloc_range(mem->vma, mem->vmalloc,0);
-		kfree(mem->vma);
-		mem->vma=NULL;
-		if (retval<0) {
-			dprintk(1,"mmap app bug: remap_vmalloc_range area %p error %d\n",
-				mem->vmalloc,retval);
-			return retval;
+		/* The only USERPTR currently supported is the one needed for
+		   read() method.
+		 */
+
+		mem->vmalloc = vmalloc_user(pages);
+		if (!mem->vmalloc) {
+			printk(KERN_ERR "vmalloc (%d pages) failed\n", pages);
+			return -ENOMEM;
 		}
+		dprintk(1, "vmalloc is at addr %p (%d pages)\n",
+			mem->vmalloc, pages);
+
+#if 0 /* keep */
+		int rc;
+		/* Kernel userptr is used also by read() method. In this case,
+		   there's no need to remap, since data will be copied to user
+		 */
+		if (!vb->baddr)
+			return 0;
+
+		/* FIXME: to properly support USERPTR, remap should occur.
+		   The code bellow won't work, since mem->vma = NULL
+		 */
+		/* Try to remap memory */
+		rc = remap_vmalloc_range(mem->vma, (void *)vb->baddr, 0);
+		if (rc < 0) {
+			printk(KERN_ERR "mmap: remap failed with error %d. ", rc);
+			return -ENOMEM;
+		}
+#endif
+
+		break;
+	}
+	case V4L2_MEMORY_OVERLAY:
+	default:
+		dprintk(1, "%s memory method OVERLAY/unknown\n", __func__);
+
+		/* Currently, doesn't support V4L2_MEMORY_OVERLAY */
+		printk(KERN_ERR "Memory method currently unsupported.\n");
+		return -EINVAL;
 	}
 
 	return 0;
@@ -179,6 +209,7 @@ static int __videobuf_mmap_free(struct videobuf_queue *q)
 {
 	unsigned int i;
 
+	dprintk(1, "%s\n", __func__);
 	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
 		if (q->bufs[i]) {
 			if (q->bufs[i]->map)
@@ -195,10 +226,11 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	struct videobuf_vmalloc_memory *mem;
 	struct videobuf_mapping *map;
 	unsigned int first;
-	int retval;
+	int retval, pages;
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
 
-	if (! (vma->vm_flags & VM_WRITE) || ! (vma->vm_flags & VM_SHARED))
+	dprintk(1, "%s\n", __func__);
+	if (!(vma->vm_flags & VM_WRITE) || !(vma->vm_flags & VM_SHARED))
 		return -EINVAL;
 
 	/* look for first buffer to map */
@@ -218,46 +250,55 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	}
 
 	/* create mapping + update buffer list */
-	map = q->bufs[first]->map = kzalloc(sizeof(struct videobuf_mapping),GFP_KERNEL);
+	map = kzalloc(sizeof(struct videobuf_mapping), GFP_KERNEL);
 	if (NULL == map)
 		return -ENOMEM;
 
+	q->bufs[first]->map = map;
 	map->start = vma->vm_start;
 	map->end   = vma->vm_end;
 	map->q     = q;
 
 	q->bufs[first]->baddr = vma->vm_start;
 
-	vma->vm_ops          = &videobuf_vm_ops;
-	vma->vm_flags       |= VM_DONTEXPAND | VM_RESERVED;
-	vma->vm_private_data = map;
+	mem = q->bufs[first]->priv;
+	BUG_ON(!mem);
+	MAGIC_CHECK(mem->magic, MAGIC_VMAL_MEM);
 
-	mem=q->bufs[first]->priv;
-	BUG_ON (!mem);
-	MAGIC_CHECK(mem->magic,MAGIC_VMAL_MEM);
+	pages = PAGE_ALIGN(vma->vm_end - vma->vm_start);
+	mem->vmalloc = vmalloc_user(pages);
+	if (!mem->vmalloc) {
+		printk(KERN_ERR "vmalloc (%d pages) failed\n", pages);
+		goto error;
+	}
+	dprintk(1, "vmalloc is at addr %p (%d pages)\n",
+		mem->vmalloc, pages);
 
 	/* Try to remap memory */
-	retval=remap_vmalloc_range(vma, mem->vmalloc,0);
-	if (retval<0) {
-		dprintk(1,"mmap: postponing remap_vmalloc_range\n");
-
-		mem->vma=kmalloc(sizeof(*vma),GFP_KERNEL);
-		if (!mem->vma) {
-			kfree(map);
-			q->bufs[first]->map=NULL;
-			return -ENOMEM;
-		}
-		memcpy(mem->vma,vma,sizeof(*vma));
+	retval = remap_vmalloc_range(vma, mem->vmalloc, 0);
+	if (retval < 0) {
+		printk(KERN_ERR "mmap: remap failed with error %d. ", retval);
+		vfree(mem->vmalloc);
+		goto error;
 	}
 
+	vma->vm_ops          = &videobuf_vm_ops;
+	vma->vm_flags       |= VM_DONTEXPAND | VM_RESERVED;
+	vma->vm_private_data = map;
+
 	dprintk(1,"mmap %p: q=%p %08lx-%08lx (%lx) pgoff %08lx buf %d\n",
-		map,q,vma->vm_start,vma->vm_end,
+		map, q, vma->vm_start, vma->vm_end,
 		(long int) q->bufs[first]->bsize,
-		vma->vm_pgoff,first);
+		vma->vm_pgoff, first);
 
 	videobuf_vm_open(vma);
 
-	return (0);
+	return 0;
+
+error:
+	mem = NULL;
+	kfree(map);
+	return -ENOMEM;
 }
 
 static int __videobuf_copy_to_user ( struct videobuf_queue *q,
@@ -348,13 +389,24 @@ EXPORT_SYMBOL_GPL(videobuf_to_vmalloc);
 
 void videobuf_vmalloc_free (struct videobuf_buffer *buf)
 {
-	struct videobuf_vmalloc_memory *mem=buf->priv;
-	BUG_ON (!mem);
+	struct videobuf_vmalloc_memory *mem = buf->priv;
 
-	MAGIC_CHECK(mem->magic,MAGIC_VMAL_MEM);
+	if (!mem)
+		return;
+
+	MAGIC_CHECK(mem->magic, MAGIC_VMAL_MEM);
 
 	vfree(mem->vmalloc);
-	mem->vmalloc=NULL;
+	mem->vmalloc = NULL;
+
+
+#if 0
+	/* FIXME: This would be the proper place for freeing buf->map? */
+	kfree(buf->map);
+	buf->map = NULL;
+#endif
+
+	/* FIXME: need to do buf->priv = NULL? */
 
 	return;
 }