vb2: yet another attempt to fix the vb2/VBI/poll regression

RFC PATCH In review Hans Verkuil 1 participant 2 messages Sep 19, 2014

Subsystems (2)

Media (V4L/DVB) VIDEOBUF2 framework

lore

Share & tools

lore

Hans Verkuil

author

2014-09-19 16:15 UTC #0 lore raw

RFC PATCH 1/1 vb2: yet another attempt to fix the vb2/VBI/poll regression

After a long discussion on irc the decision was taken that poll() should:

- return POLLERR when not streaming or when q->error is set
- return POLLERR when streaming from a capture queue, but no buffers have been queued yet, and it is not part of an M2M device.

The first rule is logical, the second less so. It emulates vb1 behavior that some applications might rely on. It is behavior that we don't want for output devices or M2M devices because calling STREAMON without QBUF makes a lot of sense for output devices, and for M2M devices I want to avoid causing a regression by potentially changing the behavior of M2M capture queues. We don't have legacy apps to support there, so let's make sure that those queue types remain unchanged.

I do that by setting needs_buffers to false in v4l2_m2m_streamon. All M2M drivers use that function with the exception of s5p-mfc, but there STREAMON will return an error if not enough buffers are queued so it's not able to do STREAMON without a QBUF anyway.

There will be a second version, since I need to update some comments in the header and adjust the spec, but I would like to get code reviews as soon as possible.

Just explaining that second rule makes my head hurt, which is usually a bad sign.

Hans

Signed-off-by Hans Verkuil <[email protected]>

drivers/media/v4l2-core/v4l2-mem2mem.c +1 -0

index 80c588f..c8d2b5b 100644
@@ -463,6 +463,7 @@ int v4l2_m2m_streamon(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	int ret;
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, type);
+	vq->needs_buffers = false;
 	ret = vb2_streamon(vq, type);
 	if (!ret)
 		v4l2_m2m_try_schedule(m2m_ctx);

drivers/media/v4l2-core/videobuf2-core.c +13 -3

index 7e6aff6..efbf1ce 100644
@@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	 * to the userspace.
 	 */
 	req->count = allocated_buffers;
+	q->needs_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
 
 	return 0;
 }

@@ -1801,6 +1802,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	 */
 	list_add_tail(&vb->queued_entry, &q->queued_list);
 	q->queued_count++;
+	q->needs_buffers = false;
 	vb->state = VB2_BUF_STATE_QUEUED;
 	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
 		/*

@@ -2583,10 +2585,18 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	}
 
 	/*
-	 * There is nothing to wait for if no buffer has been queued and the
-	 * queue isn't streaming, or if the error flag is set.
+	 * There is nothing to wait for if the queue isn't streaming, or if the
+	 * error flag is set.
 	 */
-	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
+	if (!vb2_is_streaming(q) || q->error)
+		return res | POLLERR;
+	/*
+	 * For compatibility with vb1: if QBUF hasn't been called yet, then
+	 * return POLLERR as well. This only affects capture queues, output
+	 * queues will always initialize needs_buffers to false. M2M devices
+	 * also set needs_buffers to false in v4l2_m2m_streamon().
+	 */
+	if (q->needs_buffers)
 		return res | POLLERR;
 
 	/*

include/media/videobuf2-core.h +1 -0

index 5a10d8d..1c218b1 100644
@@ -419,6 +419,7 @@ struct vb2_queue {
 	unsigned int			streaming:1;
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
+	unsigned int			needs_buffers:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;

Hans Verkuil

author

2014-09-19 16:55 UTC #1 lore raw

Quoted 32 lines · Hans Verkuil

After a long discussion on irc the decision was taken that poll() should:
- return POLLERR when not streaming or when q->error is set
- return POLLERR when streaming from a capture queue, but no buffers have been queued yet, and it is not part of an M2M device.
The first rule is logical, the second less so. It emulates vb1 behavior that some applications might rely on. It is behavior that we don't want for output devices or M2M devices because calling STREAMON without QBUF makes a lot of sense for output devices, and for M2M devices I want to avoid causing a regression by potentially changing the behavior of M2M capture queues. We don't have legacy apps to support there, so let's make sure that those queue types remain unchanged.
I do that by setting needs_buffers to false in v4l2_m2m_streamon. All M2M drivers use that function with the exception of s5p-mfc, but there STREAMON will return an error if not enough buffers are queued so it's not able to do STREAMON without a QBUF anyway.
There will be a second version, since I need to update some comments in the header and adjust the spec, but I would like to get code reviews as soon as possible.
Just explaining that second rule makes my head hurt, which is usually a bad sign.
Hans
Signed-off-by Hans Verkuil <[email protected]>
drivers/media/v4l2-core/v4l2-mem2mem.c +0 -0
index 80c588f..c8d2b5b 100644

drivers/media/v4l2-core/v4l2-mem2mem.c int v4l2_m2m_streamon(struct file *file, struct v4l2_m2m_ctx *m2m_ctx, · line 466

Quoted 4 lines

drivers/media/v4l2-core/v4l2-mem2mem.c +1 -0

@@ -463,6 +463,7 @@ int v4l2_m2m_streamon(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	int ret;
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, type);
+	vq->needs_buffers = false;

I'm going to drop this. Up to 3.16 all vb2 drivers would return POLLERR if no buffers were queued, so it was standard behavior. That simplifies the patch a bit.

Hans

Quoted 62 lines

drivers/media/v4l2-core/v4l2-mem2mem.c +0 -0

 	ret = vb2_streamon(vq, type);
 	if (!ret)
 		v4l2_m2m_try_schedule(m2m_ctx);

drivers/media/v4l2-core/videobuf2-core.c +13 -3

index 7e6aff6..efbf1ce 100644
@@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	 * to the userspace.
 	 */
 	req->count = allocated_buffers;
+	q->needs_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
 
 	return 0;
 }

@@ -1801,6 +1802,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	 */
 	list_add_tail(&vb->queued_entry, &q->queued_list);
 	q->queued_count++;
+	q->needs_buffers = false;
 	vb->state = VB2_BUF_STATE_QUEUED;
 	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
 		/*

@@ -2583,10 +2585,18 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	}
 
 	/*
-	 * There is nothing to wait for if no buffer has been queued and the
-	 * queue isn't streaming, or if the error flag is set.
+	 * There is nothing to wait for if the queue isn't streaming, or if the
+	 * error flag is set.
 	 */
-	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
+	if (!vb2_is_streaming(q) || q->error)
+		return res | POLLERR;
+	/*
+	 * For compatibility with vb1: if QBUF hasn't been called yet, then
+	 * return POLLERR as well. This only affects capture queues, output
+	 * queues will always initialize needs_buffers to false. M2M devices
+	 * also set needs_buffers to false in v4l2_m2m_streamon().
+	 */
+	if (q->needs_buffers)
 		return res | POLLERR;
 
 	/*

include/media/videobuf2-core.h +1 -1

index 5a10d8d..1c218b1 100644
@@ -419,6 +419,7 @@ struct vb2_queue {
 	unsigned int			streaming:1;
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
+	unsigned int			needs_buffers:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
--

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html