[patch 0/9] videobuf fixes 2.6.25

[patch 0/9] videobuf fixes 2.6.25

[brandon]

Hello-

The following set fixes problems I discovered while tracking down bugs in both
vivi and videobuf.  Hopefully most of these can make it into 2.6.25 since they
all seem pretty critical.

Please take a good look at the set and test if possible.  Particularly:
  [RFC] videobuf: Avoid deadlock with QBUF

Also, is anyone using videobuf-vmalloc besides vivi?  The current videobuf API
feels over extended trying to take on the task of a second backend type. 

Thanks,

	Brandon

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 1/9] soc_camera: Introduce a spinlock for use with videobuf

[brandon]

[-- Attachment #1: require-spinlock-soc.patch --]
[-- Type: text/plain, Size: 3728 bytes --]

Videobuf will require all drivers to use a spinlock to protect the IRQ queue.
Introduce this lock for the SOC/PXA drivers.

Signed-off-by: Brandon Philips <bphilips@suse.de>
CC: Guennadi Liakhovetski <kernel@pengutronix.de>

---
 linux/drivers/media/video/pxa_camera.c |   11 ++++-------
 linux/drivers/media/video/soc_camera.c |    5 +++--
 linux/include/media/soc_camera.h       |    1 +
 3 files changed, 8 insertions(+), 9 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/soc_camera.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/soc_camera.c
+++ v4l-dvb/linux/drivers/media/video/soc_camera.c
@@ -229,8 +229,8 @@ static int soc_camera_open(struct inode 
 	dev_dbg(&icd->dev, "camera device open\n");
 
 	/* We must pass NULL as dev pointer, then all pci_* dma operations
-	 * transform to normal dma_* ones. Do we need an irqlock? */
-	videobuf_queue_sg_init(&icf->vb_vidq, ici->vbq_ops, NULL, NULL,
+	 * transform to normal dma_* ones. */
+	videobuf_queue_sg_init(&icf->vb_vidq, ici->vbq_ops, NULL, &icd->irqlock,
 				V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE,
 				ici->msize, icd);
 
@@ -714,6 +714,7 @@ static int soc_camera_probe(struct devic
 	if (ret >= 0) {
 		const struct v4l2_queryctrl *qctrl;
 
+		spin_lock_init(&icd->irqlock);
 		qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_GAIN);
 		icd->gain = qctrl ? qctrl->default_value : (unsigned short)~0;
 		qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
Index: v4l-dvb/linux/include/media/soc_camera.h
===================================================================
--- v4l-dvb.orig/linux/include/media/soc_camera.h
+++ v4l-dvb/linux/include/media/soc_camera.h
@@ -19,6 +19,7 @@ struct soc_camera_device {
 	struct list_head list;
 	struct device dev;
 	struct device *control;
+	spinlock_t irqlock;
 	unsigned short width;		/* Current window */
 	unsigned short height;		/* sizes */
 	unsigned short x_min;		/* Camera capabilities */
Index: v4l-dvb/linux/drivers/media/video/pxa_camera.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/pxa_camera.c
+++ v4l-dvb/linux/drivers/media/video/pxa_camera.c
@@ -109,8 +109,6 @@ struct pxa_camera_dev {
 
 	struct list_head	capture;
 
-	spinlock_t		lock;
-
 	struct pxa_buffer	*active;
 };
 
@@ -283,7 +281,7 @@ static void pxa_videobuf_queue(struct vi
 
 	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
 		vb, vb->baddr, vb->bsize);
-	spin_lock_irqsave(&pcdev->lock, flags);
+	spin_lock_irqsave(&icd->irqlock, flags);
 
 	list_add_tail(&vb->queue, &pcdev->capture);
 
@@ -343,7 +341,7 @@ static void pxa_videobuf_queue(struct vi
 			DCSR(pcdev->dma_chan_y) = DCSR_RUN;
 	}
 
-	spin_unlock_irqrestore(&pcdev->lock, flags);
+	spin_unlock_irqrestore(&icd->irqlock, flags);
 
 }
 
@@ -384,7 +382,7 @@ static void pxa_camera_dma_irq_y(int cha
 	unsigned int status;
 	struct videobuf_buffer *vb;
 
-	spin_lock_irqsave(&pcdev->lock, flags);
+	spin_lock_irqsave(&pcdev->icd->irqlock, flags);
 
 	status = DCSR(pcdev->dma_chan_y);
 	DCSR(pcdev->dma_chan_y) = status;
@@ -429,7 +427,7 @@ static void pxa_camera_dma_irq_y(int cha
 				   vb.queue);
 
 out:
-	spin_unlock_irqrestore(&pcdev->lock, flags);
+	spin_unlock_irqrestore(&pcdev->icd->irqlock, flags);
 }
 
 static struct videobuf_queue_ops pxa_videobuf_ops = {
@@ -869,7 +867,6 @@ static int pxa_camera_probe(struct platf
 	}
 
 	INIT_LIST_HEAD(&pcdev->capture);
-	spin_lock_init(&pcdev->lock);
 
 	/*
 	 * Request the regions.

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 2/9] videobuf: Require spinlocks for all videobuf users

[brandon]

[-- Attachment #1: require-spinlock-videobuf.patch --]
[-- Type: text/plain, Size: 4349 bytes --]

A spinlock is necessary for queue_cancel to work with every driver in the tree.
Otherwise a race exists between IRQ handlers removing buffers from the queue
and queue_cancel invalidating the queue.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-core.c |   46 +++++++++++-------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/videobuf-core.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-core.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-core.c
@@ -145,6 +145,9 @@ void videobuf_queue_core_init(struct vid
 	BUG_ON(!q->ops->buf_queue);
 	BUG_ON(!q->ops->buf_release);
 
+	/* Lock is mandatory for queue_cancel to work */
+	BUG_ON(!irqlock);
+
 	/* Having implementations for abstract methods are mandatory */
 	BUG_ON(!q->int_ops);
 
@@ -197,8 +200,7 @@ void videobuf_queue_cancel(struct videob
 	int i;
 
 	/* remove queued buffers from list */
-	if (q->irqlock)
-		spin_lock_irqsave(q->irqlock, flags);
+	spin_lock_irqsave(q->irqlock, flags);
 	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
 		if (NULL == q->bufs[i])
 			continue;
@@ -207,8 +209,7 @@ void videobuf_queue_cancel(struct videob
 			q->bufs[i]->state = VIDEOBUF_ERROR;
 		}
 	}
-	if (q->irqlock)
-		spin_unlock_irqrestore(q->irqlock, flags);
+	spin_unlock_irqrestore(q->irqlock, flags);
 
 	/* free all buffers + clear queue */
 	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
@@ -564,11 +565,9 @@ int videobuf_qbuf(struct videobuf_queue 
 
 	list_add_tail(&buf->stream, &q->stream);
 	if (q->streaming) {
-		if (q->irqlock)
-			spin_lock_irqsave(q->irqlock, flags);
+		spin_lock_irqsave(q->irqlock, flags);
 		q->ops->buf_queue(q, buf);
-		if (q->irqlock)
-			spin_unlock_irqrestore(q->irqlock, flags);
+		spin_unlock_irqrestore(q->irqlock, flags);
 	}
 	dprintk(1, "qbuf: succeded\n");
 	retval = 0;
@@ -653,13 +652,11 @@ int videobuf_streamon(struct videobuf_qu
 	if (q->streaming)
 		goto done;
 	q->streaming = 1;
-	if (q->irqlock)
-		spin_lock_irqsave(q->irqlock, flags);
+	spin_lock_irqsave(q->irqlock, flags);
 	list_for_each_entry(buf, &q->stream, stream)
 		if (buf->state == VIDEOBUF_PREPARED)
 			q->ops->buf_queue(q, buf);
-	if (q->irqlock)
-		spin_unlock_irqrestore(q->irqlock, flags);
+	spin_unlock_irqrestore(q->irqlock, flags);
 
  done:
 	mutex_unlock(&q->vb_lock);
@@ -715,11 +712,9 @@ static ssize_t videobuf_read_zerocopy(st
 		goto done;
 
 	/* start capture & wait */
-	if (q->irqlock)
-		spin_lock_irqsave(q->irqlock, flags);
+	spin_lock_irqsave(q->irqlock, flags);
 	q->ops->buf_queue(q, q->read_buf);
-	if (q->irqlock)
-		spin_unlock_irqrestore(q->irqlock, flags);
+	spin_unlock_irqrestore(q->irqlock, flags);
 	retval = videobuf_waiton(q->read_buf, 0, 0);
 	if (0 == retval) {
 		CALL(q, sync, q, q->read_buf);
@@ -780,12 +775,11 @@ ssize_t videobuf_read_one(struct videobu
 			q->read_buf = NULL;
 			goto done;
 		}
-		if (q->irqlock)
-			spin_lock_irqsave(q->irqlock, flags);
 
+		spin_lock_irqsave(q->irqlock, flags);
 		q->ops->buf_queue(q, q->read_buf);
-		if (q->irqlock)
-			spin_unlock_irqrestore(q->irqlock, flags);
+		spin_unlock_irqrestore(q->irqlock, flags);
+
 		q->read_off = 0;
 	}
 
@@ -851,12 +845,10 @@ static int __videobuf_read_start(struct 
 			return err;
 		list_add_tail(&q->bufs[i]->stream, &q->stream);
 	}
-	if (q->irqlock)
-		spin_lock_irqsave(q->irqlock, flags);
+	spin_lock_irqsave(q->irqlock, flags);
 	for (i = 0; i < count; i++)
 		q->ops->buf_queue(q, q->bufs[i]);
-	if (q->irqlock)
-		spin_unlock_irqrestore(q->irqlock, flags);
+	spin_unlock_irqrestore(q->irqlock, flags);
 	q->reading = 1;
 	return 0;
 }
@@ -970,11 +962,9 @@ ssize_t videobuf_read_stream(struct vide
 		if (q->read_off == q->read_buf->size) {
 			list_add_tail(&q->read_buf->stream,
 				      &q->stream);
-			if (q->irqlock)
-				spin_lock_irqsave(q->irqlock, flags);
+			spin_lock_irqsave(q->irqlock, flags);
 			q->ops->buf_queue(q, q->read_buf);
-			if (q->irqlock)
-				spin_unlock_irqrestore(q->irqlock, flags);
+			spin_unlock_irqrestore(q->irqlock, flags);
 			q->read_buf = NULL;
 		}
 		if (retval < 0)

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 3/9] videobuf: Wakeup queues after changing the state to ERROR

[brandon]

[-- Attachment #1: wakeup-on-queue-cancel.patch --]
[-- Type: text/plain, Size: 880 bytes --]

The waitqueues must be woken up every time state changes.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-core.c |    1 +
 1 file changed, 1 insertion(+)

Index: v4l-dvb/linux/drivers/media/video/videobuf-core.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-core.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-core.c
@@ -207,6 +207,7 @@ void videobuf_queue_cancel(struct videob
 		if (q->bufs[i]->state == VIDEOBUF_QUEUED) {
 			list_del(&q->bufs[i]->queue);
 			q->bufs[i]->state = VIDEOBUF_ERROR;
+			wake_up_all(&q->bufs[i]->done);
 		}
 	}
 	spin_unlock_irqrestore(q->irqlock, flags);

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 4/9] videobuf: Simplify videobuf_waiton logic and possibly avoid missed wakeup

[brandon]

[-- Attachment #1: simplify-videobuf_waiton-logic.patch --]
[-- Type: text/plain, Size: 1961 bytes --]

Possible missed wakeup- use kernel helpers for wait queues
  http://www.mail-archive.com/linux-usb-devel@lists.sourceforge.net/msg27983.html

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-core.c |   37 ++++++++++++------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/videobuf-core.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-core.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-core.c
@@ -65,32 +65,25 @@ void *videobuf_alloc(struct videobuf_que
 	return vb;
 }
 
+#define WAITON_CONDITION (vb->state != VIDEOBUF_ACTIVE &&\
+				vb->state != VIDEOBUF_QUEUED)
 int videobuf_waiton(struct videobuf_buffer *vb, int non_blocking, int intr)
 {
-	int retval = 0;
-	DECLARE_WAITQUEUE(wait, current);
-
 	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
-	add_wait_queue(&vb->done, &wait);
-	while (vb->state == VIDEOBUF_ACTIVE || vb->state == VIDEOBUF_QUEUED) {
-		if (non_blocking) {
-			retval = -EAGAIN;
-			break;
-		}
-		set_current_state(intr  ? TASK_INTERRUPTIBLE
-					: TASK_UNINTERRUPTIBLE);
-		if (vb->state == VIDEOBUF_ACTIVE ||
-		    vb->state == VIDEOBUF_QUEUED)
-			schedule();
-		set_current_state(TASK_RUNNING);
-		if (intr && signal_pending(current)) {
-			dprintk(1, "buffer waiton: -EINTR\n");
-			retval = -EINTR;
-			break;
-		}
+
+	if (non_blocking) {
+		if (WAITON_CONDITION)
+			return 0;
+		else
+			return -EAGAIN;
 	}
-	remove_wait_queue(&vb->done, &wait);
-	return retval;
+
+	if (intr)
+		return wait_event_interruptible(vb->done, WAITON_CONDITION);
+	else
+		wait_event(vb->done, WAITON_CONDITION);
+
+	return 0;
 }
 
 int videobuf_iolock(struct videobuf_queue *q, struct videobuf_buffer *vb,

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 5/9] videobuf-vmalloc.c: Remove buf_release from videobuf_vm_close

[brandon]

[-- Attachment #1: videobuf-buf_release-vm_close.patch --]
[-- Type: text/plain, Size: 965 bytes --]

Remove the buf_release on vm_close because it will lead to a buffer being
released multiple times since all buffers are already freed under the two
possible cases: device close or STREAMOFF.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-vmalloc.c |    2 --
 1 file changed, 2 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/videobuf-vmalloc.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-vmalloc.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-vmalloc.c
@@ -79,8 +79,6 @@ videobuf_vm_close(struct vm_area_struct 
 			if (q->bufs[i]->map != map)
 				continue;
 
-			q->ops->buf_release(q,q->bufs[i]);
-
 			q->bufs[i]->map   = NULL;
 			q->bufs[i]->baddr = 0;
 		}

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 6/9] videobuf-vmalloc.c: Fix hack of postponing mmap on remap failure

[brandon]

[-- Attachment #1: videobuf-mmap-setup.patch --]
[-- Type: text/plain, Size: 6107 bytes --]

In videobuf-vmalloc.c remap_vmalloc_range is failing when applications are
trying to mmap buffers immediately after reqbuf.  It fails because the vmalloc
area isn't setup until the first QBUF when drivers call iolock.

This patch introduces mmap_setup to the qtype_ops and it is called in
__videobuf_mmap_setup if the buffer type is mmap.  In the case of vmalloc
buffers this calls iolock, and sets the state to idle.

I don't think this is needed for dma-sg buffers and it defaults to a no-op for
everything but vmalloc.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-core.c    |   29 +++++++-----------
 linux/drivers/media/video/videobuf-vmalloc.c |   43 +++++++++++++--------------
 linux/include/media/videobuf-core.h          |    3 +
 3 files changed, 35 insertions(+), 40 deletions(-)

Index: v4l-dvb/linux/include/media/videobuf-core.h
===================================================================
--- v4l-dvb.orig/linux/include/media/videobuf-core.h
+++ v4l-dvb/linux/include/media/videobuf-core.h
@@ -144,6 +144,8 @@ struct videobuf_qtype_ops {
 				 int vbihack,
 				 int nonblocking);
 	int (*mmap_free)	(struct videobuf_queue *q);
+	int (*mmap_setup)	(struct videobuf_queue *q,
+				 struct videobuf_buffer *vb);
 	int (*mmap_mapper)	(struct videobuf_queue *q,
 				struct vm_area_struct *vma);
 };
@@ -168,7 +170,6 @@ struct videobuf_queue {
 
 	unsigned int               streaming:1;
 	unsigned int               reading:1;
-	unsigned int		   is_mmapped:1;
 
 	/* capture via mmap() + ioctl(QBUF/DQBUF) */
 	struct list_head           stream;
Index: v4l-dvb/linux/drivers/media/video/videobuf-core.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-core.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-core.c
@@ -92,25 +92,17 @@ int videobuf_iolock(struct videobuf_queu
 	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);
 }
 
 /* --------------------------------------------------------------------- */
 
+static int videobuf_mmap_setup_default(struct videobuf_queue *q,
+					struct videobuf_buffer *vb)
+{
+	return 0;
+}
+
 
 void videobuf_queue_core_init(struct videobuf_queue *q,
 			 struct videobuf_queue_ops *ops,
@@ -132,6 +124,9 @@ void videobuf_queue_core_init(struct vid
 	q->priv_data = priv;
 	q->int_ops   = int_ops;
 
+	if (!q->int_ops->mmap_setup)
+		q->int_ops->mmap_setup = videobuf_mmap_setup_default;
+
 	/* All buffer operations are mandatory */
 	BUG_ON(!q->ops->buf_setup);
 	BUG_ON(!q->ops->buf_prepare);
@@ -305,8 +300,6 @@ static int __videobuf_mmap_free(struct v
 
 	rc  = CALL(q, mmap_free, q);
 
-	q->is_mmapped = 0;
-
 	if (rc < 0)
 		return rc;
 
@@ -358,6 +351,9 @@ static int __videobuf_mmap_setup(struct 
 		switch (memory) {
 		case V4L2_MEMORY_MMAP:
 			q->bufs[i]->boff  = bsize * i;
+			err = q->int_ops->mmap_setup(q, q->bufs[i]);
+			if (err)
+				break;
 			break;
 		case V4L2_MEMORY_USERPTR:
 		case V4L2_MEMORY_OVERLAY:
@@ -1018,7 +1014,6 @@ int videobuf_mmap_mapper(struct videobuf
 
 	mutex_lock(&q->vb_lock);
 	retval = CALL(q, mmap_mapper, q, vma);
-	q->is_mmapped = 1;
 	mutex_unlock(&q->vb_lock);
 
 	return retval;
Index: v4l-dvb/linux/drivers/media/video/videobuf-vmalloc.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-vmalloc.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-vmalloc.c
@@ -152,21 +152,26 @@ static int __videobuf_iolock (struct vid
 				(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;
+	return 0;
+}
+
+static int __videobuf_mmap_setup(struct videobuf_queue *q,
+			      struct videobuf_buffer *vb)
+{
+	int retval = 0;
+	BUG_ON(vb->memory != V4L2_MEMORY_MMAP);
+	if (vb->state == VIDEOBUF_NEEDS_INIT) {
+		/* bsize == size since the buffer needs to be large enough to
+		 * hold an entire frame, not the case in the read case for
+		 * example*/
+		vb->size = vb->bsize;
+		retval = __videobuf_iolock(q, vb, NULL);
+		if (!retval) {
+			/* Don't IOLOCK later */
+			vb->state = VIDEOBUF_IDLE;
 		}
 	}
-
-	return 0;
+	return retval;
 }
 
 static int __videobuf_sync(struct videobuf_queue *q,
@@ -239,15 +244,8 @@ static int __videobuf_mmap_mapper(struct
 	/* 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));
+		dprintk(1, "mmap: failed to remap_vmalloc_range\n");
+		return -EINVAL;
 	}
 
 	dprintk(1,"mmap %p: q=%p %08lx-%08lx (%lx) pgoff %08lx buf %d\n",
@@ -315,6 +313,7 @@ static struct videobuf_qtype_ops qops = 
 	.alloc        = __videobuf_alloc,
 	.iolock       = __videobuf_iolock,
 	.sync         = __videobuf_sync,
+	.mmap_setup   = __videobuf_mmap_setup,
 	.mmap_free    = __videobuf_mmap_free,
 	.mmap_mapper  = __videobuf_mmap_mapper,
 	.video_copy_to_user = __videobuf_copy_to_user,

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 7/9] vivi: Simplify the vivi driver and avoid deadlocks

[brandon]

[-- Attachment #1: simplified-vivi.patch --]
[-- Type: text/plain, Size: 15878 bytes --]

vivi previously had a very complex queuing system and held spinlocks while
doing copy_to_user, kmalloc, etc.  This caused the driver to easily deadlock
when a multi-threaded application used it and revealed bugs in videobuf too.

This replaces the copy_to_user with memcpy since we were never copying to user
space addresses.  And makes the kmalloc atomic.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/vivi.c |  318 ++++++++++-----------------------------
 1 file changed, 82 insertions(+), 236 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/vivi.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/vivi.c
+++ v4l-dvb/linux/drivers/media/video/vivi.c
@@ -5,6 +5,7 @@
  *      Mauro Carvalho Chehab <mchehab--a.t--infradead.org>
  *      Ted Walther <ted--a.t--enumera.com>
  *      John Sokol <sokol--a.t--videotechnology.com>
+ *      Brandon Philips <brandon@ifup.org>
  *      http://v4l.videotechnology.com/
  *
  * This program is free software; you can redistribute it and/or modify
@@ -155,8 +156,6 @@ struct vivi_buffer {
 
 struct vivi_dmaqueue {
 	struct list_head       active;
-	struct list_head       queued;
-	struct timer_list      timeout;
 
 	/* thread for generating video stream*/
 	struct task_struct         *kthread;
@@ -175,11 +174,6 @@ static LIST_HEAD(vivi_devlist);
 struct vivi_dev {
 	struct list_head           vivi_devlist;
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 16)
-	struct mutex               lock;
-#else
-	struct semaphore           lock;
-#endif
 	spinlock_t                 slock;
 
 	int                        users;
@@ -339,24 +333,26 @@ static void gen_line(char *basep, int in
 end:
 	return;
 }
+
 static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
 {
 	int h , pos = 0;
 	int hmax  = buf->vb.height;
 	int wmax  = buf->vb.width;
 	struct timeval ts;
-	char *tmpbuf = kmalloc(wmax * 2, GFP_KERNEL);
+	char *tmpbuf = kmalloc(wmax * 2, GFP_ATOMIC);
 	void *vbuf = videobuf_to_vmalloc(&buf->vb);
 
 	if (!tmpbuf)
 		return;
 
+	if (!vbuf)
+		return;
+
 	for (h = 0; h < hmax; h++) {
 		gen_line(tmpbuf, 0, wmax, hmax, h, dev->mv_count,
 			 dev->timestr);
-		/* FIXME: replacing to __copy_to_user */
-		if (copy_to_user(vbuf + pos, tmpbuf, wmax * 2) != 0)
-			dprintk(dev, 2, "vivifill copy_to_user failed.\n");
+		memcpy(vbuf + pos, tmpbuf, wmax * 2);
 		pos += wmax*2;
 	}
 
@@ -389,67 +385,58 @@ static void vivi_fillbuff(struct vivi_de
 			dev->timestr, (unsigned long)tmpbuf, pos);
 
 	/* Advice that buffer was filled */
-	buf->vb.state = VIDEOBUF_DONE;
 	buf->vb.field_count++;
 	do_gettimeofday(&ts);
 	buf->vb.ts = ts;
-
-	list_del(&buf->vb.queue);
-	wake_up(&buf->vb.done);
+	buf->vb.state = VIDEOBUF_DONE;
 }
 
-static int restart_video_queue(struct vivi_dmaqueue *dma_q);
-
-static void vivi_thread_tick(struct vivi_dmaqueue  *dma_q)
+static void vivi_thread_tick(struct vivi_fh *fh)
 {
-	struct vivi_buffer    *buf;
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
+	struct vivi_buffer *buf;
+	struct vivi_dev *dev = fh->dev;
+	struct vivi_dmaqueue *dma_q = &dev->vidq;
 
-	int bc;
+	unsigned long flags = 0;
 
-	spin_lock(&dev->slock);
-	/* Announces videobuf that all went ok */
-	for (bc = 0;; bc++) {
-		if (list_empty(&dma_q->active)) {
-			dprintk(dev, 1, "No active queue to serve\n");
-			break;
-		}
+	dprintk(dev, 1, "Thread tick\n");
 
-		buf = list_entry(dma_q->active.next,
-				 struct vivi_buffer, vb.queue);
+	spin_lock_irqsave(&dev->slock, flags);
+	if (list_empty(&dma_q->active)) {
+		dprintk(dev, 1, "No active queue to serve\n");
+		goto unlock;
+	}
 
-		/* Nobody is waiting something to be done, just return */
-		if (!waitqueue_active(&buf->vb.done)) {
-			mod_timer(&dma_q->timeout, jiffies+BUFFER_TIMEOUT);
-			spin_unlock(&dev->slock);
-			return;
-		}
+	buf = list_entry(dma_q->active.next,
+			 struct vivi_buffer, vb.queue);
 
-		do_gettimeofday(&buf->vb.ts);
-		dprintk(dev, 2, "[%p/%d] wakeup\n", buf, buf->vb. i);
+	/* Nobody is waiting on this buffer, return */
+	if (!waitqueue_active(&buf->vb.done))
+		goto unlock;
 
-		/* Fill buffer */
-		vivi_fillbuff(dev, buf);
+	list_del(&buf->vb.queue);
 
-		if (list_empty(&dma_q->active)) {
-			del_timer(&dma_q->timeout);
-		} else {
-			mod_timer(&dma_q->timeout, jiffies + BUFFER_TIMEOUT);
-		}
-	}
-	if (bc != 1)
-		dprintk(dev, 1, "%s: %d buffers handled (should be 1)\n",
-			__FUNCTION__, bc);
-	spin_unlock(&dev->slock);
+	do_gettimeofday(&buf->vb.ts);
+
+	/* Fill buffer */
+	vivi_fillbuff(dev, buf);
+	dprintk(dev, 1, "filled buffer %p\n", buf);
+
+	wake_up(&buf->vb.done);
+	dprintk(dev, 2, "[%p/%d] wakeup\n", buf, buf->vb. i);
+unlock:
+	spin_unlock_irqrestore(&dev->slock, flags);
+	return;
 }
 
 #define frames_to_ms(frames)					\
 	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
 
-static void vivi_sleep(struct vivi_dmaqueue  *dma_q)
+static void vivi_sleep(struct vivi_fh *fh)
 {
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
-	int timeout, running_time;
+	struct vivi_dev *dev = fh->dev;
+	struct vivi_dmaqueue *dma_q = &dev->vidq;
+	int timeout;
 	DECLARE_WAITQUEUE(wait, current);
 
 	dprintk(dev, 1, "%s dma_q=0x%08lx\n", __FUNCTION__,
@@ -464,37 +451,10 @@ static void vivi_sleep(struct vivi_dmaqu
 		goto stop_task;
 #endif
 
-	running_time = jiffies - dma_q->ini_jiffies;
-	dma_q->frame++;
-
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(dma_q->frame)) - running_time;
-
-	if (timeout > msecs_to_jiffies(frames_to_ms(2)) || timeout <= 0) {
-		int old = dma_q->frame;
-		int nframes;
-
-		dma_q->frame = (jiffies_to_msecs(running_time) /
-			       frames_to_ms(1)) + 1;
-
-		timeout = msecs_to_jiffies(frames_to_ms(dma_q->frame))
-			  - running_time;
-
-		if (unlikely (timeout <= 0))
-			timeout = 1;
+	timeout = msecs_to_jiffies(frames_to_ms(1));
 
-		nframes = (dma_q->frame > old)?
-				  dma_q->frame - old : old - dma_q->frame;
-
-		dprintk(dev, 1, "%ld: %s %d frames. "
-			"Current frame is %d. Will sleep for %d jiffies\n",
-			jiffies,
-			(dma_q->frame > old)? "Underrun, losed" : "Overrun of",
-			nframes, dma_q->frame, timeout);
-	} else
-		dprintk(dev, 1, "will sleep for %d jiffies\n", timeout);
-
-	vivi_thread_tick(dma_q);
+	vivi_thread_tick(fh);
 
 	schedule_timeout_interruptible(timeout);
 
@@ -505,8 +465,8 @@ stop_task:
 
 static int vivi_thread(void *data)
 {
-	struct vivi_dmaqueue  *dma_q = data;
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
+	struct vivi_fh  *fh = data;
+	struct vivi_dev *dev = fh->dev;
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 5, 0)
 	daemonize();
@@ -524,11 +484,10 @@ static int vivi_thread(void *data)
 #endif
 	dprintk(dev, 1, "thread started\n");
 
-	mod_timer(&dma_q->timeout, jiffies+BUFFER_TIMEOUT);
 	set_freezable();
 
 	for (;;) {
-		vivi_sleep(dma_q);
+		vivi_sleep(fh);
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 5, 0)
 		if (dma_q->rmmod || signal_pending(current))
@@ -547,9 +506,10 @@ static int vivi_thread(void *data)
 	return 0;
 }
 
-static int vivi_start_thread(struct vivi_dmaqueue  *dma_q)
+static int vivi_start_thread(struct vivi_fh *fh)
 {
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
+	struct vivi_dev *dev = fh->dev;
+	struct vivi_dmaqueue *dma_q = &dev->vidq;
 
 	dma_q->frame = 0;
 	dma_q->ini_jiffies = jiffies;
@@ -557,7 +517,7 @@ static int vivi_start_thread(struct vivi
 	dprintk(dev, 1, "%s\n", __FUNCTION__);
 
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 5, 0)
-	dma_q->kthread = kthread_run(vivi_thread, dma_q, "vivi");
+	dma_q->kthread = kthread_run(vivi_thread, fh, "vivi");
 
 	if (IS_ERR(dma_q->kthread)) {
 		printk(KERN_ERR "vivi: kernel_thread() failed\n");
@@ -570,7 +530,7 @@ static int vivi_start_thread(struct vivi
 	dma_q->notify = &sem;
 	dma_q->rmmod = 0;
 
-	if (kernel_thread(vivi_thread, dma_q, 0) < 0) {
+	if (kernel_thread(vivi_thread, fh, 0) < 0) {
 		printk(KERN_ERR "sdim: kernel_thread() failed\n");
 		return -EINVAL;
 	}
@@ -609,91 +569,6 @@ static void vivi_stop_thread(struct vivi
 	}
 }
 
-static int restart_video_queue(struct vivi_dmaqueue *dma_q)
-{
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
-	struct vivi_buffer *buf, *prev;
-
-	dprintk(dev, 1, "%s dma_q=0x%08lx\n", __FUNCTION__,
-		(unsigned long)dma_q);
-
-	if (!list_empty(&dma_q->active)) {
-		buf = list_entry(dma_q->active.next,
-				 struct vivi_buffer, vb.queue);
-		dprintk(dev, 2, "restart_queue [%p/%d]: restart dma\n",
-			buf, buf->vb.i);
-
-		dprintk(dev, 1, "Restarting video dma\n");
-		vivi_stop_thread(dma_q);
-
-		/* cancel all outstanding capture / vbi requests */
-		list_for_each_entry_safe(buf, prev, &dma_q->active, vb.queue) {
-			list_del(&buf->vb.queue);
-			buf->vb.state = VIDEOBUF_ERROR;
-			wake_up(&buf->vb.done);
-		}
-		mod_timer(&dma_q->timeout, jiffies+BUFFER_TIMEOUT);
-
-		return 0;
-	}
-
-	prev = NULL;
-	for (;;) {
-		if (list_empty(&dma_q->queued))
-			return 0;
-		buf = list_entry(dma_q->queued.next,
-				 struct vivi_buffer, vb.queue);
-		if (NULL == prev) {
-			list_del(&buf->vb.queue);
-			list_add_tail(&buf->vb.queue, &dma_q->active);
-
-			dprintk(dev, 1, "Restarting video dma\n");
-			vivi_stop_thread(dma_q);
-			vivi_start_thread(dma_q);
-
-			buf->vb.state = VIDEOBUF_ACTIVE;
-			mod_timer(&dma_q->timeout, jiffies+BUFFER_TIMEOUT);
-			dprintk(dev, 2,
-				"[%p/%d] restart_queue - first active\n",
-				buf, buf->vb.i);
-
-		} else if (prev->vb.width  == buf->vb.width  &&
-			   prev->vb.height == buf->vb.height &&
-			   prev->fmt       == buf->fmt) {
-			list_del(&buf->vb.queue);
-			list_add_tail(&buf->vb.queue, &dma_q->active);
-			buf->vb.state = VIDEOBUF_ACTIVE;
-			dprintk(dev, 2,
-				"[%p/%d] restart_queue - move to active\n",
-				buf, buf->vb.i);
-		} else {
-			return 0;
-		}
-		prev = buf;
-	}
-}
-
-static void vivi_vid_timeout(unsigned long data)
-{
-	struct vivi_dev      *dev  = (struct vivi_dev *)data;
-	struct vivi_dmaqueue *vidq = &dev->vidq;
-	struct vivi_buffer   *buf;
-
-	spin_lock(&dev->slock);
-
-	while (!list_empty(&vidq->active)) {
-		buf = list_entry(vidq->active.next,
-				 struct vivi_buffer, vb.queue);
-		list_del(&buf->vb.queue);
-		buf->vb.state = VIDEOBUF_ERROR;
-		wake_up(&buf->vb.done);
-		printk(KERN_INFO "vivi/0: [%p/%d] timeout\n", buf, buf->vb.i);
-	}
-	restart_video_queue(vidq);
-
-	spin_unlock(&dev->slock);
-}
-
 /* ------------------------------------------------------------------
 	Videobuf operations
    ------------------------------------------------------------------*/
@@ -722,13 +597,13 @@ static void free_buffer(struct videobuf_
 	struct vivi_fh  *fh = vq->priv_data;
 	struct vivi_dev *dev  = fh->dev;
 
-	dprintk(dev, 1, "%s\n", __FUNCTION__);
+	dprintk(dev, 1, "%s, state: %i\n", __FUNCTION__, buf->vb.state);
 
 	if (in_interrupt())
 		BUG();
 
-	videobuf_waiton(&buf->vb, 0, 0);
 	videobuf_vmalloc_free(&buf->vb);
+	dprintk(dev, 1, "free_buffer: freed");
 	buf->vb.state = VIDEOBUF_NEEDS_INIT;
 }
 
@@ -741,28 +616,25 @@ buffer_prepare(struct videobuf_queue *vq
 	struct vivi_fh     *fh  = vq->priv_data;
 	struct vivi_dev    *dev = fh->dev;
 	struct vivi_buffer *buf = container_of(vb, struct vivi_buffer, vb);
-	int rc, init_buffer = 0;
+	int rc;
 
 	dprintk(dev, 1, "%s, field=%d\n", __FUNCTION__, field);
 
 	BUG_ON(NULL == fh->fmt);
+
 	if (fh->width  < 48 || fh->width  > norm_maxw() ||
 	    fh->height < 32 || fh->height > norm_maxh())
 		return -EINVAL;
+
 	buf->vb.size = fh->width*fh->height*2;
 	if (0 != buf->vb.baddr  &&  buf->vb.bsize < buf->vb.size)
 		return -EINVAL;
 
-	if (buf->fmt       != fh->fmt    ||
-	    buf->vb.width  != fh->width  ||
-	    buf->vb.height != fh->height ||
-	buf->vb.field  != field) {
-		buf->fmt       = fh->fmt;
-		buf->vb.width  = fh->width;
-		buf->vb.height = fh->height;
-		buf->vb.field  = field;
-		init_buffer = 1;
-	}
+	/* These properties only change when queue is idle, see s_fmt */
+	buf->fmt       = fh->fmt;
+	buf->vb.width  = fh->width;
+	buf->vb.height = fh->height;
+	buf->vb.field  = field;
 
 	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
 		rc = videobuf_iolock(vq, &buf->vb, NULL);
@@ -785,45 +657,12 @@ buffer_queue(struct videobuf_queue *vq, 
 	struct vivi_buffer    *buf  = container_of(vb, struct vivi_buffer, vb);
 	struct vivi_fh        *fh   = vq->priv_data;
 	struct vivi_dev       *dev  = fh->dev;
-	struct vivi_dmaqueue  *vidq = &dev->vidq;
-	struct vivi_buffer    *prev;
+	struct vivi_dmaqueue *vidq = &dev->vidq;
 
-	if (!list_empty(&vidq->queued)) {
-		dprintk(dev, 1, "adding vb queue=0x%08lx\n",
-			(unsigned long)&buf->vb.queue);
-		list_add_tail(&buf->vb.queue, &vidq->queued);
-		buf->vb.state = VIDEOBUF_QUEUED;
-		dprintk(dev, 2, "[%p/%d] buffer_queue - append to queued\n",
-			buf, buf->vb.i);
-	} else if (list_empty(&vidq->active)) {
-		list_add_tail(&buf->vb.queue, &vidq->active);
-
-		buf->vb.state = VIDEOBUF_ACTIVE;
-		mod_timer(&vidq->timeout, jiffies+BUFFER_TIMEOUT);
-		dprintk(dev, 2, "[%p/%d] buffer_queue - first active\n",
-			buf, buf->vb.i);
-
-		vivi_start_thread(vidq);
-	} else {
-		prev = list_entry(vidq->active.prev,
-				  struct vivi_buffer, vb.queue);
-		if (prev->vb.width  == buf->vb.width  &&
-		    prev->vb.height == buf->vb.height &&
-		    prev->fmt       == buf->fmt) {
-			list_add_tail(&buf->vb.queue, &vidq->active);
-			buf->vb.state = VIDEOBUF_ACTIVE;
-			dprintk(dev, 2,
-				"[%p/%d] buffer_queue - append to active\n",
-				buf, buf->vb.i);
-
-		} else {
-			list_add_tail(&buf->vb.queue, &vidq->queued);
-			buf->vb.state = VIDEOBUF_QUEUED;
-			dprintk(dev, 2,
-				"[%p/%d] buffer_queue - first queued\n",
-				buf, buf->vb.i);
-		}
-	}
+	dprintk(dev, 1, "%s\n", __FUNCTION__);
+
+	buf->vb.state = VIDEOBUF_QUEUED;
+	list_add_tail(&buf->vb.queue, &vidq->active);
 }
 
 static void buffer_release(struct videobuf_queue *vq,
@@ -832,12 +671,9 @@ static void buffer_release(struct videob
 	struct vivi_buffer   *buf  = container_of(vb, struct vivi_buffer, vb);
 	struct vivi_fh       *fh   = vq->priv_data;
 	struct vivi_dev      *dev  = (struct vivi_dev *)fh->dev;
-	struct vivi_dmaqueue *vidq = &dev->vidq;
 
 	dprintk(dev, 1, "%s\n", __FUNCTION__);
 
-	vivi_stop_thread(vidq);
-
 	free_buffer(vq, buf);
 }
 
@@ -943,17 +779,31 @@ static int vidioc_s_fmt_cap(struct file 
 					struct v4l2_format *f)
 {
 	struct vivi_fh  *fh = priv;
+	struct videobuf_queue *q = &fh->vb_vidq;
+
 	int ret = vidioc_try_fmt_cap(file, fh, f);
 	if (ret < 0)
 		return (ret);
 
+	mutex_lock(&q->vb_lock);
+
+	if (videobuf_queue_is_busy(&fh->vb_vidq)) {
+		dprintk(fh->dev, 1, "%s queue busy\n", __FUNCTION__);
+		ret = -EBUSY;
+		goto out;
+	}
+
 	fh->fmt           = &format;
 	fh->width         = f->fmt.pix.width;
 	fh->height        = f->fmt.pix.height;
 	fh->vb_vidq.field = f->fmt.pix.field;
 	fh->type          = f->type;
 
-	return (0);
+	ret = 0;
+out:
+	mutex_unlock(&q->vb_lock);
+
+	return (ret);
 }
 
 static int vidioc_reqbufs(struct file *file, void *priv,
@@ -1158,6 +1008,8 @@ found:
 			NULL, &dev->slock, fh->type, V4L2_FIELD_INTERLACED,
 			sizeof(struct vivi_buffer), fh);
 
+	vivi_start_thread(fh);
+
 	return 0;
 }
 
@@ -1310,17 +1162,11 @@ static int __init vivi_init(void)
 
 		/* init video dma queues */
 		INIT_LIST_HEAD(&dev->vidq.active);
-		INIT_LIST_HEAD(&dev->vidq.queued);
 		init_waitqueue_head(&dev->vidq.wq);
 
 		/* initialize locks */
-		mutex_init(&dev->lock);
 		spin_lock_init(&dev->slock);
 
-		dev->vidq.timeout.function = vivi_vid_timeout;
-		dev->vidq.timeout.data     = (unsigned long)dev;
-		init_timer(&dev->vidq.timeout);
-
 		vfd = video_device_alloc();
 		if (NULL == vfd)
 			break;

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 8/9] videobuf: Avoid deadlock with QBUF and bring up to spec for empty queue

[brandon]

[-- Attachment #1: videobuf-add-waitqueue-for-dqbuf.patch --]
[-- Type: text/plain, Size: 5603 bytes --]

Add a waitqueue to wait on when there are no buffers in the buffer queue.
DQBUF waits on this queue without holding vb_lock to allow a QBUF to happen.
Once a buffer has been queued we recheck that the queue is still streaming and
wait on the new buffer's waitqueue while holding the vb_lock.  The driver
should come along in a timely manner and put the buffer into its next state
finishing the DQBUF.

By implementing this waitqueue it also brings the videobuf DQBUF up to spec and
it now blocks on O_NONBLOCK even when no buffers have been queued via QBUF:

"By default VIDIOC_DQBUF blocks when no buffer is in the outgoing queue." 
 - V4L2 spec

CC: Trent Piepho <xyzzy@speakeasy.org>
CC: Carl Karsten <carl@personnelware.com>
CC: Jonathan Corbet <corbet@lwn.net>

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-core.c |   96 +++++++++++++++++++++++-------
 linux/include/media/videobuf-core.h       |    2 
 2 files changed, 78 insertions(+), 20 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/videobuf-core.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-core.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-core.c
@@ -140,6 +140,7 @@ void videobuf_queue_core_init(struct vid
 	BUG_ON(!q->int_ops);
 
 	mutex_init(&q->vb_lock);
+	init_waitqueue_head(&q->wait);
 	INIT_LIST_HEAD(&q->stream);
 }
 
@@ -187,6 +188,10 @@ void videobuf_queue_cancel(struct videob
 	unsigned long flags = 0;
 	int i;
 
+	q->streaming = 0;
+	q->reading  = 0;
+	wake_up_all(&q->wait);
+
 	/* remove queued buffers from list */
 	spin_lock_irqsave(q->irqlock, flags);
 	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
@@ -561,6 +566,7 @@ int videobuf_qbuf(struct videobuf_queue 
 	}
 	dprintk(1, "qbuf: succeded\n");
 	retval = 0;
+	wake_up(&q->wait);
 
  done:
 	mutex_unlock(&q->vb_lock);
@@ -571,37 +577,88 @@ int videobuf_qbuf(struct videobuf_queue 
 	return retval;
 }
 
-int videobuf_dqbuf(struct videobuf_queue *q,
-	       struct v4l2_buffer *b, int nonblocking)
+
+/* Locking: Caller holds q->vb_lock */
+static int stream_next_buffer_check_queue(struct videobuf_queue *q, int noblock)
 {
-	struct videobuf_buffer *buf;
 	int retval;
 
-	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
-
-	mutex_lock(&q->vb_lock);
-	retval = -EBUSY;
-	if (q->reading) {
-		dprintk(1, "dqbuf: Reading running...\n");
-		goto done;
-	}
-	retval = -EINVAL;
-	if (b->type != q->type) {
-		dprintk(1, "dqbuf: Wrong type.\n");
+checks:
+	if (!q->streaming) {
+		dprintk(1, "next_buffer: Not streaming\n");
+		retval = -EINVAL;
 		goto done;
 	}
+
 	if (list_empty(&q->stream)) {
-		dprintk(1, "dqbuf: stream running\n");
-		goto done;
+		if (noblock) {
+			retval = -EAGAIN;
+			dprintk(2, "next_buffer: no buffers to dequeue\n");
+			goto done;
+		} else {
+			dprintk(2, "next_buffer: waiting on buffer\n");
+
+			/* Drop lock to avoid deadlock with qbuf */
+			mutex_unlock(&q->vb_lock);
+
+			/* Checking list_empty and streaming is safe without
+			 * locks because we goto checks to validate while
+			 * holding locks before proceeding */
+			retval = wait_event_interruptible(q->wait,
+				!list_empty(&q->stream) || !q->streaming);
+			mutex_lock(&q->vb_lock);
+
+			if (retval)
+				goto done;
+
+			goto checks;
+		}
 	}
+
+	retval = 0;
+
+done:
+	return retval;
+}
+
+
+/* Locking: Caller holds q->vb_lock */
+static int stream_next_buffer(struct videobuf_queue *q,
+			struct videobuf_buffer **vb, int nonblocking)
+{
+	int retval;
+	struct videobuf_buffer *buf = NULL;
+
+	retval = stream_next_buffer_check_queue(q, nonblocking);
+	if (retval)
+		goto done;
+
 	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
-	mutex_unlock(&q->vb_lock);
 	retval = videobuf_waiton(buf, nonblocking, 1);
+	if (retval < 0)
+		goto done;
+
+	*vb = buf;
+done:
+	return retval;
+}
+
+int videobuf_dqbuf(struct videobuf_queue *q,
+	       struct v4l2_buffer *b, int nonblocking)
+{
+	struct videobuf_buffer *buf = NULL;
+	int retval;
+
+	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
+
 	mutex_lock(&q->vb_lock);
+
+	retval = stream_next_buffer(q, &buf, nonblocking);
 	if (retval < 0) {
-		dprintk(1, "dqbuf: waiton returned %d\n", retval);
+		dprintk(1, "dqbuf: next_buffer error: %i\n", retval);
 		goto done;
 	}
+
 	switch (buf->state) {
 	case VIDEOBUF_ERROR:
 		dprintk(1, "dqbuf: state is error\n");
@@ -648,6 +705,7 @@ int videobuf_streamon(struct videobuf_qu
 			q->ops->buf_queue(q, buf);
 	spin_unlock_irqrestore(q->irqlock, flags);
 
+	wake_up(&q->wait);
  done:
 	mutex_unlock(&q->vb_lock);
 	return retval;
@@ -660,7 +718,6 @@ static int __videobuf_streamoff(struct v
 		return -EINVAL;
 
 	videobuf_queue_cancel(q);
-	q->streaming = 0;
 
 	return 0;
 }
@@ -858,7 +915,6 @@ static void __videobuf_read_stop(struct 
 		q->bufs[i] = NULL;
 	}
 	q->read_buf = NULL;
-	q->reading  = 0;
 
 }
 
Index: v4l-dvb/linux/include/media/videobuf-core.h
===================================================================
--- v4l-dvb.orig/linux/include/media/videobuf-core.h
+++ v4l-dvb/linux/include/media/videobuf-core.h
@@ -159,6 +159,8 @@ struct videobuf_queue {
 	spinlock_t                 *irqlock;
 	struct device		   *dev;
 
+	wait_queue_head_t	   wait; /* wait if queue is empty */
+
 	enum v4l2_buf_type         type;
 	unsigned int               inputs; /* for V4L2_BUF_FLAG_INPUT */
 	unsigned int               msize;

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[patch 9/9] videobuf-dma-sg.c: Avoid NULL dereference and add comment about backwards compatibility

[brandon]

[-- Attachment #1: videobuf-dma-sg-comments-and-avoid-NULL-deref.patch --]
[-- Type: text/plain, Size: 1625 bytes --]

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 linux/drivers/media/video/videobuf-dma-sg.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Index: v4l-dvb/linux/drivers/media/video/videobuf-dma-sg.c
===================================================================
--- v4l-dvb.orig/linux/drivers/media/video/videobuf-dma-sg.c
+++ v4l-dvb/linux/drivers/media/video/videobuf-dma-sg.c
@@ -592,6 +592,14 @@ static int __videobuf_mmap_mapper(struct
 		goto done;
 	}
 
+	/* This function maintains backwards compatibility with V4L1 and will
+	 * map more than one buffer if the vma length is equal to the combined
+	 * size of multiple buffers than it will map them together.  See
+	 * VIDIOCGMBUF in the v4l spec
+	 *
+	 * TODO: Allow drivers to specify if they support this mode
+	 */
+
 	/* look for first buffer to map */
 	for (first = 0; first < VIDEO_MAX_FRAME; first++) {
 		if (NULL == q->bufs[first])
@@ -636,10 +644,16 @@ static int __videobuf_mmap_mapper(struct
 	map = kmalloc(sizeof(struct videobuf_mapping),GFP_KERNEL);
 	if (NULL == map)
 		goto done;
-	for (size = 0, i = first; i <= last; size += q->bufs[i++]->bsize) {
+
+	size = 0;
+	for (i = first; i <= last; i++) {
+		if (NULL == q->bufs[i])
+			continue;
 		q->bufs[i]->map   = map;
 		q->bufs[i]->baddr = vma->vm_start + size;
+		size += q->bufs[i]->bsize;
 	}
+
 	map->count    = 1;
 	map->start    = vma->vm_start;
 	map->end      = vma->vm_end;

-- 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list