[PATCH/RFC v2 0/2] vb2: Report POLLERR for fatal errors only

[PATCH/RFC v2 0/2] vb2: Report POLLERR for fatal errors only

[Laurent Pinchart]

Hello,

This patch set modifies the vb2 implementation of the poll() operation to set
the POLLERR flag for fatal errors only. The rationale and implementation
details are explained in the individual commit messages.

Changes since to v1:

- Rebased on top of the latest media tree master branch
- Fixed typos

Laurent Pinchart (2):
  v4l: vb2: Don't return POLLERR during transient buffer underruns
  v4l: vb2: Add fatal error condition flag

 drivers/media/v4l2-core/videobuf2-core.c | 40 +++++++++++++++++++++++++++++---
 include/media/videobuf2-core.h           |  3 +++
 2 files changed, 40 insertions(+), 3 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Laurent Pinchart]

The V4L2 specification states that

"When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
the poll() function succeeds, but sets the POLLERR flag in the revents
field."

The vb2_poll() function sets POLLERR when the queued buffers list is
empty, regardless of whether this is caused by the stream not being
active yet, or by a transient buffer underrun.

Bring the implementation in line with the specification by returning
POLLERR only when the queue is not streaming. Buffer underruns during
streaming are not treated specially anymore and just result in poll()
blocking until the next event.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 349e659..fd428e0 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	}
 
 	/*
-	 * There is nothing to wait for if no buffers have already been queued.
+	 * There is nothing to wait for if the queue isn't streaming.
 	 */
-	if (list_empty(&q->queued_list))
+	if (!vb2_is_streaming(q))
 		return res | POLLERR;
 
 	if (list_empty(&q->done_list))
-- 
1.8.5.5

[PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Laurent Pinchart]

When a fatal error occurs that render the device unusable, the only
options for a driver to signal the error condition to userspace is to
set the V4L2_BUF_FLAG_ERROR flag when dequeuing buffers and to return an
error from the buffer prepare handler when queuing buffers.

The buffer error flag indicates a transient error and can't be used by
applications to detect fatal errors. Returning an error from vb2_qbuf()
is thus the only real indication that a fatal error occurred. However,
this is difficult to handle for multithreaded applications that requeue
buffers from a thread other than the control thread. In particular the
poll() call in the control thread will not notify userspace of the
error.

This patch adds an explicit mechanism to report fatal errors to
userspace. Applications can call the vb2_queue_error() function to
signal a fatal error. From this moment on, buffer preparation will
return -EIO to userspace, and vb2_poll() will set the POLLERR flag and
return immediately. The error flag is cleared when cancelling the queue,
either at stream off time (through vb2_streamoff) or when releasing the
queue with vb2_queue_release().

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index fd428e0..c7aa07d 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1582,6 +1582,11 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		return -EINVAL;
 	}
 
+	if (q->error) {
+		dprintk(1, "fatal error occurred on queue\n");
+		return -EIO;
+	}
+
 	vb->state = VB2_BUF_STATE_PREPARING;
 	vb->v4l2_buf.timestamp.tv_sec = 0;
 	vb->v4l2_buf.timestamp.tv_usec = 0;
@@ -1877,6 +1882,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 			return -EINVAL;
 		}
 
+		if (q->error) {
+			dprintk(1, "Queue in error state, will not wait for buffers\n");
+			return -EIO;
+		}
+
 		if (!list_empty(&q->done_list)) {
 			/*
 			 * Found a buffer that we were waiting for.
@@ -1902,7 +1912,8 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 		 */
 		dprintk(3, "will sleep waiting for buffers\n");
 		ret = wait_event_interruptible(q->done_wq,
-				!list_empty(&q->done_list) || !q->streaming);
+				!list_empty(&q->done_list) || !q->streaming ||
+				q->error);
 
 		/*
 		 * We need to reevaluate both conditions again after reacquiring
@@ -2099,6 +2110,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 	q->streaming = 0;
 	q->start_streaming_called = 0;
 	q->queued_count = 0;
+	q->error = 0;
 
 	/*
 	 * Remove all buffers from videobuf's list...
@@ -2176,6 +2188,27 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 }
 
 /**
+ * vb2_queue_error() - signal a fatal error on the queue
+ * @q:		videobuf2 queue
+ *
+ * Flag that a fatal unrecoverable error has occurred and wake up all processes
+ * waiting on the queue. Polling will now set POLLERR and queuing and dequeuing
+ * buffers will return -EIO.
+ *
+ * The error flag will be cleared when cancelling the queue, either from
+ * vb2_streamoff or vb2_queue_release. Drivers should thus not call this
+ * function before starting the stream, otherwise the error flag will remain set
+ * until the queue is released when closing the device node.
+ */
+void vb2_queue_error(struct vb2_queue *q)
+{
+	q->error = 1;
+
+	wake_up_all(&q->done_wq);
+}
+EXPORT_SYMBOL_GPL(vb2_queue_error);
+
+/**
  * vb2_streamon - start streaming
  * @q:		videobuf2 queue
  * @type:	type argument passed from userspace to vidioc_streamon handler
@@ -2533,9 +2566,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	}
 
 	/*
-	 * There is nothing to wait for if the queue isn't streaming.
+	 * There is nothing to wait for if the queue isn't streaming or if the
+	 * error flag is set.
 	 */
-	if (!vb2_is_streaming(q))
+	if (!vb2_is_streaming(q) || q->error)
 		return res | POLLERR;
 
 	if (list_empty(&q->done_list))
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bca25dc..5a67f31 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -375,6 +375,7 @@ struct v4l2_fh;
  * @streaming:	current streaming state
  * @start_streaming_called: start_streaming() was called successfully and we
  *		started streaming.
+ * @error:	a fatal error occured on the queue
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
  */
@@ -411,6 +412,7 @@ struct vb2_queue {
 
 	unsigned int			streaming:1;
 	unsigned int			start_streaming_called:1;
+	unsigned int			error:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
@@ -443,6 +445,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b);
 int __must_check vb2_queue_init(struct vb2_queue *q);
 
 void vb2_queue_release(struct vb2_queue *q);
+void vb2_queue_error(struct vb2_queue *q);
 
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
 int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb);
-- 
1.8.5.5

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Pawel Osciak]

