[PATCHv2 0/3] vb2: fix VBI/poll regression

[PATCHv2 0/3] vb2: fix VBI/poll regression

[Hans Verkuil]

OK, so v1 wasn't the final patch series :-) Let's see if this is.

Changes since v1:

- Also initialize waiting_for_buffers in STREAMOFF and when CREATE_BUFS is
  called and no buffers have been allocated yet.
- Improve some of the wording in patch 2 based on suggestions from Laurent.

This patch series resolves the vb2 VBI poll regression where alevt and mtt
fail on drivers using vb2.

These applications call REQBUFS, queue the buffers and then poll() without
calling STREAMON first. They rely on poll() to return POLLERR in that case
and they do the STREAMON at that time. This is correct according to the spec,
but this was never implemented in vb2.

This is fixed together with an other vb2 regression: calling REQBUFS, then
STREAMON, then poll() without doing a QBUF first should return POLLERR as
well according to the spec. This has been fixed as well and the spec has
been clarified that this is only done for capture queues. Output queues in
the same situation will return as well, but with POLLOUT|POLLWRNORM set
instead of POLLERR.

The final patch adds missing documentation to poll() regarding event handling
and improves the documentation regarding stream I/O and output queues.

Regards,

	Hans

[PATCHv2 1/3] vb2: fix VBI/poll regression

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The recent conversion of saa7134 to vb2 unconvered a poll() bug that
broke the teletext applications alevt and mtt. These applications
expect that calling poll() without having called VIDIOC_STREAMON will
cause poll() to return POLLERR. That did not happen in vb2.

This patch fixes that behavior. It also fixes what should happen when
poll() is called when STREAMON is called but no buffers have been
queued. In that case poll() will also return POLLERR, but only for
capture queues since output queues will always return POLLOUT
anyway in that situation.

This brings the vb2 behavior in line with the old videobuf behavior.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
 include/media/videobuf2-core.h           |  4 ++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7e6aff6..a0aa694 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	 * to the userspace.
 	 */
 	req->count = allocated_buffers;
+	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
 
 	return 0;
 }
@@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
 		memset(q->plane_sizes, 0, sizeof(q->plane_sizes));
 		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
 		q->memory = create->memory;
+		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
 	}
 
 	num_buffers = min(create->count, VIDEO_MAX_FRAME - q->num_buffers);
@@ -1801,6 +1803,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->waiting_for_buffers = false;
 	vb->state = VB2_BUF_STATE_QUEUED;
 	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
 		/*
@@ -2261,6 +2264,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 	 * their normal dequeued state.
 	 */
 	__vb2_queue_cancel(q);
+	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
 
 	dprintk(3, "successful\n");
 	return 0;
@@ -2583,10 +2587,17 @@ 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 waiting_for_buffers to false.
+	 */
+	if (q->waiting_for_buffers)
 		return res | POLLERR;
 
 	/*
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5a10d8d..84f790c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -381,6 +381,9 @@ struct v4l2_fh;
  * @start_streaming_called: start_streaming() was called successfully and we
  *		started streaming.
  * @error:	a fatal error occurred on the queue
+ * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
+ *		buffers. Only set for capture queues if qbuf has not yet been
+ *		called since poll() needs to return POLLERR in that situation.
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
  */
