| Message ID | f1a14e0985ddaa053e45522fe7bbdfae56057ec2.1307458245.git.hans.verkuil@cisco.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Em 07-06-2011 12:05, Hans Verkuil escreveu: > From: Hans Verkuil <hans.verkuil@cisco.com> > > The vb2_poll function would start read-DMA if called without any streaming > in progress. This unfortunately does not work if the application just wants > to poll for exceptions. This information of what the application is polling > for is sadly unavailable in the driver. > > Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first > call to read() start the DMA. This initial read() call will return EAGAIN > since no actual data is available yet, but it does start the DMA. > > Applications must handle EAGAIN in any case since there can be other reasons > for EAGAIN as well. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/video/videobuf2-core.c | 17 +++-------------- > 1 files changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 6ba1461..ad75c95 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q); > unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) > { > unsigned long flags; > - unsigned int ret; > struct vb2_buffer *vb = NULL; > > /* > * 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. > - */ > + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) > + return POLLIN | POLLRDNORM; > + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) > return POLLOUT | POLLWRNORM; > - } > } There is one behavior change on this patchset: __vb2_init_fileio() checks for some troubles that may happen during device streaming initialization, returning POLLERR if there is a problem there. By moving this code to be called only at read, it means the poll() will not fail anymore, but the first read() will fail. The man page for read() doesn't tell that -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is clearly violating kernel ABI. NACK. 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 Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote: > Em 07-06-2011 12:05, Hans Verkuil escreveu: > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > The vb2_poll function would start read-DMA if called without any streaming > > in progress. This unfortunately does not work if the application just wants > > to poll for exceptions. This information of what the application is polling > > for is sadly unavailable in the driver. > > > > Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first > > call to read() start the DMA. This initial read() call will return EAGAIN > > since no actual data is available yet, but it does start the DMA. > > > > Applications must handle EAGAIN in any case since there can be other reasons > > for EAGAIN as well. > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > drivers/media/video/videobuf2-core.c | 17 +++-------------- > > 1 files changed, 3 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > > index 6ba1461..ad75c95 100644 > > --- a/drivers/media/video/videobuf2-core.c > > +++ b/drivers/media/video/videobuf2-core.c > > @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q); > > unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) > > { > > unsigned long flags; > > - unsigned int ret; > > struct vb2_buffer *vb = NULL; > > > > /* > > * 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. > > - */ > > + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) > > + return POLLIN | POLLRDNORM; > > + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) > > return POLLOUT | POLLWRNORM; > > - } > > } > > There is one behavior change on this patchset: __vb2_init_fileio() checks for some > troubles that may happen during device streaming initialization, returning POLLERR > if there is a problem there. > > By moving this code to be called only at read, it means the poll() will not fail > anymore, but the first read() will fail. The man page for read() doesn't tell that > -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is > clearly violating kernel ABI. > > NACK. Actually, almost nothing changes in the ABI. It's counterintuitive, but this is what happens: 1) The poll() function in a driver does not actually return any error codes. It never did. It returns a poll mask, which is expected to be POLLERR in case of any error. All that it does is that select() returns if it waits for reading or writing. Any actual error codes are never propagated. This means BTW that our poll manual page is wrong (what a surprise), most of those error codes can never be returned. 2) Suppose we try to start streaming in poll. If this works, then poll returns, with POLLIN set, and the next read() will succeed (actually, it can return an error as well, but for other error conditions in case of e.g. hardware errors). The problem with this is of course that this will also start the streaming if all we wanted to wait for was an exception. That's not what we want at all. Ideally we could inspect in the driver what the caller wanted to wait for, but that information is not available, unfortunately. 3) The other case is that we try to start streaming in poll and it fails. In that case any errors are lost and poll returns POLLERR (note that the poll(2) manual page says that POLLERR is for output only, but clearly in the linux kernel it is accepted both input and output). But for the select() call this POLLERR information is lost as well and the application will call read() anyway, which now will attempt to start the streaming (again after poll() tried it the first time) and this time it returns the actual error code. Just try this with capture-example: start it in mmap mode, then try to run it in read mode from a second console. The EBUSY error comes from the read() command, not from select(). With or without this patch, capture-example behaves exactly the same. The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no longer returned. Since this isn't documented at all anyway (and the poll(2) manual page is actually unclear about whether you can expect it), I am actually quite happy with this change. After this analysis I realized it is even better than expected. I never liked that poll starts streaming, poll should be a fast function, not something that does a lot of other things. It's actually a very nice change, but I admit that it is tricky to analyze. 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
On Tuesday, June 28, 2011 09:33:57 Hans Verkuil wrote: > On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote: > > Em 07-06-2011 12:05, Hans Verkuil escreveu: > > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > > > The vb2_poll function would start read-DMA if called without any streaming > > > in progress. This unfortunately does not work if the application just wants > > > to poll for exceptions. This information of what the application is polling > > > for is sadly unavailable in the driver. > > > > > > Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first > > > call to read() start the DMA. This initial read() call will return EAGAIN > > > since no actual data is available yet, but it does start the DMA. > > > > > > Applications must handle EAGAIN in any case since there can be other reasons > > > for EAGAIN as well. > > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > > --- > > > drivers/media/video/videobuf2-core.c | 17 +++-------------- > > > 1 files changed, 3 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > > > index 6ba1461..ad75c95 100644 > > > --- a/drivers/media/video/videobuf2-core.c > > > +++ b/drivers/media/video/videobuf2-core.c > > > @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q); > > > unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) > > > { > > > unsigned long flags; > > > - unsigned int ret; > > > struct vb2_buffer *vb = NULL; > > > > > > /* > > > * 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. > > > - */ > > > + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) > > > + return POLLIN | POLLRDNORM; > > > + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) > > > return POLLOUT | POLLWRNORM; > > > - } > > > } > > > > There is one behavior change on this patchset: __vb2_init_fileio() checks for some > > troubles that may happen during device streaming initialization, returning POLLERR > > if there is a problem there. > > > > By moving this code to be called only at read, it means the poll() will not fail > > anymore, but the first read() will fail. The man page for read() doesn't tell that > > -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is > > clearly violating kernel ABI. > > > > NACK. > > Actually, almost nothing changes in the ABI. It's counterintuitive, but > this is what happens: > > 1) The poll() function in a driver does not actually return any error codes. > It never did. It returns a poll mask, which is expected to be POLLERR in case > of any error. All that it does is that select() returns if it waits for reading > or writing. Any actual error codes are never propagated. This means BTW that > our poll manual page is wrong (what a surprise), most of those error codes can > never be returned. > > 2) Suppose we try to start streaming in poll. If this works, then poll returns, > with POLLIN set, and the next read() will succeed (actually, it can return an > error as well, but for other error conditions in case of e.g. hardware errors). > > The problem with this is of course that this will also start the streaming if > all we wanted to wait for was an exception. That's not what we want at all. > Ideally we could inspect in the driver what the caller wanted to wait for, but > that information is not available, unfortunately. > > 3) The other case is that we try to start streaming in poll and it fails. > In that case any errors are lost and poll returns POLLERR (note that the poll(2) > manual page says that POLLERR is for output only, but clearly in the linux > kernel it is accepted both input and output). > > But for the select() call this POLLERR information is lost as well and the > application will call read() anyway, which now will attempt to start the > streaming (again after poll() tried it the first time) and this time it > returns the actual error code. > > Just try this with capture-example: start it in mmap mode, then try to run > it in read mode from a second console. The EBUSY error comes from the read() > command, not from select(). With or without this patch, capture-example > behaves exactly the same. > > The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no > longer returned. Since this isn't documented at all anyway (and the poll(2) > manual page is actually unclear about whether you can expect it), I am > actually quite happy with this change. After this analysis I realized it is > even better than expected. > > I never liked that poll starts streaming, poll should be a fast function, > not something that does a lot of other things. > > It's actually a very nice change, but I admit that it is tricky to analyze. Just two more remarks: POLLERR as a poll return code is pretty useless: applications can't use it to determine what the error is. There is a big difference between a ENOMEM or a EBUSY. The only way to find out is to do a read(). Another problem I have with starting streaming in poll() is that starting streaming can take a long time depending on the hardware. That's really not what poll() is for. If this fd is part of a larger set with e.g. socket fds, then you really don't want to have the event polling thread block for 10s of milliseconds. It's a very poor design. Changing this part doesn't affect applications as far as I can see, but it makes poll() much simpler and usable again when you just want to poll for events. Note also that this change only affects applications that use the poll API as opposed to select(), test for POLLERR in revents, use read() instead of the streaming API and do not test for read() errors. That's one API change I'm happy to make. 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 28-06-2011 04:33, Hans Verkuil escreveu: > On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote: >> Em 07-06-2011 12:05, Hans Verkuil escreveu: >>> From: Hans Verkuil <hans.verkuil@cisco.com> >>> >>> The vb2_poll function would start read-DMA if called without any streaming >>> in progress. This unfortunately does not work if the application just wants >>> to poll for exceptions. This information of what the application is polling >>> for is sadly unavailable in the driver. >>> >>> Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first >>> call to read() start the DMA. This initial read() call will return EAGAIN >>> since no actual data is available yet, but it does start the DMA. >>> >>> Applications must handle EAGAIN in any case since there can be other reasons >>> for EAGAIN as well. >>> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>> --- >>> drivers/media/video/videobuf2-core.c | 17 +++-------------- >>> 1 files changed, 3 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >>> index 6ba1461..ad75c95 100644 >>> --- a/drivers/media/video/videobuf2-core.c >>> +++ b/drivers/media/video/videobuf2-core.c >>> @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q); >>> unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) >>> { >>> unsigned long flags; >>> - unsigned int ret; >>> struct vb2_buffer *vb = NULL; >>> >>> /* >>> * 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. >>> - */ >>> + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) >>> + return POLLIN | POLLRDNORM; >>> + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) >>> return POLLOUT | POLLWRNORM; >>> - } >>> } >> >> There is one behavior change on this patchset: __vb2_init_fileio() checks for some >> troubles that may happen during device streaming initialization, returning POLLERR >> if there is a problem there. >> >> By moving this code to be called only at read, it means the poll() will not fail >> anymore, but the first read() will fail. The man page for read() doesn't tell that >> -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is >> clearly violating kernel ABI. >> >> NACK. > > Actually, almost nothing changes in the ABI. It's counterintuitive, but > this is what happens: > > 1) The poll() function in a driver does not actually return any error codes. > It never did. It returns a poll mask, which is expected to be POLLERR in case > of any error. All that it does is that select() returns if it waits for reading > or writing. Any actual error codes are never propagated. Yes, but POLLERR will return error on the vb2 cases that return -EBUSY or -ENOMEM. This doesn't violate the ioctl ABI. > This means BTW that > our poll manual page is wrong (what a surprise), most of those error codes can > never be returned. True. The DocBook needs a fix. Posix describes it as: http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html > > 2) Suppose we try to start streaming in poll. If this works, then poll returns, > with POLLIN set, and the next read() will succeed (actually, it can return an > error as well, but for other error conditions in case of e.g. hardware errors). > > The problem with this is of course that this will also start the streaming if > all we wanted to wait for was an exception. That's not what we want at all. > Ideally we could inspect in the driver what the caller wanted to wait for, but > that information is not available, unfortunately. > > 3) The other case is that we try to start streaming in poll and it fails. > In that case any errors are lost and poll returns POLLERR (note that the poll(2) > manual page says that POLLERR is for output only, but clearly in the linux > kernel it is accepted both input and output). Posix doesn't impose such restriction. > But for the select() call this POLLERR information is lost as well and the > application will call read() anyway, which now will attempt to start the > streaming (again after poll() tried it the first time) and this time it > returns the actual error code. The posix list of acceptable errors are: http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html On that list, ENOMEM seems to be acceptable, but EBUSY doesn't. We should not use anything outside the acceptable error codes there, as otherwise, applications like cat, more, less, etc may not work. The read interface should be simple enough to allow applications that are not aware of the V4L2 api to work. So, it should follow whatever supported by standard files. > Just try this with capture-example: start it in mmap mode, then try to run > it in read mode from a second console. The EBUSY error comes from the read() > command, not from select(). With or without this patch, capture-example > behaves exactly the same. > > The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no > longer returned. Since this isn't documented at all anyway (and the poll(2) > manual page is actually unclear about whether you can expect it), Posix seems clear enough to me. > I am > actually quite happy with this change. After this analysis I realized it is > even better than expected. > I never liked that poll starts streaming, poll should be a fast function, > not something that does a lot of other things. > > It's actually a very nice change, but I admit that it is tricky to analyze. I'm not very comfortable with vb2 returning unexpected errors there. Also, for me it is clear that, if read will fail, POLLERR should be rised. 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
Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >Em 28-06-2011 04:33, Hans Verkuil escreveu: >> On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote: >>> Em 07-06-2011 12:05, Hans Verkuil escreveu: >>>> From: Hans Verkuil <hans.verkuil@cisco.com> >>>> >>>> The vb2_poll function would start read-DMA if called without any >streaming >>>> in progress. This unfortunately does not work if the application >just wants >>>> to poll for exceptions. This information of what the application is >polling >>>> for is sadly unavailable in the driver. >>>> >>>> Andy Walls suggested to just return POLLIN | POLLRDNORM and let the >first >>>> call to read() start the DMA. This initial read() call will return >EAGAIN >>>> since no actual data is available yet, but it does start the DMA. >>>> >>>> Applications must handle EAGAIN in any case since there can be >other reasons >>>> for EAGAIN as well. >>>> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>>> --- >>>> drivers/media/video/videobuf2-core.c | 17 +++-------------- >>>> 1 files changed, 3 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/media/video/videobuf2-core.c >b/drivers/media/video/videobuf2-core.c >>>> index 6ba1461..ad75c95 100644 >>>> --- a/drivers/media/video/videobuf2-core.c >>>> +++ b/drivers/media/video/videobuf2-core.c >>>> @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct >vb2_queue *q); >>>> unsigned int vb2_poll(struct vb2_queue *q, struct file *file, >poll_table *wait) >>>> { >>>> unsigned long flags; >>>> - unsigned int ret; >>>> struct vb2_buffer *vb = NULL; >>>> >>>> /* >>>> * 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. >>>> - */ >>>> + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) >>>> + return POLLIN | POLLRDNORM; >>>> + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) >>>> return POLLOUT | POLLWRNORM; >>>> - } >>>> } >>> >>> There is one behavior change on this patchset: __vb2_init_fileio() >checks for some >>> troubles that may happen during device streaming initialization, >returning POLLERR >>> if there is a problem there. >>> >>> By moving this code to be called only at read, it means the poll() >will not fail >>> anymore, but the first read() will fail. The man page for read() >doesn't tell that >>> -EBUSY or -ENOMEM could happen there. The same happens with V4L2 >specs. So, it is >>> clearly violating kernel ABI. >>> >>> NACK. >> >> Actually, almost nothing changes in the ABI. It's counterintuitive, >but >> this is what happens: >> >> 1) The poll() function in a driver does not actually return any error >codes. >> It never did. It returns a poll mask, which is expected to be POLLERR >in case >> of any error. All that it does is that select() returns if it waits >for reading >> or writing. Any actual error codes are never propagated. > >Yes, but POLLERR will return error on the vb2 cases that return -EBUSY >or -ENOMEM. >This doesn't violate the ioctl ABI. > >> This means BTW that >> our poll manual page is wrong (what a surprise), most of those error >codes can >> never be returned. > >True. The DocBook needs a fix. Posix describes it as: > http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html > >> >> 2) Suppose we try to start streaming in poll. If this works, then >poll returns, >> with POLLIN set, and the next read() will succeed (actually, it can >return an >> error as well, but for other error conditions in case of e.g. >hardware errors). >> >> The problem with this is of course that this will also start the >streaming if >> all we wanted to wait for was an exception. That's not what we want >at all. >> Ideally we could inspect in the driver what the caller wanted to wait >for, but >> that information is not available, unfortunately. >> >> 3) The other case is that we try to start streaming in poll and it >fails. >> In that case any errors are lost and poll returns POLLERR (note that >the poll(2) >> manual page says that POLLERR is for output only, but clearly in the >linux >> kernel it is accepted both input and output). > >Posix doesn't impose such restriction. > >> But for the select() call this POLLERR information is lost as well >and the >> application will call read() anyway, which now will attempt to start >the >> streaming (again after poll() tried it the first time) and this time >it >> returns the actual error code. > >The posix list of acceptable errors are: > http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html >On that list, ENOMEM seems to be acceptable, but EBUSY doesn't. > >We should not use anything outside the acceptable error codes there, as >otherwise, >applications like cat, more, less, etc may not work. The read interface >should >be simple enough to allow applications that are not aware of the V4L2 >api to >work. So, it should follow whatever supported by standard files. > >> Just try this with capture-example: start it in mmap mode, then try >to run >> it in read mode from a second console. The EBUSY error comes from the >read() >> command, not from select(). With or without this patch, >capture-example >> behaves exactly the same. >> >> The only ABI change I see is with poll(2) and epoll(7) where POLLERR >is no >> longer returned. Since this isn't documented at all anyway (and the >poll(2) >> manual page is actually unclear about whether you can expect it), > >Posix seems clear enough to me. > >> I am >> actually quite happy with this change. After this analysis I realized >it is >> even better than expected. > >> I never liked that poll starts streaming, poll should be a fast >function, >> not something that does a lot of other things. >> >> It's actually a very nice change, but I admit that it is tricky to >analyze. > >I'm not very comfortable with vb2 returning unexpected errors there. >Also, >for me it is clear that, if read will fail, POLLERR should be rised. > >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 It is also the case that a driver's poll method should never sleep. I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this. One issue is how to start streaming with apps that: - Open /dev/video/ in a nonblocking mode, and - Use the read() method while doing it in a way that is POSIX compliant and doesn't break existing apps. The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects. I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it. The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().) I did not do analysis for apps that use mmap(), which I guess is the current concern. Regards, Andy -- 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 28-06-2011 09:21, Andy Walls escreveu: > Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >> I'm not very comfortable with vb2 returning unexpected errors there. >> Also, >> for me it is clear that, if read will fail, POLLERR should be rised. >> >> 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 > > It is also the case that a driver's poll method should never sleep. True. > I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this. > > One issue is how to start streaming with apps that: > - Open /dev/video/ in a nonblocking mode, and > - Use the read() method > > while doing it in a way that is POSIX compliant and doesn't break existing apps. Well, a first call for poll() may rise a thread that will prepare the buffers, and return with 0 while there's no data available. > The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects. > > I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it. > > The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().) > > I did not do analysis for apps that use mmap(), which I guess is the current concern. The concern is that it is pointing that there are available data, even when there is an error. This looks like a POSIX violation for 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 Tuesday, June 28, 2011 14:43:22 Mauro Carvalho Chehab wrote: > Em 28-06-2011 09:21, Andy Walls escreveu: > > Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > > >> I'm not very comfortable with vb2 returning unexpected errors there. > >> Also, > >> for me it is clear that, if read will fail, POLLERR should be rised. > >> > >> 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 > > > > It is also the case that a driver's poll method should never sleep. > > True. Actually, it is allowed, but only since kernel 2.6.29 (before that it could apparently give rise to busy looping if you were unlucky). But the main use case is userspace file systems like fuse. Not so much in regular drivers. Since drivers can sleep when starting streaming (ivtv will do that, in any case), we were in violation of the poll kernel API for a long time :-) > > I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this. > > > > One issue is how to start streaming with apps that: > > - Open /dev/video/ in a nonblocking mode, and > > - Use the read() method > > > > while doing it in a way that is POSIX compliant and doesn't break existing apps. > > Well, a first call for poll() may rise a thread that will prepare the buffers, and > return with 0 while there's no data available. There is actually no guarantee whatsoever that if poll says you can read(), that that read also has to succeed. Other threads can have read the data already, and errors may have occured. And in fact, just starting streaming gives no guarantee that there is anything to read. For example, starting the DMA engine when there is no valid input signal. Many drivers (certainly those dealing with digital interfaces as opposed to analog) will just sit and wait. A non-blocking read will just return 0 without reading anything. So the current poll implementation (and that includes the one in videobuf-core.c as well) actually does *not* give any guarantee about whether data will be available in read(). And from the same POSIX link you posted: "The poll() function shall support regular files, terminal and pseudo-terminal devices, FIFOs, pipes, sockets and [XSR] [Option Start] STREAMS-based files. [Option End] The behavior of poll() on elements of fds that refer to other types of file is unspecified." Note the last line: we do not fall under this posix document. > > The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects. > > > > I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it. > > > > The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().) > > > > I did not do analysis for apps that use mmap(), which I guess is the current concern. There isn't a problem with mmap(). For the stream I/O API you have to call STREAMON explicitly in order to start streaming. poll() will not do that for you. I was thinking that one improvement that could be realized is that vb2_poll could do some basic checks, such as checking whether streaming was already in progress (EBUSY), but then I realized that it already does that: this code is only active if there is no streaming in progress anyway. Regards, Hans > > The concern is that it is pointing that there are available data, even when there is an error. > This looks like a POSIX violation for 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 > -- 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 Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote: > Em 28-06-2011 09:21, Andy Walls escreveu: > > It is also the case that a driver's poll method should never sleep. > > True. > > One issue is how to start streaming with apps that: > > - Open /dev/video/ in a nonblocking mode, and > > - Use the read() method > > > > while doing it in a way that is POSIX compliant and doesn't break existing apps. > > Well, a first call for poll() may rise a thread that will prepare the buffers, and > return with 0 while there's no data available. Sure, but that doesn't solve the problem of an app only select()-ing or poll()-ing for exception fd's and not starting any IO. > > The other constraint is to ensure when only poll()-ing for exception > conditions, not having significant IO side effects. > > > > I'm pretty sure sleeping in a driver's poll() method, or having > significant side effects, is not ine the spirit of the POSIX select() > and poll(), even if the letter of POSIX says nothing about it. > > > > The method I suggested to Hans is completely POSIX compliant for > apps using read() and select() and was checked against MythTV as > having no bad side effects. (And by thought experiment doesn't break > any sensible app using nonblocking IO with select() and read().) > > > > I did not do analysis for apps that use mmap(), which I guess is the > current concern. > > The concern is that it is pointing that there are available data, even > when there is an error. > This looks like a POSIX violation for me. It isn't. From the specification for select(): http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html "A descriptor shall be considered ready for reading when a call to an input function with O_NONBLOCK clear would not block, whether or not the function would transfer data successfully. (The function might return data, an end-of-file indication, or an error other than one indicating that it is blocked, and in each of these cases the descriptor shall be considered ready for reading.)" To a userspace app, a non-blocking read() can always return an error, regarless of the previous select() or poll() result. And all applications that use select() or poll() folowed by a nonblocking read() should be prepared to handle an errno from the read(). However, that excerpt from the select() specification does imply, that perhaps, the driver should probably start streaming using a work item and one of the CMWQ workers, so that the read() doesn't block. Regards, Andy -- 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 28-06-2011 20:14, Andy Walls escreveu: > On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote: >> Em 28-06-2011 09:21, Andy Walls escreveu: > >>> It is also the case that a driver's poll method should never sleep. >> >> True. > >>> One issue is how to start streaming with apps that: >>> - Open /dev/video/ in a nonblocking mode, and >>> - Use the read() method >>> >>> while doing it in a way that is POSIX compliant and doesn't break existing apps. >> >> Well, a first call for poll() may rise a thread that will prepare the buffers, and >> return with 0 while there's no data available. > > Sure, but that doesn't solve the problem of an app only select()-ing or > poll()-ing for exception fd's and not starting any IO. Well, a file descriptor can be used only for one thing: or it is a stream file descriptor, or it is an event descriptor. You can't have both for the same file descriptor. If an application need to check for both, the standard Unix way is: fd_set set; FD_ZERO (&set); FD_SET (fd_stream, &set); FD_SET (fd_event, &set); select (FD_SETSIZE, &set, NULL, NULL, &timeout); In other words, or the events nodes need to be different, or an ioctl is needed in order to tell the Kernel that the associated file descriptor will be used for an event, and that vb2 should not bother with it. >>> The other constraint is to ensure when only poll()-ing for exception >> conditions, not having significant IO side effects. >>> >>> I'm pretty sure sleeping in a driver's poll() method, or having >> significant side effects, is not ine the spirit of the POSIX select() >> and poll(), even if the letter of POSIX says nothing about it. >>> >>> The method I suggested to Hans is completely POSIX compliant for >> apps using read() and select() and was checked against MythTV as >> having no bad side effects. (And by thought experiment doesn't break >> any sensible app using nonblocking IO with select() and read().) >>> >>> I did not do analysis for apps that use mmap(), which I guess is the >> current concern. >> >> The concern is that it is pointing that there are available data, even >> when there is an error. >> This looks like a POSIX violation for me. > > It isn't. > > From the specification for select(): > http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html > > "A descriptor shall be considered ready for reading when a call to an > input function with O_NONBLOCK clear would not block, whether or not the > function would transfer data successfully. (The function might return > data, an end-of-file indication, or an error other than one indicating > that it is blocked, and in each of these cases the descriptor shall be > considered ready for reading.)" My understanding from the above is that "ready" means: - data; - EOF; - error. if POLLIN (or POLLOUT) is returned, it should mean one of the above, e. g. the device is ready to provide data or some error occurred. Btw, we're talking about poll(), and not select(). The poll() doc is clearer (http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html): POLLIN Data other than high-priority data may be read without blocking. POLLOUT Normal data may be written without blocking. Saying that a -EAGAIN means that data is "ready" is an API abuse. This can actually work, but it will effectively mean that poll or select won't do anything, except for wasting a some CPU cycles. > To a userspace app, a non-blocking read() can always return an error, > regarless of the previous select() or poll() result. And all > applications that use select() or poll() folowed by a nonblocking read() > should be prepared to handle an errno from the read(). > > However, that excerpt from the select() specification does imply, that > perhaps, the driver should probably start streaming using a work item > and one of the CMWQ workers, so that the read() doesn't block. 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 Tue, 2011-06-28 at 21:00 -0300, Mauro Carvalho Chehab wrote: > Em 28-06-2011 20:14, Andy Walls escreveu: > > On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote: > >> Em 28-06-2011 09:21, Andy Walls escreveu: > > > >>> It is also the case that a driver's poll method should never sleep. > >> > >> True. > > > >>> One issue is how to start streaming with apps that: > >>> - Open /dev/video/ in a nonblocking mode, and > >>> - Use the read() method > >>> > >>> while doing it in a way that is POSIX compliant and doesn't break existing apps. > >> > >> Well, a first call for poll() may rise a thread that will prepare the buffers, and > >> return with 0 while there's no data available. > > > > Sure, but that doesn't solve the problem of an app only select()-ing or > > poll()-ing for exception fd's and not starting any IO. > > Well, a file descriptor can be used only for one thing: or it is a stream file > descriptor, or it is an event descriptor. You can't have both for the same > file descriptor. If an application need to check for both, the standard Unix way is: > > fd_set set; > > FD_ZERO (&set); > FD_SET (fd_stream, &set); > FD_SET (fd_event, &set); > > select (FD_SETSIZE, &set, NULL, NULL, &timeout); > > In other words, or the events nodes need to be different, or an ioctl is needed > in order to tell the Kernel that the associated file descriptor will be used > for an event, and that vb2 should not bother with it. Um, no, that is not correct for Unix fd's and socket descriptors in general. I realize that v4l2 events need to be enabled with an ioctl(), but do we have a restriction that that can't happen on the same fd as the one used for streaming? Back in the days before threads were commonly available on Unix systems, a process would use a single thread calling select() to handle I/O on a serial port: fd_set rfds, wfds; int ttyfd; ... FD_ZERO(&rfds); FD_SET(ttyfd, &rfds); FD_ZERO(&wfds); FD_SET(ttyfd, &wfds); n = select(ttyfd+1, &rfds, &wfds, NULL, NULL); Or TCP socket fd_set rfds, wfds, efds; int sockd; ... FD_ZERO(&rfds); FD_SET(sockd, &rfds); FD_ZERO(&wfds); FD_SET(sockd, &wfds); FD_ZERO(&efds); FD_SET(sockd, &efds); n = select(sockd+1, &rfds, &wfds, &efds, NULL); > >>> The other constraint is to ensure when only poll()-ing for exception > >> conditions, not having significant IO side effects. > >>> > >>> I'm pretty sure sleeping in a driver's poll() method, or having > >> significant side effects, is not ine the spirit of the POSIX select() > >> and poll(), even if the letter of POSIX says nothing about it. > >>> > >>> The method I suggested to Hans is completely POSIX compliant for > >> apps using read() and select() and was checked against MythTV as > >> having no bad side effects. (And by thought experiment doesn't break > >> any sensible app using nonblocking IO with select() and read().) > >>> > >>> I did not do analysis for apps that use mmap(), which I guess is the > >> current concern. > >> > >> The concern is that it is pointing that there are available data, even > >> when there is an error. > >> This looks like a POSIX violation for me. > > > > It isn't. > > > > From the specification for select(): > > http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html > > > > "A descriptor shall be considered ready for reading when a call to an > > input function with O_NONBLOCK clear would not block, whether or not the > > function would transfer data successfully. (The function might return > > data, an end-of-file indication, or an error other than one indicating > > that it is blocked, and in each of these cases the descriptor shall be > > considered ready for reading.)" > > My understanding from the above is that "ready" means: > - data; > - EOF; > - error. Waiting for data to arrive on an fd, while not streaming is an error condition for select() should return. Something has to be done about that fd in that case, or select()-ing on it is futile. > if POLLIN (or POLLOUT) is returned, it should mean one of the above, e. g. > the device is ready to provide data or some error occurred. > > Btw, we're talking about poll(), and not select(). The poll() doc is clearer > (http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html): > > POLLIN > Data other than high-priority data may be read without blocking. > > POLLOUT > Normal data may be written without blocking. > > Saying that a -EAGAIN means that data is "ready" is an API abuse. EAGAIN is the correct errno fo the read that returns no data. The abuse is the revent value returned from poll(). However, for any application that open()s a device node that supports mutliple open()s, it is a valid circumstance that can happen with POSIX. One way to avoid abusing the revent value returned by poll(), for the initial poll() call where streaming has not started, is to modify the kernel infrastructure that calls the driver poll() methods. If the kernel passes down to the driver for what the caller is polling, the driver poll() method could know that only the exception set for an fd was being poll()-ed. In that case, the driver would know not to start streaming and use a return value that made sense. That solution seems like a lot of work and a large perturbation to the kernel. The only other solution I can think of is to do nothing. Then v4l2-event monitoring will always have the unfortunate side effect of starting the stream. > This > can actually work, but it will effectively mean that poll or select won't > do anything, except for wasting a some CPU cycles. The wasted CPU cycles are an insignificant, one-time penalty during the first iteration of a loop that will use several orders of magnitude more CPU cycles to fetch and process video data. It avoids unecessarily prohibiting modification of v4l2 controls or other resources that cannot be modified while streaming, for an application that is in a phase where it only wishes to poll() for v4l2 events without starting a stream. It mainatins backward compatability with existing programs. Those existing programs *must* handle read() errors after select() or poll() indicates that the fd won't block on the next read(), and the call to read() will start the stream. Regards, Andy > 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 Tuesday, June 28, 2011 15:58:36 Hans Verkuil wrote: > On Tuesday, June 28, 2011 14:43:22 Mauro Carvalho Chehab wrote: > > Em 28-06-2011 09:21, Andy Walls escreveu: > > > Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > > > > >> I'm not very comfortable with vb2 returning unexpected errors there. > > >> Also, > > >> for me it is clear that, if read will fail, POLLERR should be rised. > > >> > > >> 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 > > > > > > It is also the case that a driver's poll method should never sleep. > > > > True. > > Actually, it is allowed, but only since kernel 2.6.29 (before that it could > apparently give rise to busy looping if you were unlucky). But the main use > case is userspace file systems like fuse. Not so much in regular drivers. > > Since drivers can sleep when starting streaming (ivtv will do that, in any > case), we were in violation of the poll kernel API for a long time :-) > > > > I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this. > > > > > > One issue is how to start streaming with apps that: > > > - Open /dev/video/ in a nonblocking mode, and > > > - Use the read() method > > > > > > while doing it in a way that is POSIX compliant and doesn't break existing apps. > > > > Well, a first call for poll() may rise a thread that will prepare the buffers, and > > return with 0 while there's no data available. > > There is actually no guarantee whatsoever that if poll says you can read(), that that > read also has to succeed. Other threads can have read the data already, and errors may > have occured. And in fact, just starting streaming gives no guarantee that there is > anything to read. For example, starting the DMA engine when there is no valid input > signal. Many drivers (certainly those dealing with digital interfaces as opposed to > analog) will just sit and wait. A non-blocking read will just return 0 without > reading anything. > > So the current poll implementation (and that includes the one in videobuf-core.c as > well) actually does *not* give any guarantee about whether data will be available > in read(). Never mind this. I didn't look carefully enough: it does start the DMA and then poll waits for data to arrive. So when poll says there is data, then there is really data. Although applications should always handle EAGAIN anyway: some drivers do return it, even when data is supposed to be available (I had to add that to qv4l2 at one time). Regards, Hans > > And from the same POSIX link you posted: > > "The poll() function shall support regular files, terminal and pseudo-terminal devices, > FIFOs, pipes, sockets and [XSR] [Option Start] STREAMS-based files. [Option End] > The behavior of poll() on elements of fds that refer to other types of file is unspecified." > > Note the last line: we do not fall under this posix document. > > > > The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects. > > > > > > I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it. > > > > > > The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().) > > > > > > I did not do analysis for apps that use mmap(), which I guess is the current concern. > > There isn't a problem with mmap(). For the stream I/O API you have to call STREAMON > explicitly in order to start streaming. poll() will not do that for you. > > I was thinking that one improvement that could be realized is that vb2_poll could > do some basic checks, such as checking whether streaming was already in progress > (EBUSY), but then I realized that it already does that: this code is only active > if there is no streaming in progress anyway. > > Regards, > > Hans > > > > > The concern is that it is pointing that there are available data, even when there is an error. > > This looks like a POSIX violation for 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 > > > -- > 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 29-06-2011 02:08, Andy Walls escreveu: > On Tue, 2011-06-28 at 21:00 -0300, Mauro Carvalho Chehab wrote: >> Em 28-06-2011 20:14, Andy Walls escreveu: >>> On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote: >>>> Em 28-06-2011 09:21, Andy Walls escreveu: >>> >>>>> It is also the case that a driver's poll method should never sleep. >>>> >>>> True. >>> >>>>> One issue is how to start streaming with apps that: >>>>> - Open /dev/video/ in a nonblocking mode, and >>>>> - Use the read() method >>>>> >>>>> while doing it in a way that is POSIX compliant and doesn't break existing apps. >>>> >>>> Well, a first call for poll() may rise a thread that will prepare the buffers, and >>>> return with 0 while there's no data available. >>> >>> Sure, but that doesn't solve the problem of an app only select()-ing or >>> poll()-ing for exception fd's and not starting any IO. >> >> Well, a file descriptor can be used only for one thing: or it is a stream file >> descriptor, or it is an event descriptor. You can't have both for the same >> file descriptor. If an application need to check for both, the standard Unix way is: >> >> fd_set set; >> >> FD_ZERO (&set); >> FD_SET (fd_stream, &set); >> FD_SET (fd_event, &set); >> >> select (FD_SETSIZE, &set, NULL, NULL, &timeout); >> >> In other words, or the events nodes need to be different, or an ioctl is needed >> in order to tell the Kernel that the associated file descriptor will be used >> for an event, and that vb2 should not bother with it. > > Um, no, that is not correct for Unix fd's and socket descriptors in > general. I realize that v4l2 events need to be enabled with an ioctl(), > but do we have a restriction that that can't happen on the same fd as > the one used for streaming? > > Back in the days before threads were commonly available on Unix systems, > a process would use a single thread calling select() to handle I/O on a > serial port: > > fd_set rfds, wfds; > int ttyfd; > ... > FD_ZERO(&rfds); > FD_SET(ttyfd, &rfds); > FD_ZERO(&wfds); > FD_SET(ttyfd, &wfds); > > n = select(ttyfd+1, &rfds, &wfds, NULL, NULL); > > Or TCP socket > > fd_set rfds, wfds, efds; > int sockd; > ... > FD_ZERO(&rfds); > FD_SET(sockd, &rfds); > FD_ZERO(&wfds); > FD_SET(sockd, &wfds); > FD_ZERO(&efds); > FD_SET(sockd, &efds); > > n = select(sockd+1, &rfds, &wfds, &efds, NULL); On both serial and socket devices, if select returns a file descriptor, the data is there or the device/socket got disconnected. > Waiting for data to arrive on an fd, while not streaming is an error > condition for select() should return. Yes, but poll() starts the streaming, if the mmap mode were not started, according with the V4L2 spec: http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html Thanks, 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
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index 6ba1461..ad75c95 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q); unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) { unsigned long flags; - unsigned int ret; struct vb2_buffer *vb = NULL; /* * 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. - */ + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) + return POLLIN | POLLRDNORM; + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) return POLLOUT | POLLWRNORM; - } } /*