videobuf fixes

Hello-

Sorry for the mess-up on the first round. :-(

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.

Pullable from http://ifup.org/hg/v4l-dvb

- soc_camera: Introduce a spinlock for use with videobuf
- videobuf: Require spinlocks for all videobuf users
- videobuf: Wakeup queues after changing the state to ERROR
- videobuf: Simplify videobuf_waiton logic and possibly avoid missed wakeup
- videobuf-vmalloc.c: Remove buf_release from videobuf_vm_close
- videobuf-vmalloc.c: Fix hack of postponing mmap on remap failure
- vivi: Simplify the vivi driver and avoid deadlocks
- videobuf: Avoid deadlock with QBUF and bring up to spec for empty queue
- videobuf-dma-sg.c: Avoid NULL dereference and add comment about backwards compatibility

 b/linux/drivers/media/video/pxa_camera.c       |   11 
 b/linux/drivers/media/video/soc_camera.c       |    5 
 b/linux/drivers/media/video/videobuf-core.c    |   46 -
 b/linux/drivers/media/video/videobuf-dma-sg.c  |   16 
 b/linux/drivers/media/video/videobuf-vmalloc.c |    2 
 b/linux/drivers/media/video/vivi.c             |  332 ++--------
 b/linux/include/media/soc_camera.h             |    1 
 b/linux/include/media/videobuf-core.h          |    3 
 linux/drivers/media/video/videobuf-core.c      |  169 +++--
 linux/drivers/media/video/videobuf-vmalloc.c   |   43 -
 linux/include/media/videobuf-core.h            |    2 
 11 files changed, 262 insertions(+), 368 deletions(-)

Thanks,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699276 25200
# Node ID 7876c2bc2446dc3e3630e7a30a76f50874116cf1 # Parent 1df4f66ca1fc98ccad4c8377484a56adaf23e49d
soc_camera: Introduce a spinlock for use with videobuf

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

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

@@ -108,8 +108,6 @@ struct pxa_camera_dev {
 	unsigned long		platform_mclk_10khz;
 
 	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.

@@ -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);

@@ -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 */

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

I have a couple of questions to this one.

Good- I obviously had no way of testing and just took a shot :D

Oops, yes it should only happen once per allocation.

This is what should happen. The spinlock has to protect the struct videobuf_buffer.queue and should also be held throughout the interrupt handler as you manipulate the buffer.

What do you mean by per IRQ thread?

Yes, I think 2 is correct.

I think so.

I don't know enough about the hardware to comment.

Thank You,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

All drivers should provide a spinlock to be used in videobuf operations.

---

How about this one? Mauro, note: it should be applied after the previous patch, introducing soc_camera_host_ops.

index 9758f7e..bef3c9c 100644
@@ -803,6 +803,15 @@ static int pxa_camera_querycap(struct soc_camera_host *ici,
 	return 0;
 }
 
+static spinlock_t *pxa_camera_spinlock_alloc(struct soc_camera_file *icf)
+{
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icf->icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+
+	return &pcdev->lock;
+}
+
 static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.owner		= THIS_MODULE,
 	.add		= pxa_camera_add_device,

@@ -814,6 +823,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.querycap	= pxa_camera_querycap,
 	.try_bus_param	= pxa_camera_try_bus_param,
 	.set_bus_param	= pxa_camera_set_bus_param,
+	.spinlock_alloc	= pxa_camera_spinlock_alloc,
 };
 
 /* Should be allocated dynamically too, but we have only one. */

