[RFC/PATCH 0/2] Convert uvcvideo to videobuf2

[RFC/PATCH 0/2] Convert uvcvideo to videobuf2

[Laurent Pinchart]

Hi everybody,

Those two RFC patches convert the uvcvideo driver to videobuf2.

The transition was pretty smooth, except for an issue with videobuf2 and
hot-pluggable devices. When a hot-pluggable device is disconnected, all pending
video buffers must be marked as erroneous and waiters must be woken up. Drivers
(and/or videobuf2) must then not allow new buffers to be queued, otherwise
applications would wait forever on VIDIOC_DQBUF (side note: maybe it's time for
V4L2 to explictly state what should happen in that case, and what error codes
must be returned).

This isn't a big issue in itself, except that handling the disconnection event
in QBUF introduces a race condition. Fixing it can only be done with the help
of a spinlock which must be held across the disconnection check and the
list_add_tail call in buf_queue. Unfortunately, buf_queue returns void, which
prevents checking for the disconnection event there.

There are multiple ways to solve this problem. the one I've implemented in this
RFC modifies buf_queue to return an error code, and lets drivers implement
buffers queue cancellation on disconnect. As this could be a tricky problem, a
better solution might be to move disconnection handling inside videobuf2. The
downside is that a new spinlock will be needed in videobuf2.

Yet another solution would be to let QBUF succeed, but marking the buffer as
erroneous and waking up userspace immediately. I don't like this though, as the
error flag on buffers is meant to indicate transcient errors, and device
disconnection is more on the fatal error side :-)

Laurent Pinchart (2):
  v4l: videobuf2: Handle buf_queue errors
  uvcvideo: Use videobuf2

 drivers/media/video/uvc/Kconfig      |    1 +
 drivers/media/video/uvc/uvc_isight.c |   10 +-
 drivers/media/video/uvc/uvc_queue.c  |  494 ++++++++++------------------------
 drivers/media/video/uvc/uvc_v4l2.c   |   19 +--
 drivers/media/video/uvc/uvc_video.c  |   30 +-
 drivers/media/video/videobuf2-core.c |   32 ++-
 drivers/usb/gadget/uvc.h             |    2 +-
 include/linux/uvcvideo.h             |   37 ++--
 include/media/videobuf2-core.h       |    2 +-
 9 files changed, 203 insertions(+), 424 deletions(-)

-- 
Regards,

Laurent Pinchart

[RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Laurent Pinchart]

videobuf2 expects drivers to check buffer in the buf_prepare operation
and to return success only if the buffer can queued without any issue.

For hot-pluggable devices, disconnection events need to be handled at
buf_queue time. Checking the disconnected flag and adding the buffer to
the driver queue need to be performed without releasing the driver
spinlock, otherwise race conditions can occur in which the driver could
still allow a buffer to be queued after the disconnected flag has been
set, resulting in a hang during the next DQBUF operation.

This problem can be solved either in the videobuf2 core or in the device
drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
an int and handle buf_queue failures in videobuf2. Drivers must not
return an error in buf_queue if the condition leading to the error can
be caught in buf_prepare.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/videobuf2-core.c |   32 ++++++++++++++++++++++++++------
 include/media/videobuf2-core.h       |    2 +-
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index cc7ab0a..1d81536 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb, struct v4l2_buffer *b)
 /**
  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
  */
-static void __enqueue_in_driver(struct vb2_buffer *vb)
+static int __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	int ret;
 
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->queued_count);
-	q->ops->buf_queue(vb);
+	ret = q->ops->buf_queue(vb);
+	if (ret == 0)
+		return 0;
+
+	vb->state = VB2_BUF_STATE_ERROR;
+	atomic_dec(&q->queued_count);
+	wake_up_all(&q->done_wq);
+
+	return ret;
 }
 
 /**
@@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	 * If already streaming, give the buffer to driver for processing.
 	 * If not, the buffer will be given to driver on next streamon.
 	 */
-	if (q->streaming)
-		__enqueue_in_driver(vb);
+	if (q->streaming) {
+		ret = __enqueue_in_driver(vb);
+		if (ret < 0) {
+			dprintk(1, "qbuf: buffer queue failed\n");
+			return ret;
+		}
+	}
 
 	dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
 	return 0;
@@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
 int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	struct vb2_buffer *vb;
+	int ret;
 
 	if (q->fileio) {
 		dprintk(1, "streamon: file io in progress\n");
@@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 	 * If any buffers were queued before streamon,
 	 * we can now pass them to driver for processing.
 	 */
-	list_for_each_entry(vb, &q->queued_list, queued_entry)
-		__enqueue_in_driver(vb);
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		ret = __enqueue_in_driver(vb);
+		if (ret < 0) {
+			dprintk(1, "streamon: buffer queue failed\n");
+			return ret;
+		}
+	}
 
 	dprintk(3, "Streamon successful\n");
 	return 0;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 597efe6..3a92f75 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -225,7 +225,7 @@ struct vb2_ops {
 	int (*start_streaming)(struct vb2_queue *q);
 	int (*stop_streaming)(struct vb2_queue *q);
 
-	void (*buf_queue)(struct vb2_buffer *vb);
+	int (*buf_queue)(struct vb2_buffer *vb);
 };
 
 /**
-- 
1.7.3.4

[RFC/PATCH 2/2] uvcvideo: Use videobuf2

[Laurent Pinchart]

Replace the uvcvideo buffer queue implementation by videobuf2.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/uvc/Kconfig      |    1 +
 drivers/media/video/uvc/uvc_isight.c |   10 +-
 drivers/media/video/uvc/uvc_queue.c  |  494 ++++++++++------------------------
 drivers/media/video/uvc/uvc_v4l2.c   |   19 +--
 drivers/media/video/uvc/uvc_video.c  |   30 +-
 drivers/usb/gadget/uvc.h             |    2 +-
 include/linux/uvcvideo.h             |   37 ++--
 7 files changed, 176 insertions(+), 417 deletions(-)

diff --git a/drivers/media/video/uvc/Kconfig b/drivers/media/video/uvc/Kconfig
index 2956a76..6c197da 100644
--- a/drivers/media/video/uvc/Kconfig
+++ b/drivers/media/video/uvc/Kconfig
@@ -1,5 +1,6 @@
 config USB_VIDEO_CLASS
 	tristate "USB Video Class (UVC)"
+	select VIDEOBUF2_VMALLOC
 	---help---
 	  Support for the USB Video Class (UVC).  Currently only video
 	  input devices, such as webcams, are supported.
diff --git a/drivers/media/video/uvc/uvc_isight.c b/drivers/media/video/uvc/uvc_isight.c
index 8217c5d..eaa5df0 100644
--- a/drivers/media/video/uvc/uvc_isight.c
+++ b/drivers/media/video/uvc/uvc_isight.c
@@ -73,7 +73,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf,
 	 * Empty buffers (bytesused == 0) don't trigger end of frame detection
 	 * as it doesn't make sense to return an empty buffer.
 	 */
-	if (is_header && buf->buf.bytesused != 0) {
+	if (is_header && buf->bytesused != 0) {
 		buf->state = UVC_BUF_STATE_DONE;
 		return -EAGAIN;
 	}
@@ -82,13 +82,13 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf,
 	 * contain no data.
 	 */
 	if (!is_header) {
-		maxlen = buf->buf.length - buf->buf.bytesused;
-		mem = queue->mem + buf->buf.m.offset + buf->buf.bytesused;
+		maxlen = buf->length - buf->bytesused;
+		mem = buf->mem + buf->bytesused;
 		nbytes = min(len, maxlen);
 		memcpy(mem, data, nbytes);
-		buf->buf.bytesused += nbytes;
+		buf->bytesused += nbytes;
 
-		if (len > maxlen || buf->buf.bytesused == buf->buf.length) {
+		if (len > maxlen || buf->bytesused == buf->length) {
 			uvc_trace(UVC_TRACE_FRAME, "Frame complete "
 				  "(overflow).\n");
 			buf->state = UVC_BUF_STATE_DONE;
diff --git a/drivers/media/video/uvc/uvc_queue.c b/drivers/media/video/uvc/uvc_queue.c
index 36f3c58..cb73b08 100644
--- a/drivers/media/video/uvc/uvc_queue.c
+++ b/drivers/media/video/uvc/uvc_queue.c
@@ -20,6 +20,7 @@
 #include <linux/videodev2.h>
 #include <linux/vmalloc.h>
 #include <linux/wait.h>
+#include <media/videobuf2-vmalloc.h>
 #include <asm/atomic.h>
 
 /* ------------------------------------------------------------------------
@@ -68,7 +69,7 @@
  *
  *    When the device is disconnected, the kernel calls the completion handler
  *    with an appropriate status code. The handler marks all buffers in the
- *    irq queue as being erroneous (UVC_BUF_STATE_ERROR) and wakes them up so
+ *    irq queue as being erroneous (UVC_BUF_STATE_ERROR) and completes them so
  *    that any process waiting on a buffer gets woken up.
  *
  *    Waking up up the first buffer on the irq list is not enough, as the
@@ -77,414 +78,192 @@
  *
  */
 
-void uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
-		    int drop_corrupted)
-{
-	mutex_init(&queue->mutex);
-	spin_lock_init(&queue->irqlock);
-	INIT_LIST_HEAD(&queue->mainqueue);
-	INIT_LIST_HEAD(&queue->irqqueue);
-	queue->flags = drop_corrupted ? UVC_QUEUE_DROP_CORRUPTED : 0;
-	queue->type = type;
-}
-
-/*
- * Free the video buffers.
- *
- * This function must be called with the queue lock held.
+/* -----------------------------------------------------------------------------
+ * videobuf2 queue operations
  */
-static int __uvc_free_buffers(struct uvc_video_queue *queue)
+
+static int uvc_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+			   unsigned int *nplanes, unsigned long sizes[],
+			   void *alloc_ctxs[])
 {
-	unsigned int i;
+	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+	struct uvc_streaming *stream =
+			container_of(queue, struct uvc_streaming, queue);
 
-	for (i = 0; i < queue->count; ++i) {
-		if (queue->buffer[i].vma_use_count != 0)
-			return -EBUSY;
-	}
+	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
+		*nbuffers = UVC_MAX_VIDEO_BUFFERS;
 
-	if (queue->count) {
-		vfree(queue->mem);
-		queue->count = 0;
-	}
+	*nplanes = 1;
+
+	sizes[0] = stream->ctrl.dwMaxVideoFrameSize;
 
 	return 0;
 }
 
-int uvc_free_buffers(struct uvc_video_queue *queue)
+static int uvc_buffer_prepare(struct vb2_buffer *vb)
 {
-	int ret;
+	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
 
-	mutex_lock(&queue->mutex);
-	ret = __uvc_free_buffers(queue);
-	mutex_unlock(&queue->mutex);
+	if (vb->v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	    vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
+		uvc_trace(UVC_TRACE_CAPTURE, "[E] Bytes used out of bounds.\n");
+		return -EINVAL;
+	}
 
-	return ret;
+	buf->state = UVC_BUF_STATE_QUEUED;
+	buf->error = 0;
+	buf->mem = vb2_plane_vaddr(vb, 0);
+	buf->length = vb2_plane_size(vb, 0);
+	if (vb->v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		buf->bytesused = 0;
+	else
+		buf->bytesused = vb2_get_plane_payload(vb, 0);
+
+	return 0;
 }
 
-/*
- * Allocate the video buffers.
- *
- * Pages are reserved to make sure they will not be swapped, as they will be
- * filled in the URB completion handler.
- *
- * Buffers will be individually mapped, so they must all be page aligned.
- */
-int uvc_alloc_buffers(struct uvc_video_queue *queue, unsigned int nbuffers,
-		unsigned int buflength)
+static int uvc_buffer_queue(struct vb2_buffer *vb)
 {
-	unsigned int bufsize = PAGE_ALIGN(buflength);
-	unsigned int i;
-	void *mem = NULL;
-	int ret;
-
-	if (nbuffers > UVC_MAX_VIDEO_BUFFERS)
-		nbuffers = UVC_MAX_VIDEO_BUFFERS;
-
-	mutex_lock(&queue->mutex);
-
-	if ((ret = __uvc_free_buffers(queue)) < 0)
-		goto done;
+	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
+	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
+	unsigned long flags;
+	int ret = 0;
 
-	/* Bail out if no buffers should be allocated. */
-	if (nbuffers == 0)
+	spin_lock_irqsave(&queue->irqlock, flags);
+	if (queue->flags & UVC_QUEUE_DISCONNECTED) {
+		ret = -ENODEV;
 		goto done;
-
-	/* Decrement the number of buffers until allocation succeeds. */
-	for (; nbuffers > 0; --nbuffers) {
-		mem = vmalloc_32(nbuffers * bufsize);
-		if (mem != NULL)
-			break;
 	}
 
-	if (mem == NULL) {
-		ret = -ENOMEM;
-		goto done;
-	}
+	list_add_tail(&buf->queue, &queue->irqqueue);
+done:
+	spin_unlock_irqrestore(&queue->irqlock, flags);
+	return ret;
+}
 
-	for (i = 0; i < nbuffers; ++i) {
-		memset(&queue->buffer[i], 0, sizeof queue->buffer[i]);
-		queue->buffer[i].buf.index = i;
-		queue->buffer[i].buf.m.offset = i * bufsize;
-		queue->buffer[i].buf.length = buflength;
-		queue->buffer[i].buf.type = queue->type;
-		queue->buffer[i].buf.field = V4L2_FIELD_NONE;
-		queue->buffer[i].buf.memory = V4L2_MEMORY_MMAP;
-		queue->buffer[i].buf.flags = 0;
-		init_waitqueue_head(&queue->buffer[i].wait);
-	}
+static struct vb2_ops uvc_queue_qops = {
+	.queue_setup = uvc_queue_setup,
+	.buf_prepare = uvc_buffer_prepare,
+	.buf_queue = uvc_buffer_queue,
+};
 
-	queue->mem = mem;
-	queue->count = nbuffers;
-	queue->buf_size = bufsize;
-	ret = nbuffers;
+void uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
+		    int drop_corrupted)
+{
+	queue->queue.type = type;
+	queue->queue.io_modes = VB2_MMAP;
+	queue->queue.drv_priv = queue;
+	queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
+	queue->queue.ops = &uvc_queue_qops;
+	queue->queue.mem_ops = &vb2_vmalloc_memops;
+	vb2_queue_init(&queue->queue);
 
-done:
-	mutex_unlock(&queue->mutex);
-	return ret;
+	mutex_init(&queue->mutex);
+	spin_lock_init(&queue->irqlock);
+	INIT_LIST_HEAD(&queue->irqqueue);
+	queue->flags = drop_corrupted ? UVC_QUEUE_DROP_CORRUPTED : 0;
 }
 
-/*
- * Check if buffers have been allocated.
+/* -----------------------------------------------------------------------------
+ * V4L2 queue operations
  */
-int uvc_queue_allocated(struct uvc_video_queue *queue)
+
+int uvc_alloc_buffers(struct uvc_video_queue *queue,
+		struct v4l2_requestbuffers *rb, unsigned int buflength)
 {
-	int allocated;
+	int ret;
 
 	mutex_lock(&queue->mutex);
-	allocated = queue->count != 0;
+	ret = vb2_reqbufs(&queue->queue, rb);
 	mutex_unlock(&queue->mutex);
 
-	return allocated;
+	return ret ? ret : rb->count;
 }
 
-static void __uvc_query_buffer(struct uvc_buffer *buf,
-		struct v4l2_buffer *v4l2_buf)
+void uvc_free_buffers(struct uvc_video_queue *queue)
 {
-	memcpy(v4l2_buf, &buf->buf, sizeof *v4l2_buf);
-
-	if (buf->vma_use_count)
-		v4l2_buf->flags |= V4L2_BUF_FLAG_MAPPED;
-
-	switch (buf->state) {
-	case UVC_BUF_STATE_ERROR:
-	case UVC_BUF_STATE_DONE:
-		v4l2_buf->flags |= V4L2_BUF_FLAG_DONE;
-		break;
-	case UVC_BUF_STATE_QUEUED:
-	case UVC_BUF_STATE_ACTIVE:
-	case UVC_BUF_STATE_READY:
-		v4l2_buf->flags |= V4L2_BUF_FLAG_QUEUED;
-		break;
-	case UVC_BUF_STATE_IDLE:
-	default:
-		break;
-	}
+	vb2_queue_release(&queue->queue);
 }
 
-int uvc_query_buffer(struct uvc_video_queue *queue,
-		struct v4l2_buffer *v4l2_buf)
+int uvc_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)
 {
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&queue->mutex);
-	if (v4l2_buf->index >= queue->count) {
-		ret = -EINVAL;
-		goto done;
-	}
-
-	__uvc_query_buffer(&queue->buffer[v4l2_buf->index], v4l2_buf);
-
-done:
+	ret = vb2_querybuf(&queue->queue, buf);
 	mutex_unlock(&queue->mutex);
+
 	return ret;
 }
 
-/*
- * Queue a video buffer. Attempting to queue a buffer that has already been
- * queued will return -EINVAL.
- */
-int uvc_queue_buffer(struct uvc_video_queue *queue,
-	struct v4l2_buffer *v4l2_buf)
+int uvc_queue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)
 {
-	struct uvc_buffer *buf;
-	unsigned long flags;
-	int ret = 0;
-
-	uvc_trace(UVC_TRACE_CAPTURE, "Queuing buffer %u.\n", v4l2_buf->index);
-
-	if (v4l2_buf->type != queue->type ||
-	    v4l2_buf->memory != V4L2_MEMORY_MMAP) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) "
-			"and/or memory (%u).\n", v4l2_buf->type,
-			v4l2_buf->memory);
-		return -EINVAL;
-	}
+	int ret;
 