@@ -419,6 +422,7 @@ struct vb2_queue {
 	unsigned int			streaming:1;
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
+	unsigned int			waiting_for_buffers:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
-- 
2.1.0

[PATCHv2 2/3] DocBook media: fix the poll() 'no QBUF' documentation

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Clarify what poll() returns if STREAMON was called but not QBUF.
Make explicit the different behavior for this scenario for
capture and output devices.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/func-poll.xml | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/func-poll.xml b/Documentation/DocBook/media/v4l/func-poll.xml
index 85cad8b..bd07104 100644
--- a/Documentation/DocBook/media/v4l/func-poll.xml
+++ b/Documentation/DocBook/media/v4l/func-poll.xml
@@ -44,10 +44,18 @@ Capture devices set the <constant>POLLIN</constant> and
 flags. When the function timed out it returns a value of zero, on
 failure it returns <returnvalue>-1</returnvalue> and the
 <varname>errno</varname> variable is set appropriately. When the
-application did not call &VIDIOC-QBUF; or &VIDIOC-STREAMON; yet the
+application did not call &VIDIOC-STREAMON; the
 <function>poll()</function> function succeeds, but sets the
 <constant>POLLERR</constant> flag in the
-<structfield>revents</structfield> field.</para>
+<structfield>revents</structfield> field. When the
+application has called &VIDIOC-STREAMON; for a capture device but hasn't
+yet called &VIDIOC-QBUF;, the <function>poll()</function> function
+succeeds and sets the <constant>POLLERR</constant> flag in the
+<structfield>revents</structfield> field. For output devices this
+same situation will cause <function>poll()</function> to succeed
+as well, but it sets the <constant>POLLOUT</constant> and
+<constant>POLLWRNORM</constant> flags in the <structfield>revents</structfield>
+field.</para>
 
     <para>When use of the <function>read()</function> function has
 been negotiated and the driver does not capture yet, the
-- 
2.1.0

[PATCHv2 3/3] DocBook media: improve the poll() documentation

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The poll documentation was incomplete: document how events (POLLPRI)
are handled and fix the documentation of what poll does for display devices
and streaming I/O.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/func-poll.xml | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/func-poll.xml b/Documentation/DocBook/media/v4l/func-poll.xml
index bd07104..4c73f11 100644
--- a/Documentation/DocBook/media/v4l/func-poll.xml
+++ b/Documentation/DocBook/media/v4l/func-poll.xml
@@ -29,9 +29,12 @@ can suspend execution until the driver has captured data or is ready
 to accept data for output.</para>
 
     <para>When streaming I/O has been negotiated this function waits
-until a buffer has been filled or displayed and can be dequeued with
-the &VIDIOC-DQBUF; ioctl. When buffers are already in the outgoing
-queue of the driver the function returns immediately.</para>
+until a buffer has been filled by the capture device and can be dequeued
+with the &VIDIOC-DQBUF; ioctl. For output devices this function waits
+until the device is ready to accept a new buffer to be queued up with
+the &VIDIOC-QBUF; ioctl for display. When buffers are already in the outgoing
+queue of the driver (capture) or the incoming queue isn't full (display)
+the function returns immediately.</para>
 
     <para>On success <function>poll()</function> returns the number of
 file descriptors that have been selected (that is, file descriptors
@@ -57,6 +60,10 @@ as well, but it sets the <constant>POLLOUT</constant> and
 <constant>POLLWRNORM</constant> flags in the <structfield>revents</structfield>
 field.</para>
 
+    <para>If an event occurred (see &VIDIOC-DQEVENT;) then
+<constant>POLLPRI</constant> will be set in the <structfield>revents</structfield>
+field and <function>poll()</function> will return.</para>
+
     <para>When use of the <function>read()</function> function has
 been negotiated and the driver does not capture yet, the
 <function>poll</function> function starts capturing. When that fails
@@ -66,10 +73,18 @@ continuously (as opposed to, for example, still images) the function
 may return immediately.</para>
 
     <para>When use of the <function>write()</function> function has
-been negotiated the <function>poll</function> function just waits
+been negotiated and the driver does not stream yet, the
+<function>poll</function> function starts streaming. When that fails
+it returns a <constant>POLLERR</constant> as above. Otherwise it waits
 until the driver is ready for a non-blocking
 <function>write()</function> call.</para>
 
+    <para>If the caller is only interested in events (just
+<constant>POLLPRI</constant> is set in the <structfield>events</structfield>
+field), then <function>poll()</function> will <emphasis>not</emphasis>
+start streaming if the driver does not stream yet. This makes it
+possible to just poll for events and not for buffers.</para>
+
     <para>All drivers implementing the <function>read()</function> or
 <function>write()</function> function or streaming I/O must also
 support the <function>poll()</function> function.</para>
-- 
2.1.0

Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
> broke the teletext applications alevt and mtt. These applications
> expect that calling poll() without having called VIDIOC_STREAMON will
> cause poll() to return POLLERR. That did not happen in vb2.
> 
> This patch fixes that behavior. It also fixes what should happen when
> poll() is called when STREAMON is called but no buffers have been
> queued. In that case poll() will also return POLLERR, but only for
> capture queues since output queues will always return POLLOUT
> anyway in that situation.
> 
> This brings the vb2 behavior in line with the old videobuf behavior.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
>  include/media/videobuf2-core.h           |  4 ++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) * to the userspace.
>  	 */
>  	req->count = allocated_buffers;
> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> 
>  	return 0;
>  }
> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create memset(q->plane_sizes, 0,
> sizeof(q->plane_sizes));
>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
>  		q->memory = create->memory;
> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);