On Thu, Jun 5, 2014 at 9:23 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The V4L2 specification states that
>
> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> the poll() function succeeds, but sets the POLLERR flag in the revents
> field."
>
> The vb2_poll() function sets POLLERR when the queued buffers list is
> empty, regardless of whether this is caused by the stream not being
> active yet, or by a transient buffer underrun.
>
> Bring the implementation in line with the specification by returning
> POLLERR only when the queue is not streaming. Buffer underruns during
> streaming are not treated specially anymore and just result in poll()
> blocking until the next event.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 349e659..fd428e0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>         }
>
>         /*
> -        * There is nothing to wait for if no buffers have already been queued.
> +        * There is nothing to wait for if the queue isn't streaming.
>          */
> -       if (list_empty(&q->queued_list))
> +       if (!vb2_is_streaming(q))
>                 return res | POLLERR;
>
>         if (list_empty(&q->done_list))
> --
> 1.8.5.5
>

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Pawel Osciak]

Hi Laurent,
Thanks for the patch. Did you test this to work in fileio mode? Looks
like it should, but would like to make sure.
Thanks,
Pawel

On Thu, Jun 5, 2014 at 9:23 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> When a fatal error occurs that render the device unusable, the only
> options for a driver to signal the error condition to userspace is to
> set the V4L2_BUF_FLAG_ERROR flag when dequeuing buffers and to return an
> error from the buffer prepare handler when queuing buffers.
>
> The buffer error flag indicates a transient error and can't be used by
> applications to detect fatal errors. Returning an error from vb2_qbuf()
> is thus the only real indication that a fatal error occurred. However,
> this is difficult to handle for multithreaded applications that requeue
> buffers from a thread other than the control thread. In particular the
> poll() call in the control thread will not notify userspace of the
> error.
>
> This patch adds an explicit mechanism to report fatal errors to
> userspace. Applications can call the vb2_queue_error() function to
> signal a fatal error. From this moment on, buffer preparation will
> return -EIO to userspace, and vb2_poll() will set the POLLERR flag and
> return immediately. The error flag is cleared when cancelling the queue,
> either at stream off time (through vb2_streamoff) or when releasing the
> queue with vb2_queue_release().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 40 +++++++++++++++++++++++++++++---
>  include/media/videobuf2-core.h           |  3 +++
>  2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index fd428e0..c7aa07d 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1582,6 +1582,11 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 return -EINVAL;
>         }
>
> +       if (q->error) {
> +               dprintk(1, "fatal error occurred on queue\n");
> +               return -EIO;
> +       }
> +
>         vb->state = VB2_BUF_STATE_PREPARING;
>         vb->v4l2_buf.timestamp.tv_sec = 0;
>         vb->v4l2_buf.timestamp.tv_usec = 0;
> @@ -1877,6 +1882,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>                         return -EINVAL;
>                 }
>
> +               if (q->error) {
> +                       dprintk(1, "Queue in error state, will not wait for buffers\n");
> +                       return -EIO;
> +               }
> +
>                 if (!list_empty(&q->done_list)) {
>                         /*
>                          * Found a buffer that we were waiting for.
> @@ -1902,7 +1912,8 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>                  */
>                 dprintk(3, "will sleep waiting for buffers\n");
>                 ret = wait_event_interruptible(q->done_wq,
> -                               !list_empty(&q->done_list) || !q->streaming);
> +                               !list_empty(&q->done_list) || !q->streaming ||
> +                               q->error);
>
>                 /*
>                  * We need to reevaluate both conditions again after reacquiring
> @@ -2099,6 +2110,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>         q->streaming = 0;
>         q->start_streaming_called = 0;
>         q->queued_count = 0;
> +       q->error = 0;
>
>         /*
>          * Remove all buffers from videobuf's list...
> @@ -2176,6 +2188,27 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
>  }
>
>  /**
> + * vb2_queue_error() - signal a fatal error on the queue
> + * @q:         videobuf2 queue
> + *
> + * Flag that a fatal unrecoverable error has occurred and wake up all processes
> + * waiting on the queue. Polling will now set POLLERR and queuing and dequeuing
> + * buffers will return -EIO.
> + *
> + * The error flag will be cleared when cancelling the queue, either from
> + * vb2_streamoff or vb2_queue_release. Drivers should thus not call this
> + * function before starting the stream, otherwise the error flag will remain set
> + * until the queue is released when closing the device node.
> + */
> +void vb2_queue_error(struct vb2_queue *q)
> +{
> +       q->error = 1;
> +
> +       wake_up_all(&q->done_wq);
> +}
> +EXPORT_SYMBOL_GPL(vb2_queue_error);
> +
> +/**
>   * vb2_streamon - start streaming
>   * @q:         videobuf2 queue
>   * @type:      type argument passed from userspace to vidioc_streamon handler
> @@ -2533,9 +2566,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>         }
>
>         /*
> -        * There is nothing to wait for if the queue isn't streaming.
> +        * There is nothing to wait for if the queue isn't streaming or if the
> +        * error flag is set.
>          */
> -       if (!vb2_is_streaming(q))
> +       if (!vb2_is_streaming(q) || q->error)
>                 return res | POLLERR;
>
>         if (list_empty(&q->done_list))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bca25dc..5a67f31 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -375,6 +375,7 @@ struct v4l2_fh;
>   * @streaming: current streaming state
>   * @start_streaming_called: start_streaming() was called successfully and we
>   *             started streaming.
> + * @error:     a fatal error occured on the queue
>   * @fileio:    file io emulator internal data, used only if emulator is active
>   * @threadio:  thread io internal data, used only if thread is active
>   */
> @@ -411,6 +412,7 @@ struct vb2_queue {
>
>         unsigned int                    streaming:1;
>         unsigned int                    start_streaming_called:1;
> +       unsigned int                    error:1;
>
>         struct vb2_fileio_data          *fileio;
>         struct vb2_threadio_data        *threadio;
> @@ -443,6 +445,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b);
>  int __must_check vb2_queue_init(struct vb2_queue *q);
>
>  void vb2_queue_release(struct vb2_queue *q);
> +void vb2_queue_error(struct vb2_queue *q);
>
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
>  int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb);
> --
> 1.8.5.5
>

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Laurent Pinchart]

Hi Pawel,

On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
> Hi Laurent,
> Thanks for the patch. Did you test this to work in fileio mode? Looks
> like it should, but would like to make sure.

No, I haven't tested it. The OMAP4 ISS driver, which is my test target for 
this patch, doesn't support fileio mode. Adding VB2_READ would be easy, but 
the driver requires configuring the format on the file handle used for 
streaming, so I can't just run cat /dev/video*.

> On Thu, Jun 5, 2014 at 9:23 PM, Laurent Pinchart wrote:
> > When a fatal error occurs that render the device unusable, the only
> > options for a driver to signal the error condition to userspace is to
> > set the V4L2_BUF_FLAG_ERROR flag when dequeuing buffers and to return an
> > error from the buffer prepare handler when queuing buffers.
> > 
> > The buffer error flag indicates a transient error and can't be used by
> > applications to detect fatal errors. Returning an error from vb2_qbuf()
> > is thus the only real indication that a fatal error occurred. However,
> > this is difficult to handle for multithreaded applications that requeue
> > buffers from a thread other than the control thread. In particular the
> > poll() call in the control thread will not notify userspace of the
> > error.
> > 
> > This patch adds an explicit mechanism to report fatal errors to
> > userspace. Applications can call the vb2_queue_error() function to
> > signal a fatal error. From this moment on, buffer preparation will
> > return -EIO to userspace, and vb2_poll() will set the POLLERR flag and
> > return immediately. The error flag is cleared when cancelling the queue,
> > either at stream off time (through vb2_streamoff) or when releasing the
> > queue with vb2_queue_release().
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Hans Verkuil]

On 06/06/2014 11:19 AM, Laurent Pinchart wrote:
> Hi Pawel,
> 
> On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
>> Hi Laurent,
>> Thanks for the patch. Did you test this to work in fileio mode? Looks
>> like it should, but would like to make sure.
> 
> No, I haven't tested it. The OMAP4 ISS driver, which is my test target for 
> this patch, doesn't support fileio mode. Adding VB2_READ would be easy, but 
> the driver requires configuring the format on the file handle used for 
> streaming, so I can't just run cat /dev/video*.

Just test with vivi.

Regards,

	Hans

> 
>> On Thu, Jun 5, 2014 at 9:23 PM, Laurent Pinchart wrote:
>>> When a fatal error occurs that render the device unusable, the only
>>> options for a driver to signal the error condition to userspace is to
>>> set the V4L2_BUF_FLAG_ERROR flag when dequeuing buffers and to return an
>>> error from the buffer prepare handler when queuing buffers.
>>>
>>> The buffer error flag indicates a transient error and can't be used by
>>> applications to detect fatal errors. Returning an error from vb2_qbuf()
>>> is thus the only real indication that a fatal error occurred. However,
>>> this is difficult to handle for multithreaded applications that requeue
>>> buffers from a thread other than the control thread. In particular the
>>> poll() call in the control thread will not notify userspace of the
>>> error.
>>>
>>> This patch adds an explicit mechanism to report fatal errors to
>>> userspace. Applications can call the vb2_queue_error() function to
>>> signal a fatal error. From this moment on, buffer preparation will
>>> return -EIO to userspace, and vb2_poll() will set the POLLERR flag and
>>> return immediately. The error flag is cleared when cancelling the queue,
>>> either at stream off time (through vb2_streamoff) or when releasing the
>>> queue with vb2_queue_release().
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Laurent Pinchart]

Hi Hans,

On Friday 06 June 2014 11:31:55 Hans Verkuil wrote:
> On 06/06/2014 11:19 AM, Laurent Pinchart wrote:
> > Hi Pawel,
> > 
> > On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
> >> Hi Laurent,
> >> Thanks for the patch. Did you test this to work in fileio mode? Looks
> >> like it should, but would like to make sure.
> > 
> > No, I haven't tested it. The OMAP4 ISS driver, which is my test target for
> > this patch, doesn't support fileio mode. Adding VB2_READ would be easy,
> > but the driver requires configuring the format on the file handle used for
> > streaming, so I can't just run cat /dev/video*.
> 
> Just test with vivi.

But vivi doesn't call the new vb2_queue_error() function. I understand that 
your vivi rework would make that easier as you now have an error control. 
Should I hack something similar in the existing vivi driver ? Any pointer ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Hans de Goede]

Hi,

On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> The V4L2 specification states that
> 
> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> the poll() function succeeds, but sets the POLLERR flag in the revents
> field."
> 
> The vb2_poll() function sets POLLERR when the queued buffers list is
> empty, regardless of whether this is caused by the stream not being
> active yet, or by a transient buffer underrun.
> 
> Bring the implementation in line with the specification by returning
> POLLERR only when the queue is not streaming. Buffer underruns during
> streaming are not treated specially anymore and just result in poll()
> blocking until the next event.

After your patch the implementation is still not inline with the spec,
queuing buffers, then starting a thread doing the poll, then doing the
streamon in the main thread will still cause the poll to return POLLERR,
even though buffers are queued, which according to the spec should be enough
for the poll to block.

The correct check would be:

if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
	eturn res | POLLERR;

Regards,

Hans


> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 349e659..fd428e0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  	}
>  
>  	/*
> -	 * There is nothing to wait for if no buffers have already been queued.
> +	 * There is nothing to wait for if the queue isn't streaming.
>  	 */
> -	if (list_empty(&q->queued_list))
> +	if (!vb2_is_streaming(q))
>  		return res | POLLERR;
>  
>  	if (list_empty(&q->done_list))
> 

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Hans Verkuil]

On 06/06/2014 11:46 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 06 June 2014 11:31:55 Hans Verkuil wrote:
>> On 06/06/2014 11:19 AM, Laurent Pinchart wrote:
>>> Hi Pawel,
>>>
>>> On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
>>>> Hi Laurent,
>>>> Thanks for the patch. Did you test this to work in fileio mode? Looks
>>>> like it should, but would like to make sure.
>>>
>>> No, I haven't tested it. The OMAP4 ISS driver, which is my test target for
>>> this patch, doesn't support fileio mode. Adding VB2_READ would be easy,
>>> but the driver requires configuring the format on the file handle used for
>>> streaming, so I can't just run cat /dev/video*.
>>
>> Just test with vivi.
> 
> But vivi doesn't call the new vb2_queue_error() function. I understand that 
> your vivi rework would make that easier as you now have an error control. 
> Should I hack something similar in the existing vivi driver ? Any pointer ?
> 

Just hack it in for testing. E.g. call it when the button control is pressed
(see vivi_s_ctrl).

Regards,

	Hans

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Hans Verkuil]

On 06/06/2014 11:50 AM, Hans de Goede wrote:
> Hi,
> 
> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
>> The V4L2 specification states that
>>
>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
>> the poll() function succeeds, but sets the POLLERR flag in the revents
>> field."
>>
>> The vb2_poll() function sets POLLERR when the queued buffers list is
>> empty, regardless of whether this is caused by the stream not being
>> active yet, or by a transient buffer underrun.
>>
>> Bring the implementation in line with the specification by returning
>> POLLERR only when the queue is not streaming. Buffer underruns during
>> streaming are not treated specially anymore and just result in poll()
>> blocking until the next event.
> 
> After your patch the implementation is still not inline with the spec,
> queuing buffers, then starting a thread doing the poll, then doing the
> streamon in the main thread will still cause the poll to return POLLERR,
> even though buffers are queued, which according to the spec should be enough
> for the poll to block.
> 
> The correct check would be:
> 
> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> 	eturn res | POLLERR;

Good catch! I should have seen that :-(

v4l2-compliance should certainly be extended to test this as well.

Regards,

	Hans

> 
> Regards,
> 
> Hans
> 
> 
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 349e659..fd428e0 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>>  	}
>>  
>>  	/*
>> -	 * There is nothing to wait for if no buffers have already been queued.
>> +	 * There is nothing to wait for if the queue isn't streaming.
>>  	 */
>> -	if (list_empty(&q->queued_list))
>> +	if (!vb2_is_streaming(q))
>>  		return res | POLLERR;
>>  
>>  	if (list_empty(&q->done_list))
>>

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Laurent Pinchart]

On Friday 06 June 2014 11:55:49 Hans Verkuil wrote:
> On 06/06/2014 11:46 AM, Laurent Pinchart wrote:
> > On Friday 06 June 2014 11:31:55 Hans Verkuil wrote:
> >> On 06/06/2014 11:19 AM, Laurent Pinchart wrote:
> >>> On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
> >>>> Hi Laurent,
> >>>> Thanks for the patch. Did you test this to work in fileio mode? Looks
> >>>> like it should, but would like to make sure.
> >>> 
> >>> No, I haven't tested it. The OMAP4 ISS driver, which is my test target
> >>> for this patch, doesn't support fileio mode. Adding VB2_READ would be
> >>> easy, but the driver requires configuring the format on the file handle
> >>> used for streaming, so I can't just run cat /dev/video*.
> >> 
> >> Just test with vivi.
> > 
> > But vivi doesn't call the new vb2_queue_error() function. I understand
> > that your vivi rework would make that easier as you now have an error
> > control. Should I hack something similar in the existing vivi driver ? Any
> > pointer ?
> 
> Just hack it in for testing. E.g. call it when the button control is pressed
> (see vivi_s_ctrl).

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

"cat /dev/video0" outputs data until vivi calls vb2_queue_error(), at which 
points cat prints

cat: /dev/video0: Input/output error

Restarting capture works as expected.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Laurent Pinchart]

On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
> On 06/06/2014 11:50 AM, Hans de Goede wrote:
> > Hi,
> > 
> > On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> >> The V4L2 specification states that
> >> 
> >> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> >> the poll() function succeeds, but sets the POLLERR flag in the revents
> >> field."
> >> 
> >> The vb2_poll() function sets POLLERR when the queued buffers list is
> >> empty, regardless of whether this is caused by the stream not being
> >> active yet, or by a transient buffer underrun.
> >> 
> >> Bring the implementation in line with the specification by returning
> >> POLLERR only when the queue is not streaming. Buffer underruns during
> >> streaming are not treated specially anymore and just result in poll()
> >> blocking until the next event.
> > 
> > After your patch the implementation is still not inline with the spec,
> > queuing buffers, then starting a thread doing the poll, then doing the
> > streamon in the main thread will still cause the poll to return POLLERR,
> > even though buffers are queued, which according to the spec should be
> > enough for the poll to block.
> > 
> > The correct check would be:
> > 
> > if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> > 
> > 	eturn res | POLLERR;
> 
> Good catch! I should have seen that :-(

I'll update the patch accordingly.

> v4l2-compliance should certainly be extended to test this as well.
> 
> Regards,
> 
> 	Hans
> 
> > Regards,
> > 
> > Hans
> > 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >> b/drivers/media/v4l2-core/videobuf2-core.c index 349e659..fd428e0 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct
> >> file *file, poll_table *wait)>> 
> >>  	}
> >>  	
> >>  	/*
> >> 
> >> -	 * There is nothing to wait for if no buffers have already been 
queued.
> >> +	 * There is nothing to wait for if the queue isn't streaming.
> >> 
> >>  	 */
> >> 
> >> -	if (list_empty(&q->queued_list))
> >> +	if (!vb2_is_streaming(q))
> >> 
> >>  		return res | POLLERR;
> >>  	
> >>  	if (list_empty(&q->done_list))

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 2/2] v4l: vb2: Add fatal error condition flag

[Hans Verkuil]

On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> On Friday 06 June 2014 11:55:49 Hans Verkuil wrote:
>> On 06/06/2014 11:46 AM, Laurent Pinchart wrote:
>>> On Friday 06 June 2014 11:31:55 Hans Verkuil wrote:
>>>> On 06/06/2014 11:19 AM, Laurent Pinchart wrote:
>>>>> On Friday 06 June 2014 14:31:15 Pawel Osciak wrote:
>>>>>> Hi Laurent,
>>>>>> Thanks for the patch. Did you test this to work in fileio mode? Looks
>>>>>> like it should, but would like to make sure.
>>>>>
>>>>> No, I haven't tested it. The OMAP4 ISS driver, which is my test target
>>>>> for this patch, doesn't support fileio mode. Adding VB2_READ would be
>>>>> easy, but the driver requires configuring the format on the file handle
>>>>> used for streaming, so I can't just run cat /dev/video*.
>>>>
>>>> Just test with vivi.
>>>
>>> But vivi doesn't call the new vb2_queue_error() function. I understand
>>> that your vivi rework would make that easier as you now have an error
>>> control. Should I hack something similar in the existing vivi driver ? Any
>>> pointer ?
>>
>> Just hack it in for testing. E.g. call it when the button control is pressed
>> (see vivi_s_ctrl).
> 
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> "cat /dev/video0" outputs data until vivi calls vb2_queue_error(), at which 
> points cat prints
> 
> cat: /dev/video0: Input/output error
> 
> Restarting capture works as expected.
> 

Nice.

Once this is merged I plan on adding support for this to my vivi rewrite.

Finishing the vivi rewrite (mostly cleaning things up and documentation
where appropriate) is a high-prio for me. I'd like to get this in for
3.17.

Regards,

	Hans

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Hans Verkuil]

On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
>> On 06/06/2014 11:50 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
>>>> The V4L2 specification states that
>>>>
>>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
>>>> the poll() function succeeds, but sets the POLLERR flag in the revents
>>>> field."
>>>>
>>>> The vb2_poll() function sets POLLERR when the queued buffers list is
>>>> empty, regardless of whether this is caused by the stream not being
>>>> active yet, or by a transient buffer underrun.
>>>>
>>>> Bring the implementation in line with the specification by returning
>>>> POLLERR only when the queue is not streaming. Buffer underruns during
>>>> streaming are not treated specially anymore and just result in poll()
>>>> blocking until the next event.
>>>
>>> After your patch the implementation is still not inline with the spec,
>>> queuing buffers, then starting a thread doing the poll, then doing the
>>> streamon in the main thread will still cause the poll to return POLLERR,
>>> even though buffers are queued, which according to the spec should be
>>> enough for the poll to block.
>>>
>>> The correct check would be:
>>>
>>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
>>>
>>> 	eturn res | POLLERR;
>>
>> Good catch! I should have seen that :-(

Urgh. This breaks vbi capture tools like alevt and mtt. These rely on poll
returning POLLERR if buffers are queued but STREAMON has not been called yet.

See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401

The spec also clearly says that poll should return POLLERR if STREAMON
was not called.

But that would clash with this multi-thread example.

Hans, was this based on actual code that needed this?

I am inclined to update alevt and mtt: all that is needed to make it work
is a single line that explicitly calls the vbi handler before entering the
main loop. This is effectively the same as what happens when the first
select gets a POLLERR.

We maintain alevt (dvb-apps) and mtt (xawtv3), so that's easy enough to
fix.

Note that the spec is now definitely out-of-sync since poll no longer returns
POLLERR if buffers are queued but STREAMON wasn't called.

Regards,

	Hans

> 
> I'll update the patch accordingly.
> 
>> v4l2-compliance should certainly be extended to test this as well.
>>
>> Regards,
>>
>> 	Hans
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>
>>>>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 349e659..fd428e0 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct
>>>> file *file, poll_table *wait)>> 
>>>>  	}
>>>>  	
>>>>  	/*
>>>>
>>>> -	 * There is nothing to wait for if no buffers have already been 
> queued.
>>>> +	 * There is nothing to wait for if the queue isn't streaming.
>>>>
>>>>  	 */
>>>>
>>>> -	if (list_empty(&q->queued_list))
>>>> +	if (!vb2_is_streaming(q))
>>>>
>>>>  		return res | POLLERR;
>>>>  	
>>>>  	if (list_empty(&q->done_list))
> 

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Mauro Carvalho Chehab]

Em Mon, 15 Sep 2014 13:14:51 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> > On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
> >> On 06/06/2014 11:50 AM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> >>>> The V4L2 specification states that
> >>>>
> >>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> >>>> the poll() function succeeds, but sets the POLLERR flag in the revents
> >>>> field."
> >>>>
> >>>> The vb2_poll() function sets POLLERR when the queued buffers list is
> >>>> empty, regardless of whether this is caused by the stream not being
> >>>> active yet, or by a transient buffer underrun.
> >>>>
> >>>> Bring the implementation in line with the specification by returning
> >>>> POLLERR only when the queue is not streaming. Buffer underruns during
> >>>> streaming are not treated specially anymore and just result in poll()
> >>>> blocking until the next event.
> >>>
> >>> After your patch the implementation is still not inline with the spec,
> >>> queuing buffers, then starting a thread doing the poll, then doing the
> >>> streamon in the main thread will still cause the poll to return POLLERR,
> >>> even though buffers are queued, which according to the spec should be
> >>> enough for the poll to block.
> >>>
> >>> The correct check would be:
> >>>
> >>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> >>>
> >>> 	eturn res | POLLERR;
> >>
> >> Good catch! I should have seen that :-(
> 
> Urgh. This breaks vbi capture tools like alevt and mtt. These rely on poll
> returning POLLERR if buffers are queued but STREAMON has not been called yet.
> 
> See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401
> 
> The spec also clearly says that poll should return POLLERR if STREAMON
> was not called.
> 
> But that would clash with this multi-thread example.
> 
> Hans, was this based on actual code that needed this?
> 
> I am inclined to update alevt and mtt: all that is needed to make it work
> is a single line that explicitly calls the vbi handler before entering the
> main loop. This is effectively the same as what happens when the first
> select gets a POLLERR.
> 
> We maintain alevt (dvb-apps) and mtt (xawtv3), so that's easy enough to
> fix.

No, the best is to revert the patch ASAP, as this is a regression.

We can then work to change alevt and mtt to do it, but we need to check
other tools, like zvbi.

Only after having this change at the VBI tools for a while we can change
the Kernel again, (if it makes sense: as you said, this patch is violating
the spec on VB2).

> 
> Note that the spec is now definitely out-of-sync since poll no longer returns
> POLLERR if buffers are queued but STREAMON wasn't called.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > I'll update the patch accordingly.
> > 
> >> v4l2-compliance should certainly be extended to test this as well.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>>
> >>>>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> b/drivers/media/v4l2-core/videobuf2-core.c index 349e659..fd428e0 100644
> >>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct
> >>>> file *file, poll_table *wait)>> 
> >>>>  	}
> >>>>  	
> >>>>  	/*
> >>>>
> >>>> -	 * There is nothing to wait for if no buffers have already been 
> > queued.
> >>>> +	 * There is nothing to wait for if the queue isn't streaming.
> >>>>
> >>>>  	 */
> >>>>
> >>>> -	if (list_empty(&q->queued_list))
> >>>> +	if (!vb2_is_streaming(q))
> >>>>
> >>>>  		return res | POLLERR;
> >>>>  	
> >>>>  	if (list_empty(&q->done_list))
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Laurent Pinchart]

Hi Mauro,

On Monday 15 September 2014 09:02:24 Mauro Carvalho Chehab wrote:
> Em Mon, 15 Sep 2014 13:14:51 +0200 Hans Verkuil escreveu:
> > On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> > > On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
> > >> On 06/06/2014 11:50 AM, Hans de Goede wrote:
> > >>> Hi,
> > >>> 
> > >>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> > >>>> The V4L2 specification states that
> > >>>> 
> > >>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> > >>>> the poll() function succeeds, but sets the POLLERR flag in the
> > >>>> revents field."
> > >>>> 
> > >>>> The vb2_poll() function sets POLLERR when the queued buffers list is
> > >>>> empty, regardless of whether this is caused by the stream not being
> > >>>> active yet, or by a transient buffer underrun.
> > >>>> 
> > >>>> Bring the implementation in line with the specification by returning
> > >>>> POLLERR only when the queue is not streaming. Buffer underruns during
> > >>>> streaming are not treated specially anymore and just result in poll()
> > >>>> blocking until the next event.
> > >>> 
> > >>> After your patch the implementation is still not inline with the spec,
> > >>> queuing buffers, then starting a thread doing the poll, then doing the
> > >>> streamon in the main thread will still cause the poll to return
> > >>> POLLERR, even though buffers are queued, which according to the spec
> > >>> should be enough for the poll to block.
> > >>> 
> > >>> The correct check would be:
> > >>> 
> > >>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> > >>> 
> > >>> 	eturn res | POLLERR;
> > >> 
> > >> Good catch! I should have seen that :-(
> > 
> > Urgh. This breaks vbi capture tools like alevt and mtt. These rely on poll
> > returning POLLERR if buffers are queued but STREAMON has not been called
> > yet.
> > 
> > See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401
> > 
> > The spec also clearly says that poll should return POLLERR if STREAMON
> > was not called.
> > 
> > But that would clash with this multi-thread example.
> > 
> > Hans, was this based on actual code that needed this?
> > 
> > I am inclined to update alevt and mtt: all that is needed to make it work
> > is a single line that explicitly calls the vbi handler before entering the
> > main loop. This is effectively the same as what happens when the first
> > select gets a POLLERR.
> > 
> > We maintain alevt (dvb-apps) and mtt (xawtv3), so that's easy enough to
> > fix.
> 
> No, the best is to revert the patch ASAP, as this is a regression.

Reverting the patch will also be a regression, as that would break 
applications that now rely on the new behaviour (I've developed this patch to 
fix a problem I've noticed with gstreamer). One way or another, we're screwed 
and we'll break userspace.

> We can then work to change alevt and mtt to do it, but we need to check
> other tools, like zvbi.
> 
> Only after having this change at the VBI tools for a while we can change
> the Kernel again, (if it makes sense: as you said, this patch is violating
> the spec on VB2).
> 
> > Note that the spec is now definitely out-of-sync since poll no longer
> > returns POLLERR if buffers are queued but STREAMON wasn't called.
> > 
> > > I'll update the patch accordingly.
> > > 
> > >> v4l2-compliance should certainly be extended to test this as well.
> > >>
> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > >>>> ---
> > >>>> 
> > >>>>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
> > >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>> 
> > >>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > >>>> b/drivers/media/v4l2-core/videobuf2-core.c index 349e659..fd428e0
> > >>>> 100644
> > >>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> > >>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > >>>> @@ -2533,9 +2533,9 @@ unsigned int vb2_poll(struct vb2_queue *q,
> > >>>> struct
> > >>>> file *file, poll_table *wait)>>
> > >>>>  	}
> > >>>>  	
> > >>>>  	/*
> > >>>> -	 * There is nothing to wait for if no buffers have already been
> > >>>> queued.
> > >>>> +	 * There is nothing to wait for if the queue isn't streaming.
> > >>>>  	 */
> > >>>> 
> > >>>> -	if (list_empty(&q->queued_list))
> > >>>> +	if (!vb2_is_streaming(q))
> > >>>>  		return res | POLLERR;
> > >>>>  	
> > >>>>  	if (list_empty(&q->done_list))


-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Nicolas Dufresne]

Le 2014-09-15 08:49, Laurent Pinchart a écrit :
> Reverting the patch will also be a regression, as that would break
> applications that now rely on the new behaviour (I've developed this patch to
> fix a problem I've noticed with gstreamer). One way or another, we're screwed
> and we'll break userspace.

We have worked around this issue in GStreamer 1.4+, for older version, 
the problem may be faced again by users, specially if using a newer 
libv4l2 where the locking has been fixed (or no libv4l2).

cheers,
Nicolas

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Mauro Carvalho Chehab]

Em Mon, 15 Sep 2014 08:56:40 -0400
Nicolas Dufresne <nicolas.dufresne@collabora.com> escreveu:

> 
> Le 2014-09-15 08:49, Laurent Pinchart a écrit :
> > Reverting the patch will also be a regression, as that would break
> > applications that now rely on the new behaviour (I've developed this patch to
> > fix a problem I've noticed with gstreamer). One way or another, we're screwed
> > and we'll break userspace.

Well, VB1 is working with the old behavior, as the breakage on saa7134
only happened after its migration to VB2. So, the Gstreamer version
you tested is still very likely broken with VB1.

So, I still think that the less damage is to revert the POLLERR patch,
with, according to Hans, is also a violation at the documented API.

> We have worked around this issue in GStreamer 1.4+,

Good to know.

> for older version, 
> the problem may be faced again by users, specially if using a newer 
> libv4l2 where the locking has been fixed (or no libv4l2).

The DQBUF locking fixup was merged on libv4l2 for version 1.2. So, the
potential breakage happens when libv4l2 is 1.2 and Gstreamer versions
before 1.4.

Do you have any procedure on gstreamer to fix a bug on stable releases?

Those VBI applications don't have any, as they're not actively
maintained anymore. Even if we patch them today, I guess it could take
a long time for those changes to be propagated on distros.

So, I guess that the best is to try to fix Gstreamer on the distros
that are using libv4l version 1.2 and a pre-1.4 Gstreamer version.

> 
> cheers,
> Nicolas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Nicolas Dufresne]

Le 2014-09-15 09:55, Mauro Carvalho Chehab a écrit :
> The DQBUF locking fixup was merged on libv4l2 for version 1.2. So, the
> potential breakage happens when libv4l2 is 1.2 and Gstreamer versions
> before 1.4.
>
> Do you have any procedure on gstreamer to fix a bug on stable releases?
A backport is possible, even though not trivial. Also, due to resources 
there is no guaranty of a new stable 1.2 release. For GStreamer 0.10, it 
is no longer maintained since 2 years already (mentioning since Laurent 
was using an old vendor specific version base on this). Though this 
situation isn't different from using a vendor specific kernel.
> Those VBI applications don't have any, as they're not actively
> maintained anymore. Even if we patch them today, I guess it could take
> a long time for those changes to be propagated on distros.
>
> So, I guess that the best is to try to fix Gstreamer on the distros
> that are using libv4l version 1.2 and a pre-1.4 Gstreamer version.
This seems an unlikely mix, as Gst 1.4 was released at around the same 
moment as libv4l2 1.2.

cheers,
Nicolas

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Mauro Carvalho Chehab]

Em Mon, 15 Sep 2014 10:33:01 -0400
Nicolas Dufresne <nicolas.dufresne@collabora.com> escreveu:

> 
> Le 2014-09-15 09:55, Mauro Carvalho Chehab a écrit :
> > The DQBUF locking fixup was merged on libv4l2 for version 1.2. So, the
> > potential breakage happens when libv4l2 is 1.2 and Gstreamer versions
> > before 1.4.
> >
> > Do you have any procedure on gstreamer to fix a bug on stable releases?
> A backport is possible, even though not trivial. Also, due to resources 
> there is no guaranty of a new stable 1.2 release. 

Well, if someone pops up with a problem with Gst 1.2 + libv4l 1.2
(with seems unlikely, as you pointed below), then we can provide them
some directions to either:
	1) revert the libv4l2 DQBUF patch;
	2) upgrade to gst 1.4;
	3) don't revert the POLLERR patch, if VBI is not needed on such
specific scenario.