+	uvc_printk(KERN_INFO, "-> qbuf\n");
 	mutex_lock(&queue->mutex);
-	if (v4l2_buf->index >= queue->count) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Out of range index.\n");
-		ret = -EINVAL;
-		goto done;
-	}
-
-	buf = &queue->buffer[v4l2_buf->index];
-	if (buf->state != UVC_BUF_STATE_IDLE) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer state "
-			"(%u).\n", buf->state);
-		ret = -EINVAL;
-		goto done;
-	}
-
-	if (v4l2_buf->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
-	    v4l2_buf->bytesused > buf->buf.length) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Bytes used out of bounds.\n");
-		ret = -EINVAL;
-		goto done;
-	}
-
-	spin_lock_irqsave(&queue->irqlock, flags);
-	if (queue->flags & UVC_QUEUE_DISCONNECTED) {
-		spin_unlock_irqrestore(&queue->irqlock, flags);
-		ret = -ENODEV;
-		goto done;
-	}
-	buf->state = UVC_BUF_STATE_QUEUED;
-	if (v4l2_buf->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		buf->buf.bytesused = 0;
-	else
-		buf->buf.bytesused = v4l2_buf->bytesused;
-
-	list_add_tail(&buf->stream, &queue->mainqueue);
-	list_add_tail(&buf->queue, &queue->irqqueue);
-	spin_unlock_irqrestore(&queue->irqlock, flags);
-
-done:
+	ret = vb2_qbuf(&queue->queue, buf);
 	mutex_unlock(&queue->mutex);
-	return ret;
-}
-
-static int uvc_queue_waiton(struct uvc_buffer *buf, int nonblocking)
-{
-	if (nonblocking) {
-		return (buf->state != UVC_BUF_STATE_QUEUED &&
-			buf->state != UVC_BUF_STATE_ACTIVE &&
-			buf->state != UVC_BUF_STATE_READY)
-			? 0 : -EAGAIN;
-	}
+	uvc_printk(KERN_INFO, "<- qbuf %d\n", ret);
 
-	return wait_event_interruptible(buf->wait,
-		buf->state != UVC_BUF_STATE_QUEUED &&
-		buf->state != UVC_BUF_STATE_ACTIVE &&
-		buf->state != UVC_BUF_STATE_READY);
+	return ret;
 }
 
-/*
- * Dequeue a video buffer. If nonblocking is false, block until a buffer is
- * available.
- */
-int uvc_dequeue_buffer(struct uvc_video_queue *queue,
-		struct v4l2_buffer *v4l2_buf, int nonblocking)
+int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
+		       int nonblocking)
 {
-	struct uvc_buffer *buf;
-	int ret = 0;
-
-	if (v4l2_buf->type != queue->type ||
-	    v4l2_buf->memory != V4L2_MEMORY_MMAP) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) "
-			"and/or memory (%u).\n", v4l2_buf->type,
-			v4l2_buf->memory);
-		return -EINVAL;
-	}
+	int ret;
 
+	uvc_printk(KERN_INFO, "-> dqbuf\n");
 	mutex_lock(&queue->mutex);
-	if (list_empty(&queue->mainqueue)) {
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Empty buffer queue.\n");
-		ret = -EINVAL;
-		goto done;
-	}
-
-	buf = list_first_entry(&queue->mainqueue, struct uvc_buffer, stream);
-	if ((ret = uvc_queue_waiton(buf, nonblocking)) < 0)
-		goto done;
-
-	uvc_trace(UVC_TRACE_CAPTURE, "Dequeuing buffer %u (%u, %u bytes).\n",
-		buf->buf.index, buf->state, buf->buf.bytesused);
-
-	switch (buf->state) {
-	case UVC_BUF_STATE_ERROR:
-		uvc_trace(UVC_TRACE_CAPTURE, "[W] Corrupted data "
-			"(transmission error).\n");
-		ret = -EIO;
-	case UVC_BUF_STATE_DONE:
-		buf->state = UVC_BUF_STATE_IDLE;
-		break;
-
-	case UVC_BUF_STATE_IDLE:
-	case UVC_BUF_STATE_QUEUED:
-	case UVC_BUF_STATE_ACTIVE:
-	case UVC_BUF_STATE_READY:
-	default:
-		uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer state %u "
-			"(driver bug?).\n", buf->state);
-		ret = -EINVAL;
-		goto done;
-	}
-
-	list_del(&buf->stream);
-	__uvc_query_buffer(buf, v4l2_buf);
-
-done:
+	ret = vb2_dqbuf(&queue->queue, buf, nonblocking);
 	mutex_unlock(&queue->mutex);
-	return ret;
-}
-
-/*
- * VMA operations.
- */
-static void uvc_vm_open(struct vm_area_struct *vma)
-{
-	struct uvc_buffer *buffer = vma->vm_private_data;
-	buffer->vma_use_count++;
-}
+	uvc_printk(KERN_INFO, "<- dqbuf %d (flags %u)\n", ret, buf->flags);
 
-static void uvc_vm_close(struct vm_area_struct *vma)
-{
-	struct uvc_buffer *buffer = vma->vm_private_data;
-	buffer->vma_use_count--;
+	return ret;
 }
 
-static const struct vm_operations_struct uvc_vm_ops = {
-	.open		= uvc_vm_open,
-	.close		= uvc_vm_close,
-};
-
-/*
- * Memory-map a video buffer.
- *
- * This function implements video buffers memory mapping and is intended to be
- * used by the device mmap handler.
- */
 int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
 {
-	struct uvc_buffer *uninitialized_var(buffer);
-	struct page *page;
-	unsigned long addr, start, size;
-	unsigned int i;
-	int ret = 0;
-
-	start = vma->vm_start;
-	size = vma->vm_end - vma->vm_start;
+	int ret;
 
 	mutex_lock(&queue->mutex);
+	ret = vb2_mmap(&queue->queue, vma);
+	mutex_unlock(&queue->mutex);
 
-	for (i = 0; i < queue->count; ++i) {
-		buffer = &queue->buffer[i];
-		if ((buffer->buf.m.offset >> PAGE_SHIFT) == vma->vm_pgoff)
-			break;
-	}
-
-	if (i == queue->count || size != queue->buf_size) {
-		ret = -EINVAL;
-		goto done;
-	}
-
-	/*
-	 * VM_IO marks the area as being an mmaped region for I/O to a
-	 * device. It also prevents the region from being core dumped.
-	 */
-	vma->vm_flags |= VM_IO;
-
-	addr = (unsigned long)queue->mem + buffer->buf.m.offset;
-	while (size > 0) {
-		page = vmalloc_to_page((void *)addr);
-		if ((ret = vm_insert_page(vma, start, page)) < 0)
-			goto done;
-
-		start += PAGE_SIZE;
-		addr += PAGE_SIZE;
-		size -= PAGE_SIZE;
-	}
+	return ret;
+}
 
-	vma->vm_ops = &uvc_vm_ops;
-	vma->vm_private_data = buffer;
-	uvc_vm_open(vma);
+unsigned int uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
+			    poll_table *wait)
+{
+	unsigned int ret;
 
-done:
+	uvc_printk(KERN_INFO, "-> poll\n");
+	mutex_lock(&queue->mutex);
+	ret = vb2_poll(&queue->queue, file, wait);
 	mutex_unlock(&queue->mutex);
+	uvc_printk(KERN_INFO, "<- poll %d\n", ret);
+
 	return ret;
 }
 
-/*
- * Poll the video queue.
+/* -----------------------------------------------------------------------------
  *
- * This function implements video queue polling and is intended to be used by
- * the device poll handler.
  */
-unsigned int uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-		poll_table *wait)
+
+/*
+ * Check if buffers have been allocated.
+ */
+int uvc_queue_allocated(struct uvc_video_queue *queue)
 {
-	struct uvc_buffer *buf;
-	unsigned int mask = 0;
+	int allocated;
 
 	mutex_lock(&queue->mutex);
-	if (list_empty(&queue->mainqueue)) {
-		mask |= POLLERR;
-		goto done;
-	}
-	buf = list_first_entry(&queue->mainqueue, struct uvc_buffer, stream);
-
-	poll_wait(file, &buf->wait, wait);
-	if (buf->state == UVC_BUF_STATE_DONE ||
-	    buf->state == UVC_BUF_STATE_ERROR) {
-		if (queue->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
-			mask |= POLLIN | POLLRDNORM;
-		else
-			mask |= POLLOUT | POLLWRNORM;
-	}
-
-done:
+	allocated = vb2_is_busy(&queue->queue);
 	mutex_unlock(&queue->mutex);
-	return mask;
+
+	return allocated;
 }
 
 /*
@@ -505,27 +284,24 @@ done:
  */
 int uvc_queue_enable(struct uvc_video_queue *queue, int enable)
 {
-	unsigned int i;
-	int ret = 0;
+	unsigned long flags;
+	int ret;
 
 	mutex_lock(&queue->mutex);
 	if (enable) {
-		if (uvc_queue_streaming(queue)) {
-			ret = -EBUSY;
+		ret = vb2_streamon(&queue->queue, queue->queue.type);
+		if (ret < 0)
 			goto done;
-		}
-		queue->flags |= UVC_QUEUE_STREAMING;
+
 		queue->buf_used = 0;
 	} else {
-		uvc_queue_cancel(queue, 0);
-		INIT_LIST_HEAD(&queue->mainqueue);
-
-		for (i = 0; i < queue->count; ++i) {
-			queue->buffer[i].error = 0;
-			queue->buffer[i].state = UVC_BUF_STATE_IDLE;
-		}
+		ret = vb2_streamoff(&queue->queue, queue->queue.type);
+		if (ret < 0)
+			goto done;
 
-		queue->flags &= ~UVC_QUEUE_STREAMING;
+		spin_lock_irqsave(&queue->irqlock, flags);
+		INIT_LIST_HEAD(&queue->irqqueue);
+		spin_unlock_irqrestore(&queue->irqlock, flags);
 	}
 
 done:
@@ -556,7 +332,7 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
 				       queue);
 		list_del(&buf->queue);
 		buf->state = UVC_BUF_STATE_ERROR;
-		wake_up(&buf->wait);
+		vb2_buffer_done(&buf->buf, VB2_BUF_STATE_ERROR);
 	}
 	/* This must be protected by the irqlock spinlock to avoid race
 	 * conditions between uvc_queue_buffer and the disconnection event that
@@ -564,8 +340,10 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
 	 * blindly replace this logic by checking for the UVC_DEV_DISCONNECTED
 	 * state outside the queue code.
 	 */
-	if (disconnect)
+	if (disconnect) {
+		uvc_printk(KERN_INFO, "disconnected\n");
 		queue->flags |= UVC_QUEUE_DISCONNECTED;
+	}
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 }
 
@@ -578,14 +356,12 @@ struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
 		buf->error = 0;
 		buf->state = UVC_BUF_STATE_QUEUED;
-		buf->buf.bytesused = 0;
+		vb2_set_plane_payload(&buf->buf, 0, 0);
 		return buf;
 	}
 
 	spin_lock_irqsave(&queue->irqlock, flags);
 	list_del(&buf->queue);
-	buf->error = 0;
-	buf->state = UVC_BUF_STATE_DONE;
 	if (!list_empty(&queue->irqqueue))
 		nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
 					   queue);
@@ -593,7 +369,9 @@ struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 		nextbuf = NULL;
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 
-	wake_up(&buf->wait);
+	buf->state = buf->error ? VB2_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
+	vb2_set_plane_payload(&buf->buf, 0, buf->bytesused);
+	vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
+
 	return nextbuf;
 }
-
diff --git a/drivers/media/video/uvc/uvc_v4l2.c b/drivers/media/video/uvc/uvc_v4l2.c
index fa89ebf..428e577 100644
--- a/drivers/media/video/uvc/uvc_v4l2.c
+++ b/drivers/media/video/uvc/uvc_v4l2.c
@@ -519,10 +519,7 @@ static int uvc_v4l2_release(struct file *file)
 	/* Only free resources if this is a privileged handle. */
 	if (uvc_has_privileges(handle)) {
 		uvc_video_enable(stream, 0);
-
-		if (uvc_free_buffers(&stream->queue) < 0)
-			uvc_printk(KERN_ERR, "uvc_v4l2_release: Unable to "
-					"free buffers.\n");
+		uvc_free_buffers(&stream->queue);
 	}
 
 	/* Release the file handle. */
@@ -934,18 +931,11 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 	/* Buffers & streaming */
 	case VIDIOC_REQBUFS:
-	{
-		struct v4l2_requestbuffers *rb = arg;
-
-		if (rb->type != stream->type ||
-		    rb->memory != V4L2_MEMORY_MMAP)
-			return -EINVAL;
-
 		if ((ret = uvc_acquire_privileges(handle)) < 0)
 			return ret;
 
 		mutex_lock(&stream->mutex);
-		ret = uvc_alloc_buffers(&stream->queue, rb->count,
+		ret = uvc_alloc_buffers(&stream->queue, arg,
 					stream->ctrl.dwMaxVideoFrameSize);
 		mutex_unlock(&stream->mutex);
 		if (ret < 0)
@@ -954,18 +944,13 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (ret == 0)
 			uvc_dismiss_privileges(handle);
 
-		rb->count = ret;
 		ret = 0;
 		break;
-	}
 
 	case VIDIOC_QUERYBUF:
 	{
 		struct v4l2_buffer *buf = arg;
 
-		if (buf->type != stream->type)
-			return -EINVAL;
-
 		if (!uvc_has_privileges(handle))
 			return -EBUSY;
 
diff --git a/drivers/media/video/uvc/uvc_video.c b/drivers/media/video/uvc/uvc_video.c
index 0ea7adf..316f6c8 100644
--- a/drivers/media/video/uvc/uvc_video.c
+++ b/drivers/media/video/uvc/uvc_video.c
@@ -466,9 +466,10 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 		else
 			ktime_get_real_ts(&ts);
 
-		buf->buf.sequence = stream->sequence;
-		buf->buf.timestamp.tv_sec = ts.tv_sec;
-		buf->buf.timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+		buf->buf.v4l2_buf.sequence = stream->sequence;
+		buf->buf.v4l2_buf.timestamp.tv_sec = ts.tv_sec;
+		buf->buf.v4l2_buf.timestamp.tv_usec =
+			ts.tv_nsec / NSEC_PER_USEC;
 
 		/* TODO: Handle PTS and SCR. */
 		buf->state = UVC_BUF_STATE_ACTIVE;
@@ -489,7 +490,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 	 * avoids detecting end of frame conditions at FID toggling if the
 	 * previous payload had the EOF bit set.
 	 */
-	if (fid != stream->last_fid && buf->buf.bytesused != 0) {
+	if (fid != stream->last_fid && buf->bytesused != 0) {
 		uvc_trace(UVC_TRACE_FRAME, "Frame complete (FID bit "
 				"toggled).\n");
 		buf->state = UVC_BUF_STATE_READY;
@@ -504,7 +505,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 static void uvc_video_decode_data(struct uvc_streaming *stream,
 		struct uvc_buffer *buf, const __u8 *data, int len)
 {
-	struct uvc_video_queue *queue = &stream->queue;
 	unsigned int maxlen, nbytes;
 	void *mem;
 
@@ -512,11 +512,11 @@ static void uvc_video_decode_data(struct uvc_streaming *stream,
 		return;
 
 	/* Copy the video data to the buffer. */
-	maxlen = buf->buf.length - buf->buf.bytesused;
-	mem = queue->mem + buf->buf.m.offset + buf->buf.bytesused;
+	maxlen = buf->length - buf->bytesused;
+	mem = buf->mem + buf->bytesused;
 	nbytes = min((unsigned int)len, maxlen);
 	memcpy(mem, data, nbytes);
-	buf->buf.bytesused += nbytes;
+	buf->bytesused += nbytes;
 
 	/* Complete the current frame if the buffer size was exceeded. */
 	if (len > maxlen) {
@@ -529,7 +529,7 @@ static void uvc_video_decode_end(struct uvc_streaming *stream,
 		struct uvc_buffer *buf, const __u8 *data, int len)
 {
 	/* Mark the buffer as done if the EOF marker is set. */
-	if (data[1] & UVC_STREAM_EOF && buf->buf.bytesused != 0) {
+	if (data[1] & UVC_STREAM_EOF && buf->bytesused != 0) {
 		uvc_trace(UVC_TRACE_FRAME, "Frame complete (EOF found).\n");
 		if (data[0] == len)
 			uvc_trace(UVC_TRACE_FRAME, "EOF in empty payload.\n");
@@ -567,8 +567,8 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
 	void *mem;
 
 	/* Copy video data to the URB buffer. */
-	mem = queue->mem + buf->buf.m.offset + queue->buf_used;
-	nbytes = min((unsigned int)len, buf->buf.bytesused - queue->buf_used);
+	mem = buf->mem + queue->buf_used;
+	nbytes = min((unsigned int)len, buf->bytesused - queue->buf_used);
 	nbytes = min(stream->bulk.max_payload_size - stream->bulk.payload_size,
 			nbytes);
 	memcpy(data, mem, nbytes);
@@ -623,7 +623,7 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
 			urb->iso_frame_desc[i].actual_length);
 
 		if (buf->state == UVC_BUF_STATE_READY) {
-			if (buf->buf.length != buf->buf.bytesused &&
+			if (buf->length != buf->bytesused &&
 			    !(stream->cur_format->flags &
 			      UVC_FMT_FLAG_COMPRESSED))
 				buf->error = 1;
@@ -723,12 +723,12 @@ static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming *stream,
 	stream->bulk.payload_size += ret;
 	len -= ret;
 
-	if (buf->buf.bytesused == stream->queue.buf_used ||
+	if (buf->bytesused == stream->queue.buf_used ||
 	    stream->bulk.payload_size == stream->bulk.max_payload_size) {
-		if (buf->buf.bytesused == stream->queue.buf_used) {
+		if (buf->bytesused == stream->queue.buf_used) {
 			stream->queue.buf_used = 0;
 			buf->state = UVC_BUF_STATE_READY;
-			buf->buf.sequence = ++stream->sequence;
+			buf->buf.v4l2_buf.sequence = ++stream->sequence;
 			uvc_queue_next_buffer(&stream->queue, buf);
 			stream->last_fid ^= UVC_STREAM_FID;
 		}
diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
index 5b79194..08bb61b 100644
--- a/drivers/usb/gadget/uvc.h
+++ b/drivers/usb/gadget/uvc.h
@@ -29,7 +29,7 @@
 
 struct uvc_request_data
 {
-	unsigned int length;
+	int length;
 	__u8 data[60];
 };
 
diff --git a/include/linux/uvcvideo.h b/include/linux/uvcvideo.h
index 4b53343..d4b0756 100644
--- a/include/linux/uvcvideo.h
+++ b/include/linux/uvcvideo.h
@@ -93,6 +93,7 @@ struct uvc_xu_control_query {
 
 #include <linux/poll.h>
 #include <linux/usb/video.h>
+#include <media/videobuf2-core.h>
 
 /* --------------------------------------------------------------------------
  * UVC constants
@@ -383,35 +384,28 @@ enum uvc_buffer_state {
 };
 
 struct uvc_buffer {
-	unsigned long vma_use_count;
-	struct list_head stream;
-
-	/* Touched by interrupt handler. */
-	struct v4l2_buffer buf;
+	struct vb2_buffer buf;
 	struct list_head queue;
-	wait_queue_head_t wait;
+
 	enum uvc_buffer_state state;
 	unsigned int error;
+
+	void *mem;
+	unsigned int length;
+	unsigned int bytesused;
 };
 
-#define UVC_QUEUE_STREAMING		(1 << 0)
-#define UVC_QUEUE_DISCONNECTED		(1 << 1)
-#define UVC_QUEUE_DROP_CORRUPTED	(1 << 2)
+#define UVC_QUEUE_DISCONNECTED		(1 << 0)
+#define UVC_QUEUE_DROP_CORRUPTED	(1 << 1)
 
 struct uvc_video_queue {
-	enum v4l2_buf_type type;
+	struct vb2_queue queue;
+	struct mutex mutex;			/* Protects queue */
 
-	void *mem;
 	unsigned int flags;
-
-	unsigned int count;
-	unsigned int buf_size;
 	unsigned int buf_used;
-	struct uvc_buffer buffer[UVC_MAX_VIDEO_BUFFERS];
-	struct mutex mutex;	/* protects buffers and mainqueue */
-	spinlock_t irqlock;	/* protects irqqueue */
 
-	struct list_head mainqueue;
+	spinlock_t irqlock;			/* Protects irqqueue */
 	struct list_head irqqueue;
 };
 
@@ -451,6 +445,7 @@ struct uvc_streaming {
 	 */
 	struct mutex mutex;
 
+	/* Buffers queue. */
 	unsigned int frozen:1;
 	struct uvc_video_queue queue;
 	void (*decode) (struct urb *urb, struct uvc_streaming *video,
@@ -574,8 +569,8 @@ extern struct uvc_driver uvc_driver;
 extern void uvc_queue_init(struct uvc_video_queue *queue,
 		enum v4l2_buf_type type, int drop_corrupted);
 extern int uvc_alloc_buffers(struct uvc_video_queue *queue,
-		unsigned int nbuffers, unsigned int buflength);
-extern int uvc_free_buffers(struct uvc_video_queue *queue);
+		struct v4l2_requestbuffers *rb, unsigned int buflength);
+extern void uvc_free_buffers(struct uvc_video_queue *queue);
 extern int uvc_query_buffer(struct uvc_video_queue *queue,
 		struct v4l2_buffer *v4l2_buf);
 extern int uvc_queue_buffer(struct uvc_video_queue *queue,
@@ -593,7 +588,7 @@ extern unsigned int uvc_queue_poll(struct uvc_video_queue *queue,
 extern int uvc_queue_allocated(struct uvc_video_queue *queue);
 static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
 {
-	return queue->flags & UVC_QUEUE_STREAMING;
+	return vb2_is_streaming(&queue->queue);
 }
 
 /* V4L2 interface */
-- 
1.7.3.4

RE: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Marek Szyprowski]

Hello,

On Sunday, February 27, 2011 7:13 PM Laurent Pinchart wrote:

> videobuf2 expects drivers to check buffer in the buf_prepare operation
> and to return success only if the buffer can queued without any issue.
> 
> For hot-pluggable devices, disconnection events need to be handled at
> buf_queue time. Checking the disconnected flag and adding the buffer to
> the driver queue need to be performed without releasing the driver
> spinlock, otherwise race conditions can occur in which the driver could
> still allow a buffer to be queued after the disconnected flag has been
> set, resulting in a hang during the next DQBUF operation.
> 
> This problem can be solved either in the videobuf2 core or in the device
> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> an int and handle buf_queue failures in videobuf2. Drivers must not
> return an error in buf_queue if the condition leading to the error can
> be caught in buf_prepare.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

We discussed how to solve the hot-plug issue and this is the solution I prefer.

> ---
>  drivers/media/video/videobuf2-core.c |   32 ++++++++++++++++++++++++++------
>  include/media/videobuf2-core.h       |    2 +-
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index cc7ab0a..1d81536 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
> +	int ret;
> 
>  	vb->state = VB2_BUF_STATE_ACTIVE;
>  	atomic_inc(&q->queued_count);
> -	q->ops->buf_queue(vb);
> +	ret = q->ops->buf_queue(vb);
> +	if (ret == 0)
> +		return 0;
> +
> +	vb->state = VB2_BUF_STATE_ERROR;
> +	atomic_dec(&q->queued_count);
> +	wake_up_all(&q->done_wq);
> +
> +	return ret;
>  }
> 
>  /**
> @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  	 * If already streaming, give the buffer to driver for processing.
>  	 * If not, the buffer will be given to driver on next streamon.
>  	 */
> -	if (q->streaming)
> -		__enqueue_in_driver(vb);
> +	if (q->streaming) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret < 0) {
> +			dprintk(1, "qbuf: buffer queue failed\n");
> +			return ret;
> +		}
> +	}
> 
>  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
>  	return 0;
> @@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
>  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  {
>  	struct vb2_buffer *vb;
> +	int ret;
> 
>  	if (q->fileio) {
>  		dprintk(1, "streamon: file io in progress\n");
> @@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> -		__enqueue_in_driver(vb);
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret < 0) {
> +			dprintk(1, "streamon: buffer queue failed\n");
> +			return ret;
> +		}
> +	}
> 
>  	dprintk(3, "Streamon successful\n");
>  	return 0;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 597efe6..3a92f75 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -225,7 +225,7 @@ struct vb2_ops {
>  	int (*start_streaming)(struct vb2_queue *q);
>  	int (*stop_streaming)(struct vb2_queue *q);
> 
> -	void (*buf_queue)(struct vb2_buffer *vb);
> +	int (*buf_queue)(struct vb2_buffer *vb);
>  };
> 
>  /**
> --
> 1.7.3.4

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

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Pawel Osciak]

Hi Laurent,
A few questions from me below. I feel we need to talk about this
change a bit more, it introduces some recovery and consistency
problems, unless I'm missing something.

On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> videobuf2 expects drivers to check buffer in the buf_prepare operation
> and to return success only if the buffer can queued without any issue.
>
> For hot-pluggable devices, disconnection events need to be handled at
> buf_queue time. Checking the disconnected flag and adding the buffer to
> the driver queue need to be performed without releasing the driver
> spinlock, otherwise race conditions can occur in which the driver could
> still allow a buffer to be queued after the disconnected flag has been
> set, resulting in a hang during the next DQBUF operation.
>
> This problem can be solved either in the videobuf2 core or in the device
> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> an int and handle buf_queue failures in videobuf2. Drivers must not
> return an error in buf_queue if the condition leading to the error can
> be caught in buf_prepare.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/videobuf2-core.c |   32 ++++++++++++++++++++++++++------
>  include/media/videobuf2-core.h       |    2 +-
>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index cc7ab0a..1d81536 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  /**
>  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>  */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>        struct vb2_queue *q = vb->vb2_queue;
> +       int ret;
>
>        vb->state = VB2_BUF_STATE_ACTIVE;
>        atomic_inc(&q->queued_count);
> -       q->ops->buf_queue(vb);
> +       ret = q->ops->buf_queue(vb);
> +       if (ret == 0)
> +               return 0;
> +
> +       vb->state = VB2_BUF_STATE_ERROR;
> +       atomic_dec(&q->queued_count);
> +       wake_up_all(&q->done_wq);
> +
> +       return ret;

Unless I am missing something, when this happens for an n-th buffer,
we wake up all, but only one buffer will have the ERROR state, all the
other will be in QUEUED state. This will mess up return values from
dqbuf (if this happens on streamon) for other buffers, they will have
their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
streaming may continue.

So how do we recover from this disconnection event? What is the
general idea? If buf_queue fails, can we restart from some point (i.e.
reuse the buffers later) or do we have to clean up completely (i.e.
deallocate, etc.)? Right now we are staying in an undefined state.
If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
returning a stronger error instead and maybe clean up the rest, which
is not waited for somehow. If we can recover on the other hand, we
shouldn't be probably waking up all sleepers or at least giving them
meaningful errors.

>  }
>  /**
> @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>         * If already streaming, give the buffer to driver for processing.
>         * If not, the buffer will be given to driver on next streamon.
>         */
> -       if (q->streaming)
> -               __enqueue_in_driver(vb);
> +       if (q->streaming) {
> +               ret = __enqueue_in_driver(vb);
> +               if (ret < 0) {
> +                       dprintk(1, "qbuf: buffer queue failed\n");
> +                       return ret;
> +               }
> +       }
>

What errors can be allowed to be returned from driver here? EIO? Also,
isn't returning an error here to userspace suggesting that qbuf didn't
happen? But it actually did happen, we put the buffer onto vb2 list
and set it state to QUEUED. From the point of view of vb2, the buffer
is on its queue, but the userspace may not think so.

>        dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
>        return 0;
> @@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
>  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  {
>        struct vb2_buffer *vb;
> +       int ret;
>
>        if (q->fileio) {
>                dprintk(1, "streamon: file io in progress\n");
> @@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>         * If any buffers were queued before streamon,
>         * we can now pass them to driver for processing.
>         */
> -       list_for_each_entry(vb, &q->queued_list, queued_entry)
> -               __enqueue_in_driver(vb);
> +       list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +               ret = __enqueue_in_driver(vb);
> +               if (ret < 0) {
> +                       dprintk(1, "streamon: buffer queue failed\n");
> +                       return ret;
> +               }
> +       }
>

We need to add new return values from streamon to the API if we want
to return errors here. We'd need to keep them in sync in API with
return values from qbuf as well. Maybe we need to add EIO to return
values from streamon. Is there any other error the driver might want
to return from buf_queue? ENOMEM?

>        dprintk(3, "Streamon successful\n");
>        return 0;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 597efe6..3a92f75 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -225,7 +225,7 @@ struct vb2_ops {
>        int (*start_streaming)(struct vb2_queue *q);
>        int (*stop_streaming)(struct vb2_queue *q);
>
> -       void (*buf_queue)(struct vb2_buffer *vb);
> +       int (*buf_queue)(struct vb2_buffer *vb);
>  };
>

This of course will require an update in documentation above. As we
are returning buf_queue return values to userspace directly, drivers
need to be careful what they return.

-- 
Best regards,
Pawel Osciak

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Laurent Pinchart]

Hi Pawel,

On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
> Hi Laurent,
> A few questions from me below. I feel we need to talk about this
> change a bit more, it introduces some recovery and consistency
> problems, unless I'm missing something.
> 
> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
> > videobuf2 expects drivers to check buffer in the buf_prepare operation
> > and to return success only if the buffer can queued without any issue.
> > 
> > For hot-pluggable devices, disconnection events need to be handled at
> > buf_queue time. Checking the disconnected flag and adding the buffer to
> > the driver queue need to be performed without releasing the driver
> > spinlock, otherwise race conditions can occur in which the driver could
> > still allow a buffer to be queued after the disconnected flag has been
> > set, resulting in a hang during the next DQBUF operation.
> > 
> > This problem can be solved either in the videobuf2 core or in the device
> > drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> > an int and handle buf_queue failures in videobuf2. Drivers must not
> > return an error in buf_queue if the condition leading to the error can
> > be caught in buf_prepare.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/video/videobuf2-core.c |   32
> > ++++++++++++++++++++++++++------ include/media/videobuf2-core.h       |
> >    2 +-
> >  2 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/video/videobuf2-core.c
> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
> > struct v4l2_buffer *b) /**
> >  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
> >  */
> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> >        struct vb2_queue *q = vb->vb2_queue;
> > +       int ret;
> > 
> >        vb->state = VB2_BUF_STATE_ACTIVE;
> >        atomic_inc(&q->queued_count);
> > -       q->ops->buf_queue(vb);
> > +       ret = q->ops->buf_queue(vb);
> > +       if (ret == 0)
> > +               return 0;
> > +
> > +       vb->state = VB2_BUF_STATE_ERROR;
> > +       atomic_dec(&q->queued_count);
> > +       wake_up_all(&q->done_wq);
> > +
> > +       return ret;
> 
> Unless I am missing something, when this happens for an n-th buffer,
> we wake up all, but only one buffer will have the ERROR state, all the
> other will be in QUEUED state. This will mess up return values from
> dqbuf (if this happens on streamon) for other buffers, they will have
> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
> streaming may continue.

Actually not quite, as the driver is expected to mark all buffers as erroneous 
and wake up userspace when the disconnection event is received. Subsequent 
calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not 
sure if we need to wake up userspace then, as applications shouldn't sleep on 
VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an 
error.

> So how do we recover from this disconnection event? What is the
> general idea? If buf_queue fails, can we restart from some point (i.e.
> reuse the buffers later) or do we have to clean up completely (i.e.
> deallocate, etc.)? Right now we are staying in an undefined state.
> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
> returning a stronger error instead and maybe clean up the rest, which
> is not waited for somehow. If we can recover on the other hand, we
> shouldn't be probably waking up all sleepers or at least giving them
> meaningful errors.

I think a disconnection is pretty fatal. If the user unplugs the webcam, 
there's not much that can be done anymore. Userspace needs to be woken up with 
all buffers marked as erroneous, and the next QBUF call needs to return an 
error without queuing any buffer. We need to define the expected behaviour in 
the V4L2 spec, so that applications can rely on it and implement it properly. 
I would also like to handle this inside videobuf2 if possible (something like 
vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not 
sure if that will be possible without messing locking up.

> >  }
> >  /**
> > @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer
> > *b) * If already streaming, give the buffer to driver for processing. *
> > If not, the buffer will be given to driver on next streamon. */
> > -       if (q->streaming)
> > -               __enqueue_in_driver(vb);
> > +       if (q->streaming) {
> > +               ret = __enqueue_in_driver(vb);
> > +               if (ret < 0) {
> > +                       dprintk(1, "qbuf: buffer queue failed\n");
> > +                       return ret;
> > +               }
> > +       }
> 
> What errors can be allowed to be returned from driver here? EIO? Also,
> isn't returning an error here to userspace suggesting that qbuf didn't
> happen? But it actually did happen, we put the buffer onto vb2 list
> and set it state to QUEUED. From the point of view of vb2, the buffer
> is on its queue, but the userspace may not think so.

You're right, that's an issue. The buffer shouldn't be queued at all.

Regarding error codes, I would return -ENXIO (No such device or address - 
POSIX.1) to tell that the device has been disconnected. -ENODEV is misleading, 
it's short description is "No such device", but it means that the device 
doesn't support the requested operation.

> >        dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
> >        return 0;
> > @@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
> >  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> >  {
> >        struct vb2_buffer *vb;
> > +       int ret;
> > 
> >        if (q->fileio) {
> >                dprintk(1, "streamon: file io in progress\n");
> > @@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum
> > v4l2_buf_type type) * If any buffers were queued before streamon,
> >         * we can now pass them to driver for processing.
> >         */
> > -       list_for_each_entry(vb, &q->queued_list, queued_entry)
> > -               __enqueue_in_driver(vb);
> > +       list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +               ret = __enqueue_in_driver(vb);
> > +               if (ret < 0) {
> > +                       dprintk(1, "streamon: buffer queue failed\n");
> > +                       return ret;
> > +               }
> > +       }
> 
> We need to add new return values from streamon to the API if we want
> to return errors here. We'd need to keep them in sync in API with
> return values from qbuf as well. Maybe we need to add EIO to return
> values from streamon. Is there any other error the driver might want
> to return from buf_queue? ENOMEM?

I don't think we should allow anything else than -ENXIO (or -EIO, depending on 
which error we select to mean that the device has been disconnected). Memory 
should be allocated in buf_prepare if needed, not in buf_queue.

> >        dprintk(3, "Streamon successful\n");
> >        return 0;
> > diff --git a/include/media/videobuf2-core.h
> > b/include/media/videobuf2-core.h index 597efe6..3a92f75 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -225,7 +225,7 @@ struct vb2_ops {
> >        int (*start_streaming)(struct vb2_queue *q);
> >        int (*stop_streaming)(struct vb2_queue *q);
> > 
> > -       void (*buf_queue)(struct vb2_buffer *vb);
> > +       int (*buf_queue)(struct vb2_buffer *vb);
> >  };
> 
> This of course will require an update in documentation above. As we
> are returning buf_queue return values to userspace directly, drivers
> need to be careful what they return.

Right.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Pawel Osciak]

Hi Laurent,

On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Pawel,
>
> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>> Hi Laurent,
>> A few questions from me below. I feel we need to talk about this
>> change a bit more, it introduces some recovery and consistency
>> problems, unless I'm missing something.
>>
>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
>> > videobuf2 expects drivers to check buffer in the buf_prepare operation
>> > and to return success only if the buffer can queued without any issue.
>> >
>> > For hot-pluggable devices, disconnection events need to be handled at
>> > buf_queue time. Checking the disconnected flag and adding the buffer to
>> > the driver queue need to be performed without releasing the driver
>> > spinlock, otherwise race conditions can occur in which the driver could
>> > still allow a buffer to be queued after the disconnected flag has been
>> > set, resulting in a hang during the next DQBUF operation.
>> >
>> > This problem can be solved either in the videobuf2 core or in the device
>> > drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
>> > an int and handle buf_queue failures in videobuf2. Drivers must not
>> > return an error in buf_queue if the condition leading to the error can
>> > be caught in buf_prepare.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > ---
>> >  drivers/media/video/videobuf2-core.c |   32
>> > ++++++++++++++++++++++++++------ include/media/videobuf2-core.h       |
>> >    2 +-
>> >  2 files changed, 27 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/media/video/videobuf2-core.c
>> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
>> > --- a/drivers/media/video/videobuf2-core.c
>> > +++ b/drivers/media/video/videobuf2-core.c
>> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
>> > struct v4l2_buffer *b) /**
>> >  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>> >  */
>> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
>> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
>> >  {
>> >        struct vb2_queue *q = vb->vb2_queue;
>> > +       int ret;
>> >
>> >        vb->state = VB2_BUF_STATE_ACTIVE;
>> >        atomic_inc(&q->queued_count);
>> > -       q->ops->buf_queue(vb);
>> > +       ret = q->ops->buf_queue(vb);
>> > +       if (ret == 0)
>> > +               return 0;
>> > +
>> > +       vb->state = VB2_BUF_STATE_ERROR;
>> > +       atomic_dec(&q->queued_count);
>> > +       wake_up_all(&q->done_wq);
>> > +
>> > +       return ret;
>>
>> Unless I am missing something, when this happens for an n-th buffer,
>> we wake up all, but only one buffer will have the ERROR state, all the
>> other will be in QUEUED state. This will mess up return values from
>> dqbuf (if this happens on streamon) for other buffers, they will have
>> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
>> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
>> streaming may continue.
>
> Actually not quite, as the driver is expected to mark all buffers as erroneous
> and wake up userspace when the disconnection event is received. Subsequent
> calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not
> sure if we need to wake up userspace then, as applications shouldn't sleep on
> VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an
> error.
>

Ok, but what do you mean by driver marking them as erroneous? By
issuing vb2_buffer_done with *_ERROR as parameter? Also, you meant
that vb2 should be waking userspace, right? I believe we should aim
for a solution that would require the driver to do as little as
possible and move everything to vb2.
vb2_dqbuf will return EINVAL and poll()/select() should fail because
they check for streaming state. As long as the disconnection event
(e.g. failed qbuf) disables streaming flag in vb2, we should be ok.

>> So how do we recover from this disconnection event? What is the
>> general idea? If buf_queue fails, can we restart from some point (i.e.
>> reuse the buffers later) or do we have to clean up completely (i.e.
>> deallocate, etc.)? Right now we are staying in an undefined state.
>> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
>> returning a stronger error instead and maybe clean up the rest, which
>> is not waited for somehow. If we can recover on the other hand, we
>> shouldn't be probably waking up all sleepers or at least giving them
>> meaningful errors.
>
> I think a disconnection is pretty fatal. If the user unplugs the webcam,
> there's not much that can be done anymore. Userspace needs to be woken up with
> all buffers marked as erroneous, and the next QBUF call needs to return an
> error without queuing any buffer. We need to define the expected behaviour in
> the V4L2 spec, so that applications can rely on it and implement it properly.
> I would also like to handle this inside videobuf2 if possible (something like
> vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not
> sure if that will be possible without messing locking up.
>

I definitely agree that videbuf2 should handle as much as possible and
it shouldn't be left up to drivers. Although I'm not an expert in USB,
shouldn't a disconnection event cause a removal of the device node?
Could you explain how does that work for USB devices in general? If
not, we may need a more general state in vb2, something like "device
inoperable". Not only qbuf should fail then, but almost everything
should. And memory should be freed. I feel there will be the locking
problems as well.

I definitely agree that we need to add this to the V4L2 spec. So could
we start from that point? Could we maybe start with a general flow and
expected behavior for a disconnection event? I guess we both have some
ideas in mind, but first I'd like to establish what will happen in
linux driver/USB core when a device is disconnected. My understanding
is that the driver is removed and module is unloaded, but how does
that happen if we are in the middle of something? Could you give an
example of what happens after a disconnect a camera? Then we could
start designing a general approach, beginning with the API point of
view.

>> >  }
>> >  /**
>> > @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer
>> > *b) * If already streaming, give the buffer to driver for processing. *
>> > If not, the buffer will be given to driver on next streamon. */
>> > -       if (q->streaming)
>> > -               __enqueue_in_driver(vb);
>> > +       if (q->streaming) {
>> > +               ret = __enqueue_in_driver(vb);
>> > +               if (ret < 0) {
>> > +                       dprintk(1, "qbuf: buffer queue failed\n");
>> > +                       return ret;
>> > +               }
>> > +       }
>>
>> What errors can be allowed to be returned from driver here? EIO? Also,
>> isn't returning an error here to userspace suggesting that qbuf didn't
>> happen? But it actually did happen, we put the buffer onto vb2 list
>> and set it state to QUEUED. From the point of view of vb2, the buffer
>> is on its queue, but the userspace may not think so.
>
> You're right, that's an issue. The buffer shouldn't be queued at all.
>

By the way, I'm beginning to think that the simplest way would maybe
be adding a new flag to vb2_buffer_done... The problem however is of
course that there might be a parallel qbuf going on... I really,
really would prefer not putting locks around buf_queue back...

> Regarding error codes, I would return -ENXIO (No such device or address -
> POSIX.1) to tell that the device has been disconnected. -ENODEV is misleading,
> it's short description is "No such device", but it means that the device
> doesn't support the requested operation.
>

I have no preference here, I guess both should be ok.

To sum up, it'd be great if we could design a comprehensive solution
please, starting on the abstract level, i.e. what happens during the
disconnect and how we want to react from the point of view of the API.
Could you describe what happens during a disconnect?

-- 
Best regards,
Pawel Osciak

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Andy Walls]

On Sun, 2011-03-06 at 15:20 -0800, Pawel Osciak wrote:
> Hi Laurent,
> 
> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart


> > I think a disconnection is pretty fatal. If the user unplugs the webcam,
> > there's not much that can be done anymore. Userspace needs to be woken up with
> > all buffers marked as erroneous, and the next QBUF call needs to return an
> > error without queuing any buffer. We need to define the expected behaviour in
> > the V4L2 spec, so that applications can rely on it and implement it properly.
> > I would also like to handle this inside videobuf2 if possible (something like
> > vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not
> > sure if that will be possible without messing locking up.
> >
> 
> I definitely agree that videbuf2 should handle as much as possible and
> it shouldn't be left up to drivers. Although I'm not an expert in USB,
> shouldn't a disconnection event cause a removal of the device node?

Hi Pawel,

I would think it should cause the device node to be unlink()-ed from the
filesystem.

However, even though the device node has been unlink()-ed from the
filesystem, all the currently open file descriptors still exist and need
to be intelligently handled by the driver until they are closed.



> Could you explain how does that work for USB devices in general? If
> not, we may need a more general state in vb2, something like "device
> inoperable". Not only qbuf should fail then, but almost everything
> should. And memory should be freed. I feel there will be the locking
> problems as well.

The USB layer or cdev should take a reference on the driver module so it
can't be unloaded until all the open file descriptors are closed.

The driver itself (and maybe videobuf2?) will need to reference count
structures that must not be kfree()-ed if a file descriptor is still
open.
.


> I definitely agree that we need to add this to the V4L2 spec. So could
> we start from that point? Could we maybe start with a general flow and
> expected behavior for a disconnection event? I guess we both have some
> ideas in mind, but first I'd like to establish what will happen in
> linux driver/USB core when a device is disconnected.


>  My understanding
> is that the driver is removed and module is unloaded, but how does
> that happen if we are in the middle of something? Could you give an
> example of what happens after a disconnect a camera?

A module cannot be unloaded, as long as anything has a reference to the
module text using get_module().  If a file descriptor for the driver is
still open, something should be holding a reference to the driver module
text, so that it cannot be unloaded.

In the case of an underlying device being disconnected, the driver has
to do something sensible as long as file descriptors for that
disconnected device remain open.

In fixing up lirc_zilog, an IR device driver, I used locked reference
counters to get() and put() pointers to objetcs for the underlying
devices.  I then had to modify all the driver code to sensibly handle
the case when a get() of an object pointer to an underlying device came
back as NULL.

I suspect Laurent probably had to deal with similar issues already in
his changes for v4l2_subdev's.



Because it is all still fresh in my mind, what follow are (lengthy)
details about how lirc_zilog handles the problem with a disconnect of an
IR device.  Hopefully it provides something useful for you...

lirc_zilog can have the bridge driver (ivtv, cx18, pvrusb2, or hdpvr)
removed out from under it, or have the underlying USB device
disconnected (pvrusb2, hdpvr), while /dev/lircN device nodes are still
open.

Here is the sum of my rework to support that gracefully:

http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/staging/lirc/lirc_zilog.c;h=40195ef55e6dcfb4b10c8ff132eb81d551a253a8;hb=8a1f6484fd16ef990d588cc3b979416b2dca23bd

It was more work than I expected.

Pointers to instances of the following items in that driver are
reference counted:

	struct IR	// a Z8 IR device instance
	struct IR_tx	// the Tx function instance of a Z8 instance
	struct IR_rx	// the Rx function instance of a Z8 instance

and the structres themselves maintain pointers to each other (which are
not exempt from the reference counting)

                +------------------------+
                v                        |
	struct IR <-------------------+  |
	      | |                     |  |
	      | +----> struct IR_rx --+  |
	      |	                         |
	      +------> struct IR_tx -----+


The pointers to object instances are handed out like this on device
probe; each reference given out is counted:

 a struct IR    pointer is given to the struct IR_rx instance
 a struct IR_rx pointer is given to the struct IR    instance
 a struct IR_rx pointer is given to the RX i2c_client instance

 a struct IR    pointer is given to the struct IR_tx instance
 a struct IR_tx pointer is given to the struct IR    instance
 a struct IR_tx pointer is given to the TX i2c_client instance

 a struct IR pointer is given to the Rx read polling kthread instance


During operation

 on open(),  a struct IR is given    to   the struct file instance
 on close(), a struct IR is returned from the struct file instance

 The lirc_zilog RX polling kthread instance takes and returns struct
	IR_tx pointers as needed.
 The lirc_zilog RX polling kthread instance takes and returns struct
	IR_rx pointers as needed.

 The lirc_zilog file operations, aside from open() and close(), take and
	return struct IR_tx pointers as needed.
 The lirc_zilog file operations, aside from open() and close(), take and
	return struct IR_rx pointers as needed.


Upon device unplug or bridge driver removal:

 a struct IR_rx pointer is returned from the RX i2c_client instance
 a struct IR_tx pointer is returned from the TX i2c_client instance

Upon returning the last pointer to a particular struct IR_tx instance:

 the IR_tx pointer in its parent struct IR instance is set to NULL
 the struct IR_tx instance is kfree()-ed
 the struct IR reference held by that IR_tx is returned	

Upon returning the last pointer to a particular struct IR_rx instance:

 the IR_rx pointer in its parent struct IR instance is set to NULL
 the lirc_zilog Rx polling kthread instance is destroyed
 the struct IR reference held by the polling kthread is returned
 the struct IR_rx instance is kfree()-ed
 a wakeup is performed on the read wait_queue for the poll() fileop
 the struct IR reference held by that IR_rx is returned 

Upon returning the last pointer to a particular struct IR instance:

 the IR unit is unregistered from lirc_dev
 the struct IR instance is removed from lirc_zilog's list of devices
 the struct IR instance is kfree()-ed


Registering with the lirc_dev module prompts lirc_dev to take a
reference to the lirc_zilog module text, so it can't be unloaded.  

Unregistering with lirc_dev causes that reference to the lirc_zilog
module text to be returned.

I'm not sure if cdev will also take and return references to
lirc_zilog's module text on open() and close().

Note that a module cannot take a reference to it's own module text as
that is unreliable due to a race with possible unloading.


Regards,
Andy

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Laurent Pinchart]

Hi Pawel,

On Monday 07 March 2011 00:20:36 Pawel Osciak wrote:
> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote:
> > On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
> >> Hi Laurent,
> >> A few questions from me below. I feel we need to talk about this
> >> change a bit more, it introduces some recovery and consistency
> >> problems, unless I'm missing something.
> >> 
> >> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
> >> > videobuf2 expects drivers to check buffer in the buf_prepare operation
> >> > and to return success only if the buffer can queued without any issue.
> >> > 
> >> > For hot-pluggable devices, disconnection events need to be handled at
> >> > buf_queue time. Checking the disconnected flag and adding the buffer
> >> > to the driver queue need to be performed without releasing the driver
> >> > spinlock, otherwise race conditions can occur in which the driver
> >> > could still allow a buffer to be queued after the disconnected flag
> >> > has been set, resulting in a hang during the next DQBUF operation.
> >> > 
> >> > This problem can be solved either in the videobuf2 core or in the
> >> > device drivers. To avoid adding a spinlock to videobuf2, make
> >> > buf_queue return an int and handle buf_queue failures in videobuf2.
> >> > Drivers must not return an error in buf_queue if the condition
> >> > leading to the error can be caught in buf_prepare.
> >> > 
> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> > ---
> >> >  drivers/media/video/videobuf2-core.c |   32
> >> > ++++++++++++++++++++++++++------ include/media/videobuf2-core.h      
> >> > | 2 +-
> >> >  2 files changed, 27 insertions(+), 7 deletions(-)
> >> > 
> >> > diff --git a/drivers/media/video/videobuf2-core.c
> >> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
> >> > --- a/drivers/media/video/videobuf2-core.c
> >> > +++ b/drivers/media/video/videobuf2-core.c
> >> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
> >> > struct v4l2_buffer *b) /**
> >> >  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for
> >> > processing */
> >> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
> >> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
> >> >  {
> >> >        struct vb2_queue *q = vb->vb2_queue;
> >> > +       int ret;
> >> > 
> >> >        vb->state = VB2_BUF_STATE_ACTIVE;
> >> >        atomic_inc(&q->queued_count);
> >> > -       q->ops->buf_queue(vb);
> >> > +       ret = q->ops->buf_queue(vb);
> >> > +       if (ret == 0)
> >> > +               return 0;
> >> > +
> >> > +       vb->state = VB2_BUF_STATE_ERROR;
> >> > +       atomic_dec(&q->queued_count);
> >> > +       wake_up_all(&q->done_wq);
> >> > +
> >> > +       return ret;
> >> 
> >> Unless I am missing something, when this happens for an n-th buffer,
> >> we wake up all, but only one buffer will have the ERROR state, all the
> >> other will be in QUEUED state. This will mess up return values from
> >> dqbuf (if this happens on streamon) for other buffers, they will have
> >> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
> >> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
> >> streaming may continue.
> > 
> > Actually not quite, as the driver is expected to mark all buffers as
> > erroneous and wake up userspace when the disconnection event is
> > received. Subsequent calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to
> > return an error. I'm not sure if we need to wake up userspace then, as
> > applications shouldn't sleep on VIDIOC_DQBUF or select() after
> > VIDIOC_QBUF or VIDIOC_STREAMON returned an error.
> 
> Ok, but what do you mean by driver marking them as erroneous? By
> issuing vb2_buffer_done with *_ERROR as parameter?

Yes.

> Also, you meant that vb2 should be waking userspace, right?

Yes. If an application is sleeping on a DQBUF or poll() call, it needs to be 
woken up when the device is disconnected.

> I believe we should aim for a solution that would require the driver to do
> as little as possible and move everything to vb2.

I totally agree with this. All drivers should implement the same disconnect 
behaviour, and the required locking is not trivial, so it should be moved to 
vb2.

> vb2_dqbuf will return EINVAL and poll()/select() should fail because
> they check for streaming state. As long as the disconnection event
> (e.g. failed qbuf) disables streaming flag in vb2, we should be ok.

What I'm concerned about is race conditions. Please see below for 
explanations.

> >> So how do we recover from this disconnection event? What is the
> >> general idea? If buf_queue fails, can we restart from some point (i.e.
> >> reuse the buffers later) or do we have to clean up completely (i.e.
> >> deallocate, etc.)? Right now we are staying in an undefined state.
> >> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
> >> returning a stronger error instead and maybe clean up the rest, which
> >> is not waited for somehow. If we can recover on the other hand, we
> >> shouldn't be probably waking up all sleepers or at least giving them
> >> meaningful errors.
> > 
> > I think a disconnection is pretty fatal. If the user unplugs the webcam,
> > there's not much that can be done anymore. Userspace needs to be woken up
> > with all buffers marked as erroneous, and the next QBUF call needs to
> > return an error without queuing any buffer. We need to define the
> > expected behaviour in the V4L2 spec, so that applications can rely on it
> > and implement it properly. I would also like to handle this inside
> > videobuf2 if possible (something like vb2_disconnect() ?) to ensure that
> > all drivers behave correctly, but I'm not sure if that will be possible
> > without messing locking up.
> 
> I definitely agree that videbuf2 should handle as much as possible and
> it shouldn't be left up to drivers. Although I'm not an expert in USB,
> shouldn't a disconnection event cause a removal of the device node?

Yes it should (and probably already does for most drivers), but you can't 
prevent applications from issuing calls on the already opened instances of the 
device node.

> Could you explain how does that work for USB devices in general?

USB drivers are notified of device disconnection by a callback. In a nutshell, 
the callback usually calls video_unregister_device() which removes the device 
node, and decrements a reference count. When the reference count reaches 0, 
all driver-allocated objects are freed. The same reference count is used in 
the open() and release() handlers, guaranteeing that nothing will be freed 
while an application still keeps the device node open.

In parallel, another callback (namely the URB completion handler that is 
called by the USB core when a URB has been transmitted) is called with a error 
status that signals a disconnection. This happens a bit before the 
disconnect() callback. The driver then needs to mark all buffers as erroneous 
and return them to userspace, waking up userspace in the process. Further QBUF 
calls must fail, and further DQBUF and poll() calls must return immediately 
(which shouldn't be an issue, as DQBUF and poll() will return immediately if 
all buffers have been returned to userspace).

> If not, we may need a more general state in vb2, something like "device
> inoperable". Not only qbuf should fail then, but almost everything
> should.

That's correct.

> And memory should be freed.

Memory will be freed the usual way when the application will eventually call 
REQBUFS(0) or just close the file handle.

> I feel there will be the locking problems as well.
>
> I definitely agree that we need to add this to the V4L2 spec. So could
> we start from that point? Could we maybe start with a general flow and
> expected behavior for a disconnection event? I guess we both have some
> ideas in mind, but first I'd like to establish what will happen in
> linux driver/USB core when a device is disconnected. My understanding
> is that the driver is removed and module is unloaded,

Beside the disconnect() callback and completion handlers being called, the 
device reference count is decreased, and the device will be deleted when the 
reference count reaches 0. The module is not unloaded, it just gets unbound 
from the device.

> but how does that happen if we are in the middle of something? Could you
> give an example of what happens after a disconnect a camera? Then we could
> start designing a general approach, beginning with the API point of view.

See my explanation above. As code is usually better understood than English, 
here's how the uvcvideo driver handles it :-)

static void uvc_disconnect(struct usb_interface *intf)
{
        struct uvc_device *dev = usb_get_intfdata(intf);

        /* Set the USB interface data to NULL. This can be done outside the
         * lock, as there's no other reader.
         */
        usb_set_intfdata(intf, NULL);

        if (intf->cur_altsetting->desc.bInterfaceSubClass ==
            UVC_SC_VIDEOSTREAMING)
                return;

        dev->state |= UVC_DEV_DISCONNECTED;

        uvc_unregister_video(dev);
}

As you can see, the disconnect callback more or less calls 
uvc_unregister_video(), which does

static void uvc_unregister_video(struct uvc_device *dev)
{
        struct uvc_streaming *stream;

        /* Unregistering all video devices might result in uvc_delete() being
         *called from inside the loop if there's no open file handle. To avoid
         * that, increment the stream count before iterating over the streams
         * and decrement it when done.
         */
        atomic_inc(&dev->nstreams);

        list_for_each_entry(stream, &dev->streams, list) {
                if (stream->vdev == NULL)
                        continue;

                video_unregister_device(stream->vdev);
                stream->vdev = NULL;
        }

        /* Decrement the stream count and call uvc_delete explicitly if there
         * are no stream left.
         */
        if (atomic_dec_and_test(&dev->nstreams))
                uvc_delete(dev);
}

This calls video_unregister_device() for all video nodes registered by the 
uvcvideo driver, and then calls uvc_delete() if the uvc device reference count 
reaches 0:

static void uvc_delete(struct uvc_device *dev)
{
        struct list_head *p, *n;

        usb_put_intf(dev->intf);
        usb_put_dev(dev->udev);

        uvc_status_cleanup(dev);
        uvc_ctrl_cleanup_device(dev);

        list_for_each_safe(p, n, &dev->chains) {
                struct uvc_video_chain *chain;
                chain = list_entry(p, struct uvc_video_chain, list);
                kfree(chain);
        }
        ...
}

uvc_delete() decrements the reference count on the USB interface and device 
(usb_put_intf() and usb_put_dev() calls), which results in the USB device 
being freed if nothing else uses it, and starts cleaning up by freeing 
everything it has allocated.

On the URB completion handler side, the completion handler calls 
uvc_queue_cancel() when it gets called with a status that indicates a 
disconnection (or any other fatal error):

void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
{
        struct uvc_buffer *buf;
        unsigned long flags;

        spin_lock_irqsave(&queue->irqlock, flags);
        while (!list_empty(&queue->irqqueue)) {
                buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
                                       queue);
                list_del(&buf->queue);
                buf->state = UVC_BUF_STATE_ERROR;
                wake_up(&buf->wait);
        }
        /* This must be protected by the irqlock spinlock to avoid race
         *conditions between uvc_queue_buffer and the disconnection event that
         * could result in an interruptible wait in uvc_dequeue_buffer. Do not
         * blindly replace this logic by checking for the UVC_DEV_DISCONNECTED
         * state outside the queue code.
         */
        if (disconnect)
                queue->flags |= UVC_QUEUE_DISCONNECTED;
        spin_unlock_irqrestore(&queue->irqlock, flags);
}

The UVC_QUEUE_DISCONNECT flag is checked in the QBUF handler:

        spin_lock_irqsave(&queue->irqlock, flags);
        if (queue->flags & UVC_QUEUE_DISCONNECTED) {
                spin_unlock_irqrestore(&queue->irqlock, flags);
                ret = -ENODEV;
                goto done;
        }
        buf->state = UVC_BUF_STATE_QUEUED;
        if (v4l2_buf->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
                buf->buf.bytesused = 0;
        else
                buf->buf.bytesused = v4l2_buf->bytesused;

        list_add_tail(&buf->stream, &queue->mainqueue);
        list_add_tail(&buf->queue, &queue->irqqueue);
        spin_unlock_irqrestore(&queue->irqlock, flags);

This is the logic I would like to replicate in vb2.

> >> >  }
> >> >  /**
> >> > @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct
> >> > v4l2_buffer *b) * If already streaming, give the buffer to driver for
> >> > processing. * If not, the buffer will be given to driver on next
> >> > streamon. */ -       if (q->streaming)
> >> > -               __enqueue_in_driver(vb);
> >> > +       if (q->streaming) {
> >> > +               ret = __enqueue_in_driver(vb);
> >> > +               if (ret < 0) {
> >> > +                       dprintk(1, "qbuf: buffer queue failed\n");
> >> > +                       return ret;
> >> > +               }
> >> > +       }
> >> 
> >> What errors can be allowed to be returned from driver here? EIO? Also,
> >> isn't returning an error here to userspace suggesting that qbuf didn't
> >> happen? But it actually did happen, we put the buffer onto vb2 list
> >> and set it state to QUEUED. From the point of view of vb2, the buffer
> >> is on its queue, but the userspace may not think so.
> > 
> > You're right, that's an issue. The buffer shouldn't be queued at all.
> 
> By the way, I'm beginning to think that the simplest way would maybe
> be adding a new flag to vb2_buffer_done... The problem however is of
> course that there might be a parallel qbuf going on... I really,
> really would prefer not putting locks around buf_queue back...

So would I, but I'm not sure if it's possible to solve the issue without a 
lock around it.
 
> > Regarding error codes, I would return -ENXIO (No such device or address -
> > POSIX.1) to tell that the device has been disconnected. -ENODEV is
> > misleading, it's short description is "No such device", but it means
> > that the device doesn't support the requested operation.
> 
> I have no preference here, I guess both should be ok.
> 
> To sum up, it'd be great if we could design a comprehensive solution
> please, starting on the abstract level, i.e. what happens during the
> disconnect and how we want to react from the point of view of the API.
> Could you describe what happens during a disconnect?

Sure. See above :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Sylwester Nawrocki]

Hi Pawel,

On 03/07/2011 12:20 AM, Pawel Osciak wrote:
> Hi Laurent,
> 
> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>  wrote:
>> Hi Pawel,
>>
>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>>> Hi Laurent,
>>> A few questions from me below. I feel we need to talk about this
>>> change a bit more, it introduces some recovery and consistency
>>> problems, unless I'm missing something.
>>>
>>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
>>>> videobuf2 expects drivers to check buffer in the buf_prepare operation
>>>> and to return success only if the buffer can queued without any issue.
>>>>
>>>> For hot-pluggable devices, disconnection events need to be handled at
>>>> buf_queue time. Checking the disconnected flag and adding the buffer to
>>>> the driver queue need to be performed without releasing the driver
>>>> spinlock, otherwise race conditions can occur in which the driver could
>>>> still allow a buffer to be queued after the disconnected flag has been
>>>> set, resulting in a hang during the next DQBUF operation.
>>>>
>>>> This problem can be solved either in the videobuf2 core or in the device
>>>> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
>>>> an int and handle buf_queue failures in videobuf2. Drivers must not
>>>> return an error in buf_queue if the condition leading to the error can
>>>> be caught in buf_prepare.
>>>>
>>>> Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>>>> ---
>>>>   drivers/media/video/videobuf2-core.c |   32
>>>> ++++++++++++++++++++++++++------ include/media/videobuf2-core.h       |
>>>>     2 +-
>>>>   2 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/videobuf2-core.c
>>>> b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
>>>> --- a/drivers/media/video/videobuf2-core.c
>>>> +++ b/drivers/media/video/videobuf2-core.c
>>>> @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
>>>> struct v4l2_buffer *b) /**
>>>>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>>>>   */
>>>> -static void __enqueue_in_driver(struct vb2_buffer *vb)
>>>> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>>>>   {
>>>>         struct vb2_queue *q = vb->vb2_queue;
>>>> +       int ret;
>>>>
>>>>         vb->state = VB2_BUF_STATE_ACTIVE;
>>>>         atomic_inc(&q->queued_count);
>>>> -       q->ops->buf_queue(vb);
>>>> +       ret = q->ops->buf_queue(vb);
>>>> +       if (ret == 0)
>>>> +               return 0;
>>>> +
>>>> +       vb->state = VB2_BUF_STATE_ERROR;
>>>> +       atomic_dec(&q->queued_count);
>>>> +       wake_up_all(&q->done_wq);
>>>> +
>>>> +       return ret;
>>>
>>> Unless I am missing something, when this happens for an n-th buffer,
>>> we wake up all, but only one buffer will have the ERROR state, all the
>>> other will be in QUEUED state. This will mess up return values from
>>> dqbuf (if this happens on streamon) for other buffers, they will have
>>> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
>>> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
>>> streaming may continue.
>>
>> Actually not quite, as the driver is expected to mark all buffers as erroneous
>> and wake up userspace when the disconnection event is received. Subsequent
>> calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not
>> sure if we need to wake up userspace then, as applications shouldn't sleep on
>> VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an
>> error.
>>
> 
> Ok, but what do you mean by driver marking them as erroneous? By
> issuing vb2_buffer_done with *_ERROR as parameter? Also, you meant
> that vb2 should be waking userspace, right? I believe we should aim
> for a solution that would require the driver to do as little as
> possible and move everything to vb2.
> vb2_dqbuf will return EINVAL and poll()/select() should fail because
> they check for streaming state. As long as the disconnection event
> (e.g. failed qbuf) disables streaming flag in vb2, we should be ok.
> 
>>> So how do we recover from this disconnection event? What is the
>>> general idea? If buf_queue fails, can we restart from some point (i.e.
>>> reuse the buffers later) or do we have to clean up completely (i.e.
>>> deallocate, etc.)? Right now we are staying in an undefined state.
>>> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
>>> returning a stronger error instead and maybe clean up the rest, which
>>> is not waited for somehow. If we can recover on the other hand, we
>>> shouldn't be probably waking up all sleepers or at least giving them
>>> meaningful errors.
>>
>> I think a disconnection is pretty fatal. If the user unplugs the webcam,
>> there's not much that can be done anymore. Userspace needs to be woken up with
>> all buffers marked as erroneous, and the next QBUF call needs to return an
>> error without queuing any buffer. We need to define the expected behaviour in
>> the V4L2 spec, so that applications can rely on it and implement it properly.
>> I would also like to handle this inside videobuf2 if possible (something like
>> vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not
>> sure if that will be possible without messing locking up.
>>
> 
> I definitely agree that videbuf2 should handle as much as possible and
> it shouldn't be left up to drivers. Although I'm not an expert in USB,
> shouldn't a disconnection event cause a removal of the device node?
> Could you explain how does that work for USB devices in general? If
> not, we may need a more general state in vb2, something like "device
> inoperable". Not only qbuf should fail then, but almost everything
> should. And memory should be freed. I feel there will be the locking
> problems as well.
> 
> I definitely agree that we need to add this to the V4L2 spec. So could
> we start from that point? Could we maybe start with a general flow and
> expected behavior for a disconnection event? I guess we both have some
> ideas in mind, but first I'd like to establish what will happen in
> linux driver/USB core when a device is disconnected. My understanding
> is that the driver is removed and module is unloaded, but how does
> that happen if we are in the middle of something? Could you give an
> example of what happens after a disconnect a camera? Then we could
> start designing a general approach, beginning with the API point of
> view.
> 
>>>>   }
>>>>   /**
>>>> @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer
>>>> *b) * If already streaming, give the buffer to driver for processing. *
>>>> If not, the buffer will be given to driver on next streamon. */
>>>> -       if (q->streaming)
>>>> -               __enqueue_in_driver(vb);
>>>> +       if (q->streaming) {
>>>> +               ret = __enqueue_in_driver(vb);
>>>> +               if (ret<  0) {
>>>> +                       dprintk(1, "qbuf: buffer queue failed\n");
>>>> +                       return ret;
>>>> +               }
>>>> +       }
>>>
>>> What errors can be allowed to be returned from driver here? EIO? Also,
>>> isn't returning an error here to userspace suggesting that qbuf didn't
>>> happen? But it actually did happen, we put the buffer onto vb2 list
>>> and set it state to QUEUED. From the point of view of vb2, the buffer
>>> is on its queue, but the userspace may not think so.
>>
>> You're right, that's an issue. The buffer shouldn't be queued at all.
>>
> 
> By the way, I'm beginning to think that the simplest way would maybe
> be adding a new flag to vb2_buffer_done... The problem however is of
> course that there might be a parallel qbuf going on... I really,
> really would prefer not putting locks around buf_queue back...
> 
>> Regarding error codes, I would return -ENXIO (No such device or address -
>> POSIX.1) to tell that the device has been disconnected. -ENODEV is misleading,
>> it's short description is "No such device", but it means that the device
>> doesn't support the requested operation.
>>

As buf_queue callback is called by vb2 only after start_streaming,
for a camera snapshot capture I needed to start a pipeline only from the
buf_queue handler level, i.e. subdev's video s_stream op was called from
within buf_queue. s_stream couldn't be done in the start_streaming handler
as at the time it is invoked there is always no buffer available in the
bridge H/W.
It's a consequence of how the vb2_streamon() is designed.

Before, I used to simply call s_stream in start_streaming, only deferring
the actual bridge DMA enable till a buf_queue call, thus letting first frames
in the stream to be lost. This of course cannot be done in case of single-frame
capture.

To make a long story short, it would be useful in my case to have the ability
to return error codes as per VIDIOC_STREAMON through buf_queue in the driver
(when the first buffer is queued).
At the moment mainly EPIPE comes to my mind. This error code has no meaning
in the API for QBUF though. Should the pipeline be started from buf_queue
the errors from buf_queue would be seen in userspace via VIDIOC_STREAMON
and/or VIDIOC_QBUF.

It should be also possible to signal any errors originating from the subdev
when s_stream is called on it, perhaps by EIO ?

What do you think ?

> 
> I have no preference here, I guess both should be ok.
> 
> To sum up, it'd be great if we could design a comprehensive solution
> please, starting on the abstract level, i.e. what happens during the
> disconnect and how we want to react from the point of view of the API.
> Could you describe what happens during a disconnect?
> 

--
Regards,
Sylwester Nawrocki

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Sylwester Nawrocki]

On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote:
> Hi Pawel,
> 
> On 03/07/2011 12:20 AM, Pawel Osciak wrote:
>> Hi Laurent,
>>
>> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com>   wrote:
>>> Hi Pawel,
>>>
>>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>>>> Hi Laurent,
>>>> A few questions from me below. I feel we need to talk about this
>>>> change a bit more, it introduces some recovery and consistency
>>>> problems, unless I'm missing something.
>>>>
>>>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
>>>>> videobuf2 expects drivers to check buffer in the buf_prepare operation
>>>>> and to return success only if the buffer can queued without any issue.
>>>>>
>>>>> For hot-pluggable devices, disconnection events need to be handled at
>>>>> buf_queue time. Checking the disconnected flag and adding the buffer to
>>>>> the driver queue need to be performed without releasing the driver
>>>>> spinlock, otherwise race conditions can occur in which the driver could
>>>>> still allow a buffer to be queued after the disconnected flag has been
>>>>> set, resulting in a hang during the next DQBUF operation.
>>>>>
>>>>> This problem can be solved either in the videobuf2 core or in the device
>>>>> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
>>>>> an int and handle buf_queue failures in videobuf2. Drivers must not
>>>>> return an error in buf_queue if the condition leading to the error can
>>>>> be caught in buf_prepare.
>>>>>

...
 
> As buf_queue callback is called by vb2 only after start_streaming,
> for a camera snapshot capture I needed to start a pipeline only from the
> buf_queue handler level, i.e. subdev's video s_stream op was called from
> within buf_queue. s_stream couldn't be done in the start_streaming handler
> as at the time it is invoked there is always no buffer available in the
> bridge H/W.
> It's a consequence of how the vb2_streamon() is designed.
> 
> Before, I used to simply call s_stream in start_streaming, only deferring
> the actual bridge DMA enable till a buf_queue call, thus letting first frames
> in the stream to be lost. This of course cannot be done in case of single-frame
> capture.
> 
> To make a long story short, it would be useful in my case to have the ability
> to return error codes as per VIDIOC_STREAMON through buf_queue in the driver
> (when the first buffer is queued).
> At the moment mainly EPIPE comes to my mind. This error code has no meaning
> in the API for QBUF though. Should the pipeline be started from buf_queue

Hmm, the pipeline validation could still be done in start_streaming()
so we can return any EPIPE error from there directly and effectively
in VIDIOC_STREAMON. So the only remaining errors are those related to I2C
communication etc. when streaming is actually enabled in the subdev. 

> the errors from buf_queue would be seen in userspace via VIDIOC_STREAMON
> and/or VIDIOC_QBUF.
> 
> It should be also possible to signal any errors originating from the subdev
> when s_stream is called on it, perhaps by EIO ?
> 
...

--
Regards,
Sylwester

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Laurent Pinchart]

Hi Sylwester,

On Thursday 07 April 2011 00:04:45 Sylwester Nawrocki wrote:
> On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote:
> > On 03/07/2011 12:20 AM, Pawel Osciak wrote:
> >> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote:
> >>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
> >>>> Hi Laurent,
> >>>> A few questions from me below. I feel we need to talk about this
> >>>> change a bit more, it introduces some recovery and consistency
> >>>> problems, unless I'm missing something.
> >>>> 
> >>>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
> >>>>> videobuf2 expects drivers to check buffer in the buf_prepare
> >>>>> operation and to return success only if the buffer can queued
> >>>>> without any issue.
> >>>>> 
> >>>>> For hot-pluggable devices, disconnection events need to be handled at
> >>>>> buf_queue time. Checking the disconnected flag and adding the buffer
> >>>>> to the driver queue need to be performed without releasing the
> >>>>> driver spinlock, otherwise race conditions can occur in which the
> >>>>> driver could still allow a buffer to be queued after the
> >>>>> disconnected flag has been set, resulting in a hang during the next
> >>>>> DQBUF operation.
> >>>>> 
> >>>>> This problem can be solved either in the videobuf2 core or in the
> >>>>> device drivers. To avoid adding a spinlock to videobuf2, make
> >>>>> buf_queue return an int and handle buf_queue failures in videobuf2.
> >>>>> Drivers must not return an error in buf_queue if the condition
> >>>>> leading to the error can be caught in buf_prepare.
> 
> ...
> 
> > As buf_queue callback is called by vb2 only after start_streaming,
> > for a camera snapshot capture I needed to start a pipeline only from the
> > buf_queue handler level, i.e. subdev's video s_stream op was called from
> > within buf_queue. s_stream couldn't be done in the start_streaming
> > handler as at the time it is invoked there is always no buffer available
> > in the bridge H/W.
> > It's a consequence of how the vb2_streamon() is designed.
> > 
> > Before, I used to simply call s_stream in start_streaming, only deferring
> > the actual bridge DMA enable till a buf_queue call, thus letting first
> > frames in the stream to be lost. This of course cannot be done in case
> > of single-frame capture.
> > 
> > To make a long story short, it would be useful in my case to have the
> > ability to return error codes as per VIDIOC_STREAMON through buf_queue
> > in the driver (when the first buffer is queued).
> > At the moment mainly EPIPE comes to my mind. This error code has no
> > meaning in the API for QBUF though. Should the pipeline be started from
> > buf_queue
> 
> Hmm, the pipeline validation could still be done in start_streaming()
> so we can return any EPIPE error from there directly and effectively
> in VIDIOC_STREAMON.

That's correct, and that's what the OMAP3 ISP driver does.

> So the only remaining errors are those related to I2C communication etc.
> when streaming is actually enabled in the subdev.

buf_queue is called with a spinlock help, so you can't perform I2C 
communication there.

> > the errors from buf_queue would be seen in userspace via VIDIOC_STREAMON
> > and/or VIDIOC_QBUF.
> > 
> > It should be also possible to signal any errors originating from the
> > subdev when s_stream is called on it, perhaps by EIO ?

-- 
Regards,

Laurent Pinchart

RE: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Marek Szyprowski]

Hello,

On Friday, April 08, 2011 2:53 PM Laurent Pinchart wrote:

> > ...
> >
> > > As buf_queue callback is called by vb2 only after start_streaming,
> > > for a camera snapshot capture I needed to start a pipeline only from
> the
> > > buf_queue handler level, i.e. subdev's video s_stream op was called
> from
> > > within buf_queue. s_stream couldn't be done in the start_streaming
> > > handler as at the time it is invoked there is always no buffer
> available
> > > in the bridge H/W.
> > > It's a consequence of how the vb2_streamon() is designed.
> > >
> > > Before, I used to simply call s_stream in start_streaming, only
> deferring
> > > the actual bridge DMA enable till a buf_queue call, thus letting first
> > > frames in the stream to be lost. This of course cannot be done in case
> > > of single-frame capture.
> > >
> > > To make a long story short, it would be useful in my case to have the
> > > ability to return error codes as per VIDIOC_STREAMON through buf_queue
> > > in the driver (when the first buffer is queued).
> > > At the moment mainly EPIPE comes to my mind. This error code has no
> > > meaning in the API for QBUF though. Should the pipeline be started from
> > > buf_queue
> >
> > Hmm, the pipeline validation could still be done in start_streaming()
> > so we can return any EPIPE error from there directly and effectively
> > in VIDIOC_STREAMON.
> 
> That's correct, and that's what the OMAP3 ISP driver does.
> 
> > So the only remaining errors are those related to I2C communication etc.
> > when streaming is actually enabled in the subdev.
> 
> buf_queue is called with a spinlock help, so you can't perform I2C
> communication there.

In videobuf2 buf_queue() IS NOT called with any spinlock held. buf_queue can
call functions that require sleeping. This makes a lot of sense especially
for drivers that need to perform a lot of operations for enabling/disabling
hardware.

I remember we discussed your solution where you wanted to add a spinlock for
calling buf_queue. This case shows one more reason not go that way. :)

AFAIR buf_queue callback in old videobuf was called with spinlock held.

I agree that we definitely need more documentation for vb2 and clarification
what is allowed in each callback...

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

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Laurent Pinchart]

Hi Marek,

On Friday 08 April 2011 15:09:02 Marek Szyprowski wrote:
> On Friday, April 08, 2011 2:53 PM Laurent Pinchart wrote:

[snip]

> > buf_queue is called with a spinlock help, so you can't perform I2C
> > communication there.
> 
> In videobuf2 buf_queue() IS NOT called with any spinlock held. buf_queue
> can call functions that require sleeping. This makes a lot of sense
> especially for drivers that need to perform a lot of operations for
> enabling/disabling hardware.

Oops, my bad.

> I remember we discussed your solution where you wanted to add a spinlock
> for calling buf_queue. This case shows one more reason not go that way. :)