Wouldn't it be easier to set the flag when creating the queue and in 
vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ? I'll let 
you decide.

>  	}
> 
>  	num_buffers = min(create->count, VIDEO_MAX_FRAME - q->num_buffers);
> @@ -1801,6 +1803,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->waiting_for_buffers = false;
>  	vb->state = VB2_BUF_STATE_QUEUED;
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>  		/*
> @@ -2261,6 +2264,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q,
> enum v4l2_buf_type type) * their normal dequeued state.
>  	 */
>  	__vb2_queue_cancel(q);
> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> 
>  	dprintk(3, "successful\n");
>  	return 0;
> @@ -2583,10 +2587,17 @@ 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 waiting_for_buffers to false.
> +	 */
> +	if (q->waiting_for_buffers)
>  		return res | POLLERR;
> 
>  	/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 5a10d8d..84f790c 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -381,6 +381,9 @@ struct v4l2_fh;
>   * @start_streaming_called: start_streaming() was called successfully and
> we *		started streaming.
>   * @error:	a fatal error occurred on the queue
> + * @waiting_for_buffers: used in poll() to check if vb2 is still waiting
> for + *		buffers. Only set for capture queues if qbuf has not yet been +
> *		called since poll() needs to return POLLERR in that situation. *
> @fileio:	file io emulator internal data, used only if emulator is active 
*
> @threadio:	thread io internal data, used only if thread is active */
> @@ -419,6 +422,7 @@ struct vb2_queue {
>  	unsigned int			streaming:1;
>  	unsigned int			start_streaming_called:1;
>  	unsigned int			error:1;
> +	unsigned int			waiting_for_buffers:1;
> 
>  	struct vb2_fileio_data		*fileio;
>  	struct vb2_threadio_data	*threadio;

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 0/3] vb2: fix VBI/poll regression

[Laurent Pinchart]

Hi Hans,

Thank you for the patches.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Saturday 20 September 2014 21:16:34 Hans Verkuil wrote:
> OK, so v1 wasn't the final patch series :-) Let's see if this is.
> 
> Changes since v1:
> 
> - Also initialize waiting_for_buffers in STREAMOFF and when CREATE_BUFS is
>   called and no buffers have been allocated yet.
> - Improve some of the wording in patch 2 based on suggestions from Laurent.
> 
> This patch series resolves the vb2 VBI poll regression where alevt and mtt
> fail on drivers using vb2.
> 
> These applications call REQBUFS, queue the buffers and then poll() without
> calling STREAMON first. They rely on poll() to return POLLERR in that case
> and they do the STREAMON at that time. This is correct according to the
> spec, but this was never implemented in vb2.
> 
> This is fixed together with an other vb2 regression: calling REQBUFS, then
> STREAMON, then poll() without doing a QBUF first should return POLLERR as
> well according to the spec. This has been fixed as well and the spec has
> been clarified that this is only done for capture queues. Output queues in
> the same situation will return as well, but with POLLOUT|POLLWRNORM set
> instead of POLLERR.
> 
> The final patch adds missing documentation to poll() regarding event
> handling and improves the documentation regarding stream I/O and output
> queues.
> 
> Regards,
> 
> 	Hans

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Hans Verkuil]