index 80e1193..6a8c8be 100644
@@ -48,6 +48,7 @@ struct soc_camera_device {
 struct soc_camera_file {
 	struct soc_camera_device *icd;
 	struct videobuf_queue vb_vidq;
+	spinlock_t *lock;
 };
 
 struct soc_camera_host {

@@ -73,6 +74,8 @@ struct soc_camera_host_ops {
 	int (*try_bus_param)(struct soc_camera_device *, __u32);
 	int (*set_bus_param)(struct soc_camera_device *, __u32);
 	unsigned int (*poll)(struct file *, poll_table *);
+	spinlock_t* (*spinlock_alloc)(struct soc_camera_file *);
+	void (*spinlock_free)(spinlock_t *);
 };
 
 struct soc_camera_link {

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

This looks OK. Thanks.

# HG changeset patch
# User Brandon Philips <[email protected]> # Date 1206699277 25200
# Node ID d9780aaf14ad2fca7eeaa79f3a8476e5f551ed25 # Parent 7876c2bc2446dc3e3630e7a30a76f50874116cf1
videobuf: Require spinlocks for all videobuf users

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.

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

@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699278 25200
# Node ID 3ac2b9752844e2635575e50594d50cea665ca09b
# Parent 05ec6ba3e9c3c42cf397ca71b8aefe9573d28c6a
videobuf: Simplify videobuf_waiton logic and possibly avoid missed wakeup

Possible missed wakeup- use kernel helpers for wait queues
http://www.mail-archive.com/[email protected]/msg27983.html

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

@@ -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);
 
-	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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699277 25200
# Node ID 05ec6ba3e9c3c42cf397ca71b8aefe9573d28c6a # Parent d9780aaf14ad2fca7eeaa79f3a8476e5f551ed25
videobuf: Wakeup queues after changing the state to ERROR

The waitqueues must be woken up every time state changes.

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

@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]> # Date 1206699281 25200
# Node ID 304e0a371d12f77e1575ae43fa0133855165f63e # Parent ab74ebf10c01d6a8a54a39b6750bc86ec3e72286
vivi: Simplify the vivi driver and avoid deadlocks

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.

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

@@ -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 <[email protected]>
  *      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 @@ struct vivi_dev {
 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 @@ end:
 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;
+	buf->vb.state = VIDEOBUF_DONE;
+}
+
+static void vivi_thread_tick(struct vivi_fh *fh)
+{
+	struct vivi_buffer *buf;
+	struct vivi_dev *dev = fh->dev;
+	struct vivi_dmaqueue *dma_q = &dev->vidq;
+
+	unsigned long flags = 0;
+
+	dprintk(dev, 1, "Thread tick\n");
+
+	spin_lock_irqsave(&dev->slock, flags);
+	if (list_empty(&dma_q->active)) {
+		dprintk(dev, 1, "No active queue to serve\n");
+		goto unlock;
+	}
+
+	buf = list_entry(dma_q->active.next,
+			 struct vivi_buffer, vb.queue);
+
+	/* Nobody is waiting on this buffer, return */
+	if (!waitqueue_active(&buf->vb.done))
+		goto unlock;
 
 	list_del(&buf->vb.queue);
+
+	do_gettimeofday(&buf->vb.ts);
+
+	/* Fill buffer */
+	vivi_fillbuff(dev, buf);
+	dprintk(dev, 1, "filled buffer %p\n", buf);
+
 	wake_up(&buf->vb.done);
-}
-
-static int restart_video_queue(struct vivi_dmaqueue *dma_q);
-
-static void vivi_thread_tick(struct vivi_dmaqueue  *dma_q)
-{
-	struct vivi_buffer    *buf;
-	struct vivi_dev *dev = container_of(dma_q, struct vivi_dev, vidq);
-
-	int bc;
-
-	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;
-		}
-
-		buf = list_entry(dma_q->active.next,
-				 struct vivi_buffer, vb.queue);
-
-		/* 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;
-		}
-
-		do_gettimeofday(&buf->vb.ts);
-		dprintk(dev, 2, "[%p/%d] wakeup\n", buf, buf->vb. i);
-
-		/* Fill buffer */
-		vivi_fillbuff(dev, buf);
-
-		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);
+	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(1));
 
-	/* 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;
-
-		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);
+	dprintk(dev, 1, "%s\n", __FUNCTION__);
 
-		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);
-		}
-	}
+	buf->vb.state = VIDEOBUF_QUEUED;
+	list_add_tail(&buf->vb.queue, &vidq->active);
 }
 
 static void buffer_release(struct videobuf_queue *vq,

@@ -832,11 +671,8 @@ 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,9 +779,19 @@ 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;

@@ -953,7 +799,11 @@ static int vidioc_s_fmt_cap(struct file 
 	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,16 +1162,10 @@ 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)

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi Brandon,

I'll try to test the patch series. They seems fine to my eyes, on a first look. I have just some comments about patch 7/9.

The only current driver at the tree using videobuf-vmalloc is vivi. There's another driver using it at tm6000 driver, not merged yet [1].

...

While the restart and timeout code is not needed on vivi driver, IMO, we should keep it, since the main reason for this driver is to be a reference code.

This kind of code is important on real drivers, since the IRQ's may not be called for some reason. On cx88 and on saa7134, this happens on several situations[2].

Without a timeout, the driver will wait forever to receive a buffer.

This task is also needed by tm6000 driver, for the same reasons.

[1] Available at: http://linuxtv.org/hg/~mchehab/tm6010

[2] For example, I suffered an issue yesterday with my machine, that I believe to be caused by an excess of power consumption. The effect is that cx88 weren't generating DMA interrupts, if I loaded my machine with 3 pci boards. The removal of one board made the cx88 board to work again. I'll test today again with a newer power supply. Without the timeout code, the player would just hang, waiting forever for some data at the video buffer.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Ok, this is fixed in the patch I just sent. I didn't add the "changelog" entry below the copyright because the git-log will show what I did.

I couldn't figure out how it worked well enough to fix it. In
particular code similar to the following is copied around throughout several drivers and I have no idea what it does:

@@ -785,45 +657,12 @@ buffer_queue(struct videobuf_queue *vq,
- 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);
- }
- }

What is the difference between VIDEOBUF_ACTIVE and VIDEOBUF_QUEUED?

Yes, I agree. But, as I said I couldn't figure it out.

Well, yes on real hardware. But, in the case of vivi we can just create the frames as they are needed. It is dead simple for a fake device :D

Huh? What task?

Thanks,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Ok.

I prefer if you do so. I had to much stress in the past due to those copyright messages. I very much prefer to have a very short description, when newer copyrights are added. This avoids later senseless discussions.

I think we can later try to simplify the state machine. From what I understood, VIDEOBUF_ACTIVE is used to indicate that a driver started, but hasn't yet received anything.

About the same logic used on vivi is present also on bttv, cx88 and saa7134.

I mean the watchdog task. If the device stops sending streams for more than a certain amount of time [1], a task is wake. This task unblocks the userspace app (this returns an error to userspace), and tries to restart the streaming.

[1] Since a TV device is expected to receive 25 to 30 frames/sec, In thesis, you should have a frame on each 33 ms or 40ms. The watchdog is configured to a higher value (for example, 500ms), since, on real devices, some frames could be lost due to bad signal.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699280 25200
# Node ID ab74ebf10c01d6a8a54a39b6750bc86ec3e72286
# Parent eb99bdd0a7e3f70eb40fcc6918794a8b8822ac49 videobuf-vmalloc.c: Fix hack of postponing mmap on remap failure

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.

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

@@ -92,24 +92,16 @@ 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,

@@ -131,6 +123,9 @@ void videobuf_queue_core_init(struct vid
 	q->ops       = ops;
 	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);

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

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

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

With 6/9 or with this patch, the allocation will happen when you try to use an mmap command. This can happen on streamon, reqbufs, dqbuf, qbuf or reqbuf.

Sorry, but I think I missed your point.

Yes, it works.

To declare pages var on above code. without braces, a warning is generated.

Are you thinking on an userspace application explicitly calling munmap()?

Hmm... Interesting. Test attached.

Basically, this worked as expected.

Probably, in this case, we may have some troubles, due to vfree() if we get an interrupt between:

	vfree(mem->vmalloc)
		and
	mem->vmalloc=NULL.

A fix could be to disable IRQ during that interval of time, and/or protecting with a spinlock.

I can't see any other issues. Btw, the same kind of code is also used on videobuf-dma-sg, cafe-ccic, and several other USB drivers.

By looking on your code, it didn't work previously, due to a bug on this line:

	retval=remap_vmalloc_range(vma, mem->vmalloc,0);

This bug were present on the previous version, and were corrected by this patch.

Also, all userspace clients already do something like this, on buffer_prepare:

        if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
                rc = videobuf_iolock(vq, &buf->vb, NULL);
                if (rc < 0)
                        goto fail;
        }

Btw, I think that this flag should be set automatically by videobuf_vmalloc_free().

Cheers,
Mauro

---

I'm enclosing two files:

1) A diff file, against em28xx-vb tree, with some additional printk's and a patch for capture_example.c. The patch basically captures 5 frames, stops, unmaps, request buffers, and captures 5 more frames;

2) the dmesg output of running capture_example, with vivi.

All seemed to work fine.

I also tested this (increasing the second capture to 50 frames, in order to see more):

./capture_example -o | mplayer -demuxer 26 -rawvideo w=640:h=480:format=yuy2 -

Also worked perfectly.

diff -r 8ba4c2df2736 linux/drivers/media/video/videobuf-core.c
@@ -314,6 +314,9 @@ int videobuf_mmap_free(struct videobuf_q
 int videobuf_mmap_free(struct videobuf_queue *q)
 {
 	int ret;
+
+	dprintk(1, "%s called\n", __func__);
+
 	mutex_lock(&q->vb_lock);
 	ret = __videobuf_mmap_free(q);
 	mutex_unlock(&q->vb_lock);

@@ -327,6 +330,8 @@ static int __videobuf_mmap_setup(struct 
 {
 	unsigned int i;
 	int err;
+
+	dprintk(1, "%s called\n", __func__);
 
 	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
 

diff -r 8ba4c2df2736 v4l2-apps/test/capture_example.c
@@ -178,12 +178,8 @@ read_frame                      (void)
 }
 
 static void
-mainloop                        (void)
+mainloop                        (int count)
 {
-	unsigned int count;
-
-	count = 1000;
-
 	while (count-- > 0) {
 		for (;;) {
 			fd_set fds;

@@ -670,7 +666,17 @@ main                            (int    
 
 	start_capturing ();
 
-	mainloop ();
+	mainloop (5);
+
+	stop_capturing ();
+
+	uninit_device ();
+
+	init_device ();
+
+	start_capturing ();
+
+	mainloop (5);
 
 	stop_capturing ();
 

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

I'm still missed. I don't see what's the difference on what patch 6/9 were expecting to do and the proposed patch, in terms of adding a restriction of not accepting other mmap commands, without getting a REQBUFS.

The current way that videobuf works is the one that userspace apps expect. If we change videobuf behavior, for a given userspace API command, we risk to break some userspace app.

The tests I did covers the issue of unmapping/remapping. As we've already discussed, something else should be added to allow unmapping without streamoff.

The same kind of issue seems to apply also to videobuf-dma-sg. In fact, it is even worse with dma: since buffers will be filled without CPU, just disabling IRQ, or running a spinlock doesn't solve. You'll need to stop DMA before unmapping.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

One patch that worked is this one:

	http://linuxtv.org/hg/~mchehab/tm6010/rev/e1a2b9e33bd2

I tried first to use spinlock before vfree(), but kernel complained. I suspect that vfree() can sleep.

So, what the above patch does is to streamoff() before unmapping. This seems to be correct to my eyes, since there's no sense of keeping streamon() if the buffers are unmapped.

I didn't pushed it on em28xx yet, since I'm trying to fix another bug there.

I've started to write a patch to add the same functionality. Didn't finished yet. I'm not sure if it would be important to provide userspace mmapped IO. I suspect that most apps are using read() or kernel mmap() interfaces.

I suspect that the issue exists with DMA, although there's a sync routine there, that may help to avoid such issues. But I think that the same strategy of doing a streamoff before unmap should be used for kernel mmapped memory.

It should work. you can test it with "capture_example -u". Unfortunately, I can't test here right now, since the desktop I use here for testing PCI devices is burned. I'll need to save some money to buy another motherboard or to replace the machine.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Ah, ok. I don't see any technical issue of postponing vmalloc to be done on iolock (but see bellow). The code will just become a little bit more complex. On the other hand, I don't see any advantage, since vmalloc_user() doesn't really alloc memory, AFAIK.

If you don't call REQBUF, vb->memory and baddr will be 0, as if you were doing a read(). The end result is that QBUF won't work. The same behaviour will also occur with your patch.

I've already tested this, by accident. No OOPS. it just returns -EINVAL.

Ok, I'll change it on a next patch.

True. The lock is needed.

I'm not quite sure on how to properly protect videobuf-vmalloc and videobuf-dma-sg against this kind of threat. Any suggestions?

A lock for vmalloc would be easy, but the worse case seems to be dma, since a transfer may be in course, while unmap is happening. I couldn't find any place where this is handled on videobuf-dma-sg. I think that some drivers try to handle it, by calling videobuf_dma_sync(). Yet, this seems to be a feature that should be inside videobuf, not externally handled.

Before my patch, mem->vmalloc were equal to NULL.

That's why this were failing with em28xx: it needs FIRST to alloc something, THEN remap it.

If we move vmalloc to iolock, we'll need to move also remap_vmalloc_range to there.

Moving remap_vmalloc_range() to iolock will also work properly, but it doesn't seem to be right to use mmap() callback just to store some things to be handled inside iolock. That's the reason why I've opted to move just the vmalloc to __videobuf_mmap_mapper().

What were happening before my and your patches is that the above line (remap) were always failing for real devices. So, there was another remap_vmalloc_range() inside iolock. The last one were the one who were really working for tm6000 and em28xx.

I dunno why, but, for vivi, it seems that the first vmalloc were actually working.

I was meaning to say "videobuf clients".

What I tried to say is that you've added a similar code to this inside patch 6/9. It would be interesting if we could remove the above code from videobuf clients to be handled internally, and simplifying the logic inside the drivers, but this seems to be complicated, since buffer should be filled inside buffer_prepare.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Interesting. However, I don't see much advantages of doing that, especially if this behaviour will work only with dma-sg drivers. I think that the better is to require mmap() before QBUF at V4L2 API.

We have a bad experience with V4L1 apps that were abusing on relaxed checks at the API. Those apps (like vlc) first starts streaming, then changes the buffer size, by altering the image size. This bad behaviour is still not supported by V4L1 compat layer.

IMO, the V4L2 API should define the valid command order of ioctls for stream to work.

Yes.

Hmm.. I think I got your point. It should be cancelling only the affected buffer. I can't see any sense of continuing the stream for the cancelled buffer.

It seems sane.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699279 25200
# Node ID eb99bdd0a7e3f70eb40fcc6918794a8b8822ac49 # Parent 3ac2b9752844e2635575e50594d50cea665ca09b videobuf-vmalloc.c: Remove buf_release from videobuf_vm_close

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.

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

@@ -78,8 +78,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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699281 25200
# Node ID 0b7eea4e7b7dc24b1c015e5768fdb8f70f70c751
# Parent 304e0a371d12f77e1575ae43fa0133855165f63e
videobuf: Avoid deadlock with QBUF and bring up to spec for empty queue

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

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

@@ -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);
 }
 

@@ -186,6 +187,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);

@@ -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;
 }
 
+
+/* Locking: Caller holds q->vb_lock */
+static int stream_next_buffer_check_queue(struct videobuf_queue *q, int noblock)
+{
+	int retval;
+
+checks:
+	if (!q->streaming) {
+		dprintk(1, "next_buffer: Not streaming\n");
+		retval = -EINVAL;
+		goto done;
+	}
+
+	if (list_empty(&q->stream)) {
+		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);
+	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;
+	struct videobuf_buffer *buf = NULL;
 	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");
+
+	retval = stream_next_buffer(q, &buf, nonblocking);
+	if (retval < 0) {
+		dprintk(1, "dqbuf: next_buffer error: %i\n", retval);
 		goto done;
 	}
-	retval = -EINVAL;
-	if (b->type != q->type) {
-		dprintk(1, "dqbuf: Wrong type.\n");
-		goto done;
-	}
-	if (list_empty(&q->stream)) {
-		dprintk(1, "dqbuf: stream running\n");
-		goto done;
-	}
-	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
-	mutex_unlock(&q->vb_lock);
-	retval = videobuf_waiton(buf, nonblocking, 1);
-	mutex_lock(&q->vb_lock);
-	if (retval < 0) {
-		dprintk(1, "dqbuf: waiton returned %d\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;
 
 }
 

@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1206699283 25200
# Node ID 91e7d2afab2c180c435e5dc85b4cb749b1001e5a
# Parent 0b7eea4e7b7dc24b1c015e5768fdb8f70f70c751
videobuf-dma-sg.c: Avoid NULL dereference and add comment about backwards compatibility

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

@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hmm... it seems that there are still some troubles.

I've opened 3 mplayer windows, reading /dev/video0. I closed the second one and opened again. I got an error:

[  665.465701] vivi: open called (minor=0)
[  677.931020] vivi: open called (minor=0)
[  686.961359] vivi: open called (minor=0)
[  700.881962] vivi: open called (minor=0)
[  705.709878] general protection fault: 0000 [1] PREEMPT SMP
[  705.709885] CPU 0
[  705.709888] Modules linked in: vivi compat_ioctl32 videodev v4l1_compat videobuf_vmalloc videobuf_core arc4 ecb blkcipher cryptomgr crypto_algapi rtl8187 mac80211 cfg80211 eeprom_93cx6 vmnet(P) vmblock vmmon(P) af_packet snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device iptable_filter ip_tables x_tables binfmt_misc nls_cp437 vfat fat nls_cp850 ntfs nls_base pata_atiixp libata scsi_mod cpufreq_ondemand cpufreq_conservative cpufreq_powersave powernow_k8 tifm_7xx1 nvram cryptoloop loop mmc_block tifm_sd mmc_core tifm_core snd_pcm_oss pcmcia snd_mixer_oss snd_hda_intel snd_pcm snd_timer yenta_socket snd_page_alloc snd_hwdep rtc_cmos rsrc_nonstatic ohci1394 snd ide_cd rtc_core pcmcia_core ieee1394 k8temp cdrom i2c_piix4 rtc_lib hwmon pcspkr sky2 i2c_core thermal soundcore processor battery ac button bitrev crc32 evdev unix ide_generic ide_disk ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd usbcore atiixp ide_core
[  705.709967] Pid: 5583, comm: vivi Tainted: P        2.6.24.3-4mnbcustom #4
[  705.709970] RIP: 0010:[<ffffffff885536cc>]  [<ffffffff885536cc>] :vivi:vivi_thread+0xdc/0x760
[  705.709980] RSP: 0018:ffff810058039e30  EFLAGS: 00010246
[  705.709983] RAX: ffff81005968cc00 RBX: 01c0055b0007033b RCX: ffff81005953b800
[  705.709986] RDX: ffffffff88553652 RSI: 0000000000000001 RDI: 0000000000000001
[  705.709988] RBP: 0000000000096000 R08: 0000000000000002 R09: 0000000000000000
[  705.709991] R10: ffffffff80253d69 R11: 0000000000000001 R12: 0000000000000140
[  705.709994] R13: ffff810059649000 R14: ffff8100773f9c80 R15: 0000000000000140
[  705.709997] FS:  00002ac9a2490a00(0000) GS:ffffffff80547000(0000) knlGS:0000000000000000
[  705.710000] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[  705.710003] CR2: 000000000090bdd0 CR3: 0000000065d03000 CR4: 00000000000006e0
[  705.710005] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  705.710008] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  705.710011] Process vivi (pid: 5583, threadinfo ffff810058038000, task ffff81005968cc00)
[  705.710014] Stack:  0000000000000000 ffff810059649080 0000000000000140 0000014102b32e00
[  705.710021]  ffff8100773f9c90 ffff8100773f9d58 ffff81005953b800 ffff8100773f9c80
[  705.710027]  01c0055b00070393 0000000000000009 0000000000000282 ffff810068890c00
[  705.710032] Call Trace:
[  705.710052]  [<ffffffff80233eb0>] default_wake_function+0x0/0x10
[  705.710061]  [<ffffffff885535f0>] :vivi:vivi_thread+0x0/0x760
[  705.710068]  [<ffffffff885535f0>] :vivi:vivi_thread+0x0/0x760
[  705.710076]  [<ffffffff8025379b>] kthread+0x4b/0x80
[  705.710083]  [<ffffffff8020d428>] child_rip+0xa/0x12
[  705.710089]  [<ffffffff802366d7>] finish_task_switch+0x57/0xe0
[  705.710094]  [<ffffffff8020cb13>] restore_args+0x0/0x30
[  705.710098]  [<ffffffff80253897>] kthreadd+0xc7/0x150
[  705.710107]  [<ffffffff80253750>] kthread+0x0/0x80
[  705.710111]  [<ffffffff8020d41e>] child_rip+0x0/0x12
[  705.710116]
[  705.710117]
[  705.710117] Code: 48 8b 43 50 44 8b 98 8c 03 00 00 45 85 db 7e 1a 48 8b 54 24
[  705.710131] RIP  [<ffffffff885536cc>] :vivi:vivi_thread+0xdc/0x760
[  705.710136]  RSP <ffff810058039e30>
[  705.710142] ---[ end trace 4d468849a38ba2e0 ]---
[  710.814272] vivi: open called (minor=0)