Hehe. I totally agree with you that we should avoid locking wherever possible. 
We still have no solution for the disconnection problem though.

> AFAIR buf_queue callback in old videobuf was called with spinlock held.

That's correct, yes.

> I agree that we definitely need more documentation for vb2 and clarification
> what is allowed in each callback...

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors

[Sylwester Nawrocki]

Hi Laurent!

On 04/08/2011 02:53 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Thursday 07 April 2011 00:04:45 Sylwester Nawrocki wrote:
>> On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote:
>>> On 03/07/2011 12:20 AM, Pawel Osciak wrote:
>>>> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote:
>>>>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>>>>>> Hi Laurent,
>>>>>> A few questions from me below. I feel we need to talk about this
>>>>>> change a bit more, it introduces some recovery and consistency
>>>>>> problems, unless I'm missing something.
>>>>>>
>>>>>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
>>>>>>> videobuf2 expects drivers to check buffer in the buf_prepare
>>>>>>> operation and to return success only if the buffer can queued
>>>>>>> without any issue.
>>>>>>>
>>>>>>> For hot-pluggable devices, disconnection events need to be handled at
>>>>>>> buf_queue time. Checking the disconnected flag and adding the buffer
>>>>>>> to the driver queue need to be performed without releasing the
>>>>>>> driver spinlock, otherwise race conditions can occur in which the
>>>>>>> driver could still allow a buffer to be queued after the
>>>>>>> disconnected flag has been set, resulting in a hang during the next
>>>>>>> DQBUF operation.
>>>>>>>
>>>>>>> This problem can be solved either in the videobuf2 core or in the
>>>>>>> device drivers. To avoid adding a spinlock to videobuf2, make
>>>>>>> buf_queue return an int and handle buf_queue failures in videobuf2.
>>>>>>> Drivers must not return an error in buf_queue if the condition
>>>>>>> leading to the error can be caught in buf_prepare.
>>
>> ...
>>
>>> As buf_queue callback is called by vb2 only after start_streaming,
>>> for a camera snapshot capture I needed to start a pipeline only from the
>>> buf_queue handler level, i.e. subdev's video s_stream op was called from
>>> within buf_queue. s_stream couldn't be done in the start_streaming
>>> handler as at the time it is invoked there is always no buffer available
>>> in the bridge H/W.
>>> It's a consequence of how the vb2_streamon() is designed.
>>>
>>> Before, I used to simply call s_stream in start_streaming, only deferring
>>> the actual bridge DMA enable till a buf_queue call, thus letting first
>>> frames in the stream to be lost. This of course cannot be done in case
>>> of single-frame capture.
>>>
>>> To make a long story short, it would be useful in my case to have the
>>> ability to return error codes as per VIDIOC_STREAMON through buf_queue
>>> in the driver (when the first buffer is queued).
>>> At the moment mainly EPIPE comes to my mind. This error code has no
>>> meaning in the API for QBUF though. Should the pipeline be started from
>>> buf_queue
>>
>> Hmm, the pipeline validation could still be done in start_streaming()
>> so we can return any EPIPE error from there directly and effectively
>> in VIDIOC_STREAMON.
> 
> That's correct, and that's what the OMAP3 ISP driver does.
> 
>> So the only remaining errors are those related to I2C communication etc.
>> when streaming is actually enabled in the subdev.
> 
> buf_queue is called with a spinlock help, so you can't perform I2C
> communication there.

This sounds like a really simple requirement - configure the capture format,
allocate and queue a single buffer and trigger the hardware to fill exactly
that buffer. Yet the drivers are forced to perform some acrobatics to achieve
that fairly simple task. 

Luckily videobuf2 does not enforce atomic context in buf_queue as Marek 
pointed out. And I know it from experience as I've experimented with 
S5P FIMC and M-5MOLS drivers for still JPEG capture, on top of your
patch allowing to return errors from buf_queue btw :)

VIDIOC_STREAMON has rather fuzzy meaning for me and I assume there has been
an agreement that allowing drivers to return errors from buf_queue and 
performing blocking I/O from there is the expected way to work around
the freedom the API gives to applications in regards to VIDIOC_STREAMON.
But someone could prove me wrong here. 

I guess there are some issues with streaming over USB when buf_queue
is called in a non atomic context?
I'll have a closer look at your email explaining the USB disconnection 
handling.

--
Regards,
Sylwester Nawrocki