> For GStreamer 0.10, it 
> is no longer maintained since 2 years already (mentioning since Laurent 
> was using an old vendor specific version base on this). Though this 
> situation isn't different from using a vendor specific kernel.

Well, we can't do much with vendor-specific versions, especially
when they rely on too old Kernels and/or too old Gstreamer versions.

Anyway, I guess that the POLLERR patch were not backported to -stable.
So, vendor-specific versions with old stacks won't be affected, as
the POLLERR fixup was added on Kernel 3.16.

I suspect that there aren't many places where a vendor distro
using Gst 0.10 has Kernel 3.16.

> > Those VBI applications don't have any, as they're not actively
> > maintained anymore. Even if we patch them today, I guess it could take
> > a long time for those changes to be propagated on distros.
> >
> > So, I guess that the best is to try to fix Gstreamer on the distros
> > that are using libv4l version 1.2 and a pre-1.4 Gstreamer version.
> This seems an unlikely mix, as Gst 1.4 was released at around the same 
> moment as libv4l2 1.2.

Yes, I agree.

Regards,
Mauro

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Laurent Pinchart]

Hi Hans,

On Monday 15 September 2014 13:14:51 Hans Verkuil wrote:
> On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> > On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
> >> On 06/06/2014 11:50 AM, Hans de Goede wrote:
> >>> Hi,
> >>> 
> >>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> >>>> The V4L2 specification states that
> >>>> 
> >>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> >>>> the poll() function succeeds, but sets the POLLERR flag in the revents
> >>>> field."
> >>>> 
> >>>> The vb2_poll() function sets POLLERR when the queued buffers list is
> >>>> empty, regardless of whether this is caused by the stream not being
> >>>> active yet, or by a transient buffer underrun.
> >>>> 
> >>>> Bring the implementation in line with the specification by returning
> >>>> POLLERR only when the queue is not streaming. Buffer underruns during
> >>>> streaming are not treated specially anymore and just result in poll()
> >>>> blocking until the next event.
> >>> 
> >>> After your patch the implementation is still not inline with the spec,
> >>> queuing buffers, then starting a thread doing the poll, then doing the
> >>> streamon in the main thread will still cause the poll to return POLLERR,
> >>> even though buffers are queued, which according to the spec should be
> >>> enough for the poll to block.
> >>> 
> >>> The correct check would be:
> >>> 
> >>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> >>> 
> >>> 	eturn res | POLLERR;
> >> 
> >> Good catch! I should have seen that :-(
> 
> Urgh. This breaks vbi capture tools like alevt and mtt. These rely on poll
> returning POLLERR if buffers are queued but STREAMON has not been called
> yet.

Then there's something I don't get. Before this commit, the implementation was

    /*
     * There is nothing to wait for if no buffers have already been queued.
     */
    if (list_empty(&q->queued_list))
            return res | POLLERR;

If buffers are queued and STREAMON hasn't been called yet, vb2_poll() would 
not return POLLERR.

> See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401
> 
> The spec also clearly says that poll should return POLLERR if STREAMON
> was not called.

The V4L2 specification says

"When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet the 
poll() function succeeds, but sets the POLLERR flag in the revents field."

How about then

- adding a flag to vb2_queue that tells that no buffer has been queued
- clearing the flag when a buffer is queued
- setting the flag when the stream is stopped
- modifying the poll implementation to

    if (q->no_buffer || !vb2_is_streaming(q) || q->error)
            return res | POLLERR;

(a better name is probably needed for the flag)

This would make poll() not return immediately during transient buffer 
underruns.

The other option would be to modify the V4L2 specification to state that 
poll() will return immediately with POLLERR at any time no buffer is queued, 
and forcing all userspace applications to implement an explicit buffer 
underrun check. I don't really like that though.

> But that would clash with this multi-thread example.
> 
> Hans, was this based on actual code that needed this?
> 
> I am inclined to update alevt and mtt: all that is needed to make it work
> is a single line that explicitly calls the vbi handler before entering the
> main loop. This is effectively the same as what happens when the first
> select gets a POLLERR.
> 
> We maintain alevt (dvb-apps) and mtt (xawtv3), so that's easy enough to
> fix.
> 
> Note that the spec is now definitely out-of-sync since poll no longer
> returns POLLERR if buffers are queued but STREAMON wasn't called.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Hans Verkuil]

On 09/16/14 12:29, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 15 September 2014 13:14:51 Hans Verkuil wrote:
>> On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
>>> On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
>>>> On 06/06/2014 11:50 AM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
>>>>>> The V4L2 specification states that
>>>>>>
>>>>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
>>>>>> the poll() function succeeds, but sets the POLLERR flag in the revents
>>>>>> field."
>>>>>>
>>>>>> The vb2_poll() function sets POLLERR when the queued buffers list is
>>>>>> empty, regardless of whether this is caused by the stream not being
>>>>>> active yet, or by a transient buffer underrun.
>>>>>>
>>>>>> Bring the implementation in line with the specification by returning
>>>>>> POLLERR only when the queue is not streaming. Buffer underruns during
>>>>>> streaming are not treated specially anymore and just result in poll()
>>>>>> blocking until the next event.
>>>>>
>>>>> After your patch the implementation is still not inline with the spec,
>>>>> queuing buffers, then starting a thread doing the poll, then doing the
>>>>> streamon in the main thread will still cause the poll to return POLLERR,
>>>>> even though buffers are queued, which according to the spec should be
>>>>> enough for the poll to block.
>>>>>
>>>>> The correct check would be:
>>>>>
>>>>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
>>>>>
>>>>> 	eturn res | POLLERR;
>>>>
>>>> Good catch! I should have seen that :-(
>>
>> Urgh. This breaks vbi capture tools like alevt and mtt. These rely on poll
>> returning POLLERR if buffers are queued but STREAMON has not been called
>> yet.
> 
> Then there's something I don't get. Before this commit, the implementation was
> 
>     /*
>      * There is nothing to wait for if no buffers have already been queued.
>      */
>     if (list_empty(&q->queued_list))
>             return res | POLLERR;
> 
> If buffers are queued and STREAMON hasn't been called yet, vb2_poll() would 
> not return POLLERR.

You are right, I think this has been broken from the beginning. Most of
the initial drivers that were converted to vb2 didn't use vbi. The first
complaint came when saa7134 was converted. So it is not actually your
commit that was wrong, it simply has always been wrong.

> 
>> See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401
>>
>> The spec also clearly says that poll should return POLLERR if STREAMON
>> was not called.
> 
> The V4L2 specification says
> 
> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet the 
> poll() function succeeds, but sets the POLLERR flag in the revents field."
> 
> How about then
> 
> - adding a flag to vb2_queue that tells that no buffer has been queued
> - clearing the flag when a buffer is queued
> - setting the flag when the stream is stopped
> - modifying the poll implementation to
> 
>     if (q->no_buffer || !vb2_is_streaming(q) || q->error)
>             return res | POLLERR;

I thought about something like that as well, but I don't think this is a good
idea. Mostly because you can call STREAMON without having queued buffers with
vb2. In fact, that is pretty much necessary for video output so you don't
have to queue up buffers with dummy contents.

Regards,

	Hans

> 
> (a better name is probably needed for the flag)
> 
> This would make poll() not return immediately during transient buffer 
> underruns.
> 
> The other option would be to modify the V4L2 specification to state that 
> poll() will return immediately with POLLERR at any time no buffer is queued, 
> and forcing all userspace applications to implement an explicit buffer 
> underrun check. I don't really like that though.
> 
>> But that would clash with this multi-thread example.
>>
>> Hans, was this based on actual code that needed this?
>>
>> I am inclined to update alevt and mtt: all that is needed to make it work
>> is a single line that explicitly calls the vbi handler before entering the
>> main loop. This is effectively the same as what happens when the first
>> select gets a POLLERR.
>>
>> We maintain alevt (dvb-apps) and mtt (xawtv3), so that's easy enough to
>> fix.
>>
>> Note that the spec is now definitely out-of-sync since poll no longer
>> returns POLLERR if buffers are queued but STREAMON wasn't called.
> 

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Laurent Pinchart]

Hi Hans,

On Tuesday 16 September 2014 13:18:59 Hans Verkuil wrote:
> On 09/16/14 12:29, Laurent Pinchart wrote:
> > On Monday 15 September 2014 13:14:51 Hans Verkuil wrote:
> >> On 06/06/2014 03:42 PM, Laurent Pinchart wrote:
> >>> On Friday 06 June 2014 11:58:18 Hans Verkuil wrote:
> >>>> On 06/06/2014 11:50 AM, Hans de Goede wrote:
> >>>>> On 06/05/2014 02:23 PM, Laurent Pinchart wrote:
> >>>>>> The V4L2 specification states that
> >>>>>> 
> >>>>>> "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> >>>>>> the poll() function succeeds, but sets the POLLERR flag in the
> >>>>>> revents field."
> >>>>>> 
> >>>>>> The vb2_poll() function sets POLLERR when the queued buffers list is
> >>>>>> empty, regardless of whether this is caused by the stream not being
> >>>>>> active yet, or by a transient buffer underrun.
> >>>>>> 
> >>>>>> Bring the implementation in line with the specification by returning
> >>>>>> POLLERR only when the queue is not streaming. Buffer underruns during
> >>>>>> streaming are not treated specially anymore and just result in poll()
> >>>>>> blocking until the next event.
> >>>>> 
> >>>>> After your patch the implementation is still not inline with the spec,
> >>>>> queuing buffers, then starting a thread doing the poll, then doing the
> >>>>> streamon in the main thread will still cause the poll to return
> >>>>> POLLERR, even though buffers are queued, which according to the spec
> >>>>> should be enough for the poll to block.
> >>>>> 
> >>>>> The correct check would be:
> >>>>> 
> >>>>> if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
> >>>>> 
> >>>>> 	eturn res | POLLERR;
> >>>> 
> >>>> Good catch! I should have seen that :-(
> >> 
> >> Urgh. This breaks vbi capture tools like alevt and mtt. These rely on
> >> poll returning POLLERR if buffers are queued but STREAMON has not been
> >> called yet.
> > 
> > Then there's something I don't get. Before this commit, the implementation
> > was
> >     /*
> >      * There is nothing to wait for if no buffers have already been
> >      queued.
> >      */
> >     if (list_empty(&q->queued_list))
> >     
> >             return res | POLLERR;
> > 
> > If buffers are queued and STREAMON hasn't been called yet, vb2_poll()
> > would not return POLLERR.
> 
> You are right, I think this has been broken from the beginning. Most of
> the initial drivers that were converted to vb2 didn't use vbi. The first
> complaint came when saa7134 was converted. So it is not actually your
> commit that was wrong, it simply has always been wrong.

What a relief :-D
 
> >> See bug report https://bugzilla.kernel.org/show_bug.cgi?id=84401
> >> 
> >> The spec also clearly says that poll should return POLLERR if STREAMON
> >> was not called.
> > 
> > The V4L2 specification says
> > 
> > "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet the
> > poll() function succeeds, but sets the POLLERR flag in the revents field."
> > 
> > How about then
> > 
> > - adding a flag to vb2_queue that tells that no buffer has been queued
> > - clearing the flag when a buffer is queued
> > - setting the flag when the stream is stopped
> > - modifying the poll implementation to
> > 
> >     if (q->no_buffer || !vb2_is_streaming(q) || q->error)
> >             return res | POLLERR;
> 
> I thought about something like that as well, but I don't think this is a
> good idea. Mostly because you can call STREAMON without having queued
> buffers with vb2. In fact, that is pretty much necessary for video output
> so you don't have to queue up buffers with dummy contents.

Given that we will change both the code and the spec, let's take a minute to 
think about what will be the most beneficial implementation for userspace.

I believe poll() should not return POLLERR during transient buffer underruns, 
as that would put an unnecessary burden on all userspace applications that 
would need to explicitly handle that case. a list_empty() check alone is thus 
out of question.

We thus have three options regarding when to return POLLERR

1. At any time the queue isn't streaming (!vb2_is_streaming())
2. Only before buffers are queued for the first time (q->no_buffer)
3. When both 1. and 2. are true
4. When either 1. or 2. is true

(POLLERR should always be returned when q->error is set, let's skip that to 
keep the discussion simple)

I think we should always return POLLERR when the queue isn't streaming as 
there's nothing an application can possibly wait on in that case (with the 
exception of events, but those are handled separately, and POLLERR won't be 
returned if the application isn't interest in POLLIN and/or POLLOUT). This 
means that applications won't be able to poll() on a device before starting 
streaming, and multithreaded applications that start/stop streaming and poll 
in different threads will need to synchronize both. I don't see that as a 
problem, but feel free to disagree.

This thus rules out options 2. and 3., leaving 1. (your proposal) and 4. (my 
proposal). 4. has the advantage of complying with the V4L2 specification. 1. 
has the advantage of being simpler on both the kernel and application sides, 
as applications will be able to poll before the first buffer is queued. I 
don't really see a use case where an application would benefit from getting 
POLLERR after STREAMON when no buffer has been queued yet, but once again 
please feel free to be more creative than me :-)

If we go for option 1. I would like the patch to fix the specification.

-- 
Regards,

Laurent Pinchart