| Message ID | 201106291543.51271.hansverk@cisco.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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 -- 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
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 -- 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
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 > > -- 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
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);