[PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.

The commit 9241650d62f7 was meant to solve an issue with Gstreamer
version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
a bad behavior ag Gstreamer.

It does that by returning POLERR if VB2 is not streaming.

However, it broke VBI userspace support on alevt and mtt (and maybe
other VBI apps), as they rely on the old behavior.

Due to that, we need to roll back and restore the previous behavior.

It means that there are still some potential regressions by reverting it,
but those are known to occur only if:
	- libv4l is version 1.2 or upper (due to DQBUF fixup);
	- Gstreamer version 1.2 or before are being used, as this bug
got fixed on Gstreamer 1.4.

As both libv4l 1.2 and Gstreamer version 1.4 were released about the same
time, and the fix went only on Kernel 3.16 and were not backported to
stable, it is very unlikely that reverting it would cause much harm.

For more details, see:
	https://bugzilla.kernel.org/show_bug.cgi?id=84401

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Pawel Osciak <pawel@osciak.com>
Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7e6aff673a5a..7387821e7c72 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2583,10 +2583,10 @@ 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 no buffer has been queued
+	 * or if the error flag is set.
 	 */
-	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
+	if ((list_empty(&q->queued_list) || q->error)
 		return res | POLLERR;
 
 	/*
-- 
1.9.3

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Laurent Pinchart]

Hi Mauro,

On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> 
> The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> a bad behavior ag Gstreamer.

That's not correct. The patch was created to solve an issue observed with the 
Gstreamer 0.10 v4l2src element accessing the video device directly, *without* 
libv4l.

The V4L2 specification documents poll() as follows.

"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 implementation didn't conform with that, as it returned POLLERR 
when the buffer list was empty due to a transient buffer underrun, even if 
both VIDIOC_STREAMON and VIDIOC_QBUF have been called.

The commit thus brought the vb2 poll implementation in line with the 
specification. If we really want to revert it to its broken behaviour, then it 
would be fair to explain this in the revert message, and I want to know how 
you propose fixing this properly, as the revert really causes issues for 
userspace.

> It does that by returning POLERR if VB2 is not streaming.
> 
> However, it broke VBI userspace support on alevt and mtt (and maybe
> other VBI apps), as they rely on the old behavior.
> 
> Due to that, we need to roll back and restore the previous behavior.
> 
> It means that there are still some potential regressions by reverting it,
> but those are known to occur only if:
> 	- libv4l is version 1.2 or upper (due to DQBUF fixup);
> 	- Gstreamer version 1.2 or before are being used, as this bug
> got fixed on Gstreamer 1.4.
> 
> As both libv4l 1.2 and Gstreamer version 1.4 were released about the same
> time, and the fix went only on Kernel 3.16 and were not backported to
> stable, it is very unlikely that reverting it would cause much harm.
> 
> For more details, see:
> 	https://bugzilla.kernel.org/show_bug.cgi?id=84401
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Pawel Osciak <pawel@osciak.com>
> Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff673a5a..7387821e7c72
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2583,10 +2583,10 @@ 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 no buffer has been queued
> +	 * or if the error flag is set.
>  	 */
> -	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
> +	if ((list_empty(&q->queued_list) || q->error)
>  		return res | POLLERR;
> 
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Tue, 16 Sep 2014 12:09:01 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > 
> > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > a bad behavior ag Gstreamer.
> 
> That's not correct. The patch was created to solve an issue observed with the 
> Gstreamer 0.10 v4l2src element accessing the video device directly, *without* 
> libv4l.

Ok. From the discussions we took yesterday on the thread, I got the
wrong impression from Nicolas comments that this happens only with
gst < 1.4 and libv4l >= 1.2.

> 
> The V4L2 specification documents poll() as follows.
> 
> "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 implementation didn't conform with that, as it returned POLLERR 
> when the buffer list was empty due to a transient buffer underrun, even if 
> both VIDIOC_STREAMON and VIDIOC_QBUF have been called.
> 
> The commit thus brought the vb2 poll implementation in line with the 
> specification. If we really want to revert it to its broken behaviour, then it 
> would be fair to explain this in the revert message,

Ok, I'll rewrite the text. We likely want to fix the documentation too,
in order to reflect the way it is.

> and I want to know how 
> you propose fixing this properly, as the revert really causes issues for 
> userspace.

This patch simply broke all VBI applications. So, it should be reverted.

>From what you're saying, using Gst 0.10 with a kernel before 3.16 and
VB2 was always broken, right?

And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
and the fix should be a patch for it, or a recommendation to upgrade
to a newer version without such bug.

If, otherwise, it works with VB1, then we need to patch VB2 to have
exactly the same behavior as VB1 with that regards, as VBI works
with VB1.

Regards,
Mauro.

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Laurent Pinchart]

Hi Mauro,

On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
> Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
> > On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > > 
> > > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > > a bad behavior ag Gstreamer.
> > 
> > That's not correct. The patch was created to solve an issue observed with
> > the Gstreamer 0.10 v4l2src element accessing the video device directly,
> > *without* libv4l.
> 
> Ok. From the discussions we took yesterday on the thread, I got the
> wrong impression from Nicolas comments that this happens only with
> gst < 1.4 and libv4l >= 1.2.

My understanding is that recent gst versions worked around the problem, and 
the above combination of versions might be problematic, but gst 0.10 is 
definitely affected.

> > The V4L2 specification documents poll() as follows.
> > 
> > "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 implementation didn't conform with that, as it returned
> > POLLERR when the buffer list was empty due to a transient buffer
> > underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been called.
> > 
> > The commit thus brought the vb2 poll implementation in line with the
> > specification. If we really want to revert it to its broken behaviour,
> > then it would be fair to explain this in the revert message,
> 
> Ok, I'll rewrite the text. We likely want to fix the documentation too,
> in order to reflect the way it is.
> 
> > and I want to know how you propose fixing this properly, as the revert
> > really causes issues for userspace.
> 
> This patch simply broke all VBI applications. So, it should be reverted.
>
> From what you're saying, using Gst 0.10 with a kernel before 3.16 and
> VB2 was always broken, right?

Correct. And not only gst 0.10, any userspace application that doesn't 
specifically handles transient buffer underruns will be affected.

vb2 doesn't conform to the V4L2 specification, and I believe the specification 
is right in this case. Reverting this patch will push the problem to 
userspace, where all applications will have to handle buffer underruns 
manually.

> And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
> and the fix should be a patch for it, or a recommendation to upgrade
> to a newer version without such bug.

As explained above, this isn't a gst bug.

> If, otherwise, it works with VB1, then we need to patch VB2 to have
> exactly the same behavior as VB1 with that regards, as VBI works
> with VB1.

One option would be to have implement a different poll behaviour for VBI and 
video.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Tue, 16 Sep 2014 13:15:27 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
> > Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
> > > On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > > > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > > > 
> > > > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > > > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > > > a bad behavior ag Gstreamer.
> > > 
> > > That's not correct. The patch was created to solve an issue observed with
> > > the Gstreamer 0.10 v4l2src element accessing the video device directly,
> > > *without* libv4l.
> > 
> > Ok. From the discussions we took yesterday on the thread, I got the
> > wrong impression from Nicolas comments that this happens only with
> > gst < 1.4 and libv4l >= 1.2.
> 
> My understanding is that recent gst versions worked around the problem, and 
> the above combination of versions might be problematic, but gst 0.10 is 
> definitely affected.
> 
> > > The V4L2 specification documents poll() as follows.
> > > 
> > > "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 implementation didn't conform with that, as it returned
> > > POLLERR when the buffer list was empty due to a transient buffer
> > > underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been called.
> > > 
> > > The commit thus brought the vb2 poll implementation in line with the
> > > specification. If we really want to revert it to its broken behaviour,
> > > then it would be fair to explain this in the revert message,
> > 
> > Ok, I'll rewrite the text. We likely want to fix the documentation too,
> > in order to reflect the way it is.
> > 
> > > and I want to know how you propose fixing this properly, as the revert
> > > really causes issues for userspace.
> > 
> > This patch simply broke all VBI applications. So, it should be reverted.
> >
> > From what you're saying, using Gst 0.10 with a kernel before 3.16 and
> > VB2 was always broken, right?
> 
> Correct. And not only gst 0.10, any userspace application that doesn't 
> specifically handles transient buffer underruns will be affected.
> 
> vb2 doesn't conform to the V4L2 specification, and I believe the specification 
> is right in this case. Reverting this patch will push the problem to 
> userspace, where all applications will have to handle buffer underruns 
> manually.

What happens with VB1? How is it solved there?

I don't generally use gst 0.10, but I don't remember a single error
report about gst 0.10 and VB1-based drivers.

> > And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
> > and the fix should be a patch for it, or a recommendation to upgrade
> > to a newer version without such bug.
> 
> As explained above, this isn't a gst bug.
> 
> > If, otherwise, it works with VB1, then we need to patch VB2 to have
> > exactly the same behavior as VB1 with that regards, as VBI works
> > with VB1.
> 
> One option would be to have implement a different poll behaviour for VBI and 
> video.

That would be a nightmare.
> 


-- 

Cheers,
Mauro

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Laurent Pinchart]

Hi Mauro,

On Tuesday 16 September 2014 07:58:12 Mauro Carvalho Chehab wrote:
> Em Tue, 16 Sep 2014 13:15:27 +0300 Laurent Pinchart escreveu:
> > On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
> > > Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
> > > > On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > > > > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > > > > 
> > > > > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > > > > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > > > > a bad behavior ag Gstreamer.
> > > > 
> > > > That's not correct. The patch was created to solve an issue observed
> > > > with the Gstreamer 0.10 v4l2src element accessing the video device
> > > > directly, *without* libv4l.
> > > 
> > > Ok. From the discussions we took yesterday on the thread, I got the
> > > wrong impression from Nicolas comments that this happens only with
> > > gst < 1.4 and libv4l >= 1.2.
> > 
> > My understanding is that recent gst versions worked around the problem,
> > and the above combination of versions might be problematic, but gst 0.10
> > is definitely affected.
> > 
> > > > The V4L2 specification documents poll() as follows.
> > > > 
> > > > "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 implementation didn't conform with that, as it returned
> > > > POLLERR when the buffer list was empty due to a transient buffer
> > > > underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been
> > > > called.
> > > > 
> > > > The commit thus brought the vb2 poll implementation in line with the
> > > > specification. If we really want to revert it to its broken behaviour,
> > > > then it would be fair to explain this in the revert message,
> > > 
> > > Ok, I'll rewrite the text. We likely want to fix the documentation too,
> > > in order to reflect the way it is.
> > > 
> > > > and I want to know how you propose fixing this properly, as the revert
> > > > really causes issues for userspace.
> > > 
> > > This patch simply broke all VBI applications. So, it should be reverted.
> > > 
> > > From what you're saying, using Gst 0.10 with a kernel before 3.16 and
> > > VB2 was always broken, right?
> > 
> > Correct. And not only gst 0.10, any userspace application that doesn't
> > specifically handles transient buffer underruns will be affected.
> > 
> > vb2 doesn't conform to the V4L2 specification, and I believe the
> > specification is right in this case. Reverting this patch will push the
> > problem to userspace, where all applications will have to handle buffer
> > underruns manually.
> 
> What happens with VB1? How is it solved there?
> 
> I don't generally use gst 0.10, but I don't remember a single error
> report about gst 0.10 and VB1-based drivers.

I haven't tried VB1, but a quick look at the source code shows it to be 
affected as well.

The problem with gst 0.10 is only noticeable when a buffer underrun occurs 
(when you don't requeue buffers fast enough and the queue buffers list becomes 
temporarily empty), so it can very well go unnoticed for a long time.

> > > And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
> > > and the fix should be a patch for it, or a recommendation to upgrade
> > > to a newer version without such bug.
> > 
> > As explained above, this isn't a gst bug.
> > 
> > > If, otherwise, it works with VB1, then we need to patch VB2 to have
> > > exactly the same behavior as VB1 with that regards, as VBI works
> > > with VB1.
> > 
> > One option would be to have implement a different poll behaviour for VBI
> > and video.
> 
> That would be a nightmare.

I don't like it much either. Hans has posted a proposal to fix the problem an 
hour ago, let's discuss it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Tue, 16 Sep 2014 14:42:36 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 16 September 2014 07:58:12 Mauro Carvalho Chehab wrote:
> > Em Tue, 16 Sep 2014 13:15:27 +0300 Laurent Pinchart escreveu:
> > > On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
> > > > Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
> > > > > On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > > > > > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > > > > > 
> > > > > > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > > > > > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > > > > > a bad behavior ag Gstreamer.
> > > > > 
> > > > > That's not correct. The patch was created to solve an issue observed
> > > > > with the Gstreamer 0.10 v4l2src element accessing the video device
> > > > > directly, *without* libv4l.
> > > > 
> > > > Ok. From the discussions we took yesterday on the thread, I got the
> > > > wrong impression from Nicolas comments that this happens only with
> > > > gst < 1.4 and libv4l >= 1.2.
> > > 
> > > My understanding is that recent gst versions worked around the problem,
> > > and the above combination of versions might be problematic, but gst 0.10
> > > is definitely affected.
> > > 
> > > > > The V4L2 specification documents poll() as follows.
> > > > > 
> > > > > "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 implementation didn't conform with that, as it returned
> > > > > POLLERR when the buffer list was empty due to a transient buffer
> > > > > underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been
> > > > > called.
> > > > > 
> > > > > The commit thus brought the vb2 poll implementation in line with the
> > > > > specification. If we really want to revert it to its broken behaviour,
> > > > > then it would be fair to explain this in the revert message,
> > > > 
> > > > Ok, I'll rewrite the text. We likely want to fix the documentation too,
> > > > in order to reflect the way it is.
> > > > 
> > > > > and I want to know how you propose fixing this properly, as the revert
> > > > > really causes issues for userspace.
> > > > 
> > > > This patch simply broke all VBI applications. So, it should be reverted.
> > > > 
> > > > From what you're saying, using Gst 0.10 with a kernel before 3.16 and
> > > > VB2 was always broken, right?
> > > 
> > > Correct. And not only gst 0.10, any userspace application that doesn't
> > > specifically handles transient buffer underruns will be affected.
> > > 
> > > vb2 doesn't conform to the V4L2 specification, and I believe the
> > > specification is right in this case. Reverting this patch will push the
> > > problem to userspace, where all applications will have to handle buffer
> > > underruns manually.
> > 
> > What happens with VB1? How is it solved there?
> > 
> > I don't generally use gst 0.10, but I don't remember a single error
> > report about gst 0.10 and VB1-based drivers.
> 
> I haven't tried VB1, but a quick look at the source code shows it to be 
> affected as well.

Well, from a quick look I did at VB1, when the device is not streaming,
VB1 starts streaming, returning POLLERR only when stream start fails.

VB2, on the other hand, doesn't try to start streaming. That's likely
what broke VBI applications when you added the condition to return
POLLERR is vb2 is not streaming.

In other words, the VB2 equivalent to what VB1 does is:

	if (!vb2_is_streaming(q))
		vb2_internal_streamon(q, type);

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

> The problem with gst 0.10 is only noticeable when a buffer underrun occurs 
> (when you don't requeue buffers fast enough and the queue buffers list becomes 
> temporarily empty), so it can very well go unnoticed for a long time.
> 
> > > > And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
> > > > and the fix should be a patch for it, or a recommendation to upgrade
> > > > to a newer version without such bug.
> > > 
> > > As explained above, this isn't a gst bug.
> > > 
> > > > If, otherwise, it works with VB1, then we need to patch VB2 to have
> > > > exactly the same behavior as VB1 with that regards, as VBI works
> > > > with VB1.
> > > 
> > > One option would be to have implement a different poll behaviour for VBI
> > > and video.
> > 
> > That would be a nightmare.
> 
> I don't like it much either. Hans has posted a proposal to fix the problem an 
> hour ago, let's discuss it.
> 

Ok, I'll add my comments there.

PS.: I'm in a travel today, so unable to test or to do much comments.

-- 

Cheers,
Mauro

Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Hans Verkuil]

