RFC: poll behavior

RFC: poll behavior

[Hans Verkuil]

Hi all,

This RFC is based on recent discussions with regards to how the poll function 
in a V4L2 driver should behave. 

Some relevant documents:

POSIX:

http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html

V4L2 Spec:

http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html

I've copied some descriptions from the POSIX poll document below:

POLLIN
Data other than high-priority data may be read without blocking.

POLLERR
An error has occurred on the device or stream. This flag is only valid in the 
revents bitmask; it shall be ignored in the events member.

POLLHUP
The device has been disconnected. This event and POLLOUT are mutually-
exclusive; a stream can never be writable if a hangup has occurred. However, 
this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-
exclusive. This flag is only valid in the revents bitmask; it shall be ignored 
in the events member.



1) How *should* it work?

Suppose we could redesign the way V4L2 handles poll(), how should it work?

There are three cases:

a) poll() called when streaming is in progress.

This is easy: if a buffer is available, then return POLLIN (or POLLOUT for 
output). If an event has arrived, then OR with POLLPRI.

b) poll() called when no streaming has started and the driver does not support 
the read/write API.

In this case poll() should return POLLERR, since calling poll() when no 
streaming is in progress makes no sense. POLLHUP seems an interesting 
alternative, but other than for end-of-file or end-of-stream conditions it's 
meaning is very fuzzy. Linux also has a POLLRDHUP to make things even more 
confusing. So I would just stick with POLLERR.

c) poll() called when no streaming has started and the driver supports the 
read/write API.

I see no reason why we would do anything different from b). Calling poll() 
when there is no streaming in progress clearly means that there is no data 
available and that there won't be any data either until streaming starts 
somehow.


2) How does it work today?

a) poll() called when streaming is in progress.

We follow the procedure outlined above. So this is fine.

b) poll() called when no streaming has started and the driver does not support 
the read/write API.

In this case POLLERR is returned. So this is also fine.

c) poll() called when no streaming has started and the driver supports the 
read/write API.

Here things are different. poll() will in this case start streaming 
automatically and either return POLLERR if there was an error when starting 
streaming, or return 0 so it will wait until the first buffer arrives.

The main problem is that it will also do this if the application only wants to 
poll for exceptions (POLLPRI). Unfortunately there is no reliable way in the 
kernel to discover whether the application wanted to poll for POLLIN or just 
for POLLPRI. This behavior simply kills applications that want to wait for 
e.g. changes in controls without actually starting streaming. An example is 
qv4l2.

Another problem is that starting streaming from within poll can take a long 
time depending on the hardware. And poll isn't supposed to take a long time 
(exceptions are calling poll for userspace file systems). Actually, until 
2.6.29 sleeping inside poll() wasn't even supported.

In my opinion poll should never start the capture process. There is nothing in 
the poll POSIX document that says that it should. It complicates drivers and 
it is an ugly side-effect.


3) What are the consequences if we changed the behavior of c?

Suppose we just changed the current behavior to the desired behavior. What 
would change for applications?

Obviously, nothing will change for drivers that do not support read/write.

For the others there will be a difference depending on whether the 
applications uses select(2) or poll(2). The select() function hides the exact 
event masked returned by the driver, so POLLERR will not show up as such, 
instead select() will just return and set the input or output mask of the 
corresponding fd. So there is no difference between POLLERR and POLLIN or 
POLLOUT as seen by the application.

What will change is when read() is called: if it is in non-blocking mode, then 
it will start the streaming and return -EAGAIN. If it is in blocking mode, 
then there is no visible change in behavior. So drivers must be able to handle 
that error code.

However, all applications must be able to handle that error anyway. It's part 
of the POSIX specification of a non-blocking read().

For applications that call poll(2) there will be a difference depending on 
what they do when they see a POLLERR: if they just ignore it and call read, 
then it is effectively identical to the select() case.

If they actually test for POLLERR and return an error, then this change will 
break such an application.

So the only time that a well-written application would break is when it uses 
poll(2) and read(2), and returns an error if POLLERR was set instead of just 
calling read().


4) Proposal to change the poll behavior

For the short term I propose that condition c is handled as follows:

If for the filehandle passed to poll() no events have been subscribed, then 
keep the old behavior (i.e. start streaming). If events have been subscribed, 
however, then implement the new behavior (return POLLERR).

Since events are new I think this is a safe change that will not affect 
existing applications.

In the long term I propose that we put this in the feature-removal-schedule 
for v3.3 or so and stop poll from starting streaming altogether.

Comments?

	Hans

Re: RFC: poll behavior

[Hans de Goede]

Hi,

On 06/29/2011 01:26 PM, Hans Verkuil wrote:
> Hi all,
>
> This RFC is based on recent discussions with regards to how the poll function
> in a V4L2 driver should behave.
>
> Some relevant documents:
>
> POSIX:
>
> http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
> http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
> http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
>
> V4L2 Spec:
>
> http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
>
> I've copied some descriptions from the POSIX poll document below:
>
> POLLIN
> Data other than high-priority data may be read without blocking.
>
> POLLERR
> An error has occurred on the device or stream. This flag is only valid in the
> revents bitmask; it shall be ignored in the events member.
>
> POLLHUP
> The device has been disconnected. This event and POLLOUT are mutually-
> exclusive; a stream can never be writable if a hangup has occurred. However,
> this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-
> exclusive. This flag is only valid in the revents bitmask; it shall be ignored
> in the events member.
>
>
>
> 1) How *should* it work?
>
> Suppose we could redesign the way V4L2 handles poll(), how should it work?
>
> There are three cases:
>
> a) poll() called when streaming is in progress.
>
> This is easy: if a buffer is available, then return POLLIN (or POLLOUT for
> output). If an event has arrived, then OR with POLLPRI.
>
> b) poll() called when no streaming has started and the driver does not support
> the read/write API.
>
> In this case poll() should return POLLERR, since calling poll() when no
> streaming is in progress makes no sense. POLLHUP seems an interesting
> alternative, but other than for end-of-file or end-of-stream conditions it's
> meaning is very fuzzy. Linux also has a POLLRDHUP to make things even more
> confusing. So I would just stick with POLLERR.
>
> c) poll() called when no streaming has started and the driver supports the
> read/write API.
>
> I see no reason why we would do anything different from b). Calling poll()
> when there is no streaming in progress clearly means that there is no data
> available and that there won't be any data either until streaming starts
> somehow.
>
>
> 2) How does it work today?
>
> a) poll() called when streaming is in progress.
>
> We follow the procedure outlined above. So this is fine.
>
> b) poll() called when no streaming has started and the driver does not support
> the read/write API.
>
> In this case POLLERR is returned. So this is also fine.
>
> c) poll() called when no streaming has started and the driver supports the
> read/write API.
>
> Here things are different. poll() will in this case start streaming
> automatically and either return POLLERR if there was an error when starting
> streaming, or return 0 so it will wait until the first buffer arrives.

