[PATCH/RFC 0/2] libv4l2: fix deadlock when DQBUF in block mode

[PATCH/RFC 0/2] libv4l2: fix deadlock when DQBUF in block mode

[Thiago Santos]

This patchset modifies v4l2grab to allow using 2 threads (one for qbuf and
another for dqbuf) to simulate multithreaded v4l2 usage.

This is done to show a issue when using libv4l2 in blocking mode, if a DQBUF
is issued when there are no buffers available it will block waiting for one but,
as it blocks holding the stream_lock, a QBUF will never happen and we have
a deadlock.

Thiago Santos (2):
  v4l2grab: Add threaded producer/consumer option
  libv4l2: release the lock before doing a DQBUF

 contrib/test/Makefile.am |   2 +-
 contrib/test/v4l2grab.c  | 265 +++++++++++++++++++++++++++++++++++++++--------
 lib/libv4l2/libv4l2.c    |   2 +
 3 files changed, 225 insertions(+), 44 deletions(-)

-- 
2.0.0

[PATCH/RFC 1/2] v4l2grab: Add threaded producer/consumer option

[Thiago Santos]

Adds options to allow the buffer dqbuf to happen on one thread while
the qbuf happens on another. This is useful to test concurrency access to
the v4l2 features. To enable this, 3 new options were added:

t: enable threaded mode (off by default and will use the loop)
b: enable blocking io mode (off by default
s: how much the consumer thread will sleep after reading a buffer, this is to
   simulate the time that it takes to process a buffer in a real application
   (in ms)

For example, you can simulate an application that takes 1s to process a buffer
with:

v4l2grab -t 1 -b 1 -s 1000

Signed-off-by: Thiago Santos <ts.santos@sisa.samsung.com>
---
 contrib/test/Makefile.am |   2 +-
 contrib/test/v4l2grab.c  | 265 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 223 insertions(+), 44 deletions(-)

diff --git a/contrib/test/Makefile.am b/contrib/test/Makefile.am
index 80c7665..c2e3860 100644
--- a/contrib/test/Makefile.am
+++ b/contrib/test/Makefile.am
@@ -25,7 +25,7 @@ pixfmt_test_CFLAGS = $(X11_CFLAGS)
 pixfmt_test_LDFLAGS = $(X11_LIBS)
 
 v4l2grab_SOURCES = v4l2grab.c
-v4l2grab_LDADD = ../../lib/libv4l2/libv4l2.la ../../lib/libv4lconvert/libv4lconvert.la
+v4l2grab_LDADD = ../../lib/libv4l2/libv4l2.la ../../lib/libv4lconvert/libv4lconvert.la -lpthread
 
 v4l2gl_SOURCES = v4l2gl.c
 v4l2gl_LDFLAGS = $(X11_LIBS) $(GL_LIBS) $(GLU_LIBS)
diff --git a/contrib/test/v4l2grab.c b/contrib/test/v4l2grab.c
index 674cbe7..2fa5bb8 100644
--- a/contrib/test/v4l2grab.c
+++ b/contrib/test/v4l2grab.c
@@ -24,8 +24,10 @@
 #include <linux/videodev2.h>
 #include "../../lib/include/libv4l2.h"
 #include <argp.h>
+#include <pthread.h>
 
-#define CLEAR(x) memset(&(x), 0, sizeof(x))
+#define CLEAR_P(x,s) memset((x), 0, s)
+#define CLEAR(x) CLEAR_P(&(x), sizeof(x))
 
 struct buffer {
 	void   *start;
@@ -46,22 +48,206 @@ static void xioctl(int fh, unsigned long int request, void *arg)
 	}
 }
 
+/* Used by the multi thread capture version */
+struct buffer_queue {
+	struct v4l2_buffer *buffers;
+	int buffers_size;
+
+	int read_pos;
+	int write_pos;
+	int n_frames;
+
+	int fd;
+
+	pthread_mutex_t mutex;
+	pthread_cond_t buffer_cond;
+};
+
+/* Gets a buffer and puts it in the buffers list at write position, then
+ * notifies the consumer that a new buffer is ready to be used */
+static void *produce_buffer (void * p)
+{
+	struct buffer_queue 		*bq;
+	fd_set				fds;
+	struct timeval			tv;
+	int				i;
+	struct v4l2_buffer		*buf;
+	int				r;
+
+	bq = p;
+
+	for (i = 0; i < bq->n_frames; i++) {
+		printf ("Prod: %d\n", i);
+		buf = &bq->buffers[bq->write_pos % bq->buffers_size];
+		do {
+			FD_ZERO(&fds);
+			FD_SET(bq->fd, &fds);
+
+			/* Timeout. */
+			tv.tv_sec = 2;
+			tv.tv_usec = 0;
+
+			r = select(bq->fd + 1, &fds, NULL, NULL, &tv);
+		} while ((r == -1 && (errno == EINTR)));
+		if (r == -1) {
+			perror("select");
+			pthread_exit (NULL);
+			return NULL;
+		}
+
+		CLEAR_P(buf, sizeof(struct v4l2_buffer));
+		buf->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf->memory = V4L2_MEMORY_MMAP;
+		xioctl(bq->fd, VIDIOC_DQBUF, buf);
+
+		pthread_mutex_lock (&bq->mutex);
+		bq->write_pos++;
+		printf ("Prod: %d (done)\n", i);
+		pthread_cond_signal (&bq->buffer_cond);
+		pthread_mutex_unlock (&bq->mutex);
+
+	}
+
+	pthread_exit (NULL);
+}
+
+/* will create a separate thread that will produce the buffers and put
+ * into a circular array while this same thread will get the buffers from
+ * this array and 'render' them */
+static int capture_threads (int fd, struct buffer *buffers, int bufpool_size,
+			struct v4l2_format fmt, int n_frames,
+			char *out_dir, int sleep_ms)
+{
+	struct v4l2_buffer		buf;
+	unsigned int			i;
+	struct buffer_queue		buf_queue;
+	pthread_t			producer;
+	char				out_name[25 + strlen(out_dir)];
+	FILE				*fout;
+	struct timespec			sleeptime;
+
+	if (sleep_ms) {
+		sleeptime.tv_sec = sleep_ms / 1000;
+		sleeptime.tv_nsec = (sleep_ms % 1000) * 1000000;
+	}
+
+	buf_queue.buffers_size = bufpool_size * 2;
+	buf_queue.buffers = calloc(buf_queue.buffers_size,
+				   sizeof(struct v4l2_buffer));
+	buf_queue.fd = fd;
+	buf_queue.read_pos = 0;
+	buf_queue.write_pos = 0;
+	buf_queue.n_frames = n_frames;
+	pthread_mutex_init (&buf_queue.mutex, NULL);
+	pthread_cond_init (&buf_queue.buffer_cond, NULL);
+
+	pthread_create (&producer, NULL, produce_buffer, &buf_queue);
+
+	for (i = 0; i < n_frames; i++) {
+		printf ("Read: %d\n", i);
+
+		/* wait for a buffer to be available in the queue */
+		pthread_mutex_lock (&buf_queue.mutex);
+		while (buf_queue.read_pos == buf_queue.write_pos) {
+			pthread_cond_wait (&buf_queue.buffer_cond,
+					   &buf_queue.mutex);
+		}
+		pthread_mutex_unlock (&buf_queue.mutex);
+
+		if (sleep_ms)
+			nanosleep (&sleeptime, NULL);
+
+		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
+		fout = fopen(out_name, "w");
+		if (!fout) {
+			perror("Cannot open image");
+			exit(EXIT_FAILURE);
+		}
+		fprintf(fout, "P6\n%d %d 255\n",
+			fmt.fmt.pix.width, fmt.fmt.pix.height);
+		buf = buf_queue.buffers[buf_queue.read_pos %
+					buf_queue.buffers_size];
+		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
+		fclose(fout);
+
+		xioctl(fd, VIDIOC_QBUF, &buf);
+
+		pthread_mutex_lock (&buf_queue.mutex);
+		buf_queue.read_pos++;
+		printf ("Read: %d (done)\n", i);
+		pthread_cond_signal (&buf_queue.buffer_cond);
+		pthread_mutex_unlock (&buf_queue.mutex);
+	}
+
+	pthread_mutex_destroy (&buf_queue.mutex);
+	pthread_cond_destroy (&buf_queue.buffer_cond);
+	free (buf_queue.buffers);
+	return 0;
+}
+
+static int capture_loop (int fd, struct buffer *buffers, struct v4l2_format fmt,
+			int n_frames, char *out_dir)
+{
+	struct v4l2_buffer		buf;
+	unsigned int			i;
+	struct timeval			tv;
+	int				r;
+	fd_set				fds;
+	FILE				*fout;
+	char				out_name[25 + strlen(out_dir)];
+
+	for (i = 0; i < n_frames; i++) {
+		do {
+			FD_ZERO(&fds);
+			FD_SET(fd, &fds);
+
+			/* Timeout. */
+			tv.tv_sec = 2;
+			tv.tv_usec = 0;
+
+			r = select(fd + 1, &fds, NULL, NULL, &tv);
+		} while ((r == -1 && (errno == EINTR)));
+		if (r == -1) {
+			perror("select");
+			return errno;
+		}
+
+		CLEAR(buf);
+		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory = V4L2_MEMORY_MMAP;
+		xioctl(fd, VIDIOC_DQBUF, &buf);
+
+		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
+		fout = fopen(out_name, "w");
+		if (!fout) {
+			perror("Cannot open image");
+			exit(EXIT_FAILURE);
+		}
+		fprintf(fout, "P6\n%d %d 255\n",
+			fmt.fmt.pix.width, fmt.fmt.pix.height);
+		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
+		fclose(fout);
+
+		xioctl(fd, VIDIOC_QBUF, &buf);
+	}
+	return 0;
+}
+
 static int capture(char *dev_name, int x_res, int y_res, int n_frames,
-		   char *out_dir)
+		   char *out_dir, int block, int threads, int sleep_ms)
 {
 	struct v4l2_format		fmt;
 	struct v4l2_buffer		buf;
 	struct v4l2_requestbuffers	req;
 	enum v4l2_buf_type		type;
-	fd_set				fds;
-	struct timeval			tv;
-	int				r, fd = -1;
+	int				fd = -1;
 	unsigned int			i, n_buffers;
-	char				out_name[25 + strlen(out_dir)];
-	FILE				*fout;
 	struct buffer			*buffers;
 
-	fd = v4l2_open(dev_name, O_RDWR | O_NONBLOCK, 0);
+	if (block)
+		fd = v4l2_open(dev_name, O_RDWR, 0);
+	else
+		fd = v4l2_open(dev_name, O_RDWR | O_NONBLOCK, 0);
 	if (fd < 0) {
 		perror("Cannot open device");
 		exit(EXIT_FAILURE);
@@ -119,40 +305,11 @@ static int capture(char *dev_name, int x_res, int y_res, int n_frames,
 	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 
 	xioctl(fd, VIDIOC_STREAMON, &type);
-	for (i = 0; i < n_frames; i++) {
-		do {
-			FD_ZERO(&fds);
-			FD_SET(fd, &fds);
-
-			/* Timeout. */
-			tv.tv_sec = 2;
-			tv.tv_usec = 0;
-
-			r = select(fd + 1, &fds, NULL, NULL, &tv);
-		} while ((r == -1 && (errno == EINTR)));
-		if (r == -1) {
-			perror("select");
-			return errno;
-		}
-
-		CLEAR(buf);
-		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		buf.memory = V4L2_MEMORY_MMAP;
-		xioctl(fd, VIDIOC_DQBUF, &buf);
-
-		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
-		fout = fopen(out_name, "w");
-		if (!fout) {
-			perror("Cannot open image");
-			exit(EXIT_FAILURE);
-		}
-		fprintf(fout, "P6\n%d %d 255\n",
-			fmt.fmt.pix.width, fmt.fmt.pix.height);
-		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
-		fclose(fout);
-
-		xioctl(fd, VIDIOC_QBUF, &buf);
-	}
+	if (threads)
+		capture_threads(fd, buffers, 2, fmt, n_frames, out_dir,
+				sleep_ms);
+	else
+		capture_loop(fd, buffers, fmt, n_frames, out_dir);
 
 	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	xioctl(fd, VIDIOC_STREAMOFF, &type);
@@ -179,6 +336,9 @@ static const struct argp_option options[] = {
 	{"xres",	'x',	"XRES",		0,	"horizontal resolution", 0},
 	{"yres",	'y',	"YRES",		0,	"vertical resolution", 0},
 	{"n-frames",	'n',	"NFRAMES",	0,	"number of frames to capture", 0},
+	{"threads",	't',	"THREADS",	0,	"if different threads should capture and save", 0},
+	{"block",	'b',	"BLOCKMODE",	0,	"if blocking mode should be used", 0},
+	{"sleep",	's',	"SLEEP",	0,	"how long should the consumer thread sleep to simulate the processing of a buffer (in ms)"},
 	{ 0, 0, 0, 0, 0, 0 }
 };
 
@@ -188,6 +348,9 @@ static char	*out_dir = ".";
 static int	x_res = 640;
 static int	y_res = 480;
 static int	n_frames = 20;
+static int	threads = 0;
+static int	block = 0;
+static int	sleep_ms = 0;
 
 static error_t parse_opt(int k, char *arg, struct argp_state *state)
 {
@@ -215,6 +378,21 @@ static error_t parse_opt(int k, char *arg, struct argp_state *state)
 		if (val)
 			n_frames = val;
 		break;
+	case 't':
+		val = atoi(arg);
+		if (val)
+			threads = 1;
+		break;
+	case 'b':
+		val = atoi(arg);
+		if (val)
+			block = 1;
+		break;
+	case 's':
+		val = atoi(arg);
+		if (val)
+			sleep_ms = val;
+		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
@@ -232,5 +410,6 @@ int main(int argc, char **argv)
 {
 	argp_parse(&argp, argc, argv, 0, 0, 0);
 
-	return capture(dev_name, x_res, y_res, n_frames, out_dir);
+	return capture(dev_name, x_res, y_res, n_frames, out_dir, block,
+		       threads, sleep_ms);
 }
-- 
2.0.0

[PATCH/RFC 2/2] libv4l2: release the lock before doing a DQBUF

[Thiago Santos]

In blocking mode, if there are no buffers available the DQBUF will block
waiting for a QBUF to be called but it will block holding the streaming
lock which will prevent any QBUF from happening, causing a deadlock.

Can be tested with: v4l2grab -t 1 -b 1 -s 2000

Signed-off-by: Thiago Santos <ts.santos@sisa.samsung.com>
---
 lib/libv4l2/libv4l2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
index c4d69f7..5589fe0 100644
--- a/lib/libv4l2/libv4l2.c
+++ b/lib/libv4l2/libv4l2.c
@@ -290,9 +290,11 @@ static int v4l2_dequeue_and_convert(int index, struct v4l2_buffer *buf,
 		return result;
 
 	do {
+		pthread_mutex_unlock(&devices[index].stream_lock);
 		result = devices[index].dev_ops->ioctl(
 				devices[index].dev_ops_priv,
 				devices[index].fd, VIDIOC_DQBUF, buf);
+		pthread_mutex_lock(&devices[index].stream_lock);
 		if (result) {
 			if (errno != EAGAIN) {
 				int saved_err = errno;
-- 
2.0.0

Re: [PATCH/RFC 1/2] v4l2grab: Add threaded producer/consumer option

[Mauro Carvalho Chehab]

Em Thu, 05 Jun 2014 12:31:23 -0300
Thiago Santos <ts.santos@sisa.samsung.com> escreveu:

> Adds options to allow the buffer dqbuf to happen on one thread while
> the qbuf happens on another. This is useful to test concurrency access to
> the v4l2 features. To enable this, 3 new options were added:
> 
> t: enable threaded mode (off by default and will use the loop)
> b: enable blocking io mode (off by default
> s: how much the consumer thread will sleep after reading a buffer, this is to
>    simulate the time that it takes to process a buffer in a real application
>    (in ms)

This is not a full review. Just a comment: as the possible values for -t and
-b are just 0 or 1, why to require an argument?

IMHO, the better would be to keep thread disabled by default, and use -t to
enable it, and do the same for -b. To make it clearer when this is called with
--help, we can even name the long option as "--thread-enable" and 
"--blockmode-enable".

Regards,
Mauro

> 
> For example, you can simulate an application that takes 1s to process a buffer
> with:
> 
> v4l2grab -t 1 -b 1 -s 1000
> 
> Signed-off-by: Thiago Santos <ts.santos@sisa.samsung.com>
> ---
>  contrib/test/Makefile.am |   2 +-
>  contrib/test/v4l2grab.c  | 265 +++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 223 insertions(+), 44 deletions(-)
> 
> diff --git a/contrib/test/Makefile.am b/contrib/test/Makefile.am
> index 80c7665..c2e3860 100644
> --- a/contrib/test/Makefile.am
> +++ b/contrib/test/Makefile.am
> @@ -25,7 +25,7 @@ pixfmt_test_CFLAGS = $(X11_CFLAGS)
>  pixfmt_test_LDFLAGS = $(X11_LIBS)
>  
>  v4l2grab_SOURCES = v4l2grab.c
> -v4l2grab_LDADD = ../../lib/libv4l2/libv4l2.la ../../lib/libv4lconvert/libv4lconvert.la
> +v4l2grab_LDADD = ../../lib/libv4l2/libv4l2.la ../../lib/libv4lconvert/libv4lconvert.la -lpthread
>  
>  v4l2gl_SOURCES = v4l2gl.c
>  v4l2gl_LDFLAGS = $(X11_LIBS) $(GL_LIBS) $(GLU_LIBS)
> diff --git a/contrib/test/v4l2grab.c b/contrib/test/v4l2grab.c
> index 674cbe7..2fa5bb8 100644
> --- a/contrib/test/v4l2grab.c
> +++ b/contrib/test/v4l2grab.c
> @@ -24,8 +24,10 @@
>  #include <linux/videodev2.h>
>  #include "../../lib/include/libv4l2.h"
>  #include <argp.h>
> +#include <pthread.h>
>  
> -#define CLEAR(x) memset(&(x), 0, sizeof(x))
> +#define CLEAR_P(x,s) memset((x), 0, s)
> +#define CLEAR(x) CLEAR_P(&(x), sizeof(x))
>  
>  struct buffer {
>  	void   *start;
> @@ -46,22 +48,206 @@ static void xioctl(int fh, unsigned long int request, void *arg)
>  	}
>  }
>  
> +/* Used by the multi thread capture version */
> +struct buffer_queue {
> +	struct v4l2_buffer *buffers;
> +	int buffers_size;
> +
> +	int read_pos;
> +	int write_pos;
> +	int n_frames;
> +
> +	int fd;
> +
> +	pthread_mutex_t mutex;
> +	pthread_cond_t buffer_cond;
> +};
> +
> +/* Gets a buffer and puts it in the buffers list at write position, then
> + * notifies the consumer that a new buffer is ready to be used */
> +static void *produce_buffer (void * p)
> +{
> +	struct buffer_queue 		*bq;
> +	fd_set				fds;
> +	struct timeval			tv;
> +	int				i;
> +	struct v4l2_buffer		*buf;
> +	int				r;
> +
> +	bq = p;
> +
> +	for (i = 0; i < bq->n_frames; i++) {
> +		printf ("Prod: %d\n", i);
> +		buf = &bq->buffers[bq->write_pos % bq->buffers_size];
> +		do {
> +			FD_ZERO(&fds);
> +			FD_SET(bq->fd, &fds);
> +
> +			/* Timeout. */
> +			tv.tv_sec = 2;
> +			tv.tv_usec = 0;
> +
> +			r = select(bq->fd + 1, &fds, NULL, NULL, &tv);
> +		} while ((r == -1 && (errno == EINTR)));
> +		if (r == -1) {
> +			perror("select");
> +			pthread_exit (NULL);
> +			return NULL;
> +		}
> +
> +		CLEAR_P(buf, sizeof(struct v4l2_buffer));
> +		buf->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		buf->memory = V4L2_MEMORY_MMAP;
> +		xioctl(bq->fd, VIDIOC_DQBUF, buf);
> +
> +		pthread_mutex_lock (&bq->mutex);
> +		bq->write_pos++;
> +		printf ("Prod: %d (done)\n", i);
> +		pthread_cond_signal (&bq->buffer_cond);
> +		pthread_mutex_unlock (&bq->mutex);
> +
> +	}
> +
> +	pthread_exit (NULL);
> +}
> +
> +/* will create a separate thread that will produce the buffers and put
> + * into a circular array while this same thread will get the buffers from
> + * this array and 'render' them */
> +static int capture_threads (int fd, struct buffer *buffers, int bufpool_size,
> +			struct v4l2_format fmt, int n_frames,
> +			char *out_dir, int sleep_ms)
> +{
> +	struct v4l2_buffer		buf;
> +	unsigned int			i;
> +	struct buffer_queue		buf_queue;
> +	pthread_t			producer;
> +	char				out_name[25 + strlen(out_dir)];
> +	FILE				*fout;
> +	struct timespec			sleeptime;
> +
> +	if (sleep_ms) {
> +		sleeptime.tv_sec = sleep_ms / 1000;
> +		sleeptime.tv_nsec = (sleep_ms % 1000) * 1000000;
> +	}
> +
> +	buf_queue.buffers_size = bufpool_size * 2;
> +	buf_queue.buffers = calloc(buf_queue.buffers_size,
> +				   sizeof(struct v4l2_buffer));
> +	buf_queue.fd = fd;
> +	buf_queue.read_pos = 0;
> +	buf_queue.write_pos = 0;
> +	buf_queue.n_frames = n_frames;
> +	pthread_mutex_init (&buf_queue.mutex, NULL);
> +	pthread_cond_init (&buf_queue.buffer_cond, NULL);
> +
> +	pthread_create (&producer, NULL, produce_buffer, &buf_queue);
> +
> +	for (i = 0; i < n_frames; i++) {
> +		printf ("Read: %d\n", i);
> +
> +		/* wait for a buffer to be available in the queue */
> +		pthread_mutex_lock (&buf_queue.mutex);
> +		while (buf_queue.read_pos == buf_queue.write_pos) {
> +			pthread_cond_wait (&buf_queue.buffer_cond,
> +					   &buf_queue.mutex);
> +		}
> +		pthread_mutex_unlock (&buf_queue.mutex);
> +
> +		if (sleep_ms)
> +			nanosleep (&sleeptime, NULL);
> +
> +		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
> +		fout = fopen(out_name, "w");
> +		if (!fout) {
> +			perror("Cannot open image");
> +			exit(EXIT_FAILURE);
> +		}
> +		fprintf(fout, "P6\n%d %d 255\n",
> +			fmt.fmt.pix.width, fmt.fmt.pix.height);
> +		buf = buf_queue.buffers[buf_queue.read_pos %
> +					buf_queue.buffers_size];
> +		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
> +		fclose(fout);
> +
> +		xioctl(fd, VIDIOC_QBUF, &buf);
> +
> +		pthread_mutex_lock (&buf_queue.mutex);
> +		buf_queue.read_pos++;
> +		printf ("Read: %d (done)\n", i);
> +		pthread_cond_signal (&buf_queue.buffer_cond);
> +		pthread_mutex_unlock (&buf_queue.mutex);
> +	}
> +
> +	pthread_mutex_destroy (&buf_queue.mutex);
> +	pthread_cond_destroy (&buf_queue.buffer_cond);
> +	free (buf_queue.buffers);
> +	return 0;
> +}
> +
> +static int capture_loop (int fd, struct buffer *buffers, struct v4l2_format fmt,
> +			int n_frames, char *out_dir)
> +{
> +	struct v4l2_buffer		buf;
> +	unsigned int			i;
> +	struct timeval			tv;
> +	int				r;
> +	fd_set				fds;
> +	FILE				*fout;
> +	char				out_name[25 + strlen(out_dir)];
> +
> +	for (i = 0; i < n_frames; i++) {
> +		do {
> +			FD_ZERO(&fds);
> +			FD_SET(fd, &fds);
> +
> +			/* Timeout. */
> +			tv.tv_sec = 2;
> +			tv.tv_usec = 0;
> +
> +			r = select(fd + 1, &fds, NULL, NULL, &tv);
> +		} while ((r == -1 && (errno == EINTR)));
> +		if (r == -1) {
> +			perror("select");
> +			return errno;
> +		}
> +
> +		CLEAR(buf);
> +		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		buf.memory = V4L2_MEMORY_MMAP;
> +		xioctl(fd, VIDIOC_DQBUF, &buf);
> +
> +		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
> +		fout = fopen(out_name, "w");
> +		if (!fout) {
> +			perror("Cannot open image");
> +			exit(EXIT_FAILURE);
> +		}
> +		fprintf(fout, "P6\n%d %d 255\n",
> +			fmt.fmt.pix.width, fmt.fmt.pix.height);
> +		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
> +		fclose(fout);
> +
> +		xioctl(fd, VIDIOC_QBUF, &buf);
> +	}
> +	return 0;
> +}
> +
>  static int capture(char *dev_name, int x_res, int y_res, int n_frames,
> -		   char *out_dir)
> +		   char *out_dir, int block, int threads, int sleep_ms)
>  {
>  	struct v4l2_format		fmt;
>  	struct v4l2_buffer		buf;
>  	struct v4l2_requestbuffers	req;
>  	enum v4l2_buf_type		type;
> -	fd_set				fds;
> -	struct timeval			tv;
> -	int				r, fd = -1;
> +	int				fd = -1;
>  	unsigned int			i, n_buffers;
> -	char				out_name[25 + strlen(out_dir)];
> -	FILE				*fout;
>  	struct buffer			*buffers;
>  
> -	fd = v4l2_open(dev_name, O_RDWR | O_NONBLOCK, 0);
> +	if (block)
> +		fd = v4l2_open(dev_name, O_RDWR, 0);
> +	else
> +		fd = v4l2_open(dev_name, O_RDWR | O_NONBLOCK, 0);
>  	if (fd < 0) {
>  		perror("Cannot open device");
>  		exit(EXIT_FAILURE);
> @@ -119,40 +305,11 @@ static int capture(char *dev_name, int x_res, int y_res, int n_frames,
>  	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  
>  	xioctl(fd, VIDIOC_STREAMON, &type);
> -	for (i = 0; i < n_frames; i++) {
> -		do {
> -			FD_ZERO(&fds);
> -			FD_SET(fd, &fds);
> -
> -			/* Timeout. */
> -			tv.tv_sec = 2;
> -			tv.tv_usec = 0;
> -
> -			r = select(fd + 1, &fds, NULL, NULL, &tv);
> -		} while ((r == -1 && (errno == EINTR)));
> -		if (r == -1) {
> -			perror("select");
> -			return errno;
> -		}
> -
> -		CLEAR(buf);
> -		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -		buf.memory = V4L2_MEMORY_MMAP;
> -		xioctl(fd, VIDIOC_DQBUF, &buf);
> -
> -		sprintf(out_name, "%s/out%03d.ppm", out_dir, i);
> -		fout = fopen(out_name, "w");
> -		if (!fout) {
> -			perror("Cannot open image");
> -			exit(EXIT_FAILURE);
> -		}
> -		fprintf(fout, "P6\n%d %d 255\n",
> -			fmt.fmt.pix.width, fmt.fmt.pix.height);
> -		fwrite(buffers[buf.index].start, buf.bytesused, 1, fout);
> -		fclose(fout);
> -
> -		xioctl(fd, VIDIOC_QBUF, &buf);
> -	}
> +	if (threads)
> +		capture_threads(fd, buffers, 2, fmt, n_frames, out_dir,
> +				sleep_ms);
> +	else
> +		capture_loop(fd, buffers, fmt, n_frames, out_dir);
>  
>  	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	xioctl(fd, VIDIOC_STREAMOFF, &type);
> @@ -179,6 +336,9 @@ static const struct argp_option options[] = {
>  	{"xres",	'x',	"XRES",		0,	"horizontal resolution", 0},
>  	{"yres",	'y',	"YRES",		0,	"vertical resolution", 0},
>  	{"n-frames",	'n',	"NFRAMES",	0,	"number of frames to capture", 0},
> +	{"threads",	't',	"THREADS",	0,	"if different threads should capture and save", 0},
> +	{"block",	'b',	"BLOCKMODE",	0,	"if blocking mode should be used", 0},
> +	{"sleep",	's',	"SLEEP",	0,	"how long should the consumer thread sleep to simulate the processing of a buffer (in ms)"},
>  	{ 0, 0, 0, 0, 0, 0 }
>  };
>  
> @@ -188,6 +348,9 @@ static char	*out_dir = ".";
>  static int	x_res = 640;
>  static int	y_res = 480;
>  static int	n_frames = 20;
> +static int	threads = 0;
> +static int	block = 0;
> +static int	sleep_ms = 0;
>  
>  static error_t parse_opt(int k, char *arg, struct argp_state *state)
>  {
> @@ -215,6 +378,21 @@ static error_t parse_opt(int k, char *arg, struct argp_state *state)
>  		if (val)
>  			n_frames = val;
>  		break;
> +	case 't':
> +		val = atoi(arg);
> +		if (val)
> +			threads = 1;
> +		break;
> +	case 'b':
> +		val = atoi(arg);
> +		if (val)
> +			block = 1;
> +		break;
> +	case 's':
> +		val = atoi(arg);
> +		if (val)
> +			sleep_ms = val;
> +		break;
>  	default:
>  		return ARGP_ERR_UNKNOWN;
>  	}
> @@ -232,5 +410,6 @@ int main(int argc, char **argv)
>  {
>  	argp_parse(&argp, argc, argv, 0, 0, 0);
>  
> -	return capture(dev_name, x_res, y_res, n_frames, out_dir);
> +	return capture(dev_name, x_res, y_res, n_frames, out_dir, block,
> +		       threads, sleep_ms);
>  }

Re: [PATCH/RFC 2/2] libv4l2: release the lock before doing a DQBUF

[Hans de Goede]

Hi,

On 06/05/2014 05:31 PM, Thiago Santos wrote:
> In blocking mode, if there are no buffers available the DQBUF will block
> waiting for a QBUF to be called but it will block holding the streaming
> lock which will prevent any QBUF from happening, causing a deadlock.
> 
> Can be tested with: v4l2grab -t 1 -b 1 -s 2000
> 
> Signed-off-by: Thiago Santos <ts.santos@sisa.samsung.com>

Thanks for catching this, and thanks for the patch fixing it.

I'm afraid it is not that simple though. Without the lock the app may do the
following:

Control-thread: set_fmt(fmt), requestbuf(x), map buffers, queue buffers, streamon
Work-thread: dqbuf, process, qbuf, rinse-repeat
Control-thread: streamoff, requestbuf(0), wait for work thread, unmap buffers.

There is a race here with dqbuf succeeding, but not yet being processed
while the control-thread is tearing things down.

If we hit this race then the v4lconvert_convert call will be passed a no longer
valid pointer in the form of devices[index].frame_pointers[buf->index] and likewise
its other parameters may also no longer be accurate (or point to invalid mem
alltogether).

Fixing this without breaking stuff / causing the risk of regressions is very much
non trivial. Since this is a race (normally the dqbuf call would return with an
error because of the streamoff), I believe the best way to fix this is to just
detect this situation and make v4l2_dequeue_and_convert return an error in this
case.

So I suggest that you respin the patch with the following additions.
* Add a frame_info_generation variable to struct v4l2_dev_info
  (below the frame_queued field)
* In v4l2_check_buffer_change_ok() increment this field *before* calling v4l2_unmap_buffers()
* Read frame_info_generation into a local variable before dropping the lock for the VIDIOC_DQBUF
* Check if frame_info_generation is not changed after this line:
  devices[index].frame_queued &= ~(1 << buf->index);
* If it is changed set errno to -EINVAL (this is what the kernel does when a streamoff is done
  while a dqbuf is pending), and return -1.

* You should also unlock + relock around the DQBUF for the non converted case around line 1297
  in this case no special handling is needed.

Regards,

Hans


> ---
>  lib/libv4l2/libv4l2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
> index c4d69f7..5589fe0 100644
> --- a/lib/libv4l2/libv4l2.c
> +++ b/lib/libv4l2/libv4l2.c
> @@ -290,9 +290,11 @@ static int v4l2_dequeue_and_convert(int index, struct v4l2_buffer *buf,
>  		return result;
>  
>  	do {
> +		pthread_mutex_unlock(&devices[index].stream_lock);
>  		result = devices[index].dev_ops->ioctl(
>  				devices[index].dev_ops_priv,
>  				devices[index].fd, VIDIOC_DQBUF, buf);
> +		pthread_mutex_lock(&devices[index].stream_lock);
>  		if (result) {
>  			if (errno != EAGAIN) {
>  				int saved_err = errno;
>