On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
>> broke the teletext applications alevt and mtt. These applications
>> expect that calling poll() without having called VIDIOC_STREAMON will
>> cause poll() to return POLLERR. That did not happen in vb2.
>>
>> This patch fixes that behavior. It also fixes what should happen when
>> poll() is called when STREAMON is called but no buffers have been
>> queued. In that case poll() will also return POLLERR, but only for
>> capture queues since output queues will always return POLLOUT
>> anyway in that situation.
>>
>> This brings the vb2 behavior in line with the old videobuf behavior.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
>>  include/media/videobuf2-core.h           |  4 ++++
>>  2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
>> v4l2_requestbuffers *req) * to the userspace.
>>  	 */
>>  	req->count = allocated_buffers;
>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>
>>  	return 0;
>>  }
>> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q, struct
>> v4l2_create_buffers *create memset(q->plane_sizes, 0,
>> sizeof(q->plane_sizes));
>>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
>>  		q->memory = create->memory;
>> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> 
> Wouldn't it be easier to set the flag when creating the queue and in 
> vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ? I'll let 
> you decide.

Sorry, I don't follow. 'When creating the queue'? __create_bufs and __reqbufs
are the functions that create the queue (i.e. allocate the buffers).

Regards,

	Hans

> 
>>  	}
>>
>>  	num_buffers = min(create->count, VIDEO_MAX_FRAME - q->num_buffers);
>> @@ -1801,6 +1803,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->waiting_for_buffers = false;
>>  	vb->state = VB2_BUF_STATE_QUEUED;
>>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>>  		/*
>> @@ -2261,6 +2264,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q,
>> enum v4l2_buf_type type) * their normal dequeued state.
>>  	 */
>>  	__vb2_queue_cancel(q);
>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>
>>  	dprintk(3, "successful\n");
>>  	return 0;
>> @@ -2583,10 +2587,17 @@ 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 waiting_for_buffers to false.
>> +	 */
>> +	if (q->waiting_for_buffers)
>>  		return res | POLLERR;
>>
>>  	/*
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 5a10d8d..84f790c 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -381,6 +381,9 @@ struct v4l2_fh;
>>   * @start_streaming_called: start_streaming() was called successfully and
>> we *		started streaming.
>>   * @error:	a fatal error occurred on the queue
>> + * @waiting_for_buffers: used in poll() to check if vb2 is still waiting
>> for + *		buffers. Only set for capture queues if qbuf has not yet been +
>> *		called since poll() needs to return POLLERR in that situation. *
>> @fileio:	file io emulator internal data, used only if emulator is active 
> *
>> @threadio:	thread io internal data, used only if thread is active */
>> @@ -419,6 +422,7 @@ struct vb2_queue {
>>  	unsigned int			streaming:1;
>>  	unsigned int			start_streaming_called:1;
>>  	unsigned int			error:1;
>> +	unsigned int			waiting_for_buffers:1;
>>
>>  	struct vb2_fileio_data		*fileio;
>>  	struct vb2_threadio_data	*threadio;
> 

Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Laurent Pinchart]

Hi Hans,

On Sunday 21 September 2014 11:00:02 Hans Verkuil wrote:
> On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
> > On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
> >> broke the teletext applications alevt and mtt. These applications
> >> expect that calling poll() without having called VIDIOC_STREAMON will
> >> cause poll() to return POLLERR. That did not happen in vb2.
> >> 
> >> This patch fixes that behavior. It also fixes what should happen when
> >> poll() is called when STREAMON is called but no buffers have been
> >> queued. In that case poll() will also return POLLERR, but only for
> >> capture queues since output queues will always return POLLOUT
> >> anyway in that situation.
> >> 
> >> This brings the vb2 behavior in line with the old videobuf behavior.
> >> 
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
> >>  include/media/videobuf2-core.h           |  4 ++++
> >>  2 files changed, 18 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
> >> v4l2_requestbuffers *req) * to the userspace.
> >>  	 */
> >>  	
> >>  	req->count = allocated_buffers;
> >> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> >>  	return 0;
> >>  }
> >> 
> >> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q,
> >> struct
> >> v4l2_create_buffers *create memset(q->plane_sizes, 0,
> >> sizeof(q->plane_sizes));
> >>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
> >>  		q->memory = create->memory;
> >> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> > 
> > Wouldn't it be easier to set the flag when creating the queue and in
> > vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ? I'll
> > let you decide.
> 
> Sorry, I don't follow. 'When creating the queue'? __create_bufs and
> __reqbufs are the functions that create the queue (i.e. allocate the
> buffers).

I meant vb2_queue_init.

> >>  	}
> >>  	
> >>  	num_buffers = min(create->count, VIDEO_MAX_FRAME - q->num_buffers);
> >> 
> >> @@ -1801,6 +1803,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->waiting_for_buffers = false;
> >> 
> >>  	vb->state = VB2_BUF_STATE_QUEUED;
> >>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> >>  	
> >>  		/*
> >> 
> >> @@ -2261,6 +2264,7 @@ static int vb2_internal_streamoff(struct vb2_queue
> >> *q, enum v4l2_buf_type type) * their normal dequeued state.
> >> 
> >>  	 */
> >>  	
> >>  	__vb2_queue_cancel(q);
> >> 
> >> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> >> 
> >>  	dprintk(3, "successful\n");
> >>  	return 0;
> >> 
> >> @@ -2583,10 +2587,17 @@ 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 waiting_for_buffers to false.
> >> +	 */
> >> +	if (q->waiting_for_buffers)
> >> 
> >>  		return res | POLLERR;
> >>  	
> >>  	/*
> >> 
> >> diff --git a/include/media/videobuf2-core.h
> >> b/include/media/videobuf2-core.h index 5a10d8d..84f790c 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -381,6 +381,9 @@ struct v4l2_fh;
> >>   * @start_streaming_called: start_streaming() was called successfully
> >>   and we
> >>   *		started streaming.
> >>   * @error:	a fatal error occurred on the queue
> >> + * @waiting_for_buffers: used in poll() to check if vb2 is still waiting
> >> for
> >> + *		buffers. Only set for capture queues if qbuf has not yet been
> >> + *		called since poll() needs to return POLLERR in that situation.
> >>   * @fileio:	file io emulator internal data, used only if emulator is
> >>   active
> >>   * @threadio:	thread io internal data, used only if thread is active
> >>   */
> >> @@ -419,6 +422,7 @@ struct vb2_queue {
> >>  	unsigned int			streaming:1;
> >>  	unsigned int			start_streaming_called:1;
> >>  	unsigned int			error:1;
> >> +	unsigned int			waiting_for_buffers:1;
> >> 
> >>  	struct vb2_fileio_data		*fileio;
> >>  	struct vb2_threadio_data	*threadio;

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Hans Verkuil]