Yet, all 3 windows are working fine.

I'm running here with all spinlock modules here.

After closing all modules, I got another error dump:

[  934.913481] general protection fault: 0000 [2] PREEMPT SMP
[  934.913489] CPU 0
[  934.913492] Modules linked in: vivi compat_ioctl32 videodev v4l1_compat videobuf_vmalloc videobuf_core arc4 ecb blkcipher cryptomgr crypto_algapi rtl8187 mac80211 cfg80211 eeprom_93cx6 vmnet(P) vmblock vmmon(P) af_packet snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device iptable_filter ip_tables x_tables binfmt_misc nls_cp437 vfat fat nls_cp850 ntfs nls_base pata_atiixp libata scsi_mod cpufreq_ondemand cpufreq_conservative cpufreq_powersave powernow_k8 tifm_7xx1 nvram cryptoloop loop mmc_block tifm_sd mmc_core tifm_core snd_pcm_oss pcmcia snd_mixer_oss snd_hda_intel snd_pcm snd_timer yenta_socket snd_page_alloc snd_hwdep rtc_cmos rsrc_nonstatic ohci1394 snd ide_cd rtc_core pcmcia_core ieee1394 k8temp cdrom i2c_piix4 rtc_lib hwmon pcspkr sky2 i2c_core thermal soundcore processor battery ac button bitrev crc32 evdev unix ide_generic ide_disk ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd usbcore atiixp ide_core
[  934.913573] Pid: 5566, comm: vivi Tainted: P      D 2.6.24.3-4mnbcustom #4
[  934.913576] RIP: 0010:[<ffffffff885536cc>]  [<ffffffff885536cc>] :vivi:vivi_thread+0xdc/0x760
[  934.913588] RSP: 0018:ffff810063eb1e30  EFLAGS: 00010246
[  934.913590] RAX: ffff8100580ceb40 RBX: 0420000222e20012 RCX: ffff8100637bf400
[  934.913593] RDX: ffffffff88553652 RSI: 0000000000000001 RDI: 0000000000000001
[  934.913596] RBP: ffff8100773f9cd8 R08: 0000000000000002 R09: 0000000000000000
[  934.913599] R10: ffffffff80253d69 R11: 0000000000000001 R12: 0000000000000140
[  934.913602] R13: ffff810059649800 R14: ffff8100773f9c80 R15: 0000000000000140
[  934.913605] FS:  00002ac4a8b4b730(0000) GS:ffffffff80547000(0000) knlGS:0000000000000000
[  934.913608] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[  934.913610] CR2: 00002b38780da350 CR3: 000000006f8d1000 CR4: 00000000000006e0
[  934.913613] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  934.913616] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  934.913619] Process vivi (pid: 5566, threadinfo ffff810063eb0000, task ffff8100580ceb40)
[  934.913621] Stack:  0000000000000000 ffff810059649880 0000000000000140 0000014102acae00
[  934.913629]  ffff8100773f9c90 ffff8100773f9d58 ffff8100637bf400 ffff8100773f9c80
[  934.913635]  0420000222e2006a 0000000000000009 0000000000000282 ffff810068890c00
[  934.913640] Call Trace:
[  934.913660]  [<ffffffff80233eb0>] default_wake_function+0x0/0x10
[  934.913669]  [<ffffffff885535f0>] :vivi:vivi_thread+0x0/0x760
[  934.913676]  [<ffffffff885535f0>] :vivi:vivi_thread+0x0/0x760
[  934.913685]  [<ffffffff8025379b>] kthread+0x4b/0x80
[  934.913692]  [<ffffffff8020d428>] child_rip+0xa/0x12
[  934.913698]  [<ffffffff802366d7>] finish_task_switch+0x57/0xe0
[  934.913704]  [<ffffffff8020cb13>] restore_args+0x0/0x30
[  934.913707]  [<ffffffff80253897>] kthreadd+0xc7/0x150
[  934.913715]  [<ffffffff80253750>] kthread+0x0/0x80
[  934.913720]  [<ffffffff8020d41e>] child_rip+0x0/0x12
[  934.913724]
[  934.913725]
[  934.913726] Code: 48 8b 43 50 44 8b 98 8c 03 00 00 45 85 db 7e 1a 48 8b 54 24
[  934.913739] RIP  [<ffffffff885536cc>] :vivi:vivi_thread+0xdc/0x760
[  934.913745]  RSP <ffff810063eb1e30>
[  934.913751] ---[ end trace 4d468849a38ba2e0 ]---