Right, to clarify, in this case the drivers assume the app wants r/w mode and
streaming is started for r/w mode, not in mmap mode even if the drivers support
both.

> The main problem is that it will also do this if the application only wants to
> poll for exceptions (POLLPRI). Unfortunately there is no reliable way in the
> kernel to discover whether the application wanted to poll for POLLIN or just
> for POLLPRI. This behavior simply kills applications that want to wait for
> e.g. changes in controls without actually starting streaming. An example is
> qv4l2.
>
> Another problem is that starting streaming from within poll can take a long
> time depending on the hardware. And poll isn't supposed to take a long time
> (exceptions are calling poll for userspace file systems). Actually, until
> 2.6.29 sleeping inside poll() wasn't even supported.
>
> In my opinion poll should never start the capture process. There is nothing in
> the poll POSIX document that says that it should. It complicates drivers and
> it is an ugly side-effect.
>

Agreed, even more important then the timing issue, poll should simply not
have side effects, and starting the stream is a rather large side effect!

>
> 3) What are the consequences if we changed the behavior of c?
>
> Suppose we just changed the current behavior to the desired behavior. What
> would change for applications?
>
> Obviously, nothing will change for drivers that do not support read/write.
>
> For the others there will be a difference depending on whether the
> applications uses select(2) or poll(2). The select() function hides the exact
> event masked returned by the driver, so POLLERR will not show up as such,
> instead select() will just return and set the input or output mask of the
> corresponding fd. So there is no difference between POLLERR and POLLIN or
> POLLOUT as seen by the application.
>
> What will change is when read() is called: if it is in non-blocking mode, then
> it will start the streaming and return -EAGAIN. If it is in blocking mode,
> then there is no visible change in behavior. So drivers must be able to handle
> that error code.
>
> However, all applications must be able to handle that error anyway. It's part
> of the POSIX specification of a non-blocking read().

Note that the only app I know of which actually does a select() *before* its
first read() is:
http://v4l2spec.bytesex.org/spec/capture-example.html

I think this is the only one as libv4l did not handle this case
correctly for a long time, until someone complained that our example own
code did not work with libv4l. But someone may have copied our example
code somewhere ...

> For applications that call poll(2) there will be a difference depending on
> what they do when they see a POLLERR: if they just ignore it and call read,
> then it is effectively identical to the select() case.
>
> If they actually test for POLLERR and return an error, then this change will
> break such an application.
>
> So the only time that a well-written application would break is when it uses
> poll(2) and read(2), and returns an error if POLLERR was set instead of just
> calling read().

Right, I don't think such an application exists not even a proprietary one,
most apps tend to stick with select rather then use poll. and most v4l apps
tend to be a bit old anyways, so...

Hmm, then again this may bite gstreamer in the case of r/w
mode only devices.

> 4) Proposal to change the poll behavior
>
> For the short term I propose that condition c is handled as follows:
>
> If for the filehandle passed to poll() no events have been subscribed, then
> keep the old behavior (i.e. start streaming). If events have been subscribed,
> however, then implement the new behavior (return POLLERR).
>

If events have been subscribed and no events are pending then the right
behavior would be to return 0, not POLLERR, otherwise a waiting app
will return from the poll immediately, or am I missing something?

> Since events are new I think this is a safe change that will not affect
> existing applications.
>
> In the long term I propose that we put this in the feature-removal-schedule
> for v3.3 or so and stop poll from starting streaming altogether.

Alternative suggestion:

In scenario c + there are no events subscribed simply return POLL_IN /
POLL_OUT, iow signal the app, sure you're free to do a read / write to
start streaming, no need to wait with that :)

Advantages
-we stop starting the stream from poll *now*
-for select we only set an fd in read or write fds, depending
  on the devices capabilities + file rights, rather then both
-for poll we don't return POLL_ERR, potentially breaking apps
  (I highly doubt broken apps will be fixed as quickly as you
   want, skype has yet to move to libv4l ...)

Disadvantages:
-if an app falls in scenario c, and uses poll, and non blocking
  mode and does not handle EAGAIN from read, the app breaks

As said already not handling EAGAIN in non blocking mode is
against posix...

Regards,

Hans

Re: RFC: poll behavior

[Hans Verkuil]

On Wednesday, June 29, 2011 14:10:44 Hans de Goede wrote:
> Hi,
> 
> On 06/29/2011 01:26 PM, Hans Verkuil wrote:
> > Hi all,
> >
> > This RFC is based on recent discussions with regards to how the poll function
> > in a V4L2 driver should behave.
> >
> > Some relevant documents:
> >
> > POSIX:
> >
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
> >
> > V4L2 Spec:
> >
> > http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
> >
> > I've copied some descriptions from the POSIX poll document below:
> >
> > POLLIN
> > Data other than high-priority data may be read without blocking.
> >
> > POLLERR
> > An error has occurred on the device or stream. This flag is only valid in the
> > revents bitmask; it shall be ignored in the events member.
> >
> > POLLHUP
> > The device has been disconnected. This event and POLLOUT are mutually-
> > exclusive; a stream can never be writable if a hangup has occurred. However,
> > this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-
> > exclusive. This flag is only valid in the revents bitmask; it shall be ignored
> > in the events member.
> >
> >
> >
> > 1) How *should* it work?
> >
> > Suppose we could redesign the way V4L2 handles poll(), how should it work?
> >
> > There are three cases:
> >
> > a) poll() called when streaming is in progress.
> >
> > This is easy: if a buffer is available, then return POLLIN (or POLLOUT for
> > output). If an event has arrived, then OR with POLLPRI.
> >
> > b) poll() called when no streaming has started and the driver does not support
> > the read/write API.
> >
> > In this case poll() should return POLLERR, since calling poll() when no
> > streaming is in progress makes no sense. POLLHUP seems an interesting
> > alternative, but other than for end-of-file or end-of-stream conditions it's
> > meaning is very fuzzy. Linux also has a POLLRDHUP to make things even more
> > confusing. So I would just stick with POLLERR.
> >
> > c) poll() called when no streaming has started and the driver supports the
> > read/write API.
> >
> > I see no reason why we would do anything different from b). Calling poll()
> > when there is no streaming in progress clearly means that there is no data
> > available and that there won't be any data either until streaming starts
> > somehow.
> >
> >
> > 2) How does it work today?
> >
> > a) poll() called when streaming is in progress.
> >
> > We follow the procedure outlined above. So this is fine.
> >
> > b) poll() called when no streaming has started and the driver does not support
> > the read/write API.
> >
> > In this case POLLERR is returned. So this is also fine.
> >
> > c) poll() called when no streaming has started and the driver supports the
> > read/write API.
> >
> > Here things are different. poll() will in this case start streaming
> > automatically and either return POLLERR if there was an error when starting
> > streaming, or return 0 so it will wait until the first buffer arrives.
> 
> Right, to clarify, in this case the drivers assume the app wants r/w mode and
> streaming is started for r/w mode, not in mmap mode even if the drivers support
> both.

Correct.

> > The main problem is that it will also do this if the application only wants to
> > poll for exceptions (POLLPRI). Unfortunately there is no reliable way in the
> > kernel to discover whether the application wanted to poll for POLLIN or just
> > for POLLPRI. This behavior simply kills applications that want to wait for
> > e.g. changes in controls without actually starting streaming. An example is
> > qv4l2.
> >
> > Another problem is that starting streaming from within poll can take a long
> > time depending on the hardware. And poll isn't supposed to take a long time
> > (exceptions are calling poll for userspace file systems). Actually, until
> > 2.6.29 sleeping inside poll() wasn't even supported.
> >
> > In my opinion poll should never start the capture process. There is nothing in
> > the poll POSIX document that says that it should. It complicates drivers and
> > it is an ugly side-effect.
> >
> 
> Agreed, even more important then the timing issue, poll should simply not
> have side effects, and starting the stream is a rather large side effect!
> 
> >
> > 3) What are the consequences if we changed the behavior of c?
> >
> > Suppose we just changed the current behavior to the desired behavior. What
> > would change for applications?
> >
> > Obviously, nothing will change for drivers that do not support read/write.
> >
> > For the others there will be a difference depending on whether the
> > applications uses select(2) or poll(2). The select() function hides the exact
> > event masked returned by the driver, so POLLERR will not show up as such,
> > instead select() will just return and set the input or output mask of the
> > corresponding fd. So there is no difference between POLLERR and POLLIN or
> > POLLOUT as seen by the application.
> >
> > What will change is when read() is called: if it is in non-blocking mode, then
> > it will start the streaming and return -EAGAIN. If it is in blocking mode,
> > then there is no visible change in behavior. So drivers must be able to handle
> > that error code.
> >
> > However, all applications must be able to handle that error anyway. It's part
> > of the POSIX specification of a non-blocking read().
> 
> Note that the only app I know of which actually does a select() *before* its
> first read() is:
> http://v4l2spec.bytesex.org/spec/capture-example.html

qv4l2 uses it as well. In general, using select() before read() is not unreasonable
for devices where read() is the primary I/O method (such as MPEG encoders).

> I think this is the only one as libv4l did not handle this case
> correctly for a long time, until someone complained that our example own
> code did not work with libv4l. But someone may have copied our example
> code somewhere ...
> 
> > For applications that call poll(2) there will be a difference depending on
> > what they do when they see a POLLERR: if they just ignore it and call read,
> > then it is effectively identical to the select() case.
> >
> > If they actually test for POLLERR and return an error, then this change will
> > break such an application.
> >
> > So the only time that a well-written application would break is when it uses
> > poll(2) and read(2), and returns an error if POLLERR was set instead of just
> > calling read().
> 
> Right, I don't think such an application exists not even a proprietary one,
> most apps tend to stick with select rather then use poll. and most v4l apps
> tend to be a bit old anyways, so...
> 
> Hmm, then again this may bite gstreamer in the case of r/w
> mode only devices.
> 
> > 4) Proposal to change the poll behavior
> >
> > For the short term I propose that condition c is handled as follows:
> >
> > If for the filehandle passed to poll() no events have been subscribed, then
> > keep the old behavior (i.e. start streaming). If events have been subscribed,
> > however, then implement the new behavior (return POLLERR).
> >
> 
> If events have been subscribed and no events are pending then the right
> behavior would be to return 0, not POLLERR, otherwise a waiting app
> will return from the poll immediately, or am I missing something?

Yes and no. For select() POLLERR is ignored if you are only waiting for POLLPRI.

But I see that that does not happen for the poll(2) API (see do_pollfd() in
fs/select.c). This means that POLLERR is indeed not a suitable event it
return. It will have to be POLLIN or POLLOUT instead.

This is actually a real problem with poll(2): if there is no streaming in progress
and the driver does not support r/w, and you want to poll for just POLLPRI, then
POLLERR will be set, and poll(2) will always return. But select(2) will work fine.

In other words, poll(2) and select(2) handle POLLPRI differently with respect to
POLLERR. What a mess. You can't really return POLLERR and support POLLPRI at the
same time.

> > Since events are new I think this is a safe change that will not affect
> > existing applications.
> >
> > In the long term I propose that we put this in the feature-removal-schedule
> > for v3.3 or so and stop poll from starting streaming altogether.
> 
> Alternative suggestion:
> 
> In scenario c + there are no events subscribed simply return POLL_IN /
> POLL_OUT, iow signal the app, sure you're free to do a read / write to
> start streaming, no need to wait with that :)
> 
> Advantages
> -we stop starting the stream from poll *now*
> -for select we only set an fd in read or write fds, depending
>   on the devices capabilities + file rights, rather then both

Nobody ever sets both, why would you do that?

> -for poll we don't return POLL_ERR, potentially breaking apps
>   (I highly doubt broken apps will be fixed as quickly as you
>    want, skype has yet to move to libv4l ...)

I'm happy with that, but Mauro isn't.

> Disadvantages:
> -if an app falls in scenario c, and uses poll, and non blocking
>   mode and does not handle EAGAIN from read, the app breaks
> 
> As said already not handling EAGAIN in non blocking mode is
> against posix...

Regards,

	Hans

Re: RFC: poll behavior

[Hans de Goede]

Hi,

On 06/29/2011 02:42 PM, Hans Verkuil wrote:
> On Wednesday, June 29, 2011 14:10:44 Hans de Goede wrote:
>> Hi,
>>
>> On 06/29/2011 01:26 PM, Hans Verkuil wrote:

<Snip>

>>> 4) Proposal to change the poll behavior
>>>
>>> For the short term I propose that condition c is handled as follows:
>>>
>>> If for the filehandle passed to poll() no events have been subscribed, then
>>> keep the old behavior (i.e. start streaming). If events have been subscribed,
>>> however, then implement the new behavior (return POLLERR).
>>>
>>
>> If events have been subscribed and no events are pending then the right
>> behavior would be to return 0, not POLLERR, otherwise a waiting app
>> will return from the poll immediately, or am I missing something?
>
> Yes and no. For select() POLLERR is ignored if you are only waiting for POLLPRI.
>
> But I see that that does not happen for the poll(2) API (see do_pollfd() in
> fs/select.c). This means that POLLERR is indeed not a suitable event it
> return. It will have to be POLLIN or POLLOUT instead.
>
> This is actually a real problem with poll(2): if there is no streaming in progress
> and the driver does not support r/w, and you want to poll for just POLLPRI, then
> POLLERR will be set, and poll(2) will always return. But select(2) will work fine.
>
> In other words, poll(2) and select(2) handle POLLPRI differently with respect to
> POLLERR. What a mess. You can't really return POLLERR and support POLLPRI at the
> same time.
>

Ok, yet more reason to go with my proposal, but then simplified to:

When streaming has not started return POLLIN or POLLOUT (or-ed with
POLLPRI if events are pending).


>>> Since events are new I think this is a safe change that will not affect
>>> existing applications.
>>>
>>> In the long term I propose that we put this in the feature-removal-schedule
>>> for v3.3 or so and stop poll from starting streaming altogether.
>>
>> Alternative suggestion:
>>
>> In scenario c + there are no events subscribed simply return POLL_IN /
>> POLL_OUT, iow signal the app, sure you're free to do a read / write to
>> start streaming, no need to wait with that :)
>>
>> Advantages
>> -we stop starting the stream from poll *now*
>> -for select we only set an fd in read or write fds, depending
>>    on the devices capabilities + file rights, rather then both
>
> Nobody ever sets both, why would you do that?
>

True, still the fact that POLLERR would result in the fd getting
set in both sets if the fd is specified in both also pleas against
using POLLERR

>> -for poll we don't return POLLERR, potentially breaking apps
>>    (I highly doubt broken apps will be fixed as quickly as you
>>     want, skype has yet to move to libv4l ...)
>
> I'm happy with that, but Mauro isn't.

I thought Mauro was against returning POLLERR when not streaming,
since this may break apps, if we return POLLIN or POLLOUT, the app
will call read() or write() and we can do the starting of the
stream there (and then wait or return -EAGAIN).

This way apps who don't like POLLERR won't break, AFAIK that was
Mauro's main point that POLLERR might break apps, but I could be
wrong there, Mauro ?

Regards,

Hans

Re: RFC: poll behavior

[Hans Verkuil]

On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> Hi,
> 
> On 06/29/2011 02:42 PM, Hans Verkuil wrote:
> > On Wednesday, June 29, 2011 14:10:44 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 06/29/2011 01:26 PM, Hans Verkuil wrote:
> 
> <Snip>
> 
> >>> 4) Proposal to change the poll behavior
> >>>
> >>> For the short term I propose that condition c is handled as follows:
> >>>
> >>> If for the filehandle passed to poll() no events have been subscribed, then
> >>> keep the old behavior (i.e. start streaming). If events have been subscribed,
> >>> however, then implement the new behavior (return POLLERR).
> >>>
> >>
> >> If events have been subscribed and no events are pending then the right
> >> behavior would be to return 0, not POLLERR, otherwise a waiting app
> >> will return from the poll immediately, or am I missing something?
> >
> > Yes and no. For select() POLLERR is ignored if you are only waiting for POLLPRI.
> >
> > But I see that that does not happen for the poll(2) API (see do_pollfd() in
> > fs/select.c). This means that POLLERR is indeed not a suitable event it
> > return. It will have to be POLLIN or POLLOUT instead.
> >
> > This is actually a real problem with poll(2): if there is no streaming in progress
> > and the driver does not support r/w, and you want to poll for just POLLPRI, then
> > POLLERR will be set, and poll(2) will always return. But select(2) will work fine.
> >
> > In other words, poll(2) and select(2) handle POLLPRI differently with respect to
> > POLLERR. What a mess. You can't really return POLLERR and support POLLPRI at the
> > same time.
> >
> 
> Ok, yet more reason to go with my proposal, but then simplified to:
> 
> When streaming has not started return POLLIN or POLLOUT (or-ed with
> POLLPRI if events are pending).

So would this be what you are looking for:

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 6ba1461..a3ce5a3 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -1371,35 +1371,37 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
  */
 unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 {
+	struct video_device *vfd = video_devdata(file);
 	unsigned long flags;
 	unsigned int ret;
 	struct vb2_buffer *vb = NULL;
+	bool have_events = false;
+	unsigned int res = 0;
+
+	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
+		struct v4l2_fh *fh = file->private_data;
+
+		/* Is this file handle subscribed to any events? */
+		have_events = fh->events != NULL;
+		if (have_events && v4l2_event_pending(fh))
+			res = POLLPRI;
+	} 
 
 	/*
 	 * Start file I/O emulator only if streaming API has not been used yet.
 	 */
 	if (q->num_buffers == 0 && q->fileio == NULL) {
-		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
-			ret = __vb2_init_fileio(q, 1);
-			if (ret)
-				return POLLERR;
-		}
-		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
-			ret = __vb2_init_fileio(q, 0);
-			if (ret)
-				return POLLERR;
-			/*
-			 * Write to OUTPUT queue can be done immediately.
-			 */
-			return POLLOUT | POLLWRNORM;
-		}
+		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
+			return res | POLLIN | POLLRDNORM;
+		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
+			return res | POLLOUT | POLLWRNORM;
 	}
 
 	/*
 	 * There is nothing to wait for if no buffers have already been queued.
 	 */
 	if (list_empty(&q->queued_list))
-		return POLLERR;
+		return have_events ? res : POLLERR;
 
 	poll_wait(file, &q->done_wq, wait);
 
@@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 
 	if (vb && (vb->state == VB2_BUF_STATE_DONE
 			|| vb->state == VB2_BUF_STATE_ERROR)) {
-		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
+		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
 			POLLIN | POLLRDNORM;
 	}
-	return 0;
+	return res;
 }
 EXPORT_SYMBOL_GPL(vb2_poll);
 

One note: the only time POLLERR is now returned is if no buffers have been queued
and no events have been subscribed to. I think that qualifies as an error condition.
I am not 100% certain, though.

Comments?

	Hans

Re: RFC: poll behavior

[Hans de Goede]

Hi,

On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:

<snip>

>> Ok, yet more reason to go with my proposal, but then simplified to:
>>
>> When streaming has not started return POLLIN or POLLOUT (or-ed with
>> POLLPRI if events are pending).
>
> So would this be what you are looking for:
>

Yes, although I do have some comments:

> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index 6ba1461..a3ce5a3 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -1371,35 +1371,37 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
>    */
>   unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>   {
> +	struct video_device *vfd = video_devdata(file);
>   	unsigned long flags;
>   	unsigned int ret;
>   	struct vb2_buffer *vb = NULL;
> +	bool have_events = false;
> +	unsigned int res = 0;
> +
> +	if (test_bit(V4L2_FL_USES_V4L2_FH,&vfd->flags)) {
> +		struct v4l2_fh *fh = file->private_data;
> +
> +		/* Is this file handle subscribed to any events? */
> +		have_events = fh->events != NULL;
> +		if (have_events && v4l2_event_pending(fh))
> +			res = POLLPRI;
> +	}
>
>   	/*
>   	 * Start file I/O emulator only if streaming API has not been used yet.
>   	 */

Comment needs to be changed to match the new code.

>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> -			ret = __vb2_init_fileio(q, 1);
> -			if (ret)
> -				return POLLERR;
> -		}
> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> -			ret = __vb2_init_fileio(q, 0);
> -			if (ret)
> -				return POLLERR;
> -			/*
> -			 * Write to OUTPUT queue can be done immediately.
> -			 */
> -			return POLLOUT | POLLWRNORM;
> -		}
> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> +			return res | POLLIN | POLLRDNORM;
> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> +			return res | POLLOUT | POLLWRNORM;
>   	}
>
>   	/*
>   	 * There is nothing to wait for if no buffers have already been queued.
>   	 */
>   	if (list_empty(&q->queued_list))
> -		return POLLERR;
> +		return have_events ? res : POLLERR;
>

This seems more accurate to me, given that in case of select the 2 influence
different fd sets:

		return res | POLLERR;

>   	poll_wait(file,&q->done_wq, wait);
>
> @@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>
>   	if (vb&&  (vb->state == VB2_BUF_STATE_DONE
>   			|| vb->state == VB2_BUF_STATE_ERROR)) {
> -		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> +		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
>   			POLLIN | POLLRDNORM;

I would prefer to see this as:
		res |= (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
			POLLIN | POLLRDNORM;


>   	}
> -	return 0;
> +	return res;
>   }
>   EXPORT_SYMBOL_GPL(vb2_poll);
>
>
> One note: the only time POLLERR is now returned is if no buffers have been queued
> and no events have been subscribed to. I think that qualifies as an error condition.
> I am not 100% certain, though.

I think it would be better to simply wait (iow return 0) then. I know that
gstreamer for example uses separate consumer and producer threads, so it is
possible for the producer thread to wait in select while all buffers have been
handed to the (lagging) consumer thread, once the consumer thread has consumed
a buffer it will queue it, and once filled the select will return it to
the producer thread, who shoves it into the pipeline again, etc.

Regards,

Hans

Re: RFC: poll behavior

[Hans Verkuil]

On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
> Hi,
> 
> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> > On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> 
> <snip>
> 
> >   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> > -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> > -			ret = __vb2_init_fileio(q, 1);
> > -			if (ret)
> > -				return POLLERR;
> > -		}
> > -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> > -			ret = __vb2_init_fileio(q, 0);
> > -			if (ret)
> > -				return POLLERR;
> > -			/*
> > -			 * Write to OUTPUT queue can be done immediately.
> > -			 */
> > -			return POLLOUT | POLLWRNORM;
> > -		}
> > +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> > +			return res | POLLIN | POLLRDNORM;
> > +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> > +			return res | POLLOUT | POLLWRNORM;
> >   	}
> >
> >   	/*
> >   	 * There is nothing to wait for if no buffers have already been 
queued.
> >   	 */
> >   	if (list_empty(&q->queued_list))
> > -		return POLLERR;
> > +		return have_events ? res : POLLERR;
> >
> 
> This seems more accurate to me, given that in case of select the 2 influence
> different fd sets:
> 
> 		return res | POLLERR;

Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
even if you only want to wait on POLLPRI. That's a perfectly valid thing to 
do. An alternative is to just not use POLLERR and return res|POLLIN or res|
POLLOUT depending on V4L2_TYPE_IS_OUTPUT().

Another option is to just return res (which is your suggestion below as well).
I think this is also a reasonable approach. It would in fact allow one thread 
to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
filehandle. And the other thread would return from poll(2) as soon as the 
first frame becomes available.

This also leads to another ambiguity with poll(): what should poll do if 
another filehandle started streaming? So fh1 called STREAMON (and so becomes 
the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
should fh2 wake up? Is fh2 allowed to call DQBUF?

To be honest, I think vb2 should keep track of the filehandle that started 
streaming rather than leaving that to drivers, but that's a separate issue.

I really wonder whether we should ever use POLLERR at all: it is extremely
vague how it should be interpreted, and it doesn't actually tell you what is 
wrong. And is it really an error if you poll on a non-streaming node?

As shown by the use-case above, I don't think it is an error at all.

The default poll mask that is returned when the device doesn't support poll
is #define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM).

So if they don't return POLLERR for such devices, perhaps we shouldn't either.

Regards,

	Hans

> 
> >   	poll_wait(file,&q->done_wq, wait);
> >
> > @@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct 
file *file, poll_table *wait)
> >
> >   	if (vb&&  (vb->state == VB2_BUF_STATE_DONE
> >   			|| vb->state == VB2_BUF_STATE_ERROR)) {
> > -		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> > +		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | 
POLLWRNORM :
> >   			POLLIN | POLLRDNORM;
> 
> I would prefer to see this as:
> 		res |= (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> 			POLLIN | POLLRDNORM;
> 
> 
> >   	}
> > -	return 0;
> > +	return res;
> >   }
> >   EXPORT_SYMBOL_GPL(vb2_poll);
> >
> >
> > One note: the only time POLLERR is now returned is if no buffers have been 
queued
> > and no events have been subscribed to. I think that qualifies as an error 
condition.
> > I am not 100% certain, though.
> 
> I think it would be better to simply wait (iow return 0) then. I know that
> gstreamer for example uses separate consumer and producer threads, so it is
> possible for the producer thread to wait in select while all buffers have 
been
> handed to the (lagging) consumer thread, once the consumer thread has 
consumed
> a buffer it will queue it, and once filled the select will return it to
> the producer thread, who shoves it into the pipeline again, etc.
> 
> Regards,
> 
> Hans
> 
> 

Re: RFC: poll behavior

[Mauro Carvalho Chehab]

Em 29-06-2011 09:10, Hans de Goede escreveu:
> Hi,
> 
> On 06/29/2011 01:26 PM, Hans Verkuil wrote:
>> Hi all,
>>
>> This RFC is based on recent discussions with regards to how the poll function
>> in a V4L2 driver should behave.
>>
>> Some relevant documents:
>>
>> POSIX:
>>
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
>>
>> V4L2 Spec:
>>
>> http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
>>
>> I've copied some descriptions from the POSIX poll document below:
>>
>> POLLIN
>> Data other than high-priority data may be read without blocking.
>>
>> POLLERR
>> An error has occurred on the device or stream. This flag is only valid in the
>> revents bitmask; it shall be ignored in the events member.
>>
>> POLLHUP
>> The device has been disconnected. This event and POLLOUT are mutually-
>> exclusive; a stream can never be writable if a hangup has occurred. However,
>> this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-
>> exclusive. This flag is only valid in the revents bitmask; it shall be ignored
>> in the events member.
>>
>>
>>
>> 1) How *should* it work?
>>
>> Suppose we could redesign the way V4L2 handles poll(), how should it work?
>>
>> There are three cases:
>>
>> a) poll() called when streaming is in progress.
>>
>> This is easy: if a buffer is available, then return POLLIN (or POLLOUT for
>> output). If an event has arrived, then OR with POLLPRI.
>>
>> b) poll() called when no streaming has started and the driver does not support
>> the read/write API.
>>
>> In this case poll() should return POLLERR, since calling poll() when no
>> streaming is in progress makes no sense. POLLHUP seems an interesting
>> alternative, but other than for end-of-file or end-of-stream conditions it's
>> meaning is very fuzzy. Linux also has a POLLRDHUP to make things even more
>> confusing. So I would just stick with POLLERR.
>>
>> c) poll() called when no streaming has started and the driver supports the
>> read/write API.
>>
>> I see no reason why we would do anything different from b). Calling poll()
>> when there is no streaming in progress clearly means that there is no data
>> available and that there won't be any data either until streaming starts
>> somehow.

This is where we have different a different understanding. On all cases outside V4L that
I know, calling poll (or select) before a read is a valid operation and doesn't
return POLLERR, except when there's really an error.

Userspace applications assume that, as they generally open a device and calls
poll/select to check if they can read or write into it.

The V4L2 API seems to understand it at the same way, as it says that calling poll()
will start DMA, if the mmap mode is not selected.

It should be noticed that there's no splicit command to start DMA via poll/select/read
method. So, things like:
	cat /dev/video0
	tail -f /var/video0
	mplayer /dev/video0
	...

needs to keep working. Requiring a read() to start streaming may break some
applications.

>>
>>
>> 2) How does it work today?
>>
>> a) poll() called when streaming is in progress.
>>
>> We follow the procedure outlined above. So this is fine.
>>
>> b) poll() called when no streaming has started and the driver does not support
>> the read/write API.
>>
>> In this case POLLERR is returned. So this is also fine.
>>
>> c) poll() called when no streaming has started and the driver supports the
>> read/write API.
>>
>> Here things are different. poll() will in this case start streaming
>> automatically and either return POLLERR if there was an error when starting
>> streaming, or return 0 so it will wait until the first buffer arrives.
> 
> Right, to clarify, in this case the drivers assume the app wants r/w mode and
> streaming is started for r/w mode, not in mmap mode even if the drivers support
> both.

Yes. There is one API violation in this case: poll() is waiting for the stream to
start on non-block mode.

The right thing would be to defer a working thread to start streaming and return
0 while there's no buffer available, in non-block mode.

>> The main problem is that it will also do this if the application only wants to
>> poll for exceptions (POLLPRI). Unfortunately there is no reliable way in the
>> kernel to discover whether the application wanted to poll for POLLIN or just
>> for POLLPRI. This behavior simply kills applications that want to wait for
>> e.g. changes in controls without actually starting streaming. An example is
>> qv4l2.

This is the exception usecase. 

On most cases, the same application controls streaming, events, etc. So, such 
type of conflicts don't exist, in practice. It might make some sense to have
an ioctl that would restrict a "spy only" type of application to actually 
change anything.

One alternative would be to open the device in read only mode. We'll probably 
need to fix some things in order to respect the read-only permission, but it 
seems to cover such usecase.

>>
>> Another problem is that starting streaming from within poll can take a long
>> time depending on the hardware. And poll isn't supposed to take a long time
>> (exceptions are calling poll for userspace file systems). Actually, until
>> 2.6.29 sleeping inside poll() wasn't even supported.
>>
>> In my opinion poll should never start the capture process. There is nothing in
>> the poll POSIX document that says that it should. It complicates drivers and
>> it is an ugly side-effect.
>>
> 
> Agreed, even more important then the timing issue, poll should simply not
> have side effects, and starting the stream is a rather large side effect!

Starting stream is not a side effect. if you use poll() for a tty device of
for a network socket, for example, the same thing will happen: the socket/tty
doesn't require a read() or write() to be ready to receive/transmit.

>> 3) What are the consequences if we changed the behavior of c?
>>
>> Suppose we just changed the current behavior to the desired behavior. What
>> would change for applications?

We can't change applications like "cat, tail, ...". We need to stick with POSIX
in this case.

>> Obviously, nothing will change for drivers that do not support read/write.
>>
>> For the others there will be a difference depending on whether the
>> applications uses select(2) or poll(2). The select() function hides the exact
>> event masked returned by the driver, so POLLERR will not show up as such,
>> instead select() will just return and set the input or output mask of the
>> corresponding fd. So there is no difference between POLLERR and POLLIN or
>> POLLOUT as seen by the application.
>>
>> What will change is when read() is called: if it is in non-blocking mode, then
>> it will start the streaming and return -EAGAIN. If it is in blocking mode,
>> then there is no visible change in behavior. So drivers must be able to handle
>> that error code.
>>
>> However, all applications must be able to handle that error anyway. It's part
>> of the POSIX specification of a non-blocking read().
> 
> Note that the only app I know of which actually does a select() *before* its
> first read() is:
> http://v4l2spec.bytesex.org/spec/capture-example.html

coreutils "tail" does that also:

http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/tail.c

see line 1467. It should noticed that it uses inotify API to monitor the
file descriptors. 

> I think this is the only one as libv4l did not handle this case
> correctly for a long time, until someone complained that our example own
> code did not work with libv4l. But someone may have copied our example
> code somewhere ...

People that use read interface generally don't use libv4l. They generally just
call some program to work directly into the device node, like:
	mplayer /dev/video0
or
	tail -f /var/video0 |nc [some IP address]
or even
	nc [some ip address] < /dev/video0

(This is a typical usage for people with devices like ivtv and pvrusb2, that
produces mpeg streams directly).

>> For applications that call poll(2) there will be a difference depending on
>> what they do when they see a POLLERR: if they just ignore it and call read,
>> then it is effectively identical to the select() case.
>>
>> If they actually test for POLLERR and return an error, then this change will
>> break such an application.
>>
>> So the only time that a well-written application would break is when it uses
>> poll(2) and read(2), and returns an error if POLLERR was set instead of just
>> calling read().
> 
> Right, I don't think such an application exists not even a proprietary one,
> most apps tend to stick with select rather then use poll. and most v4l apps
> tend to be a bit old anyways, so...
> 
> Hmm, then again this may bite gstreamer in the case of r/w
> mode only devices.
> 
>> 4) Proposal to change the poll behavior
>>
>> For the short term I propose that condition c is handled as follows:
>>
>> If for the filehandle passed to poll() no events have been subscribed, then
>> keep the old behavior (i.e. start streaming). If events have been subscribed,
>> however, then implement the new behavior (return POLLERR).
>>
> 
> If events have been subscribed and no events are pending then the right
> behavior would be to return 0, not POLLERR, otherwise a waiting app
> will return from the poll immediately, or am I missing something?

Yes. POLLERR makes the poll() useless, as it will just return immediately to
the userspace, wasting CPU on an endless loop of poll()/read() without actually
sleeping.

>> Since events are new I think this is a safe change that will not affect
>> existing applications.
>>
>> In the long term I propose that we put this in the feature-removal-schedule
>> for v3.3 or so and stop poll from starting streaming altogether.
> 
> Alternative suggestion:
> 
> In scenario c + there are no events subscribed simply return POLL_IN /
> POLL_OUT, iow signal the app, sure you're free to do a read / write to
> start streaming, no need to wait with that :)
> 
> Advantages
> -we stop starting the stream from poll *now*

This violates V4L2 API specs.

> -for select we only set an fd in read or write fds, depending
>  on the devices capabilities + file rights, rather then both

This already happens. If not, it is a driver bug.

> -for poll we don't return POLL_ERR, potentially breaking apps
>  (I highly doubt broken apps will be fixed as quickly as you
>   want, skype has yet to move to libv4l ...)

POLLERR should only be rised if read or write would return an error.

> Disadvantages:
> -if an app falls in scenario c, and uses poll, and non blocking
>  mode and does not handle EAGAIN from read, the app breaks

- While DMA is not started, the userspace application will
execute a non-sleeping endless loop of: poll()/read(), as poll
will keep returning POLLIN, while the device is not ready.

The proper behavior is to return 0, if the data is not ready
for read.

> As said already not handling EAGAIN in non blocking mode is
> against posix...
> 
> Regards,
> 
> Hans
> -- 
> 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: RFC: poll behavior

[Mauro Carvalho Chehab]

Em 30-06-2011 10:46, Hans Verkuil escreveu:
> On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
>> Hi,
>>
>> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
>>> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
>>
>> <snip>
>>
>>>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
>>> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
>>> -			ret = __vb2_init_fileio(q, 1);
>>> -			if (ret)
>>> -				return POLLERR;
>>> -		}
>>> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
>>> -			ret = __vb2_init_fileio(q, 0);
>>> -			if (ret)
>>> -				return POLLERR;
>>> -			/*
>>> -			 * Write to OUTPUT queue can be done immediately.
>>> -			 */
>>> -			return POLLOUT | POLLWRNORM;
>>> -		}
>>> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
>>> +			return res | POLLIN | POLLRDNORM;
>>> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
>>> +			return res | POLLOUT | POLLWRNORM;

It is wrong to return POLLIN/POLLOUT if the stream hasn't started yet. You should
return it only when data is ready. Otherwise you should return 0.

>>>   	}
>>>
>>>   	/*
>>>   	 * There is nothing to wait for if no buffers have already been 
> queued.
>>>   	 */
>>>   	if (list_empty(&q->queued_list))
>>> -		return POLLERR;
>>> +		return have_events ? res : POLLERR;
>>>
>>
>> This seems more accurate to me, given that in case of select the 2 influence
>> different fd sets:
>>
>> 		return res | POLLERR;
> 
> Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
> even if you only want to wait on POLLPRI.

Yes, but this is the right thing to do: an error condition has happened. If you're
in doubt, think that poll() is being used for a text file or a socket: if the connection
has dropped, or there's a problem to access the file, poll() needs to return, as there is
a condition error that needs to be handled.

> That's a perfectly valid thing to 
> do. An alternative is to just not use POLLERR and return res|POLLIN or res|
> POLLOUT depending on V4L2_TYPE_IS_OUTPUT().

You should only rise POLLERR if a problem happened at the events delivery or at
the device streaming.

> Another option is to just return res (which is your suggestion below as well).
> I think this is also a reasonable approach. It would in fact allow one thread 
> to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
> filehandle. And the other thread would return from poll(2) as soon as the 
> first frame becomes available.
> 
> This also leads to another ambiguity with poll(): what should poll do if 
> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> should fh2 wake up? Is fh2 allowed to call DQBUF?

IMO, both fh's should get the same results. This is what happens if you're
writing into a file and two or more processes are selecting at the EOF.

Anyway, changing from the current behavior may break applications.

> To be honest, I think vb2 should keep track of the filehandle that started 
> streaming rather than leaving that to drivers, but that's a separate issue.
> 
> I really wonder whether we should ever use POLLERR at all: it is extremely
> vague how it should be interpreted, and it doesn't actually tell you what is 
> wrong. And is it really an error if you poll on a non-streaming node?

See above. You need to rise it if, for example, an error occurred, and no data
will be ready for read(), write() or DQEVENT. That's the reason why POLLERR
exists.

Cheers,
Mauro

Re: RFC: poll behavior

[Hans Verkuil]

On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> Em 30-06-2011 10:46, Hans Verkuil escreveu:
> > On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> >>> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> >>
> >> <snip>
> >>
> >>>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> >>> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> >>> -			ret = __vb2_init_fileio(q, 1);
> >>> -			if (ret)
> >>> -				return POLLERR;
> >>> -		}
> >>> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> >>> -			ret = __vb2_init_fileio(q, 0);
> >>> -			if (ret)
> >>> -				return POLLERR;
> >>> -			/*
> >>> -			 * Write to OUTPUT queue can be done immediately.
> >>> -			 */
> >>> -			return POLLOUT | POLLWRNORM;
> >>> -		}
> >>> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> >>> +			return res | POLLIN | POLLRDNORM;
> >>> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> >>> +			return res | POLLOUT | POLLWRNORM;
> 
> It is wrong to return POLLIN/POLLOUT if the stream hasn't started yet. You should
> return it only when data is ready. Otherwise you should return 0.
> 
> >>>   	}
> >>>
> >>>   	/*
> >>>   	 * There is nothing to wait for if no buffers have already been 
> > queued.
> >>>   	 */
> >>>   	if (list_empty(&q->queued_list))
> >>> -		return POLLERR;
> >>> +		return have_events ? res : POLLERR;
> >>>
> >>
> >> This seems more accurate to me, given that in case of select the 2 influence
> >> different fd sets:
> >>
> >> 		return res | POLLERR;
> > 
> > Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
> > even if you only want to wait on POLLPRI.
> 
> Yes, but this is the right thing to do: an error condition has happened. If you're
> in doubt, think that poll() is being used for a text file or a socket: if the connection
> has dropped, or there's a problem to access the file, poll() needs to return, as there is
> a condition error that needs to be handled.
> 
> > That's a perfectly valid thing to 
> > do. An alternative is to just not use POLLERR and return res|POLLIN or res|
> > POLLOUT depending on V4L2_TYPE_IS_OUTPUT().
> 
> You should only rise POLLERR if a problem happened at the events delivery or at
> the device streaming.
> 
> > Another option is to just return res (which is your suggestion below as well).
> > I think this is also a reasonable approach. It would in fact allow one thread 
> > to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
> > filehandle. And the other thread would return from poll(2) as soon as the 
> > first frame becomes available.
> > 
> > This also leads to another ambiguity with poll(): what should poll do if 
> > another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> > the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> > should fh2 wake up? Is fh2 allowed to call DQBUF?
> 
> IMO, both fh's should get the same results. This is what happens if you're
> writing into a file and two or more processes are selecting at the EOF.

Yes, but multiple filehandles are allowed to write/read from a file at the
same time. That's not true for V4L2. Only one filehandle can do I/O at a time.

I'm going to look into changing fs/select.c so that the poll driver function
can actually see the event mask provided by the application.

Regards,

	Hans

> 
> Anyway, changing from the current behavior may break applications.
> 
> > To be honest, I think vb2 should keep track of the filehandle that started 
> > streaming rather than leaving that to drivers, but that's a separate issue.
> > 
> > I really wonder whether we should ever use POLLERR at all: it is extremely
> > vague how it should be interpreted, and it doesn't actually tell you what is 
> > wrong. And is it really an error if you poll on a non-streaming node?
> 
> See above. You need to rise it if, for example, an error occurred, and no data
> will be ready for read(), write() or DQEVENT. That's the reason why POLLERR
> exists.
> 
> Cheers,
> Mauro
> --
> 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: RFC: poll behavior

[Mauro Carvalho Chehab]

Em 01-07-2011 06:45, Hans Verkuil escreveu:
> On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:

>>> This also leads to another ambiguity with poll(): what should poll do if 
>>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
>>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
>>> should fh2 wake up? Is fh2 allowed to call DQBUF?
>>
>> IMO, both fh's should get the same results. This is what happens if you're
>> writing into a file and two or more processes are selecting at the EOF.
> 
> Yes, but multiple filehandles are allowed to write/read from a file at the
> same time. That's not true for V4L2. Only one filehandle can do I/O at a time.

Actually, this is not quite true currently, as you could, for example use one fd
for QBUF, and another for DQBUF, with the current behavior, but, with luck,
no applications are doing weird things like that. Yet, tests are needed to avoid
breaking something, if we're willing to change it.

> I'm going to look into changing fs/select.c so that the poll driver function
> can actually see the event mask provided by the application.

Why? A POLLERR should be notified, whatever mask is there, as the application
may need to abort (for example, in cases like hardware removal).

Mauro.

Re: RFC: poll behavior

[Mauro Carvalho Chehab]

Em 01-07-2011 08:59, Mauro Carvalho Chehab escreveu:
> Em 01-07-2011 06:45, Hans Verkuil escreveu:
>> On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> 
>>>> This also leads to another ambiguity with poll(): what should poll do if 
>>>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
>>>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
>>>> should fh2 wake up? Is fh2 allowed to call DQBUF?
>>>
>>> IMO, both fh's should get the same results. This is what happens if you're
>>> writing into a file and two or more processes are selecting at the EOF.
>>
>> Yes, but multiple filehandles are allowed to write/read from a file at the
>> same time. That's not true for V4L2. Only one filehandle can do I/O at a time.
> 
> Actually, this is not quite true currently, as you could, for example use one fd
> for QBUF, and another for DQBUF, with the current behavior, but, with luck,
> no applications are doing weird things like that. Yet, tests are needed to avoid
> breaking something, if we're willing to change it.
> 
>> I'm going to look into changing fs/select.c so that the poll driver function
>> can actually see the event mask provided by the application.
> 
> Why? A POLLERR should be notified, whatever mask is there, as the application
> may need to abort (for example, in cases like hardware removal).

I was too quick on my last comment. Your patch is clear: you want to start it only
if the poll mask has "in" or "out" file descriptiors. Ok, this makes sense to me.

Cheers,
Mauro

Re: RFC: poll behavior

[Hans Verkuil]

On Friday, July 01, 2011 13:59:14 Mauro Carvalho Chehab wrote:
> Em 01-07-2011 06:45, Hans Verkuil escreveu:
> > On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> 
> >>> This also leads to another ambiguity with poll(): what should poll do if 
> >>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> >>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> >>> should fh2 wake up? Is fh2 allowed to call DQBUF?
> >>
> >> IMO, both fh's should get the same results. This is what happens if you're
> >> writing into a file and two or more processes are selecting at the EOF.
> > 
> > Yes, but multiple filehandles are allowed to write/read from a file at the
> > same time. That's not true for V4L2. Only one filehandle can do I/O at a time.
> 
> Actually, this is not quite true currently, as you could, for example use one fd
> for QBUF, and another for DQBUF, with the current behavior, but, with luck,
> no applications are doing weird things like that. Yet, tests are needed to avoid
> breaking something, if we're willing to change it.

Many drivers prevent such things. But it is very much up to the driver. My
gut-feeling is that it is a 50-50 split between drivers that allow it (whether
it actually works is another matter) and drivers that prevent this and return
-EBUSY.

> > I'm going to look into changing fs/select.c so that the poll driver function
> > can actually see the event mask provided by the application.
> 
> Why? A POLLERR should be notified, whatever mask is there, as the application
> may need to abort (for example, in cases like hardware removal).

If an application isn't interested in POLLIN or POLLOUT, but just POLLPRI, then
poll() doesn't need to start streaming.

It obviously makes no sense to start streaming if the application isn't polling
for input.

Regards,

	Hans