On 09/21/2014 11:30 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sunday 21 September 2014 11:00:02 Hans Verkuil wrote:
>> On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
>>> On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
>>>> broke the teletext applications alevt and mtt. These applications
>>>> expect that calling poll() without having called VIDIOC_STREAMON will
>>>> cause poll() to return POLLERR. That did not happen in vb2.
>>>>
>>>> This patch fixes that behavior. It also fixes what should happen when
>>>> poll() is called when STREAMON is called but no buffers have been
>>>> queued. In that case poll() will also return POLLERR, but only for
>>>> capture queues since output queues will always return POLLOUT
>>>> anyway in that situation.
>>>>
>>>> This brings the vb2 behavior in line with the old videobuf behavior.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>
>>>>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
>>>>  include/media/videobuf2-core.h           |  4 ++++
>>>>  2 files changed, 18 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
>>>> v4l2_requestbuffers *req) * to the userspace.
>>>>  	 */
>>>>  	
>>>>  	req->count = allocated_buffers;
>>>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>>>  	return 0;
>>>>  }
>>>>
>>>> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q,
>>>> struct
>>>> v4l2_create_buffers *create memset(q->plane_sizes, 0,
>>>> sizeof(q->plane_sizes));
>>>>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
>>>>  		q->memory = create->memory;
>>>> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>>
>>> Wouldn't it be easier to set the flag when creating the queue and in
>>> vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ? I'll
>>> let you decide.
>>
>> Sorry, I don't follow. 'When creating the queue'? __create_bufs and
>> __reqbufs are the functions that create the queue (i.e. allocate the
>> buffers).
> 
> I meant vb2_queue_init.

That's not an option as it is called only once at probe time. And this
flag needs to be set every time you setup the queue for streaming, when
you stop streaming and when you queue a buffer.

Regards,

	Hans

Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Laurent Pinchart]

Hi Hans,

On Sunday 21 September 2014 11:34:26 Hans Verkuil wrote:
> On 09/21/2014 11:30 AM, Laurent Pinchart wrote:
> > On Sunday 21 September 2014 11:00:02 Hans Verkuil wrote:
> >> On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
> >>> On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> 
> >>>> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
> >>>> broke the teletext applications alevt and mtt. These applications
> >>>> expect that calling poll() without having called VIDIOC_STREAMON will
> >>>> cause poll() to return POLLERR. That did not happen in vb2.
> >>>> 
> >>>> This patch fixes that behavior. It also fixes what should happen when
> >>>> poll() is called when STREAMON is called but no buffers have been
> >>>> queued. In that case poll() will also return POLLERR, but only for
> >>>> capture queues since output queues will always return POLLOUT
> >>>> anyway in that situation.
> >>>> 
> >>>> This brings the vb2 behavior in line with the old videobuf behavior.
> >>>> 
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>> 
> >>>>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
> >>>>  include/media/videobuf2-core.h           |  4 ++++
> >>>>  2 files changed, 18 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694
> >>>> 100644
> >>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
> >>>> v4l2_requestbuffers *req) * to the userspace.
> >>>> 
> >>>>  	 */
> >>>>  	
> >>>>  	req->count = allocated_buffers;
> >>>> 
> >>>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> >>>> 
> >>>>  	return 0;
> >>>>  
> >>>>  }
> >>>> 
> >>>> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q,
> >>>> struct
> >>>> v4l2_create_buffers *create memset(q->plane_sizes, 0,
> >>>> sizeof(q->plane_sizes));
> >>>> 
> >>>>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
> >>>>  		q->memory = create->memory;
> >>>> 
> >>>> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> >>> 
> >>> Wouldn't it be easier to set the flag when creating the queue and in
> >>> vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ?
> >>> I'll
> >>> let you decide.
> >> 
> >> Sorry, I don't follow. 'When creating the queue'? __create_bufs and
> >> __reqbufs are the functions that create the queue (i.e. allocate the
> >> buffers).
> > 
> > I meant vb2_queue_init.
> 
> That's not an option as it is called only once at probe time.

I know :-)

> And this flag needs to be set every time you setup the queue for streaming,
> when you stop streaming and when you queue a buffer.

I was thinking it would be enough to set the flag when stopping the stream, as 
it would then be set for the next REQBUFS or CREATE_BUFS operation, but that 
would break if an application calls QBUF - close() - open() - ...

Let's proceed with your patch.

-- 
Regards,

Laurent Pinchart