I suspect that the errors happened at the close().

I'm enclosing the full dmesg.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Shouldn't V4L2 devices not be able to stream to multiple applications at once? Quoting the spec:

"V4L2 drivers should not support multiple applications reading or writing the same data stream on a device by copying buffers, time multiplexing or similar means. This is better handled by a proxy application in user space. When the driver supports stream sharing anyway it must be implemented transparently. The V4L2 API does not specify how conflicts are solved."

How about this patch?

---
 linux/drivers/media/video/vivi.c |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Index: v4l-dvb/linux/drivers/media/video/vivi.c
===================================================================
@@ -5,9 +5,10 @@
  *      Mauro Carvalho Chehab <mchehab--a.t--infradead.org>
  *      Ted Walther <ted--a.t--enumera.com>
  *      John Sokol <sokol--a.t--videotechnology.com>
- *      Brandon Philips <[email protected]>
  *      http://v4l.videotechnology.com/
  *
+ * Copyright (c) 2008 by Brandon Philips <[email protected]>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the BSD Licence, GNU General Public License
  * as published by the Free Software Foundation; either version 2 of the

@@ -175,6 +176,7 @@ struct vivi_dev {
 	struct list_head           vivi_devlist;
 
 	spinlock_t                 slock;
+	mutex			   mutex;
 
 	int                        users;
 

@@ -960,6 +962,7 @@ static int vivi_open(struct inode *inode
 	struct vivi_dev *dev;
 	struct vivi_fh *fh;
 	int i;
+	int retval;
 
 	printk(KERN_DEBUG "vivi: open called (minor=%d)\n", minor);
 

@@ -969,9 +972,15 @@ static int vivi_open(struct inode *inode
 	return -ENODEV;
 
 found:
-	/* If more than one user, mutex should be added */
+	mutex_lock(&dev->mutex);
 	dev->users++;
 
+	if (dev->users > 1) {
+		dev->users--;
+		retval = -EBUSY;
+		goto unlock;
+	}
+
 	dprintk(dev, 1, "open minor=%d type=%s users=%d\n", minor,
 		v4l2_type_names[V4L2_BUF_TYPE_VIDEO_CAPTURE], dev->users);
 

@@ -979,8 +988,13 @@ found:
 	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
 	if (NULL == fh) {
 		dev->users--;
-		return -ENOMEM;
+		retval = -ENOMEM;
+		goto unlock;
 	}
+unlock:
+	if (retval)
+		return retval;
+	mutex_unlock(&dev->mutex);
 
 	file->private_data = fh;
 	fh->dev      = dev;

@@ -1054,7 +1068,9 @@ static int vivi_close(struct inode *inod
 
 	kfree(fh);
 
+	mutex_lock(&dev->mutex);
 	dev->users--;
+	mutex_unlock(&dev->mutex);
 
 	dprintk(dev, 1, "close called (minor=%d, users=%d)\n",
 		minor, dev->users);

@@ -1166,6 +1182,7 @@ static int __init vivi_init(void)
 
 		/* initialize locks */
 		spin_lock_init(&dev->slock);
+		INIT_MUTEX(&dev->mutex);
 
 		vfd = video_device_alloc();
 		if (NULL == vfd)

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

I'm not exactly on top of current driver development, so I apologize if this response if misdirected.

I disagree with the proposal.

In certain instances, it is desirable for more than one application at a time to read from a stream (provided both are receiving the exact same stream, i.e. without lost/duplicate frames). The case could be argued that one may want to use a command-line tool set like mjpegtools and ffmpeg to record a stream, while using xawtv to view the stream. In point of fact, I've done this myself and have found it incredibly handy.

I can see how it would be especially useful on machines with fewer resources than are needed for complex applications like mythtv.

Rather that lose a useful capability, why not just extend the spec to allow for it (and the potential error condition of course)? It's not as though all of the applications prior to this would suddenly stop working once you do.

"Life is full of happy and sad events.  If you take the time
to concentrate on the former, you'll get further in life."
Vanessa Ezekowitz  <[email protected]>

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

The patch is wrong.

V4L2 API states that it should be possible for a driver to have multiple opens[1]. Most drivers support this. There are two main usages for multiple open:

1) a driver may open the device for streaming, while another thread or userspace app will open to configure. This is generally the way I use here to test video controls: I open "qv4l2" (under v4l2-apps/util) while streaming. This way, I can test any changes at the driver, without needing to have the feature implemented at the userspace app;

2) you can see a program with an userspace app and record the stream with another app. Both xawtv and xdtv do this, when you ask for record: They call mencoder, asking it to read from /dev/video0. This way, you'll have the tv app reading, using mmap() or overlay methods, while mencoder is calling read() to receive the stream.

In fact, at least bttv, cx88 and saa7134 allows multiple opens for streaming. They have a limit, due to driver/hardware constraints: You cannot open two transfers of the same type. But you can open one overlay, one mmap() and one read() at the same time.

While I don't have much concerns of not allowing multiple streaming on vivi for the same device (since you can ask vivi to create several different virtual devices, by using "n_devs" modprobe parameter), for sure videobuf should keep supporting this feature, otherwise it will break the other drivers that relies on this feature. Yet, vivi should allow multiple open to allow "panel" applications, to be compliant with V4L2 API.

[1] From V4L2 API doc, rev 0.24:

"1.1.3. Multiple Opens

In general, V4L2 devices can be opened more than once. When this is supported by the driver, users can for example start a "panel" application to change controls like brightness or audio volume, while another application captures video and audio. In other words, panel applications are comparable to an OSS or ALSA audio mixer application. When a device supports multiple functions like capturing and overlay simultaneously, multiple opens allow concurrent use of the device by forked processes or specialized applications.

Multiple opens are optional, although drivers should permit at least concurrent accesses without data exchange, i. e. panel applications. This implies open() can return an EBUSY error code when the device is already in use, as well as ioctl() functions initiating data exchange (namely the VIDIOC_S_FMT ioctl), and the read() and write() functions."

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

The patch fixes the unsafe way vivi is doing multiple opens right now and I am uninterested in spending anymore time trying to fix up vivi right now. If you could fix vivi up that would be great.

Here are the obvious things I see that would would need to be fixed:

- fillbuff would need a mutex for access to all of the vivi_dev fields
- mv_count would should only be updated by the first user
- vivi_stop_thread should only be called once users reaches 0

Otherwise, this patch will need to go in to keep vivi from blowing up in 2.6.25.

Yes, I know. But, vivi has no permission control mechanism right now for differentiating between streaming file handles and control ones.

I think videobuf can handle this case just fine. But, vivi doesn't do the proper locking and management to support it.

Yes, but someone needs to add the code to support this in vivi. And until someone does vivi can only support one open at a time without crashing.

Thanks,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

If the issue is specific to to vivi, that's ok. I'm just wandering if the changesets would break a driver for a real device.

Unfortunately, I couldn't test your patch on a real hardware. The hardware I use here to test PCI boards is currently broken[1].

What the other drivers do is to implement a small code for this lock. You may take a look on res_get(), for example, on cx88.

Btw, we had this lock on past versions of vivi, but this were removed on this changeset:

changeset:   6275:cba23263534b
user:        [email protected] <[email protected]>
date:        Thu Sep 27 20:55:17 2007 -0300
files:       linux/drivers/media/video/vivi.c

description:
V4L: vivi.c remove the "resource" locking

The "resource" locking in vivi isn't needed since streamon/streamoff/read_stream do mutual exclusion using q->reading/q->streaming.

Maybe we can just revert this changeset.

This is a small regression, but I think it is OK for now.

[1] I tried to fix it during the weekend, without success. The Ethernet interface is not working fine - most of the time, it doesn't send packets. Also, with 2 PCI's inside, only one is working fine. If I plug 3 PCI devices, all devices work badly. I tried to replace the power supply, but the troubles still continue. I think I'll need to start saving some money to buy a newer hardware :(

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

I have tested my series on saa7134 hardware and it went OK. Although, having someone else play with my patchset would be nice :)

Yes, I know. But, I don't don't have time to fix vivi up all the way.

That won't fix it. The locking I removed in cba23263534b is not related to the locking that is required to protect the problems that we are seeing with opening up multiple mplayers. The multiple mplayers issue is mostly related to the shutting down of the kthreads and lack of locking on the vivi_dev fields.

Cheers,

Brandon

Isn't shipping hardware to Brazil is really hard?

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Have you tested to run ffmpeg or mencoder under /dev/video0, while listening to something using mmap()? The errors I got with vivi seems to occur if I do something like:

App A starts streaming;
App B starts streaming;
Stop A app -> OOPS

I didn't got error with this sequence:

App A starts streaming;
App B starts streaming;
Stop B;
Stop A.

Ok. Could you please update your tree with the latest patches? I think the better way is to merge they on v4l/dvb and ask more people to test.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

On saa7134 I can't start more than one application streaming at once. The other application gets -EBUSY when it tries to do streamon.

I tried with mplayer and fswebcam and combinations of them.

I did get fswebcam using read() and mplayer using mmap() and it worked fine, although each application was only getting half the frames.

How do you use ffmpeg/mencoder with v4l2 devices? I Googled around but couldn't find anything that worked.

Updated with Jon's comments too and I just removed the copyright updates for vivi- too much trouble: http://ifup.org/hg/v4l-dvb

Thanks,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

The resource lock won't let you open two applications trying to read using the same method.

Weird. You should be able to get the entire frame on each.

There's a small script at V4L/DVB tree, at:

v4l2-apps/util/v4l_rec.pl

It calls mencoder. Also there is inside a commented is a line for it usage with ffmpeg.

The syntax is about the same as when you activate record inside xawtv or xdtv.

Ok, I've merged it at master.

People,

Please test.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Oh, I also dropped the spin_lock patches for soc/pxa/videobuf until Guennadi has a chance to send in working patches for his driver.

http://ifup.org/hg/v4l-dvb

Thanks,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list