* [RFCv2 PATCH 2/9] vb2: simplify qbuf/prepare_buf by removing callback.
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 3/9] vb2: remove the 'fileio = NULL' hack Hans Verkuil
` (7 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The callback used to merge the common code of the qbuf/prepare_buf
code can be removed now that the mmap_sem handling is pushed down to
__buf_prepare(). This makes the code more readable.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 118 +++++++++++++++----------------
1 file changed, 59 insertions(+), 59 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index c8a1f22b5..0cbe620 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1262,14 +1262,8 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
}
static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
- const char *opname,
- int (*handler)(struct vb2_queue *,
- struct v4l2_buffer *,
- struct vb2_buffer *))
+ const char *opname)
{
- struct vb2_buffer *vb;
- int ret;
-
if (q->fileio) {
dprintk(1, "%s(): file io in progress\n", opname);
return -EBUSY;
@@ -1285,8 +1279,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
return -EINVAL;
}
- vb = q->bufs[b->index];
- if (NULL == vb) {
+ if (q->bufs[b->index] == NULL) {
/* Should never happen */
dprintk(1, "%s(): buffer is NULL\n", opname);
return -EINVAL;
@@ -1297,30 +1290,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
return -EINVAL;
}
- ret = __verify_planes_array(vb, b);
- if (ret)
- return ret;
-
- ret = handler(q, b, vb);
- if (!ret) {
- /* Fill buffer information for the userspace */
- __fill_v4l2_buffer(vb, b);
-
- dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
- }
- return ret;
-}
-
-static int __vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
- struct vb2_buffer *vb)
-{
- if (vb->state != VB2_BUF_STATE_DEQUEUED) {
- dprintk(1, "%s(): invalid buffer state %d\n", __func__,
- vb->state);
- return -EINVAL;
- }
-
- return __buf_prepare(vb, b);
+ return __verify_planes_array(q->bufs[b->index], b);
}
/**
@@ -1340,20 +1310,68 @@ static int __vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
*/
int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
{
- return vb2_queue_or_prepare_buf(q, b, "prepare_buf", __vb2_prepare_buf);
+ int ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
+ struct vb2_buffer *vb;
+
+ if (ret)
+ return ret;
+
+ vb = q->bufs[b->index];
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ dprintk(1, "%s(): invalid buffer state %d\n", __func__,
+ vb->state);
+ return -EINVAL;
+ }
+
+ ret = __buf_prepare(vb, b);
+ if (!ret) {
+ /* Fill buffer information for the userspace */
+ __fill_v4l2_buffer(vb, b);
+
+ dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
+ }
+ return ret;
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
-static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b,
- struct vb2_buffer *vb)
+/**
+ * vb2_qbuf() - Queue a buffer from userspace
+ * @q: videobuf2 queue
+ * @b: buffer structure passed from userspace to vidioc_qbuf handler
+ * in driver
+ *
+ * Should be called from vidioc_qbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
+ * which driver-specific buffer initialization can be performed,
+ * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
+ * callback for processing.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_qbuf handler in driver.
+ */
+int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
{
- int ret;
+ int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
+ struct vb2_buffer *vb;
+
+ if (ret)
+ return ret;
+
+ vb = q->bufs[b->index];
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ dprintk(1, "%s(): invalid buffer state %d\n", __func__,
+ vb->state);
+ return -EINVAL;
+ }
switch (vb->state) {
case VB2_BUF_STATE_DEQUEUED:
ret = __buf_prepare(vb, b);
if (ret)
return ret;
+ break;
case VB2_BUF_STATE_PREPARED:
break;
case VB2_BUF_STATE_PREPARING:
@@ -1378,29 +1396,11 @@ static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b,
if (q->streaming)
__enqueue_in_driver(vb);
- return 0;
-}
+ /* Fill buffer information for the userspace */
+ __fill_v4l2_buffer(vb, b);
-/**
- * vb2_qbuf() - Queue a buffer from userspace
- * @q: videobuf2 queue
- * @b: buffer structure passed from userspace to vidioc_qbuf handler
- * in driver
- *
- * Should be called from vidioc_qbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
- * which driver-specific buffer initialization can be performed,
- * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
- * callback for processing.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_qbuf handler in driver.
- */
-int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
-{
- return vb2_queue_or_prepare_buf(q, b, "qbuf", __vb2_qbuf);
+ dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
+ return 0;
}
EXPORT_SYMBOL_GPL(vb2_qbuf);
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFCv2 PATCH 3/9] vb2: remove the 'fileio = NULL' hack.
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 2/9] vb2: simplify qbuf/prepare_buf by removing callback Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 4/9] vb2: retry start_streaming in case of insufficient buffers Hans Verkuil
` (6 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The read/write implementation in vb2 reuses existing vb2 functions, but
it sets q->fileio to NULL before calling them in order to skip the
'q->fileio != NULL' check.
This works today due to the synchronous nature of read/write, but it
1) is ugly, and 2) will fail in an asynchronous use-case such as a
thread queuing and dequeuing buffers. This last example will be necessary
in order to implement vb2 DVB support.
This patch removes the hack by splitting up the dqbuf/qbuf/streamon/streamoff
functions into an external and an internal version. The external version
checks q->fileio and then calls the internal version. The read/write
implementation now just uses the internal version, removing the need to
set q->fileio to NULL.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 223 ++++++++++++++++---------------
1 file changed, 112 insertions(+), 111 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 0cbe620..9857540 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1264,11 +1264,6 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
const char *opname)
{
- if (q->fileio) {
- dprintk(1, "%s(): file io in progress\n", opname);
- return -EBUSY;
- }
-
if (b->type != q->type) {
dprintk(1, "%s(): invalid buffer type\n", opname);
return -EINVAL;
@@ -1310,9 +1305,15 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
*/
int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
{
- int ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
struct vb2_buffer *vb;
+ int ret;
+
+ if (q->fileio) {
+ dprintk(1, "%s(): file io in progress\n", __func__);
+ return -EBUSY;
+ }
+ ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
if (ret)
return ret;
@@ -1334,24 +1335,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
-/**
- * vb2_qbuf() - Queue a buffer from userspace
- * @q: videobuf2 queue
- * @b: buffer structure passed from userspace to vidioc_qbuf handler
- * in driver
- *
- * Should be called from vidioc_qbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
- * which driver-specific buffer initialization can be performed,
- * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
- * callback for processing.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_qbuf handler in driver.
- */
-int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
+static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
{
int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
struct vb2_buffer *vb;
@@ -1402,6 +1386,33 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
return 0;
}
+
+/**
+ * vb2_qbuf() - Queue a buffer from userspace
+ * @q: videobuf2 queue
+ * @b: buffer structure passed from userspace to vidioc_qbuf handler
+ * in driver
+ *
+ * Should be called from vidioc_qbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
+ * which driver-specific buffer initialization can be performed,
+ * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
+ * callback for processing.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_qbuf handler in driver.
+ */
+int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
+{
+ if (q->fileio) {
+ dprintk(1, "%s(): file io in progress\n", __func__);
+ return -EBUSY;
+ }
+
+ return vb2_internal_qbuf(q, b);
+}
EXPORT_SYMBOL_GPL(vb2_qbuf);
/**
@@ -1550,37 +1561,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
}
}
-/**
- * vb2_dqbuf() - Dequeue a buffer to the userspace
- * @q: videobuf2 queue
- * @b: buffer structure passed from userspace to vidioc_dqbuf handler
- * in driver
- * @nonblocking: if true, this call will not sleep waiting for a buffer if no
- * buffers ready for dequeuing are present. Normally the driver
- * would be passing (file->f_flags & O_NONBLOCK) here
- *
- * Should be called from vidioc_dqbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) calls buf_finish callback in the driver (if provided), in which
- * driver can perform any additional operations that may be required before
- * returning the buffer to userspace, such as cache sync,
- * 3) the buffer struct members are filled with relevant information for
- * the userspace.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_dqbuf handler in driver.
- */
-int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
+static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
{
struct vb2_buffer *vb = NULL;
int ret;
- if (q->fileio) {
- dprintk(1, "dqbuf: file io in progress\n");
- return -EBUSY;
- }
-
if (b->type != q->type) {
dprintk(1, "dqbuf: invalid buffer type\n");
return -EINVAL;
@@ -1619,6 +1604,36 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
return 0;
}
+
+/**
+ * vb2_dqbuf() - Dequeue a buffer to the userspace
+ * @q: videobuf2 queue
+ * @b: buffer structure passed from userspace to vidioc_dqbuf handler
+ * in driver
+ * @nonblocking: if true, this call will not sleep waiting for a buffer if no
+ * buffers ready for dequeuing are present. Normally the driver
+ * would be passing (file->f_flags & O_NONBLOCK) here
+ *
+ * Should be called from vidioc_dqbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) calls buf_finish callback in the driver (if provided), in which
+ * driver can perform any additional operations that may be required before
+ * returning the buffer to userspace, such as cache sync,
+ * 3) the buffer struct members are filled with relevant information for
+ * the userspace.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_dqbuf handler in driver.
+ */
+int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
+{
+ if (q->fileio) {
+ dprintk(1, "dqbuf: file io in progress\n");
+ return -EBUSY;
+ }
+ return vb2_internal_dqbuf(q, b, nonblocking);
+}
EXPORT_SYMBOL_GPL(vb2_dqbuf);
/**
@@ -1658,29 +1673,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
__vb2_dqbuf(q->bufs[i]);
}
-/**
- * vb2_streamon - start streaming
- * @q: videobuf2 queue
- * @type: type argument passed from userspace to vidioc_streamon handler
- *
- * Should be called from vidioc_streamon handler of a driver.
- * This function:
- * 1) verifies current state
- * 2) passes any previously queued buffers to the driver and starts streaming
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_streamon handler in the driver.
- */
-int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
+static int vb2_internal_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");
- return -EBUSY;
- }
-
if (type != q->type) {
dprintk(1, "streamon: invalid stream type\n");
return -EINVAL;
@@ -1713,31 +1710,32 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
dprintk(3, "Streamon successful\n");
return 0;
}
-EXPORT_SYMBOL_GPL(vb2_streamon);
-
/**
- * vb2_streamoff - stop streaming
+ * vb2_streamon - start streaming
* @q: videobuf2 queue
- * @type: type argument passed from userspace to vidioc_streamoff handler
+ * @type: type argument passed from userspace to vidioc_streamon handler
*
- * Should be called from vidioc_streamoff handler of a driver.
+ * Should be called from vidioc_streamon handler of a driver.
* This function:
- * 1) verifies current state,
- * 2) stop streaming and dequeues any queued buffers, including those previously
- * passed to the driver (after waiting for the driver to finish).
+ * 1) verifies current state
+ * 2) passes any previously queued buffers to the driver and starts streaming
*
- * This call can be used for pausing playback.
* The return values from this function are intended to be directly returned
- * from vidioc_streamoff handler in the driver
+ * from vidioc_streamon handler in the driver.
*/
-int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
{
if (q->fileio) {
- dprintk(1, "streamoff: file io in progress\n");
+ dprintk(1, "streamon: file io in progress\n");
return -EBUSY;
}
+ return vb2_internal_streamon(q, type);
+}
+EXPORT_SYMBOL_GPL(vb2_streamon);
+static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+{
if (type != q->type) {
dprintk(1, "streamoff: invalid stream type\n");
return -EINVAL;
@@ -1757,6 +1755,30 @@ int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
dprintk(3, "Streamoff successful\n");
return 0;
}
+
+/**
+ * vb2_streamoff - stop streaming
+ * @q: videobuf2 queue
+ * @type: type argument passed from userspace to vidioc_streamoff handler
+ *
+ * Should be called from vidioc_streamoff handler of a driver.
+ * This function:
+ * 1) verifies current state,
+ * 2) stop streaming and dequeues any queued buffers, including those previously
+ * passed to the driver (after waiting for the driver to finish).
+ *
+ * This call can be used for pausing playback.
+ * The return values from this function are intended to be directly returned
+ * from vidioc_streamoff handler in the driver
+ */
+int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+{
+ if (q->fileio) {
+ dprintk(1, "streamoff: file io in progress\n");
+ return -EBUSY;
+ }
+ return vb2_internal_streamoff(q, type);
+}
EXPORT_SYMBOL_GPL(vb2_streamoff);
/**
@@ -2279,13 +2301,8 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q)
struct vb2_fileio_data *fileio = q->fileio;
if (fileio) {
- /*
- * Hack fileio context to enable direct calls to vb2 ioctl
- * interface.
- */
+ vb2_internal_streamoff(q, q->type);
q->fileio = NULL;
-
- vb2_streamoff(q, q->type);
fileio->req.count = 0;
vb2_reqbufs(q, &fileio->req);
kfree(fileio);
@@ -2328,12 +2345,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
}
fileio = q->fileio;
- /*
- * Hack fileio context to enable direct calls to vb2 ioctl interface.
- * The pointer will be restored before returning from this function.
- */
- q->fileio = NULL;
-
index = fileio->index;
buf = &fileio->bufs[index];
@@ -2350,10 +2361,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
fileio->b.type = q->type;
fileio->b.memory = q->memory;
fileio->b.index = index;
- ret = vb2_dqbuf(q, &fileio->b, nonblock);
+ ret = vb2_internal_dqbuf(q, &fileio->b, nonblock);
dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
if (ret)
- goto end;
+ return ret;
fileio->dq_count += 1;
/*
@@ -2383,8 +2394,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
ret = copy_from_user(buf->vaddr + buf->pos, data, count);
if (ret) {
dprintk(3, "file io: error copying data\n");
- ret = -EFAULT;
- goto end;
+ return -EFAULT;
}
/*
@@ -2404,10 +2414,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
if (read && (fileio->flags & VB2_FILEIO_READ_ONCE) &&
fileio->dq_count == 1) {
dprintk(3, "file io: read limit reached\n");
- /*
- * Restore fileio pointer and release the context.
- */
- q->fileio = fileio;
return __vb2_cleanup_fileio(q);
}
@@ -2419,10 +2425,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
fileio->b.memory = q->memory;
fileio->b.index = index;
fileio->b.bytesused = buf->pos;
- ret = vb2_qbuf(q, &fileio->b);
+ ret = vb2_internal_qbuf(q, &fileio->b);
dprintk(5, "file io: vb2_dbuf result: %d\n", ret);
if (ret)
- goto end;
+ return ret;
/*
* Buffer has been queued, update the status
@@ -2441,9 +2447,9 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
* Start streaming if required.
*/
if (!read && !q->streaming) {
- ret = vb2_streamon(q, q->type);
+ ret = vb2_internal_streamon(q, q->type);
if (ret)
- goto end;
+ return ret;
}
}
@@ -2452,11 +2458,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
*/
if (ret == 0)
ret = count;
-end:
- /*
- * Restore the fileio context and block vb2 ioctl interface.
- */
- q->fileio = fileio;
return ret;
}
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFCv2 PATCH 4/9] vb2: retry start_streaming in case of insufficient buffers.
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 2/9] vb2: simplify qbuf/prepare_buf by removing callback Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 3/9] vb2: remove the 'fileio = NULL' hack Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-12-04 13:42 ` Marek Szyprowski
2013-11-29 9:58 ` [RFCv2 PATCH 5/9] vb2: don't set index, don't start streaming for write() Hans Verkuil
` (5 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
If start_streaming returns -ENODATA, then it will be retried the next time
a buffer is queued. This means applications no longer need to know how many
buffers need to be queued before STREAMON can be called. This is particularly
useful for output stream I/O.
If a DMA engine needs at least X buffers before it can start streaming, then
for applications to get a buffer out as soon as possible they need to know
the minimum number of buffers to queue before STREAMON can be called. You can't
just try STREAMON after every buffer since on failure STREAMON will dequeue
all your buffers. (Is that a bug or a feature? Frankly, I'm not sure).
This patch simplifies applications substantially: they can just call STREAMON
at the beginning and then start queuing buffers and the DMA engine will
kick in automagically once enough buffers are available.
This also fixes using write() to stream video: the fileio implementation
calls streamon without having any queued buffers, which will fail today for
any driver that requires a minimum number of buffers.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 68 ++++++++++++++++++++++++++------
include/media/videobuf2-core.h | 15 +++++--
2 files changed, 66 insertions(+), 17 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 9857540..db1104f 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1335,6 +1335,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
+/**
+ * vb2_start_streaming() - Attempt to start streaming.
+ * @q: videobuf2 queue
+ *
+ * If there are not enough buffers, then retry_start_streaming is set to
+ * 1 and 0 is returned. The next time a buffer is queued and
+ * retry_start_streaming is 1, this function will be called again to
+ * retry starting the DMA engine.
+ */
+static int vb2_start_streaming(struct vb2_queue *q)
+{
+ int ret;
+
+ /* Tell the driver to start streaming */
+ ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
+
+ /*
+ * If there are not enough buffers queued to start streaming, then
+ * the start_streaming operation will return -ENODATA and you have to
+ * retry when the next buffer is queued.
+ */
+ if (ret == -ENODATA) {
+ dprintk(1, "qbuf: not enough buffers, retry when more buffers are queued.\n");
+ q->retry_start_streaming = 1;
+ return 0;
+ }
+ if (ret)
+ dprintk(1, "qbuf: driver refused to start streaming\n");
+ else
+ q->retry_start_streaming = 0;
+ return ret;
+}
+
static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
{
int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
@@ -1383,6 +1416,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
/* Fill buffer information for the userspace */
__fill_v4l2_buffer(vb, b);
+ if (q->retry_start_streaming) {
+ ret = vb2_start_streaming(q);
+ if (ret)
+ return ret;
+ }
+
dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
return 0;
}
@@ -1532,7 +1571,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
return -EINVAL;
}
- wait_event(q->done_wq, !atomic_read(&q->queued_count));
+ if (!q->retry_start_streaming)
+ wait_event(q->done_wq, !atomic_read(&q->queued_count));
return 0;
}
EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
@@ -1646,6 +1686,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
{
unsigned int i;
+ if (q->retry_start_streaming) {
+ q->retry_start_streaming = 0;
+ q->streaming = 0;
+ }
+
/*
* Tell driver to stop all transactions and release all queued
* buffers.
@@ -1695,12 +1740,9 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
list_for_each_entry(vb, &q->queued_list, queued_entry)
__enqueue_in_driver(vb);
- /*
- * Let driver notice that streaming state has been enabled.
- */
- ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
+ /* Tell driver to start streaming. */
+ ret = vb2_start_streaming(q);
if (ret) {
- dprintk(1, "streamon: driver refused to start streaming\n");
__vb2_queue_cancel(q);
return ret;
}
@@ -2270,15 +2312,15 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
goto err_reqbufs;
fileio->bufs[i].queued = 1;
}
-
- /*
- * Start streaming.
- */
- ret = vb2_streamon(q, q->type);
- if (ret)
- goto err_reqbufs;
}
+ /*
+ * Start streaming.
+ */
+ ret = vb2_streamon(q, q->type);
+ if (ret)
+ goto err_reqbufs;
+
q->fileio = fileio;
return ret;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4bc4ad2..1be7f39 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -252,10 +252,13 @@ struct vb2_buffer {
* receive buffers with @buf_queue callback before
* @start_streaming is called; the driver gets the number
* of already queued buffers in count parameter; driver
- * can return an error if hardware fails or not enough
- * buffers has been queued, in such case all buffers that
- * have been already given by the @buf_queue callback are
- * invalidated.
+ * can return an error if hardware fails, in that case all
+ * buffers that have been already given by the @buf_queue
+ * callback are invalidated.
+ * If there were not enough queued buffers to start
+ * streaming, then this callback returns -ENODATA, and the
+ * vb2 core will retry calling @start_streaming when a new
+ * buffer is queued.
* @stop_streaming: called when 'streaming' state must be disabled; driver
* should stop any DMA transactions or wait until they
* finish and give back all buffers it got from buf_queue()
@@ -323,6 +326,9 @@ struct v4l2_fh;
* @done_wq: waitqueue for processes waiting for buffers ready to be dequeued
* @alloc_ctx: memory type/allocator-specific contexts for each plane
* @streaming: current streaming state
+ * @retry_start_streaming: start_streaming() was called, but there were not enough
+ * buffers queued. If set, then retry calling start_streaming when
+ * queuing a new buffer.
* @fileio: file io emulator internal data, used only if emulator is active
*/
struct vb2_queue {
@@ -355,6 +361,7 @@ struct vb2_queue {
unsigned int plane_sizes[VIDEO_MAX_PLANES];
unsigned int streaming:1;
+ unsigned int retry_start_streaming:1;
struct vb2_fileio_data *fileio;
};
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 4/9] vb2: retry start_streaming in case of insufficient buffers.
2013-11-29 9:58 ` [RFCv2 PATCH 4/9] vb2: retry start_streaming in case of insufficient buffers Hans Verkuil
@ 2013-12-04 13:42 ` Marek Szyprowski
0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2013-12-04 13:42 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: pawel, laurent.pinchart, awalls, Hans Verkuil
Hello,
On 2013-11-29 10:58, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> If start_streaming returns -ENODATA, then it will be retried the next time
> a buffer is queued. This means applications no longer need to know how many
> buffers need to be queued before STREAMON can be called. This is particularly
> useful for output stream I/O.
>
> If a DMA engine needs at least X buffers before it can start streaming, then
> for applications to get a buffer out as soon as possible they need to know
> the minimum number of buffers to queue before STREAMON can be called. You can't
> just try STREAMON after every buffer since on failure STREAMON will dequeue
> all your buffers. (Is that a bug or a feature? Frankly, I'm not sure).
>
> This patch simplifies applications substantially: they can just call STREAMON
> at the beginning and then start queuing buffers and the DMA engine will
> kick in automagically once enough buffers are available.
>
> This also fixes using write() to stream video: the fileio implementation
> calls streamon without having any queued buffers, which will fail today for
> any driver that requires a minimum number of buffers.
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
This patch recalls me the discussion whether it should be possible to do
STREAM_ON
before queuing the buffers or not. Your approach slightly changes the
userspace
api, but I don't expect any problems from that. The only possible
drawback of this
approach is the lack of information about real hw streaming in
userspace, but I
doubt that there is any application which relies on it.
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 68 ++++++++++++++++++++++++++------
> include/media/videobuf2-core.h | 15 +++++--
> 2 files changed, 66 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 9857540..db1104f 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1335,6 +1335,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
> }
> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>
> +/**
> + * vb2_start_streaming() - Attempt to start streaming.
> + * @q: videobuf2 queue
> + *
> + * If there are not enough buffers, then retry_start_streaming is set to
> + * 1 and 0 is returned. The next time a buffer is queued and
> + * retry_start_streaming is 1, this function will be called again to
> + * retry starting the DMA engine.
> + */
> +static int vb2_start_streaming(struct vb2_queue *q)
> +{
> + int ret;
> +
> + /* Tell the driver to start streaming */
> + ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> +
> + /*
> + * If there are not enough buffers queued to start streaming, then
> + * the start_streaming operation will return -ENODATA and you have to
> + * retry when the next buffer is queued.
> + */
> + if (ret == -ENODATA) {
> + dprintk(1, "qbuf: not enough buffers, retry when more buffers are queued.\n");
> + q->retry_start_streaming = 1;
> + return 0;
> + }
> + if (ret)
> + dprintk(1, "qbuf: driver refused to start streaming\n");
> + else
> + q->retry_start_streaming = 0;
> + return ret;
> +}
> +
> static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> {
> int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> @@ -1383,6 +1416,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> /* Fill buffer information for the userspace */
> __fill_v4l2_buffer(vb, b);
>
> + if (q->retry_start_streaming) {
> + ret = vb2_start_streaming(q);
> + if (ret)
> + return ret;
> + }
> +
> dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
> return 0;
> }
> @@ -1532,7 +1571,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
> return -EINVAL;
> }
>
> - wait_event(q->done_wq, !atomic_read(&q->queued_count));
> + if (!q->retry_start_streaming)
> + wait_event(q->done_wq, !atomic_read(&q->queued_count));
> return 0;
> }
> EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> @@ -1646,6 +1686,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> {
> unsigned int i;
>
> + if (q->retry_start_streaming) {
> + q->retry_start_streaming = 0;
> + q->streaming = 0;
> + }
> +
> /*
> * Tell driver to stop all transactions and release all queued
> * buffers.
> @@ -1695,12 +1740,9 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> list_for_each_entry(vb, &q->queued_list, queued_entry)
> __enqueue_in_driver(vb);
>
> - /*
> - * Let driver notice that streaming state has been enabled.
> - */
> - ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> + /* Tell driver to start streaming. */
> + ret = vb2_start_streaming(q);
> if (ret) {
> - dprintk(1, "streamon: driver refused to start streaming\n");
> __vb2_queue_cancel(q);
> return ret;
> }
> @@ -2270,15 +2312,15 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> goto err_reqbufs;
> fileio->bufs[i].queued = 1;
> }
> -
> - /*
> - * Start streaming.
> - */
> - ret = vb2_streamon(q, q->type);
> - if (ret)
> - goto err_reqbufs;
> }
>
> + /*
> + * Start streaming.
> + */
> + ret = vb2_streamon(q, q->type);
> + if (ret)
> + goto err_reqbufs;
> +
> q->fileio = fileio;
>
> return ret;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4bc4ad2..1be7f39 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -252,10 +252,13 @@ struct vb2_buffer {
> * receive buffers with @buf_queue callback before
> * @start_streaming is called; the driver gets the number
> * of already queued buffers in count parameter; driver
> - * can return an error if hardware fails or not enough
> - * buffers has been queued, in such case all buffers that
> - * have been already given by the @buf_queue callback are
> - * invalidated.
> + * can return an error if hardware fails, in that case all
> + * buffers that have been already given by the @buf_queue
> + * callback are invalidated.
> + * If there were not enough queued buffers to start
> + * streaming, then this callback returns -ENODATA, and the
> + * vb2 core will retry calling @start_streaming when a new
> + * buffer is queued.
> * @stop_streaming: called when 'streaming' state must be disabled; driver
> * should stop any DMA transactions or wait until they
> * finish and give back all buffers it got from buf_queue()
> @@ -323,6 +326,9 @@ struct v4l2_fh;
> * @done_wq: waitqueue for processes waiting for buffers ready to be dequeued
> * @alloc_ctx: memory type/allocator-specific contexts for each plane
> * @streaming: current streaming state
> + * @retry_start_streaming: start_streaming() was called, but there were not enough
> + * buffers queued. If set, then retry calling start_streaming when
> + * queuing a new buffer.
> * @fileio: file io emulator internal data, used only if emulator is active
> */
> struct vb2_queue {
> @@ -355,6 +361,7 @@ struct vb2_queue {
> unsigned int plane_sizes[VIDEO_MAX_PLANES];
>
> unsigned int streaming:1;
> + unsigned int retry_start_streaming:1;
>
> struct vb2_fileio_data *fileio;
> };
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFCv2 PATCH 5/9] vb2: don't set index, don't start streaming for write()
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (2 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 4/9] vb2: retry start_streaming in case of insufficient buffers Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 6/9] vb2: return ENODATA in start_streaming in case of too few buffers Hans Verkuil
` (4 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Two fixes:
- there is no need to set the index when calling dqbuf: dqbuf will
overwrite it.
- __vb2_init_fileio already starts streaming for write(), so there is
no need to do it again in __vb2_perform_fileio. It can never have
worked anyway: either __vb2_init_fileio succeeds in starting streaming
or it is never going to happen.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index db1104f..853d391 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2402,7 +2402,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
memset(&fileio->b, 0, sizeof(fileio->b));
fileio->b.type = q->type;
fileio->b.memory = q->memory;
- fileio->b.index = index;
ret = vb2_internal_dqbuf(q, &fileio->b, nonblock);
dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
if (ret)
@@ -2484,15 +2483,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
* Switch to the next buffer
*/
fileio->index = (index + 1) % q->num_buffers;
-
- /*
- * Start streaming if required.
- */
- if (!read && !q->streaming) {
- ret = vb2_internal_streamon(q, q->type);
- if (ret)
- return ret;
- }
}
/*
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFCv2 PATCH 6/9] vb2: return ENODATA in start_streaming in case of too few buffers.
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (3 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 5/9] vb2: don't set index, don't start streaming for write() Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 7/9] vb2: add thread support Hans Verkuil
` (3 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/platform/davinci/vpbe_display.c | 2 +-
drivers/media/platform/davinci/vpif_capture.c | 2 +-
drivers/media/platform/davinci/vpif_display.c | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
drivers/media/platform/s5p-tv/mixer_video.c | 2 +-
drivers/media/platform/soc_camera/mx2_camera.c | 2 +-
drivers/staging/media/davinci_vpfe/vpfe_video.c | 2 ++
7 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/davinci/vpbe_display.c b/drivers/media/platform/davinci/vpbe_display.c
index eac472b..53be7fc 100644
--- a/drivers/media/platform/davinci/vpbe_display.c
+++ b/drivers/media/platform/davinci/vpbe_display.c
@@ -347,7 +347,7 @@ static int vpbe_start_streaming(struct vb2_queue *vq, unsigned int count)
/* If buffer queue is empty, return error */
if (list_empty(&layer->dma_queue)) {
v4l2_err(&vpbe_dev->v4l2_dev, "buffer queue is empty\n");
- return -EINVAL;
+ return -ENODATA;
}
/* Get the next frame from the buffer queue */
layer->next_frm = layer->cur_frm = list_entry(layer->dma_queue.next,
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 52ac5e6..4b04a27 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -277,7 +277,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
if (list_empty(&common->dma_queue)) {
spin_unlock_irqrestore(&common->irqlock, flags);
vpif_dbg(1, debug, "buffer queue is empty\n");
- return -EIO;
+ return -ENODATA;
}
/* Get the next frame from the buffer queue */
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index c31bcf1..c5070dc 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -239,7 +239,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
if (list_empty(&common->dma_queue)) {
spin_unlock_irqrestore(&common->irqlock, flags);
vpif_err("buffer queue is empty\n");
- return -EIO;
+ return -ENODATA;
}
/* Get the next frame from the buffer queue */
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4ff3b6c..3bdfe85 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1863,7 +1863,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
if (ctx->src_bufs_cnt < ctx->pb_count) {
mfc_err("Need minimum %d OUTPUT buffers\n",
ctx->pb_count);
- return -EINVAL;
+ return -ENODATA;
}
}
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 641b1f0..7d2fe64 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -948,7 +948,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
if (count == 0) {
mxr_dbg(mdev, "no output buffers queued\n");
- return -EINVAL;
+ return -ENODATA;
}
/* block any changes in output configuration */
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 45a0276..587e3d1 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -659,7 +659,7 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
unsigned long flags;
if (count < 2)
- return -EINVAL;
+ return -ENODATA;
spin_lock_irqsave(&pcdev->lock, flags);
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index 24d98a6..a81b0ab 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -1201,6 +1201,8 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
unsigned long addr;
int ret;
+ if (count == 0)
+ return -ENODATA;
ret = mutex_lock_interruptible(&video->lock);
if (ret)
goto streamoff;
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFCv2 PATCH 7/9] vb2: add thread support
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (4 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 6/9] vb2: return ENODATA in start_streaming in case of too few buffers Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 18:21 ` Laurent Pinchart
2013-11-29 9:58 ` [RFCv2 PATCH 8/9] vb2: Add videobuf2-dvb support Hans Verkuil
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
In order to implement vb2 DVB or ALSA support you need to be able to start
a kernel thread that queues and dequeues buffers, calling a callback
function for every captured/displayed buffer. This patch adds support for
that.
It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the DVB
specific stuff stripped out, thus making it much more generic.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 134 +++++++++++++++++++++++++++++++
include/media/videobuf2-core.h | 28 +++++++
2 files changed, 162 insertions(+)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 853d391..a86d033 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -6,6 +6,9 @@
* Author: Pawel Osciak <pawel@osciak.com>
* Marek Szyprowski <m.szyprowski@samsung.com>
*
+ * The vb2_thread implementation was based on code from videobuf-dvb.c:
+ * (c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
+ *
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation.
@@ -18,6 +21,8 @@
#include <linux/poll.h>
#include <linux/slab.h>
#include <linux/sched.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <media/v4l2-dev.h>
#include <media/v4l2-fh.h>
@@ -2508,6 +2513,135 @@ size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t count,
}
EXPORT_SYMBOL_GPL(vb2_write);
+struct vb2_threadio_data {
+ struct task_struct *thread;
+ vb2_thread_fnc fnc;
+ void *priv;
+ bool stop;
+};
+
+static int vb2_thread(void *data)
+{
+ struct vb2_queue *q = data;
+ struct vb2_threadio_data *threadio = q->threadio;
+ struct vb2_fileio_data *fileio = q->fileio;
+ int prequeue = 0;
+ int index = 0;
+ int ret = 0;
+
+ if (V4L2_TYPE_IS_OUTPUT(q->type))
+ prequeue = q->num_buffers;
+
+ set_freezable();
+
+ for (;;) {
+ struct vb2_buffer *vb;
+
+ /*
+ * Call vb2_dqbuf to get buffer back.
+ */
+ memset(&fileio->b, 0, sizeof(fileio->b));
+ fileio->b.type = q->type;
+ fileio->b.memory = q->memory;
+ if (prequeue) {
+ fileio->b.index = index++;
+ prequeue--;
+ } else {
+ call_qop(q, wait_finish, q);
+ ret = vb2_internal_dqbuf(q, &fileio->b, 0);
+ call_qop(q, wait_prepare, q);
+ dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
+ }
+ if (threadio->stop)
+ break;
+ if (ret)
+ break;
+ try_to_freeze();
+
+ vb = q->bufs[fileio->b.index];
+ if (!(fileio->b.flags & V4L2_BUF_FLAG_ERROR))
+ ret = threadio->fnc(vb, threadio->priv);
+ if (ret)
+ break;
+ call_qop(q, wait_finish, q);
+ ret = vb2_internal_qbuf(q, &fileio->b);
+ call_qop(q, wait_prepare, q);
+ if (ret)
+ break;
+ }
+
+ /* Hmm, linux becomes *very* unhappy without this ... */
+ while (!kthread_should_stop()) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
+ }
+ return 0;
+}
+
+int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
+ const char *thread_name)
+{
+ struct vb2_threadio_data *threadio;
+ int ret = 0;
+
+ if (q->threadio)
+ return -EBUSY;
+ if (vb2_is_busy(q))
+ return -EBUSY;
+ if (WARN_ON(q->fileio))
+ return -EBUSY;
+
+ threadio = kzalloc(sizeof(*threadio), GFP_KERNEL);
+ if (threadio == NULL)
+ return -ENOMEM;
+ threadio->fnc = fnc;
+ threadio->priv = priv;
+
+ ret = __vb2_init_fileio(q, !V4L2_TYPE_IS_OUTPUT(q->type));
+ dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
+ if (ret)
+ goto nomem;
+ q->threadio = threadio;
+ threadio->thread = kthread_run(vb2_thread, q, "vb2-%s", thread_name);
+ if (IS_ERR(threadio->thread)) {
+ ret = PTR_ERR(threadio->thread);
+ threadio->thread = NULL;
+ goto nothread;
+ }
+ return 0;
+
+nothread:
+ __vb2_cleanup_fileio(q);
+nomem:
+ kfree(threadio);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_thread_start);
+
+int vb2_thread_stop(struct vb2_queue *q)
+{
+ struct vb2_threadio_data *threadio = q->threadio;
+ struct vb2_fileio_data *fileio = q->fileio;
+ int err;
+
+ if (threadio == NULL)
+ return 0;
+ call_qop(q, wait_finish, q);
+ threadio->stop = true;
+ vb2_internal_streamoff(q, q->type);
+ call_qop(q, wait_prepare, q);
+ q->fileio = NULL;
+ fileio->req.count = 0;
+ vb2_reqbufs(q, &fileio->req);
+ kfree(fileio);
+ err = kthread_stop(threadio->thread);
+ threadio->thread = NULL;
+ kfree(threadio);
+ q->fileio = NULL;
+ q->threadio = NULL;
+ return err;
+}
+EXPORT_SYMBOL_GPL(vb2_thread_stop);
/*
* The following functions are not part of the vb2 core API, but are helper
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 1be7f39..7dea795 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -20,6 +20,7 @@
struct vb2_alloc_ctx;
struct vb2_fileio_data;
+struct vb2_threadio_data;
/**
* struct vb2_mem_ops - memory handling/memory allocator operations
@@ -330,6 +331,7 @@ struct v4l2_fh;
* buffers queued. If set, then retry calling start_streaming when
* queuing a new buffer.
* @fileio: file io emulator internal data, used only if emulator is active
+ * @threadio: thread io internal data, used only if thread is active
*/
struct vb2_queue {
enum v4l2_buf_type type;
@@ -364,6 +366,7 @@ struct vb2_queue {
unsigned int retry_start_streaming:1;
struct vb2_fileio_data *fileio;
+ struct vb2_threadio_data *threadio;
};
void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
@@ -402,6 +405,31 @@ size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
loff_t *ppos, int nonblock);
size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t count,
loff_t *ppos, int nonblock);
+/**
+ * vb2_thread_fnc - callback function for use with vb2_thread
+ *
+ * This is called whenever a buffer is dequeued in the thread.
+ */
+typedef int (*vb2_thread_fnc)(struct vb2_buffer *vb, void *priv);
+
+/**
+ * vb2_thread_start() - start a thread for the given queue.
+ * @q: videobuf queue
+ * @fnc: callback function
+ * @priv: priv pointer passed to the callback function
+ * @thread_name:the name of the thread. This will be prefixed with "vb2-".
+ *
+ * This starts a thread that will queue and dequeue until an error occurs
+ * or @vb2_thread_stop is called.
+ */
+int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
+ const char *thread_name);
+
+/**
+ * vb2_thread_stop() - stop the thread for the given queue.
+ * @q: videobuf queue
+ */
+int vb2_thread_stop(struct vb2_queue *q);
/**
* vb2_is_streaming() - return streaming status of the queue
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-11-29 9:58 ` [RFCv2 PATCH 7/9] vb2: add thread support Hans Verkuil
@ 2013-11-29 18:21 ` Laurent Pinchart
2013-12-03 9:56 ` Hans Verkuil
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2013-11-29 18:21 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
Thank you for the patch.
On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> In order to implement vb2 DVB or ALSA support you need to be able to start
> a kernel thread that queues and dequeues buffers, calling a callback
> function for every captured/displayed buffer. This patch adds support for
> that.
>
> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the DVB
> specific stuff stripped out, thus making it much more generic.
Do you see any use for this outside of videobuf2-dvb ? If not I wonder whether
the code shouldn't be moved there. The sync objects framework being developed
for KMS will in my opinion cover the other use cases, and I'd like to
discourage non-DVB drivers to use vb2 threads in the meantime.
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 134 ++++++++++++++++++++++++++++
> include/media/videobuf2-core.h | 28 +++++++
> 2 files changed, 162 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 853d391..a86d033 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -6,6 +6,9 @@
> * Author: Pawel Osciak <pawel@osciak.com>
> * Marek Szyprowski <m.szyprowski@samsung.com>
> *
> + * The vb2_thread implementation was based on code from videobuf-dvb.c:
> + * (c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
> + *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> * the Free Software Foundation.
> @@ -18,6 +21,8 @@
> #include <linux/poll.h>
> #include <linux/slab.h>
> #include <linux/sched.h>
> +#include <linux/freezer.h>
> +#include <linux/kthread.h>
>
> #include <media/v4l2-dev.h>
> #include <media/v4l2-fh.h>
> @@ -2508,6 +2513,135 @@ size_t vb2_write(struct vb2_queue *q, const char
> __user *data, size_t count, }
> EXPORT_SYMBOL_GPL(vb2_write);
>
> +struct vb2_threadio_data {
> + struct task_struct *thread;
> + vb2_thread_fnc fnc;
> + void *priv;
> + bool stop;
> +};
> +
> +static int vb2_thread(void *data)
> +{
> + struct vb2_queue *q = data;
> + struct vb2_threadio_data *threadio = q->threadio;
> + struct vb2_fileio_data *fileio = q->fileio;
> + int prequeue = 0;
> + int index = 0;
> + int ret = 0;
> +
> + if (V4L2_TYPE_IS_OUTPUT(q->type))
> + prequeue = q->num_buffers;
> +
> + set_freezable();
> +
> + for (;;) {
> + struct vb2_buffer *vb;
> +
> + /*
> + * Call vb2_dqbuf to get buffer back.
> + */
> + memset(&fileio->b, 0, sizeof(fileio->b));
> + fileio->b.type = q->type;
> + fileio->b.memory = q->memory;
> + if (prequeue) {
> + fileio->b.index = index++;
> + prequeue--;
> + } else {
> + call_qop(q, wait_finish, q);
> + ret = vb2_internal_dqbuf(q, &fileio->b, 0);
> + call_qop(q, wait_prepare, q);
> + dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
> + }
> + if (threadio->stop)
> + break;
> + if (ret)
> + break;
> + try_to_freeze();
> +
> + vb = q->bufs[fileio->b.index];
> + if (!(fileio->b.flags & V4L2_BUF_FLAG_ERROR))
> + ret = threadio->fnc(vb, threadio->priv);
> + if (ret)
> + break;
> + call_qop(q, wait_finish, q);
> + ret = vb2_internal_qbuf(q, &fileio->b);
> + call_qop(q, wait_prepare, q);
> + if (ret)
> + break;
> + }
> +
> + /* Hmm, linux becomes *very* unhappy without this ... */
> + while (!kthread_should_stop()) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule();
> + }
> + return 0;
> +}
> +
> +int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
> + const char *thread_name)
> +{
> + struct vb2_threadio_data *threadio;
> + int ret = 0;
> +
> + if (q->threadio)
> + return -EBUSY;
> + if (vb2_is_busy(q))
> + return -EBUSY;
> + if (WARN_ON(q->fileio))
> + return -EBUSY;
> +
> + threadio = kzalloc(sizeof(*threadio), GFP_KERNEL);
> + if (threadio == NULL)
> + return -ENOMEM;
> + threadio->fnc = fnc;
> + threadio->priv = priv;
> +
> + ret = __vb2_init_fileio(q, !V4L2_TYPE_IS_OUTPUT(q->type));
> + dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
> + if (ret)
> + goto nomem;
> + q->threadio = threadio;
> + threadio->thread = kthread_run(vb2_thread, q, "vb2-%s", thread_name);
> + if (IS_ERR(threadio->thread)) {
> + ret = PTR_ERR(threadio->thread);
> + threadio->thread = NULL;
> + goto nothread;
> + }
> + return 0;
> +
> +nothread:
> + __vb2_cleanup_fileio(q);
> +nomem:
> + kfree(threadio);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_thread_start);
> +
> +int vb2_thread_stop(struct vb2_queue *q)
> +{
> + struct vb2_threadio_data *threadio = q->threadio;
> + struct vb2_fileio_data *fileio = q->fileio;
> + int err;
> +
> + if (threadio == NULL)
> + return 0;
> + call_qop(q, wait_finish, q);
> + threadio->stop = true;
> + vb2_internal_streamoff(q, q->type);
> + call_qop(q, wait_prepare, q);
> + q->fileio = NULL;
> + fileio->req.count = 0;
> + vb2_reqbufs(q, &fileio->req);
> + kfree(fileio);
> + err = kthread_stop(threadio->thread);
> + threadio->thread = NULL;
> + kfree(threadio);
> + q->fileio = NULL;
> + q->threadio = NULL;
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(vb2_thread_stop);
>
> /*
> * The following functions are not part of the vb2 core API, but are helper
> diff --git a/include/media/videobuf2-core.h
> b/include/media/videobuf2-core.h index 1be7f39..7dea795 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -20,6 +20,7 @@
>
> struct vb2_alloc_ctx;
> struct vb2_fileio_data;
> +struct vb2_threadio_data;
>
> /**
> * struct vb2_mem_ops - memory handling/memory allocator operations
> @@ -330,6 +331,7 @@ struct v4l2_fh;
> * buffers queued. If set, then retry calling start_streaming when
> * queuing a new buffer.
> * @fileio: file io emulator internal data, used only if emulator is
active
> + * @threadio: thread io internal data, used only if thread is active */
> struct vb2_queue {
> enum v4l2_buf_type type;
> @@ -364,6 +366,7 @@ struct vb2_queue {
> unsigned int retry_start_streaming:1;
>
> struct vb2_fileio_data *fileio;
> + struct vb2_threadio_data *threadio;
> };
>
> void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
> @@ -402,6 +405,31 @@ size_t vb2_read(struct vb2_queue *q, char __user *data,
> size_t count, loff_t *ppos, int nonblock);
> size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t
> count, loff_t *ppos, int nonblock);
> +/**
> + * vb2_thread_fnc - callback function for use with vb2_thread
> + *
> + * This is called whenever a buffer is dequeued in the thread.
> + */
> +typedef int (*vb2_thread_fnc)(struct vb2_buffer *vb, void *priv);
> +
> +/**
> + * vb2_thread_start() - start a thread for the given queue.
> + * @q: videobuf queue
> + * @fnc: callback function
> + * @priv: priv pointer passed to the callback function
> + * @thread_name:the name of the thread. This will be prefixed with "vb2-".
> + *
> + * This starts a thread that will queue and dequeue until an error occurs
> + * or @vb2_thread_stop is called.
> + */
> +int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
> + const char *thread_name);
> +
> +/**
> + * vb2_thread_stop() - stop the thread for the given queue.
> + * @q: videobuf queue
> + */
> +int vb2_thread_stop(struct vb2_queue *q);
>
> /**
> * vb2_is_streaming() - return streaming status of the queue
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-11-29 18:21 ` Laurent Pinchart
@ 2013-12-03 9:56 ` Hans Verkuil
2013-12-04 1:17 ` Laurent Pinchart
0 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-12-03 9:56 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
On 11/29/13 19:21, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> In order to implement vb2 DVB or ALSA support you need to be able to start
>> a kernel thread that queues and dequeues buffers, calling a callback
>> function for every captured/displayed buffer. This patch adds support for
>> that.
>>
>> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the DVB
>> specific stuff stripped out, thus making it much more generic.
>
> Do you see any use for this outside of videobuf2-dvb ? If not I wonder whether
> the code shouldn't be moved there. The sync objects framework being developed
> for KMS will in my opinion cover the other use cases, and I'd like to
> discourage non-DVB drivers to use vb2 threads in the meantime.
I'm using it for ALSA drivers which, at least in my case, require almost
identical functionality as that needed by DVB. But regardless of that, I really
don't like the way it was done in the old videobuf framework, mixing low-level
videobuf calls/data structure accesses with DVB code. That should be separate.
The vb2 core framework should provide the low-level functionality that is
needed by the videobuf2-dvb to build on.
Regards,
Hans
>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> drivers/media/v4l2-core/videobuf2-core.c | 134 ++++++++++++++++++++++++++++
>> include/media/videobuf2-core.h | 28 +++++++
>> 2 files changed, 162 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 853d391..a86d033 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -6,6 +6,9 @@
>> * Author: Pawel Osciak <pawel@osciak.com>
>> * Marek Szyprowski <m.szyprowski@samsung.com>
>> *
>> + * The vb2_thread implementation was based on code from videobuf-dvb.c:
>> + * (c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
>> + *
>> * This program is free software; you can redistribute it and/or modify
>> * it under the terms of the GNU General Public License as published by
>> * the Free Software Foundation.
>> @@ -18,6 +21,8 @@
>> #include <linux/poll.h>
>> #include <linux/slab.h>
>> #include <linux/sched.h>
>> +#include <linux/freezer.h>
>> +#include <linux/kthread.h>
>>
>> #include <media/v4l2-dev.h>
>> #include <media/v4l2-fh.h>
>> @@ -2508,6 +2513,135 @@ size_t vb2_write(struct vb2_queue *q, const char
>> __user *data, size_t count, }
>> EXPORT_SYMBOL_GPL(vb2_write);
>>
>> +struct vb2_threadio_data {
>> + struct task_struct *thread;
>> + vb2_thread_fnc fnc;
>> + void *priv;
>> + bool stop;
>> +};
>> +
>> +static int vb2_thread(void *data)
>> +{
>> + struct vb2_queue *q = data;
>> + struct vb2_threadio_data *threadio = q->threadio;
>> + struct vb2_fileio_data *fileio = q->fileio;
>> + int prequeue = 0;
>> + int index = 0;
>> + int ret = 0;
>> +
>> + if (V4L2_TYPE_IS_OUTPUT(q->type))
>> + prequeue = q->num_buffers;
>> +
>> + set_freezable();
>> +
>> + for (;;) {
>> + struct vb2_buffer *vb;
>> +
>> + /*
>> + * Call vb2_dqbuf to get buffer back.
>> + */
>> + memset(&fileio->b, 0, sizeof(fileio->b));
>> + fileio->b.type = q->type;
>> + fileio->b.memory = q->memory;
>> + if (prequeue) {
>> + fileio->b.index = index++;
>> + prequeue--;
>> + } else {
>> + call_qop(q, wait_finish, q);
>> + ret = vb2_internal_dqbuf(q, &fileio->b, 0);
>> + call_qop(q, wait_prepare, q);
>> + dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
>> + }
>> + if (threadio->stop)
>> + break;
>> + if (ret)
>> + break;
>> + try_to_freeze();
>> +
>> + vb = q->bufs[fileio->b.index];
>> + if (!(fileio->b.flags & V4L2_BUF_FLAG_ERROR))
>> + ret = threadio->fnc(vb, threadio->priv);
>> + if (ret)
>> + break;
>> + call_qop(q, wait_finish, q);
>> + ret = vb2_internal_qbuf(q, &fileio->b);
>> + call_qop(q, wait_prepare, q);
>> + if (ret)
>> + break;
>> + }
>> +
>> + /* Hmm, linux becomes *very* unhappy without this ... */
>> + while (!kthread_should_stop()) {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule();
>> + }
>> + return 0;
>> +}
>> +
>> +int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
>> + const char *thread_name)
>> +{
>> + struct vb2_threadio_data *threadio;
>> + int ret = 0;
>> +
>> + if (q->threadio)
>> + return -EBUSY;
>> + if (vb2_is_busy(q))
>> + return -EBUSY;
>> + if (WARN_ON(q->fileio))
>> + return -EBUSY;
>> +
>> + threadio = kzalloc(sizeof(*threadio), GFP_KERNEL);
>> + if (threadio == NULL)
>> + return -ENOMEM;
>> + threadio->fnc = fnc;
>> + threadio->priv = priv;
>> +
>> + ret = __vb2_init_fileio(q, !V4L2_TYPE_IS_OUTPUT(q->type));
>> + dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>> + if (ret)
>> + goto nomem;
>> + q->threadio = threadio;
>> + threadio->thread = kthread_run(vb2_thread, q, "vb2-%s", thread_name);
>> + if (IS_ERR(threadio->thread)) {
>> + ret = PTR_ERR(threadio->thread);
>> + threadio->thread = NULL;
>> + goto nothread;
>> + }
>> + return 0;
>> +
>> +nothread:
>> + __vb2_cleanup_fileio(q);
>> +nomem:
>> + kfree(threadio);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_thread_start);
>> +
>> +int vb2_thread_stop(struct vb2_queue *q)
>> +{
>> + struct vb2_threadio_data *threadio = q->threadio;
>> + struct vb2_fileio_data *fileio = q->fileio;
>> + int err;
>> +
>> + if (threadio == NULL)
>> + return 0;
>> + call_qop(q, wait_finish, q);
>> + threadio->stop = true;
>> + vb2_internal_streamoff(q, q->type);
>> + call_qop(q, wait_prepare, q);
>> + q->fileio = NULL;
>> + fileio->req.count = 0;
>> + vb2_reqbufs(q, &fileio->req);
>> + kfree(fileio);
>> + err = kthread_stop(threadio->thread);
>> + threadio->thread = NULL;
>> + kfree(threadio);
>> + q->fileio = NULL;
>> + q->threadio = NULL;
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_thread_stop);
>>
>> /*
>> * The following functions are not part of the vb2 core API, but are helper
>> diff --git a/include/media/videobuf2-core.h
>> b/include/media/videobuf2-core.h index 1be7f39..7dea795 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -20,6 +20,7 @@
>>
>> struct vb2_alloc_ctx;
>> struct vb2_fileio_data;
>> +struct vb2_threadio_data;
>>
>> /**
>> * struct vb2_mem_ops - memory handling/memory allocator operations
>> @@ -330,6 +331,7 @@ struct v4l2_fh;
>> * buffers queued. If set, then retry calling start_streaming when
>> * queuing a new buffer.
>> * @fileio: file io emulator internal data, used only if emulator is
> active
>> + * @threadio: thread io internal data, used only if thread is active */
>> struct vb2_queue {
>> enum v4l2_buf_type type;
>> @@ -364,6 +366,7 @@ struct vb2_queue {
>> unsigned int retry_start_streaming:1;
>>
>> struct vb2_fileio_data *fileio;
>> + struct vb2_threadio_data *threadio;
>> };
>>
>> void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
>> @@ -402,6 +405,31 @@ size_t vb2_read(struct vb2_queue *q, char __user *data,
>> size_t count, loff_t *ppos, int nonblock);
>> size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t
>> count, loff_t *ppos, int nonblock);
>> +/**
>> + * vb2_thread_fnc - callback function for use with vb2_thread
>> + *
>> + * This is called whenever a buffer is dequeued in the thread.
>> + */
>> +typedef int (*vb2_thread_fnc)(struct vb2_buffer *vb, void *priv);
>> +
>> +/**
>> + * vb2_thread_start() - start a thread for the given queue.
>> + * @q: videobuf queue
>> + * @fnc: callback function
>> + * @priv: priv pointer passed to the callback function
>> + * @thread_name:the name of the thread. This will be prefixed with "vb2-".
>> + *
>> + * This starts a thread that will queue and dequeue until an error occurs
>> + * or @vb2_thread_stop is called.
>> + */
>> +int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
>> + const char *thread_name);
>> +
>> +/**
>> + * vb2_thread_stop() - stop the thread for the given queue.
>> + * @q: videobuf queue
>> + */
>> +int vb2_thread_stop(struct vb2_queue *q);
>>
>> /**
>> * vb2_is_streaming() - return streaming status of the queue
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-12-03 9:56 ` Hans Verkuil
@ 2013-12-04 1:17 ` Laurent Pinchart
2013-12-04 7:47 ` Hans Verkuil
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2013-12-04 1:17 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
On Tuesday 03 December 2013 10:56:07 Hans Verkuil wrote:
> On 11/29/13 19:21, Laurent Pinchart wrote:
> > On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> In order to implement vb2 DVB or ALSA support you need to be able to
> >> start a kernel thread that queues and dequeues buffers, calling a
> >> callback function for every captured/displayed buffer. This patch adds
> >> support for that.
> >>
> >> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the
> >> DVB specific stuff stripped out, thus making it much more generic.
> >
> > Do you see any use for this outside of videobuf2-dvb ? If not I wonder
> > whether the code shouldn't be moved there. The sync objects framework
> > being developed for KMS will in my opinion cover the other use cases, and
> > I'd like to discourage non-DVB drivers to use vb2 threads in the
> > meantime.
>
> I'm using it for ALSA drivers which, at least in my case, require almost
> identical functionality as that needed by DVB.
You're using videobuf2 for audio ?
> But regardless of that, I really don't like the way it was done in the old
> videobuf framework, mixing low-level videobuf calls/data structure accesses
> with DVB code. That should be separate.
>
> The vb2 core framework should provide the low-level functionality that is
> needed by the videobuf2-dvb to build on.
Right, but I want to make sure that drivers will not start using this
directly. It should be an internal videobuf2 API.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-12-04 1:17 ` Laurent Pinchart
@ 2013-12-04 7:47 ` Hans Verkuil
2013-12-04 16:33 ` Laurent Pinchart
0 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-12-04 7:47 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
On 12/04/2013 02:17 AM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Tuesday 03 December 2013 10:56:07 Hans Verkuil wrote:
>> On 11/29/13 19:21, Laurent Pinchart wrote:
>>> On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> In order to implement vb2 DVB or ALSA support you need to be able to
>>>> start a kernel thread that queues and dequeues buffers, calling a
>>>> callback function for every captured/displayed buffer. This patch adds
>>>> support for that.
>>>>
>>>> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the
>>>> DVB specific stuff stripped out, thus making it much more generic.
>>>
>>> Do you see any use for this outside of videobuf2-dvb ? If not I wonder
>>> whether the code shouldn't be moved there. The sync objects framework
>>> being developed for KMS will in my opinion cover the other use cases, and
>>> I'd like to discourage non-DVB drivers to use vb2 threads in the
>>> meantime.
>>
>> I'm using it for ALSA drivers which, at least in my case, require almost
>> identical functionality as that needed by DVB.
>
> You're using videobuf2 for audio ?
For this particular board the audio DMA is just another DMA channel. Handling
audio DMA is identical to video DMA. Why reinvent the wheel?
The board I developed this for has somewhat peculiar audio handling (sorry,
it's an internal product and I can't go into details), but I'll do the same
exercise for another board that I can open source and there audio handling is
standard. I want to see if I can use that to develop a videobuf2-alsa.c
module that takes care of most of the alsa complexity. I don't know yet how
that will work out, I'll have to experiment a bit.
>
>> But regardless of that, I really don't like the way it was done in the old
>> videobuf framework, mixing low-level videobuf calls/data structure accesses
>> with DVB code. That should be separate.
>>
>> The vb2 core framework should provide the low-level functionality that is
>> needed by the videobuf2-dvb to build on.
>
> Right, but I want to make sure that drivers will not start using this
> directly.
What sort of use-cases were you thinking of, other than DVB and ALSA? I don't
off-hand see one.
> It should be an internal videobuf2 API.
I happily add comments to the source and header mentioning that it is for
core use only and that for any other uses the mailinglist should be contacted,
but I really don't want to mix core vb2 code with DVB code. That should remain
separate.
Regards,
Hans
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-12-04 7:47 ` Hans Verkuil
@ 2013-12-04 16:33 ` Laurent Pinchart
2013-12-04 17:03 ` Hans Verkuil
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2013-12-04 16:33 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
On Wednesday 04 December 2013 08:47:25 Hans Verkuil wrote:
> On 12/04/2013 02:17 AM, Laurent Pinchart wrote:
> > On Tuesday 03 December 2013 10:56:07 Hans Verkuil wrote:
> >> On 11/29/13 19:21, Laurent Pinchart wrote:
> >>> On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>
> >>>> In order to implement vb2 DVB or ALSA support you need to be able to
> >>>> start a kernel thread that queues and dequeues buffers, calling a
> >>>> callback function for every captured/displayed buffer. This patch adds
> >>>> support for that.
> >>>>
> >>>> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the
> >>>> DVB specific stuff stripped out, thus making it much more generic.
> >>>
> >>> Do you see any use for this outside of videobuf2-dvb ? If not I wonder
> >>> whether the code shouldn't be moved there. The sync objects framework
> >>> being developed for KMS will in my opinion cover the other use cases,
> >>> and
> >>> I'd like to discourage non-DVB drivers to use vb2 threads in the
> >>> meantime.
> >>
> >> I'm using it for ALSA drivers which, at least in my case, require almost
> >> identical functionality as that needed by DVB.
> >
> > You're using videobuf2 for audio ?
>
> For this particular board the audio DMA is just another DMA channel.
> Handling audio DMA is identical to video DMA. Why reinvent the wheel?
videobuf2 is more about buffer management than DMA management. As the code is
based around a two-dimensional, possibly multiplanar, buffer it's quite
hackish to reuse it for audio. Doesn't ALSA offer a buffer management library
?
> The board I developed this for has somewhat peculiar audio handling (sorry,
> it's an internal product and I can't go into details), but I'll do the same
> exercise for another board that I can open source and there audio handling
> is standard. I want to see if I can use that to develop a videobuf2-alsa.c
> module that takes care of most of the alsa complexity. I don't know yet how
> that will work out, I'll have to experiment a bit.
>
> >> But regardless of that, I really don't like the way it was done in the
> >> old videobuf framework, mixing low-level videobuf calls/data structure
> >> accesses with DVB code. That should be separate.
> >>
> >> The vb2 core framework should provide the low-level functionality that is
> >> needed by the videobuf2-dvb to build on.
> >
> > Right, but I want to make sure that drivers will not start using this
> > directly.
>
> What sort of use-cases were you thinking of, other than DVB and ALSA? I
> don't off-hand see one.
That's the thing, I don't see any valid use case, I just want to make sure we
won't get crazy use cases implemented with vb2 threads in the future :-)
> > It should be an internal videobuf2 API.
>
> I happily add comments to the source and header mentioning that it is for
> core use only and that for any other uses the mailinglist should be
> contacted, but I really don't want to mix core vb2 code with DVB code. That
> should remain separate.
OK, that sounds good with me.
What about moving thread support to videobuf2-thread.c ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-12-04 16:33 ` Laurent Pinchart
@ 2013-12-04 17:03 ` Hans Verkuil
2013-12-04 20:57 ` Laurent Pinchart
0 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-12-04 17:03 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
On 12/04/2013 05:33 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Wednesday 04 December 2013 08:47:25 Hans Verkuil wrote:
>> On 12/04/2013 02:17 AM, Laurent Pinchart wrote:
>>> On Tuesday 03 December 2013 10:56:07 Hans Verkuil wrote:
>>>> On 11/29/13 19:21, Laurent Pinchart wrote:
>>>>> On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>
>>>>>> In order to implement vb2 DVB or ALSA support you need to be able to
>>>>>> start a kernel thread that queues and dequeues buffers, calling a
>>>>>> callback function for every captured/displayed buffer. This patch adds
>>>>>> support for that.
>>>>>>
>>>>>> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the
>>>>>> DVB specific stuff stripped out, thus making it much more generic.
>>>>>
>>>>> Do you see any use for this outside of videobuf2-dvb ? If not I wonder
>>>>> whether the code shouldn't be moved there. The sync objects framework
>>>>> being developed for KMS will in my opinion cover the other use cases,
>>>>> and
>>>>> I'd like to discourage non-DVB drivers to use vb2 threads in the
>>>>> meantime.
>>>>
>>>> I'm using it for ALSA drivers which, at least in my case, require almost
>>>> identical functionality as that needed by DVB.
>>>
>>> You're using videobuf2 for audio ?
>>
>> For this particular board the audio DMA is just another DMA channel.
>> Handling audio DMA is identical to video DMA. Why reinvent the wheel?
>
> videobuf2 is more about buffer management than DMA management. As the code is
> based around a two-dimensional, possibly multiplanar, buffer it's quite
> hackish to reuse it for audio.
I disagree with that. vb2 has all the right hooks to start/stop DMA and
queue/dequeue buffers. It's used for VBI as well and can just as easily
support meta data. For this particular board there is no difference
between audio and video DMA.
> Doesn't ALSA offer a buffer management library?
Yes it does. But due to the peculiarities of the particular board it wasn't
sufficient. Specifically I must copy the audio data from the alsa buffers to
vb2 buffers since the layout of the data differs.
As mentioned I will also work on a different board where the audio DMA is
much more standard (i.e. the same buffer layout can be used), and I want to
investigate if using vb2 in that case makes sense or not.
>
>> The board I developed this for has somewhat peculiar audio handling (sorry,
>> it's an internal product and I can't go into details), but I'll do the same
>> exercise for another board that I can open source and there audio handling
>> is standard. I want to see if I can use that to develop a videobuf2-alsa.c
>> module that takes care of most of the alsa complexity. I don't know yet how
>> that will work out, I'll have to experiment a bit.
>>
>>>> But regardless of that, I really don't like the way it was done in the
>>>> old videobuf framework, mixing low-level videobuf calls/data structure
>>>> accesses with DVB code. That should be separate.
>>>>
>>>> The vb2 core framework should provide the low-level functionality that is
>>>> needed by the videobuf2-dvb to build on.
>>>
>>> Right, but I want to make sure that drivers will not start using this
>>> directly.
>>
>> What sort of use-cases were you thinking of, other than DVB and ALSA? I
>> don't off-hand see one.
>
> That's the thing, I don't see any valid use case, I just want to make sure we
> won't get crazy use cases implemented with vb2 threads in the future :-)
>
>>> It should be an internal videobuf2 API.
>>
>> I happily add comments to the source and header mentioning that it is for
>> core use only and that for any other uses the mailinglist should be
>> contacted, but I really don't want to mix core vb2 code with DVB code. That
>> should remain separate.
>
> OK, that sounds good with me.
>
> What about moving thread support to videobuf2-thread.c ?
I tried that originally, but in order to do that I had to make a number
of low-level vb2 functions extern instead of static, and that was quite
messy. So I decided against that. It's not that much code (106 lines),
after all.
That said, it might be interesting at some point to split off the fileio
and thread handling into a separate file.
Regards,
Hans
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFCv2 PATCH 7/9] vb2: add thread support
2013-12-04 17:03 ` Hans Verkuil
@ 2013-12-04 20:57 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2013-12-04 20:57 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
On Wednesday 04 December 2013 18:03:47 Hans Verkuil wrote:
> On 12/04/2013 05:33 PM, Laurent Pinchart wrote:
> > On Wednesday 04 December 2013 08:47:25 Hans Verkuil wrote:
> >> On 12/04/2013 02:17 AM, Laurent Pinchart wrote:
> >>> On Tuesday 03 December 2013 10:56:07 Hans Verkuil wrote:
> >>>> On 11/29/13 19:21, Laurent Pinchart wrote:
> >>>>> On Friday 29 November 2013 10:58:42 Hans Verkuil wrote:
> >>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>>
> >>>>>> In order to implement vb2 DVB or ALSA support you need to be able to
> >>>>>> start a kernel thread that queues and dequeues buffers, calling a
> >>>>>> callback function for every captured/displayed buffer. This patch
> >>>>>> adds support for that.
> >>>>>>
> >>>>>> It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all
> >>>>>> the DVB specific stuff stripped out, thus making it much more
> >>>>>> generic.
> >>>>>
> >>>>> Do you see any use for this outside of videobuf2-dvb ? If not I wonder
> >>>>> whether the code shouldn't be moved there. The sync objects framework
> >>>>> being developed for KMS will in my opinion cover the other use cases,
> >>>>> and I'd like to discourage non-DVB drivers to use vb2 threads in the
> >>>>> meantime.
> >>>>
> >>>> I'm using it for ALSA drivers which, at least in my case, require
> >>>> almost identical functionality as that needed by DVB.
> >>>
> >>> You're using videobuf2 for audio ?
> >>
> >> For this particular board the audio DMA is just another DMA channel.
> >> Handling audio DMA is identical to video DMA. Why reinvent the wheel?
> >
> > videobuf2 is more about buffer management than DMA management. As the code
> > is based around a two-dimensional, possibly multiplanar, buffer it's
> > quite hackish to reuse it for audio.
>
> I disagree with that. vb2 has all the right hooks to start/stop DMA and
> queue/dequeue buffers. It's used for VBI as well and can just as easily
> support meta data. For this particular board there is no difference
> between audio and video DMA.
>
> > Doesn't ALSA offer a buffer management library?
>
> Yes it does. But due to the peculiarities of the particular board it wasn't
> sufficient. Specifically I must copy the audio data from the alsa buffers to
> vb2 buffers since the layout of the data differs.
>
> As mentioned I will also work on a different board where the audio DMA is
> much more standard (i.e. the same buffer layout can be used), and I want to
> investigate if using vb2 in that case makes sense or not.
Let's revisit the topic when you'll work on that board then.
> >> The board I developed this for has somewhat peculiar audio handling
> >> (sorry, it's an internal product and I can't go into details), but I'll
> >> do the same exercise for another board that I can open source and there
> >> audio handling is standard. I want to see if I can use that to develop a
> >> videobuf2-alsa.c module that takes care of most of the alsa complexity. I
> >> don't know yet how that will work out, I'll have to experiment a bit.
> >>
> >>>> But regardless of that, I really don't like the way it was done in the
> >>>> old videobuf framework, mixing low-level videobuf calls/data structure
> >>>> accesses with DVB code. That should be separate.
> >>>>
> >>>> The vb2 core framework should provide the low-level functionality that
> >>>> is needed by the videobuf2-dvb to build on.
> >>>
> >>> Right, but I want to make sure that drivers will not start using this
> >>> directly.
> >>
> >> What sort of use-cases were you thinking of, other than DVB and ALSA? I
> >> don't off-hand see one.
> >
> > That's the thing, I don't see any valid use case, I just want to make sure
> > we won't get crazy use cases implemented with vb2 threads in the future
> > :-)
> >
> >>> It should be an internal videobuf2 API.
> >>
> >> I happily add comments to the source and header mentioning that it is for
> >> core use only and that for any other uses the mailinglist should be
> >> contacted, but I really don't want to mix core vb2 code with DVB code.
> >> That should remain separate.
> >
> > OK, that sounds good with me.
> >
> > What about moving thread support to videobuf2-thread.c ?
>
> I tried that originally, but in order to do that I had to make a number
> of low-level vb2 functions extern instead of static, and that was quite
> messy. So I decided against that. It's not that much code (106 lines),
> after all.
OK, fair enough. Please just add a comment to the header saying that drivers
must not use that API then.
> That said, it might be interesting at some point to split off the fileio
> and thread handling into a separate file.
That would be nice indeed.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFCv2 PATCH 8/9] vb2: Add videobuf2-dvb support.
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (5 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 7/9] vb2: add thread support Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 9:58 ` [RFCv2 PATCH 9/9] vb2: Improve file I/O emulation to handle buffers in any order Hans Verkuil
2013-11-29 18:16 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Laurent Pinchart
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
With the new vb2_thread_start/stop core code it is very easy to implement
videobuf2-dvb. This should simplify coverting existing videobuf drivers to
vb2.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/Kconfig | 4 +
drivers/media/v4l2-core/Makefile | 1 +
drivers/media/v4l2-core/videobuf2-dvb.c | 336 ++++++++++++++++++++++++++++++++
include/media/videobuf2-dvb.h | 58 ++++++
4 files changed, 399 insertions(+)
create mode 100644 drivers/media/v4l2-core/videobuf2-dvb.c
create mode 100644 include/media/videobuf2-dvb.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 8c05565..8cc76da 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -84,6 +84,10 @@ config VIDEOBUF2_DMA_SG
select VIDEOBUF2_CORE
select VIDEOBUF2_MEMOPS
+config VIDEOBUF2_DVB
+ tristate
+ select VIDEOBUF2_CORE
+
config VIDEO_V4L2_INT_DEVICE
tristate "V4L2 int device (DEPRECATED)"
depends on VIDEO_V4L2
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 1a85eee..c73ac0d 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
obj-$(CONFIG_VIDEOBUF2_DMA_CONTIG) += videobuf2-dma-contig.o
obj-$(CONFIG_VIDEOBUF2_DMA_SG) += videobuf2-dma-sg.o
+obj-$(CONFIG_VIDEOBUF2_DVB) += videobuf2-dvb.o
ccflags-y += -I$(srctree)/drivers/media/dvb-core
ccflags-y += -I$(srctree)/drivers/media/dvb-frontends
diff --git a/drivers/media/v4l2-core/videobuf2-dvb.c b/drivers/media/v4l2-core/videobuf2-dvb.c
new file mode 100644
index 0000000..d092698
--- /dev/null
+++ b/drivers/media/v4l2-core/videobuf2-dvb.c
@@ -0,0 +1,336 @@
+/*
+ *
+ * some helper function for simple DVB cards which simply DMA the
+ * complete transport stream and let the computer sort everything else
+ * (i.e. we are using the software demux, ...). Also uses the
+ * video-buf to manage DMA buffers.
+ *
+ * (c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <media/videobuf2-dvb.h>
+
+/* ------------------------------------------------------------------ */
+
+MODULE_AUTHOR("Gerd Knorr <kraxel@bytesex.org> [SuSE Labs]");
+MODULE_LICENSE("GPL");
+
+/* ------------------------------------------------------------------ */
+
+static int dvb_fnc(struct vb2_buffer *vb, void *priv)
+{
+ struct vb2_dvb *dvb = priv;
+
+ dvb_dmx_swfilter(&dvb->demux, vb2_plane_vaddr(vb, 0),
+ vb2_get_plane_payload(vb, 0));
+ return 0;
+}
+
+static int vb2_dvb_start_feed(struct dvb_demux_feed *feed)
+{
+ struct dvb_demux *demux = feed->demux;
+ struct vb2_dvb *dvb = demux->priv;
+ int rc = 0;
+
+ if (!demux->dmx.frontend)
+ return -EINVAL;
+
+ mutex_lock(&dvb->lock);
+ dvb->nfeeds++;
+
+ if (!dvb->dvbq.threadio) {
+ rc = vb2_thread_start(&dvb->dvbq, dvb_fnc, dvb, dvb->name);
+ if (rc)
+ dvb->nfeeds--;
+ }
+ if (!rc)
+ rc = dvb->nfeeds;
+ mutex_unlock(&dvb->lock);
+ return rc;
+}
+
+static int vb2_dvb_stop_feed(struct dvb_demux_feed *feed)
+{
+ struct dvb_demux *demux = feed->demux;
+ struct vb2_dvb *dvb = demux->priv;
+ int err = 0;
+
+ mutex_lock(&dvb->lock);
+ dvb->nfeeds--;
+ if (0 == dvb->nfeeds)
+ err = vb2_thread_stop(&dvb->dvbq);
+ mutex_unlock(&dvb->lock);
+ return err;
+}
+
+static int vb2_dvb_register_adapter(struct vb2_dvb_frontends *fe,
+ struct module *module,
+ void *adapter_priv,
+ struct device *device,
+ char *adapter_name,
+ short *adapter_nr,
+ int mfe_shared)
+{
+ int result;
+
+ mutex_init(&fe->lock);
+
+ /* register adapter */
+ result = dvb_register_adapter(&fe->adapter, adapter_name, module,
+ device, adapter_nr);
+ if (result < 0) {
+ pr_warn("%s: dvb_register_adapter failed (errno = %d)\n",
+ adapter_name, result);
+ }
+ fe->adapter.priv = adapter_priv;
+ fe->adapter.mfe_shared = mfe_shared;
+
+ return result;
+}
+
+static int vb2_dvb_register_frontend(struct dvb_adapter *adapter,
+ struct vb2_dvb *dvb)
+{
+ int result;
+
+ /* register frontend */
+ result = dvb_register_frontend(adapter, dvb->frontend);
+ if (result < 0) {
+ pr_warn("%s: dvb_register_frontend failed (errno = %d)\n",
+ dvb->name, result);
+ goto fail_frontend;
+ }
+
+ /* register demux stuff */
+ dvb->demux.dmx.capabilities =
+ DMX_TS_FILTERING | DMX_SECTION_FILTERING |
+ DMX_MEMORY_BASED_FILTERING;
+ dvb->demux.priv = dvb;
+ dvb->demux.filternum = 256;
+ dvb->demux.feednum = 256;
+ dvb->demux.start_feed = vb2_dvb_start_feed;
+ dvb->demux.stop_feed = vb2_dvb_stop_feed;
+ result = dvb_dmx_init(&dvb->demux);
+ if (result < 0) {
+ pr_warn("%s: dvb_dmx_init failed (errno = %d)\n",
+ dvb->name, result);
+ goto fail_dmx;
+ }
+
+ dvb->dmxdev.filternum = 256;
+ dvb->dmxdev.demux = &dvb->demux.dmx;
+ dvb->dmxdev.capabilities = 0;
+ result = dvb_dmxdev_init(&dvb->dmxdev, adapter);
+
+ if (result < 0) {
+ pr_warn("%s: dvb_dmxdev_init failed (errno = %d)\n",
+ dvb->name, result);
+ goto fail_dmxdev;
+ }
+
+ dvb->fe_hw.source = DMX_FRONTEND_0;
+ result = dvb->demux.dmx.add_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+ if (result < 0) {
+ pr_warn("%s: add_frontend failed (DMX_FRONTEND_0, errno = %d)\n",
+ dvb->name, result);
+ goto fail_fe_hw;
+ }
+
+ dvb->fe_mem.source = DMX_MEMORY_FE;
+ result = dvb->demux.dmx.add_frontend(&dvb->demux.dmx, &dvb->fe_mem);
+ if (result < 0) {
+ pr_warn("%s: add_frontend failed (DMX_MEMORY_FE, errno = %d)\n",
+ dvb->name, result);
+ goto fail_fe_mem;
+ }
+
+ result = dvb->demux.dmx.connect_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+ if (result < 0) {
+ pr_warn("%s: connect_frontend failed (errno = %d)\n",
+ dvb->name, result);
+ goto fail_fe_conn;
+ }
+
+ /* register network adapter */
+ result = dvb_net_init(adapter, &dvb->net, &dvb->demux.dmx);
+ if (result < 0) {
+ pr_warn("%s: dvb_net_init failed (errno = %d)\n",
+ dvb->name, result);
+ goto fail_fe_conn;
+ }
+ return 0;
+
+fail_fe_conn:
+ dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_mem);
+fail_fe_mem:
+ dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+fail_fe_hw:
+ dvb_dmxdev_release(&dvb->dmxdev);
+fail_dmxdev:
+ dvb_dmx_release(&dvb->demux);
+fail_dmx:
+ dvb_unregister_frontend(dvb->frontend);
+fail_frontend:
+ dvb_frontend_detach(dvb->frontend);
+ dvb->frontend = NULL;
+
+ return result;
+}
+
+/* ------------------------------------------------------------------ */
+/* Register a single adapter and one or more frontends */
+int vb2_dvb_register_bus(struct vb2_dvb_frontends *f,
+ struct module *module,
+ void *adapter_priv,
+ struct device *device,
+ short *adapter_nr,
+ int mfe_shared)
+{
+ struct list_head *list, *q;
+ struct vb2_dvb_frontend *fe;
+ int res;
+
+ fe = vb2_dvb_get_frontend(f, 1);
+ if (!fe) {
+ pr_warn("Unable to register the adapter which has no frontends\n");
+ return -EINVAL;
+ }
+
+ /* Bring up the adapter */
+ res = vb2_dvb_register_adapter(f, module, adapter_priv, device,
+ fe->dvb.name, adapter_nr, mfe_shared);
+ if (res < 0) {
+ pr_warn("vb2_dvb_register_adapter failed (errno = %d)\n", res);
+ return res;
+ }
+
+ /* Attach all of the frontends to the adapter */
+ mutex_lock(&f->lock);
+ list_for_each_safe(list, q, &f->felist) {
+ fe = list_entry(list, struct vb2_dvb_frontend, felist);
+ res = vb2_dvb_register_frontend(&f->adapter, &fe->dvb);
+ if (res < 0) {
+ pr_warn("%s: vb2_dvb_register_frontend failed (errno = %d)\n",
+ fe->dvb.name, res);
+ goto err;
+ }
+ }
+ mutex_unlock(&f->lock);
+ return 0;
+
+err:
+ mutex_unlock(&f->lock);
+ vb2_dvb_unregister_bus(f);
+ return res;
+}
+EXPORT_SYMBOL(vb2_dvb_register_bus);
+
+void vb2_dvb_unregister_bus(struct vb2_dvb_frontends *f)
+{
+ vb2_dvb_dealloc_frontends(f);
+
+ dvb_unregister_adapter(&f->adapter);
+}
+EXPORT_SYMBOL(vb2_dvb_unregister_bus);
+
+struct vb2_dvb_frontend *vb2_dvb_get_frontend(
+ struct vb2_dvb_frontends *f, int id)
+{
+ struct list_head *list, *q;
+ struct vb2_dvb_frontend *fe, *ret = NULL;
+
+ mutex_lock(&f->lock);
+
+ list_for_each_safe(list, q, &f->felist) {
+ fe = list_entry(list, struct vb2_dvb_frontend, felist);
+ if (fe->id == id) {
+ ret = fe;
+ break;
+ }
+ }
+
+ mutex_unlock(&f->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(vb2_dvb_get_frontend);
+
+int vb2_dvb_find_frontend(struct vb2_dvb_frontends *f,
+ struct dvb_frontend *p)
+{
+ struct list_head *list, *q;
+ struct vb2_dvb_frontend *fe = NULL;
+ int ret = 0;
+
+ mutex_lock(&f->lock);
+
+ list_for_each_safe(list, q, &f->felist) {
+ fe = list_entry(list, struct vb2_dvb_frontend, felist);
+ if (fe->dvb.frontend == p) {
+ ret = fe->id;
+ break;
+ }
+ }
+
+ mutex_unlock(&f->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(vb2_dvb_find_frontend);
+
+struct vb2_dvb_frontend *vb2_dvb_alloc_frontend(
+ struct vb2_dvb_frontends *f, int id)
+{
+ struct vb2_dvb_frontend *fe;
+
+ fe = kzalloc(sizeof(struct vb2_dvb_frontend), GFP_KERNEL);
+ if (fe == NULL)
+ return NULL;
+
+ fe->id = id;
+ mutex_init(&fe->dvb.lock);
+
+ mutex_lock(&f->lock);
+ list_add_tail(&fe->felist, &f->felist);
+ mutex_unlock(&f->lock);
+ return fe;
+}
+EXPORT_SYMBOL(vb2_dvb_alloc_frontend);
+
+void vb2_dvb_dealloc_frontends(struct vb2_dvb_frontends *f)
+{
+ struct list_head *list, *q;
+ struct vb2_dvb_frontend *fe;
+
+ mutex_lock(&f->lock);
+ list_for_each_safe(list, q, &f->felist) {
+ fe = list_entry(list, struct vb2_dvb_frontend, felist);
+ if (fe->dvb.net.dvbdev) {
+ dvb_net_release(&fe->dvb.net);
+ fe->dvb.demux.dmx.remove_frontend(&fe->dvb.demux.dmx,
+ &fe->dvb.fe_mem);
+ fe->dvb.demux.dmx.remove_frontend(&fe->dvb.demux.dmx,
+ &fe->dvb.fe_hw);
+ dvb_dmxdev_release(&fe->dvb.dmxdev);
+ dvb_dmx_release(&fe->dvb.demux);
+ dvb_unregister_frontend(fe->dvb.frontend);
+ }
+ if (fe->dvb.frontend)
+ /* always allocated, may have been reset */
+ dvb_frontend_detach(fe->dvb.frontend);
+ list_del(list); /* remove list entry */
+ kfree(fe); /* free frontend allocation */
+ }
+ mutex_unlock(&f->lock);
+}
+EXPORT_SYMBOL(vb2_dvb_dealloc_frontends);
diff --git a/include/media/videobuf2-dvb.h b/include/media/videobuf2-dvb.h
new file mode 100644
index 0000000..8f61456
--- /dev/null
+++ b/include/media/videobuf2-dvb.h
@@ -0,0 +1,58 @@
+#ifndef _VIDEOBUF2_DVB_H_
+#define _VIDEOBUF2_DVB_H_
+
+#include <dvbdev.h>
+#include <dmxdev.h>
+#include <dvb_demux.h>
+#include <dvb_net.h>
+#include <dvb_frontend.h>
+#include <media/videobuf2-core.h>
+
+struct vb2_dvb {
+ /* filling that the job of the driver */
+ char *name;
+ struct dvb_frontend *frontend;
+ struct vb2_queue dvbq;
+
+ /* video-buf-dvb state info */
+ struct mutex lock;
+ int nfeeds;
+
+ /* vb2_dvb_(un)register manages this */
+ struct dvb_demux demux;
+ struct dmxdev dmxdev;
+ struct dmx_frontend fe_hw;
+ struct dmx_frontend fe_mem;
+ struct dvb_net net;
+};
+
+struct vb2_dvb_frontend {
+ struct list_head felist;
+ int id;
+ struct vb2_dvb dvb;
+};
+
+struct vb2_dvb_frontends {
+ struct list_head felist;
+ struct mutex lock;
+ struct dvb_adapter adapter;
+ int active_fe_id; /* Indicates which frontend in the felist is in use */
+ int gate; /* Frontend with gate control 0=!MFE,1=fe0,2=fe1 etc */
+};
+
+int vb2_dvb_register_bus(struct vb2_dvb_frontends *f,
+ struct module *module,
+ void *adapter_priv,
+ struct device *device,
+ short *adapter_nr,
+ int mfe_shared);
+
+void vb2_dvb_unregister_bus(struct vb2_dvb_frontends *f);
+
+struct vb2_dvb_frontend *vb2_dvb_alloc_frontend(struct vb2_dvb_frontends *f, int id);
+void vb2_dvb_dealloc_frontends(struct vb2_dvb_frontends *f);
+
+struct vb2_dvb_frontend *vb2_dvb_get_frontend(struct vb2_dvb_frontends *f, int id);
+int vb2_dvb_find_frontend(struct vb2_dvb_frontends *f, struct dvb_frontend *p);
+
+#endif /* _VIDEOBUF2_DVB_H_ */
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFCv2 PATCH 9/9] vb2: Improve file I/O emulation to handle buffers in any order
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (6 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 8/9] vb2: Add videobuf2-dvb support Hans Verkuil
@ 2013-11-29 9:58 ` Hans Verkuil
2013-11-29 18:16 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Laurent Pinchart
8 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2013-11-29 9:58 UTC (permalink / raw)
To: linux-media; +Cc: m.szyprowski, pawel, laurent.pinchart, awalls, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
videobuf2 file I/O emulation assumed that buffers dequeued from the
driver would return in the order they were enqueued in the driver.
Improve the file I/O emulator's book-keeping to remove this assumption.
Also set the buf->size properly if a write() dequeues a buffer and the
VB2_FILEIO_WRITE_IMMEDIATELY flag is set.
Based on an initial patch by Andy Walls.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/media/v4l2-core/videobuf2-core.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index a86d033..2aff646 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2317,6 +2317,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
goto err_reqbufs;
fileio->bufs[i].queued = 1;
}
+ fileio->index = q->num_buffers;
}
/*
@@ -2392,15 +2393,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
}
fileio = q->fileio;
- index = fileio->index;
- buf = &fileio->bufs[index];
-
/*
* Check if we need to dequeue the buffer.
*/
- if (buf->queued) {
- struct vb2_buffer *vb;
-
+ index = fileio->index;
+ if (index >= q->num_buffers) {
/*
* Call vb2_dqbuf to get buffer back.
*/
@@ -2413,12 +2410,18 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
return ret;
fileio->dq_count += 1;
+ index = fileio->b.index;
+ buf = &fileio->bufs[index];
+
/*
* Get number of bytes filled by the driver
*/
- vb = q->bufs[index];
- buf->size = vb2_get_plane_payload(vb, 0);
+ buf->pos = 0;
buf->queued = 0;
+ buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
+ : vb2_plane_size(q->bufs[index], 0);
+ } else {
+ buf = &fileio->bufs[index];
}
/*
@@ -2481,13 +2484,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
*/
buf->pos = 0;
buf->queued = 1;
- buf->size = q->bufs[0]->v4l2_planes[0].length;
+ buf->size = vb2_plane_size(q->bufs[index], 0);
fileio->q_count += 1;
-
- /*
- * Switch to the next buffer
- */
- fileio->index = (index + 1) % q->num_buffers;
+ if (fileio->index < q->num_buffers)
+ fileio->index++;
}
/*
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare()
2013-11-29 9:58 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Hans Verkuil
` (7 preceding siblings ...)
2013-11-29 9:58 ` [RFCv2 PATCH 9/9] vb2: Improve file I/O emulation to handle buffers in any order Hans Verkuil
@ 2013-11-29 18:16 ` Laurent Pinchart
2013-12-03 9:41 ` Hans Verkuil
8 siblings, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2013-11-29 18:16 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
Thank you for the patch.
On Friday 29 November 2013 10:58:36 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Rather than taking the mmap semaphore at a relatively high-level function,
> push it down to the place where it is really needed.
>
> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
> vb2 calls. The only way I can see that a race can happen is when two
> threads queue the same buffer. The solution for that it to introduce
> a PREPARING state.
This looks better to me, but what about a vb2_reqbufs(0) call being processed
during the time window where we release the queue mutex ?
> Moving it down offers opportunities to simplify the code.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 82 +++++++++++++----------------
> include/media/videobuf2-core.h | 2 +
> 2 files changed, 38 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index b19b306..c8a1f22b5 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -462,6 +462,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
> struct v4l2_buffer *b) case VB2_BUF_STATE_PREPARED:
> b->flags |= V4L2_BUF_FLAG_PREPARED;
> break;
> + case VB2_BUF_STATE_PREPARING:
> case VB2_BUF_STATE_DEQUEUED:
> /* nothing */
> break;
> @@ -1207,6 +1208,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer
> *b) {
> struct vb2_queue *q = vb->vb2_queue;
> + struct rw_semaphore *mmap_sem;
> int ret;
>
> ret = __verify_length(vb, b);
> @@ -1216,12 +1218,31 @@ static int __buf_prepare(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) return ret;
> }
>
> + vb->state = VB2_BUF_STATE_PREPARING;
> switch (q->memory) {
> case V4L2_MEMORY_MMAP:
> ret = __qbuf_mmap(vb, b);
> break;
> case V4L2_MEMORY_USERPTR:
> + /*
> + * In case of user pointer buffers vb2 allocators need to get direct
> + * access to userspace pages. This requires getting the mmap
semaphore
> + * for read access in the current process structure. The same
semaphore
> + * is taken before calling mmap operation, while both
qbuf/prepare_buf
> + * and mmap are called by the driver or v4l2 core with the driver's
lock
> + * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in
mmap
> + * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
> + * core releases the driver's lock, takes mmap_sem and then takes the
> + * driver's lock again.
> + */
> + mmap_sem = ¤t->mm->mmap_sem;
> + call_qop(q, wait_prepare, q);
> + down_read(mmap_sem);
> + call_qop(q, wait_finish, q);
> +
> ret = __qbuf_userptr(vb, b);
> +
> + up_read(mmap_sem);
> break;
> case V4L2_MEMORY_DMABUF:
> ret = __qbuf_dmabuf(vb, b);
> @@ -1235,8 +1256,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) ret = call_qop(q, buf_prepare, vb);
> if (ret)
> dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
> - else
> - vb->state = VB2_BUF_STATE_PREPARED;
> + vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
>
> return ret;
> }
> @@ -1247,80 +1267,47 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue
> *q, struct v4l2_buffer *b, struct v4l2_buffer *,
> struct vb2_buffer *))
> {
> - struct rw_semaphore *mmap_sem = NULL;
> struct vb2_buffer *vb;
> int ret;
>
> - /*
> - * In case of user pointer buffers vb2 allocators need to get direct
> - * access to userspace pages. This requires getting the mmap semaphore
> - * for read access in the current process structure. The same semaphore
> - * is taken before calling mmap operation, while both qbuf/prepare_buf
> - * and mmap are called by the driver or v4l2 core with the driver's lock
> - * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap
> - * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
> - * core releases the driver's lock, takes mmap_sem and then takes the
> - * driver's lock again.
> - *
> - * To avoid racing with other vb2 calls, which might be called after
> - * releasing the driver's lock, this operation is performed at the
> - * beginning of qbuf/prepare_buf processing. This way the queue status
> - * is consistent after getting the driver's lock back.
> - */
> - if (q->memory == V4L2_MEMORY_USERPTR) {
> - mmap_sem = ¤t->mm->mmap_sem;
> - call_qop(q, wait_prepare, q);
> - down_read(mmap_sem);
> - call_qop(q, wait_finish, q);
> - }
> -
> if (q->fileio) {
> dprintk(1, "%s(): file io in progress\n", opname);
> - ret = -EBUSY;
> - goto unlock;
> + return -EBUSY;
> }
>
> if (b->type != q->type) {
> dprintk(1, "%s(): invalid buffer type\n", opname);
> - ret = -EINVAL;
> - goto unlock;
> + return -EINVAL;
> }
>
> if (b->index >= q->num_buffers) {
> dprintk(1, "%s(): buffer index out of range\n", opname);
> - ret = -EINVAL;
> - goto unlock;
> + return -EINVAL;
> }
>
> vb = q->bufs[b->index];
> if (NULL == vb) {
> /* Should never happen */
> dprintk(1, "%s(): buffer is NULL\n", opname);
> - ret = -EINVAL;
> - goto unlock;
> + return -EINVAL;
> }
>
> if (b->memory != q->memory) {
> dprintk(1, "%s(): invalid memory type\n", opname);
> - ret = -EINVAL;
> - goto unlock;
> + return -EINVAL;
> }
>
> ret = __verify_planes_array(vb, b);
> if (ret)
> - goto unlock;
> + return ret;
>
> ret = handler(q, b, vb);
> - if (ret)
> - goto unlock;
> -
> - /* Fill buffer information for the userspace */
> - __fill_v4l2_buffer(vb, b);
> + if (!ret) {
> + /* Fill buffer information for the userspace */
> + __fill_v4l2_buffer(vb, b);
>
> - dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
> -unlock:
> - if (mmap_sem)
> - up_read(mmap_sem);
> + dprintk(1, "%s() of buffer %d succeeded\n", opname, vb-
>v4l2_buf.index);
> + }
> return ret;
> }
>
> @@ -1369,6 +1356,9 @@ static int __vb2_qbuf(struct vb2_queue *q, struct
> v4l2_buffer *b, return ret;
> case VB2_BUF_STATE_PREPARED:
> break;
> + case VB2_BUF_STATE_PREPARING:
> + dprintk(1, "qbuf: buffer still being prepared\n");
> + return -EINVAL;
> default:
> dprintk(1, "qbuf: buffer already in use\n");
> return -EINVAL;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bd8218b..4bc4ad2 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -142,6 +142,7 @@ enum vb2_fileio_flags {
> /**
> * enum vb2_buffer_state - current video buffer state
> * @VB2_BUF_STATE_DEQUEUED: buffer under userspace control
> + * @VB2_BUF_STATE_PREPARING: buffer is being prepared in videobuf
> * @VB2_BUF_STATE_PREPARED: buffer prepared in videobuf and by the driver
> * @VB2_BUF_STATE_QUEUED: buffer queued in videobuf, but not in driver
> * @VB2_BUF_STATE_ACTIVE: buffer queued in driver and possibly used
> @@ -154,6 +155,7 @@ enum vb2_fileio_flags {
> */
> enum vb2_buffer_state {
> VB2_BUF_STATE_DEQUEUED,
> + VB2_BUF_STATE_PREPARING,
> VB2_BUF_STATE_PREPARED,
> VB2_BUF_STATE_QUEUED,
> VB2_BUF_STATE_ACTIVE,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare()
2013-11-29 18:16 ` [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare() Laurent Pinchart
@ 2013-12-03 9:41 ` Hans Verkuil
2013-12-04 1:15 ` Laurent Pinchart
0 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2013-12-03 9:41 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
On 11/29/13 19:16, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Friday 29 November 2013 10:58:36 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Rather than taking the mmap semaphore at a relatively high-level function,
>> push it down to the place where it is really needed.
>>
>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
>> vb2 calls. The only way I can see that a race can happen is when two
>> threads queue the same buffer. The solution for that it to introduce
>> a PREPARING state.
>
> This looks better to me, but what about a vb2_reqbufs(0) call being processed
> during the time window where we release the queue mutex ?
You have a nasty mind. That can still go wrong, but note that this is true for
the existing code as well as far as I can tell.
How about this patch to fix this race?
It just refuses to free buffers if any of them is in the preparing state.
It returns EAGAIN in that case (or would EBUSY be better?).
Regards,
Hans
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2aff646..c3ff993 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -284,10 +284,28 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
* related information, if no buffers are left return the queue to an
* uninitialized state. Might be called even if the queue has already been freed.
*/
-static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
+static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
{
unsigned int buffer;
+ /*
+ * Sanity check: when preparing a buffer the queue lock is released for
+ * a short while (see __buf_prepare for the details), which would allow
+ * a race with a reqbufs which can call this function. Removing the
+ * buffers from underneath __buf_prepare is obviously a bad idea, so we
+ * check if any of the buffers is in the state PREPARING, and if so we
+ * just return -EAGAIN.
+ */
+ for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
+ ++buffer) {
+ if (q->bufs[buffer] == NULL)
+ continue;
+ if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
+ dprintk(1, "reqbufs: preparing buffers, cannot free\n");
+ return -EAGAIN;
+ }
+ }
+
/* Call driver-provided cleanup function for each buffer, if provided */
if (q->ops->buf_cleanup) {
for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
@@ -312,6 +330,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
if (!q->num_buffers)
q->memory = 0;
INIT_LIST_HEAD(&q->queued_list);
+ return 0;
}
/**
@@ -644,7 +663,9 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
return -EBUSY;
}
- __vb2_queue_free(q, q->num_buffers);
+ ret = __vb2_queue_free(q, q->num_buffers);
+ if (ret)
+ return ret;
/*
* In case of REQBUFS(0) return immediately without calling
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFCv2 PATCH 1/9] vb2: push the mmap semaphore down to __buf_prepare()
2013-12-03 9:41 ` Hans Verkuil
@ 2013-12-04 1:15 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2013-12-04 1:15 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, m.szyprowski, pawel, awalls, Hans Verkuil
Hi Hans,
On Tuesday 03 December 2013 10:41:34 Hans Verkuil wrote:
> On 11/29/13 19:16, Laurent Pinchart wrote:
> > On Friday 29 November 2013 10:58:36 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Rather than taking the mmap semaphore at a relatively high-level
> >> function, push it down to the place where it is really needed.
> >>
> >> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
> >> vb2 calls. The only way I can see that a race can happen is when two
> >> threads queue the same buffer. The solution for that it to introduce
> >> a PREPARING state.
> >
> > This looks better to me, but what about a vb2_reqbufs(0) call being
> > processed during the time window where we release the queue mutex ?
>
> You have a nasty mind.
I wonder how I should take that :-)
> That can still go wrong, but note that this is true for the existing code as
> well as far as I can tell.
Yes, that's right.
> How about this patch to fix this race?
>
> It just refuses to free buffers if any of them is in the preparing state.
> It returns EAGAIN in that case (or would EBUSY be better?).
I would have argued that applications might not be prepared to handle such an
error when calling REQBUFS(0), but they shouldn't trigger that in the first
place anyway. The patch looks good to me.
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 2aff646..c3ff993 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -284,10 +284,28 @@ static void __vb2_free_mem(struct vb2_queue *q,
> unsigned int buffers) * related information, if no buffers are left return
> the queue to an * uninitialized state. Might be called even if the queue
> has already been freed. */
> -static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> +static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> {
> unsigned int buffer;
>
> + /*
> + * Sanity check: when preparing a buffer the queue lock is released for
> + * a short while (see __buf_prepare for the details), which would allow
> + * a race with a reqbufs which can call this function. Removing the
> + * buffers from underneath __buf_prepare is obviously a bad idea, so we
> + * check if any of the buffers is in the state PREPARING, and if so we
> + * just return -EAGAIN.
> + */
> + for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> + ++buffer) {
> + if (q->bufs[buffer] == NULL)
> + continue;
> + if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
> + dprintk(1, "reqbufs: preparing buffers, cannot free\n");
> + return -EAGAIN;
> + }
> + }
> +
> /* Call driver-provided cleanup function for each buffer, if provided */
> if (q->ops->buf_cleanup) {
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> @@ -312,6 +330,7 @@ static void __vb2_queue_free(struct vb2_queue *q,
> unsigned int buffers) if (!q->num_buffers)
> q->memory = 0;
> INIT_LIST_HEAD(&q->queued_list);
> + return 0;
> }
>
> /**
> @@ -644,7 +663,9 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) return -EBUSY;
> }
>
> - __vb2_queue_free(q, q->num_buffers);
> + ret = __vb2_queue_free(q, q->num_buffers);
> + if (ret)
> + return ret;
>
> /*
> * In case of REQBUFS(0) return immediately without calling
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread