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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> 
> 
> On 09/18/14 14:15, Mauro Carvalho Chehab wrote:
> > Em Thu, 18 Sep 2014 14:07:21 +0200
> > Hans Verkuil <hansverk@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux