Re: RFC: poll behavior
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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:
> >>
> >>
> >>
> >>> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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:
>>
>>
>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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
Re: RFC: poll behavior
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:
>
>
>
> > 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
Hi,
On 06/29/2011 03:43 PM, Hans Verkuil wrote:
On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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:
>
>
>
> >>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media"
Re: RFC: poll behavior
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: 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: poll behavior
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
Re: RFC: poll behavior
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
RFC: poll behavior
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-w