On 09/16/14 15:41, Mauro Carvalho Chehab wrote:
> Em Tue, 16 Sep 2014 14:42:36 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
>> Hi Mauro,
>>
>> On Tuesday 16 September 2014 07:58:12 Mauro Carvalho Chehab wrote:
>>> Em Tue, 16 Sep 2014 13:15:27 +0300 Laurent Pinchart escreveu:
>>>> On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
>>>>> Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
>>>>>> On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
>>>>>>> This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
>>>>>>>
>>>>>>> The commit 9241650d62f7 was meant to solve an issue with Gstreamer
>>>>>>> version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
>>>>>>> a bad behavior ag Gstreamer.
>>>>>>
>>>>>> That's not correct. The patch was created to solve an issue observed
>>>>>> with the Gstreamer 0.10 v4l2src element accessing the video device
>>>>>> directly, *without* libv4l.
>>>>>
>>>>> Ok. From the discussions we took yesterday on the thread, I got the
>>>>> wrong impression from Nicolas comments that this happens only with
>>>>> gst < 1.4 and libv4l >= 1.2.
>>>>
>>>> My understanding is that recent gst versions worked around the problem,
>>>> and the above combination of versions might be problematic, but gst 0.10
>>>> is definitely affected.
>>>>
>>>>>> The V4L2 specification documents poll() as follows.
>>>>>>
>>>>>> "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 implementation didn't conform with that, as it returned
>>>>>> POLLERR when the buffer list was empty due to a transient buffer
>>>>>> underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been
>>>>>> called.
>>>>>>
>>>>>> The commit thus brought the vb2 poll implementation in line with the
>>>>>> specification. If we really want to revert it to its broken behaviour,
>>>>>> then it would be fair to explain this in the revert message,
>>>>>
>>>>> Ok, I'll rewrite the text. We likely want to fix the documentation too,
>>>>> in order to reflect the way it is.
>>>>>
>>>>>> and I want to know how you propose fixing this properly, as the revert
>>>>>> really causes issues for userspace.
>>>>>
>>>>> This patch simply broke all VBI applications. So, it should be reverted.
>>>>>
>>>>> From what you're saying, using Gst 0.10 with a kernel before 3.16 and
>>>>> VB2 was always broken, right?
>>>>
>>>> Correct. And not only gst 0.10, any userspace application that doesn't
>>>> specifically handles transient buffer underruns will be affected.
>>>>
>>>> vb2 doesn't conform to the V4L2 specification, and I believe the
>>>> specification is right in this case. Reverting this patch will push the
>>>> problem to userspace, where all applications will have to handle buffer
>>>> underruns manually.
>>>
>>> What happens with VB1? How is it solved there?
>>>
>>> I don't generally use gst 0.10, but I don't remember a single error
>>> report about gst 0.10 and VB1-based drivers.
>>
>> I haven't tried VB1, but a quick look at the source code shows it to be 
>> affected as well.
> 
> Well, from a quick look I did at VB1, when the device is not streaming,
> VB1 starts streaming, returning POLLERR only when stream start fails.
> 
> VB2, on the other hand, doesn't try to start streaming. That's likely
> what broke VBI applications when you added the condition to return
> POLLERR is vb2 is not streaming.

No, that's not the reason. VB1 tries to start streaming, yes, but it
wants to set that up for read() support (and that is something vb2 does
as well). I am certain that the start streaming in vb1 fails somewhere
because REQBUFS is already called and buffers are queued and so it just
sets POLLERR. If it would succeed you would never be able to use stream
I/O for vbi since q->reading would be true (i.e. streaming uses the
read() API).

Where exactly it fails in __videobuf_read_start I would have to debug,
as usual the vb1 control flow is a nightmare.

> 
> In other words, the VB2 equivalent to what VB1 does is:
> 
> 	if (!vb2_is_streaming(q))
> 		vb2_internal_streamon(q, type);

So this is certainly not what should happen in vb2.

Regards,

	Hans

> 
>         if (list_empty(&q->queued_list) && !vb2_is_streaming(q))
>                 return res | POLLERR;
> 
>> The problem with gst 0.10 is only noticeable when a buffer underrun occurs 
>> (when you don't requeue buffers fast enough and the queue buffers list becomes 
>> temporarily empty), so it can very well go unnoticed for a long time.
>>
>>>>> And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
>>>>> and the fix should be a patch for it, or a recommendation to upgrade
>>>>> to a newer version without such bug.
>>>>
>>>> As explained above, this isn't a gst bug.
>>>>
>>>>> If, otherwise, it works with VB1, then we need to patch VB2 to have
>>>>> exactly the same behavior as VB1 with that regards, as VBI works
>>>>> with VB1.
>>>>
>>>> One option would be to have implement a different poll behaviour for VBI
>>>> and video.
>>>
>>> That would be a nightmare.
>>
>> I don't like it much either. Hans has posted a proposal to fix the problem an 
>> hour ago, let's discuss it.
>>
> 
> Ok, I'll add my comments there.
> 
> PS.: I'm in a travel today, so unable to test or to do much comments.
> 

[PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.

The commit 9241650d62f7 was meant to solve a race issue that
affects Gstreamer version 0.10, when streaming for a long time.

It does that by returning POLERR if VB2 is not streaming.

However, it broke VBI userspace support on alevt and mtt (and maybe
other VBI apps), as they rely on the old behavior.

Due to that, we need to roll back and restore the previous behavior.

For more details, see:
	https://bugzilla.kernel.org/show_bug.cgi?id=84401

So, let's rollback the change for now, and work on some other
fix for it.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Pawel Osciak <pawel@osciak.com>
Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>


--

v2: Just changed the description.

This is a regression that broke a core feature with most (all) VBI
apps. We should hurry sending a fix for it.

So, let's just revert the patch while we're discussing/testing a
solution that would solve Laurent's usecase scenario without breaking
VBI.

PS.: this patch should, of course, be c/c to 3.16 too, but I think we'll
need to do a manual backport, as the check for q->error is likely newer
than 3.16.

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7e6aff673a5a..da2d0adcc992 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2583,10 +2583,10 @@ 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 no buffer has been queued
+	 * or if the error flag is set.
 	 */
-	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
+	if (list_empty(&q->queued_list) || q->error)
 		return res | POLLERR;
 
 	/*

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Hans Verkuil]

Mauro,

This doesn't fix the problem *at all*.

See http://www.mail-archive.com/linux-media@vger.kernel.org/msg79474.html for
the right fix.

So for your patch:

Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

On 09/18/14 12:06, Mauro Carvalho Chehab wrote:
> This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> 
> The commit 9241650d62f7 was meant to solve a race issue that
> affects Gstreamer version 0.10, when streaming for a long time.
> 
> It does that by returning POLERR if VB2 is not streaming.
> 
> However, it broke VBI userspace support on alevt and mtt (and maybe
> other VBI apps), as they rely on the old behavior.
> 
> Due to that, we need to roll back and restore the previous behavior.
> 
> For more details, see:
> 	https://bugzilla.kernel.org/show_bug.cgi?id=84401
> 
> So, let's rollback the change for now, and work on some other
> fix for it.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Pawel Osciak <pawel@osciak.com>
> Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> 
> --
> 
> v2: Just changed the description.
> 
> This is a regression that broke a core feature with most (all) VBI
> apps. We should hurry sending a fix for it.
> 
> So, let's just revert the patch while we're discussing/testing a
> solution that would solve Laurent's usecase scenario without breaking
> VBI.
> 
> PS.: this patch should, of course, be c/c to 3.16 too, but I think we'll
> need to do a manual backport, as the check for q->error is likely newer
> than 3.16.
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 7e6aff673a5a..da2d0adcc992 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2583,10 +2583,10 @@ 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 no buffer has been queued
> +	 * or if the error flag is set.
>  	 */
> -	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
> +	if (list_empty(&q->queued_list) || q->error)
>  		return res | POLLERR;
>  
>  	/*
> 

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Thu, 18 Sep 2014 12:10:46 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> Mauro,
> 
> This doesn't fix the problem *at all*.
> 
> See http://www.mail-archive.com/linux-media@vger.kernel.org/msg79474.html for
> the right fix.

I won't apply patch 1/4 on stable. It is simply too risky, as it changes
a behavior that is there for a very long time.

We can of course try this at topic/devel and play with it for a while,
testing with different applications and devices, but we need a quick
solution for the regression introduced on 3.16.



> 
> So for your patch:
> 
> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Regards,
> 
> 	Hans
> 
> On 09/18/14 12:06, Mauro Carvalho Chehab wrote:
> > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > 
> > The commit 9241650d62f7 was meant to solve a race issue that
> > affects Gstreamer version 0.10, when streaming for a long time.
> > 
> > It does that by returning POLERR if VB2 is not streaming.
> > 
> > However, it broke VBI userspace support on alevt and mtt (and maybe
> > other VBI apps), as they rely on the old behavior.
> > 
> > Due to that, we need to roll back and restore the previous behavior.
> > 
> > For more details, see:
> > 	https://bugzilla.kernel.org/show_bug.cgi?id=84401
> > 
> > So, let's rollback the change for now, and work on some other
> > fix for it.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Pawel Osciak <pawel@osciak.com>
> > Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > 
> > 
> > --
> > 
> > v2: Just changed the description.
> > 
> > This is a regression that broke a core feature with most (all) VBI
> > apps. We should hurry sending a fix for it.
> > 
> > So, let's just revert the patch while we're discussing/testing a
> > solution that would solve Laurent's usecase scenario without breaking
> > VBI.
> > 
> > PS.: this patch should, of course, be c/c to 3.16 too, but I think we'll
> > need to do a manual backport, as the check for q->error is likely newer
> > than 3.16.
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 7e6aff673a5a..da2d0adcc992 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -2583,10 +2583,10 @@ 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 no buffer has been queued
> > +	 * or if the error flag is set.
> >  	 */
> > -	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
> > +	if (list_empty(&q->queued_list) || q->error)
> >  		return res | POLLERR;
> >  
> >  	/*
> > 

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Hans Verkuil]

My patch is the *only* fix for that since that's the one that addresses
the real issue.

One option is to merge my fix for 3.18 with a CC to stable for 3.16.

That way it will be in the tree for longer.

Again, the revert that you did won't solve the regression at all. Please
revert the revert.

Regards,

	Hans

On 09/18/14 12:50, Mauro Carvalho Chehab wrote:
> Em Thu, 18 Sep 2014 12:10:46 +0200
> Hans Verkuil <hansverk@cisco.com> escreveu:
> 
>> Mauro,
>>
>> This doesn't fix the problem *at all*.
>>
>> See http://www.mail-archive.com/linux-media@vger.kernel.org/msg79474.html for
>> the right fix.
> 
> I won't apply patch 1/4 on stable. It is simply too risky, as it changes
> a behavior that is there for a very long time.
> 
> We can of course try this at topic/devel and play with it for a while,
> testing with different applications and devices, but we need a quick
> solution for the regression introduced on 3.16.
> 
> 
> 
>>
>> So for your patch:
>>
>> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Regards,
>>
>> 	Hans
>>
>> On 09/18/14 12:06, Mauro Carvalho Chehab wrote:
>>> This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
>>>
>>> The commit 9241650d62f7 was meant to solve a race issue that
>>> affects Gstreamer version 0.10, when streaming for a long time.
>>>
>>> It does that by returning POLERR if VB2 is not streaming.
>>>
>>> However, it broke VBI userspace support on alevt and mtt (and maybe
>>> other VBI apps), as they rely on the old behavior.
>>>
>>> Due to that, we need to roll back and restore the previous behavior.
>>>
>>> For more details, see:
>>> 	https://bugzilla.kernel.org/show_bug.cgi?id=84401
>>>
>>> So, let's rollback the change for now, and work on some other
>>> fix for it.
>>>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>>> Cc: Pawel Osciak <pawel@osciak.com>
>>> Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>>
>>>
>>> --
>>>
>>> v2: Just changed the description.
>>>
>>> This is a regression that broke a core feature with most (all) VBI
>>> apps. We should hurry sending a fix for it.
>>>
>>> So, let's just revert the patch while we're discussing/testing a
>>> solution that would solve Laurent's usecase scenario without breaking
>>> VBI.
>>>
>>> PS.: this patch should, of course, be c/c to 3.16 too, but I think we'll
>>> need to do a manual backport, as the check for q->error is likely newer
>>> than 3.16.
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 7e6aff673a5a..da2d0adcc992 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -2583,10 +2583,10 @@ 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 no buffer has been queued
>>> +	 * or if the error flag is set.
>>>  	 */
>>> -	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
>>> +	if (list_empty(&q->queued_list) || q->error)
>>>  		return res | POLLERR;
>>>  
>>>  	/*
>>>

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Thu, 18 Sep 2014 14:07:21 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> My patch is the *only* fix for that since that's the one that addresses
> the real issue.
> 
> One option is to merge my fix for 3.18 with a CC to stable for 3.16.
> 
> That way it will be in the tree for longer.
> 
> Again, the revert that you did won't solve the regression at all. Please
> revert the revert.

Well, some patch that went between 3.15 and 3.16 broke VBI. If it was
not this patch, what's the patch that broke it?

Regards,
Mauro

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Hans Verkuil]

On 09/18/14 14:15, Mauro Carvalho Chehab wrote:
> Em Thu, 18 Sep 2014 14:07:21 +0200
> Hans Verkuil <hansverk@cisco.com> escreveu:
> 
>> My patch is the *only* fix for that since that's the one that addresses
>> the real issue.
>>
>> One option is to merge my fix for 3.18 with a CC to stable for 3.16.
>>
>> That way it will be in the tree for longer.
>>
>> Again, the revert that you did won't solve the regression at all. Please
>> revert the revert.
> 
> Well, some patch that went between 3.15 and 3.16 broke VBI. If it was
> not this patch, what's the patch that broke it?

The conversion of saa7134 to vb2 in 3.16 broke the VBI support in saa7134.

It turns out that vb2 NEVER did this right.

Remember that saa7134 was only the second driver with VBI support (after
em28xx) that was converted to vb2, and that this issue only happens with
teletext applications that do not call STREAMON before calling poll().

They rely on the fact that poll returns POLLERR to call STREAMON. Ugly
as hell, and not normal behavior for applications.

So that explains why it was never found before.

Note that em28xx (converted to vb2 quite some time before) fails as well.
So this regression has been there since 3.9 (when em28xx was converted).
I tested my fix with em28xx as well and that will worked fine.

Regards,

	Hans

Re: [PATCH v2] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Mauro Carvalho Chehab]

Em Thu, 18 Sep 2014 14:21:34 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> 
> 
> On 09/18/14 14:15, Mauro Carvalho Chehab wrote:
> > Em Thu, 18 Sep 2014 14:07:21 +0200
> > Hans Verkuil <hansverk@cisco.com> escreveu:
> > 
> >> My patch is the *only* fix for that since that's the one that addresses
> >> the real issue.
> >>
> >> One option is to merge my fix for 3.18 with a CC to stable for 3.16.
> >>
> >> That way it will be in the tree for longer.
> >>
> >> Again, the revert that you did won't solve the regression at all. Please
> >> revert the revert.
> > 
> > Well, some patch that went between 3.15 and 3.16 broke VBI. If it was
> > not this patch, what's the patch that broke it?
> 
> The conversion of saa7134 to vb2 in 3.16 broke the VBI support in saa7134.
> 
> It turns out that vb2 NEVER did this right.
> 
> Remember that saa7134 was only the second driver with VBI support (after
> em28xx) that was converted to vb2, and that this issue only happens with
> teletext applications that do not call STREAMON before calling poll().
> 
> They rely on the fact that poll returns POLLERR to call STREAMON. Ugly
> as hell, and not normal behavior for applications.
> 
> So that explains why it was never found before.

I don't doubt that your fix solved this specific VBI issue.

What I don't know is what else it broke (if any). If you take a look
at the videobuf-core, the check for an empty queue that you've removed
is also there. So, the special handling if the list is empty is at kernel
for a long time:

7a7d9a89d0307 drivers/media/video/videobuf-core.c     (Mauro Carvalho Chehab 2007-08-23 16:26:14 -0300 1125)    if (q->streaming) {
7a7d9a89d0307 drivers/media/video/videobuf-core.c     (Mauro Carvalho Chehab 2007-08-23 16:26:14 -0300 1126)            if (!list_empty(&q->stream))

This changeset was merged on v2.6.24. Before that, the videobuf1
monolithic code were also using it. So, I won't doubt that this check
is there since the start of V4L2 API.

Changing the syscall behavior of such an old code is really risky
and can bring all sorts of unpredictable results.

I won't apply any change like that without a comprehensive test,
checking every single application to be at least 99.999% sure that
it won't cause even more regressions.

So, until we proof that nothing bad happens, and keep this code
for test for a reasonable amount of time, I'm nacking your VB2 
change patch.

We need to find a solution that will make VB2 to act just like
VB1 with regards to poll() syscall, and not to redefine the
V4L2 API and apply a partial half-baked, not mature hack on it.

> Note that em28xx (converted to vb2 quite some time before) fails as well.
> So this regression has been there since 3.9 (when em28xx was converted).
> I tested my fix with em28xx as well and that will worked fine.
> 
> Regards,
> 
> 	Hans