[RFC v2 0/4]
v2 cover [RFC v2 0/4]
Hi all,
This is the second RFC set after the initial patch that makes poll return POLLERR if no events are subscribed. There are other issues as well which these patches address.
The original RFC patch is here:
<URL:http://www.spinics.net/lists/linux-media/msg68077.html>
poll(2) and select(2) can both be used for I/O multiplexing. While both provide slightly different semantics. man 2 select:
select() and pselect() allow a program to monitor multiple file descriptors, waiting until one or more of the file descriptors become "ready" for some class of I/O operation (e.g., input possible). A file descriptor is considered ready if it is possible to perform the corre‐ sponding I/O operation (e.g., read(2)) without blocking.
The two system calls provide slightly different semantics: poll(2) can signal POLLERR related to a file handle but select(2) does not: instead, on POLLERR it sets a bit corresponding to a file handle in the read and write sets. This is somewhat confusing since with the original patch --- using select(2) would suggest that there's something to read or write instead of knowing no further exceptions are coming.
Thus, also denying polling a subdev file handle using select(2) will mean the POLLERR never gets through in any form.
So the meaningful alternatives I can think of are:
1) Use POLLERR | POLLPRI. When the last event subscription is gone and select(2) IOCTL is issued, all file descriptor sets are set for a file handle. Users of poll(2) will directly see both of the flags, making the case visible to the user immediately in some cases. On sub-devices this is obvious but on V4L2 devices the user should poll(2) (or select(2)) again to know whether there's I/O waiting to be read, written or whether buffers are ready.
2) Use POLLPRI only. While this does not differ from any regular event at the level of poll(2) or select(2), the POLLIN or POLLOUT flags are not adversely affected.
In each of the cases to ascertain oneself in a generic way of whether events cannot no longer be obtained one has to call VIDIOC_DQEVENT IOCTL, which currently may block. A patch in the set makes VIDIOC_DQEVENT to signal EIO error code if no events are subscribed.
The videobuf2 changes are untested at the moment since I didn't have a device using videobuf2 at hand right now.
Comments and questions are very welcome.
Signature · 2 lines
Kind regards, Sakari
RFC v2 1/4 v4l: return POLLERR on V4L2 sub-devices if no events are subscribed
From: Teemu Tuominen <[email protected]>
Add check and return POLLERR from subdev_poll() in case of no events subscribed and wakeup once the last event subscription is removed.
This change is essentially done to add possibility to wakeup polling with concurrent unsubscribe.
Move the check after calling poll_wait(). Otherwise it's possible that we go to sleep without getting notified if the subscription went away between the two.
--- drivers/media/v4l2-core/v4l2-event.c | 15 +++++++++++++++ drivers/media/v4l2-core/v4l2-subdev.c | 3 +++ include/media/v4l2-event.h | 1 + 3 files changed, 19 insertions(+)
drivers/media/v4l2-core/v4l2-event.c +15 -0
@@ -107,6 +107,19 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(return NULL ; } +bool v4l2_event_has_subscribed (struct v4l2_fh *fh) +{ +unsigned long flags; +bool rval; + +spin_lock_irqsave (&fh->vdev->fh_lock, flags); + rval = !list_empty (&fh->subscribed); +spin_unlock_irqrestore (&fh->vdev->fh_lock, flags); + +return rval; +} +EXPORT_SYMBOL (v4l2_event_has_subscribed); +static void __v4l2_event_queue_fh (struct v4l2_fh *fh,const struct v4l2_event *ev,const struct timespec *ts) {
@@ -299,6 +312,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, fh->navailable--; }list_del (&sev->list); +if (list_empty (&fh->subscribed)) +wake_up_all (&fh->wait); }spin_unlock_irqrestore (&fh->vdev->fh_lock, flags);
drivers/media/v4l2-core/v4l2-subdev.c +3 -0
@@ -382,6 +382,9 @@ static unsigned int subdev_poll(struct file *file, poll_table *wait)if (v4l2_event_pending (fh))return POLLPRI ; +if (!v4l2_event_has_subscribed (fh)) +return POLLERR |POLLPRI ; +return 0 ; }
include/media/v4l2-event.h +1 -0
@@ -121,6 +121,7 @@ struct v4l2_subscribed_event {int v4l2_event_dequeue (struct v4l2_fh *fh,struct v4l2_event *event,int nonblocking); +bool v4l2_event_has_subscribed (struct v4l2_fh *fh);void v4l2_event_queue (struct video_device *vdev,const struct v4l2_event *ev);void v4l2_event_queue_fh (struct v4l2_fh *fh,const struct v4l2_event *ev);int v4l2_event_pending (struct v4l2_fh *fh);
Signature · 1 line
1.8.3.2
Re: [RFC v2 1/4] v4l: return POLLERR on V4L2 sub-devices if no events are subscribed
Quoted 8 lines · Sakari Ailus
From: Teemu Tuominen <[email protected]>
Add check and return POLLERR from
subdev_poll()in case of no events subscribed and wakeup once the last event subscription is removed.This change is essentially done to add possibility to wakeup polling with concurrent unsubscribe.
Quoted 8 lines
Signed-off-by Teemu Tuominen <[email protected]>Move the check after calling
poll_wait(). Otherwise it's possible that we go to sleep without getting notified if the subscription went away between the two.Signed-off-by Sakari Ailus <[email protected]>Tested-by Teemu Tuominen <[email protected]>
Regards,
Hans
Quoted 66 lines
--- drivers/media/v4l2-core/v4l2-event.c | 15 +++++++++++++++ drivers/media/v4l2-core/v4l2-subdev.c | 3 +++ include/media/v4l2-event.h | 1 + 3 files changed, 19 insertions(+)drivers/media/v4l2-core/v4l2-event.c +15
-0@@ -107,6 +107,19 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(return NULL ; } +bool v4l2_event_has_subscribed (struct v4l2_fh *fh) +{ +unsigned long flags; +bool rval; + +spin_lock_irqsave (&fh->vdev->fh_lock, flags); + rval = !list_empty (&fh->subscribed); +spin_unlock_irqrestore (&fh->vdev->fh_lock, flags); + +return rval; +} +EXPORT_SYMBOL (v4l2_event_has_subscribed); +static void __v4l2_event_queue_fh (struct v4l2_fh *fh,const struct v4l2_event *ev,const struct timespec *ts) {@@ -299,6 +312,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, fh->navailable--; }list_del (&sev->list); +if (list_empty (&fh->subscribed)) +wake_up_all (&fh->wait); }spin_unlock_irqrestore (&fh->vdev->fh_lock, flags);drivers/media/v4l2-core/v4l2-subdev.c +3
-0@@ -382,6 +382,9 @@ static unsigned int subdev_poll(struct file *file, poll_table *wait)if (v4l2_event_pending (fh))return POLLPRI ; +if (!v4l2_event_has_subscribed (fh)) +return POLLERR |POLLPRI ; +return 0 ; }include/media/v4l2-event.h +1
-0@@ -121,6 +121,7 @@ struct v4l2_subscribed_event {int v4l2_event_dequeue (struct v4l2_fh *fh,struct v4l2_event *event,int nonblocking); +bool v4l2_event_has_subscribed (struct v4l2_fh *fh);void v4l2_event_queue (struct video_device *vdev,const struct v4l2_event *ev);void v4l2_event_queue_fh (struct v4l2_fh *fh,const struct v4l2_event *ev);int v4l2_event_pending (struct v4l2_fh *fh);
RFC v2 2/4 v4l: vb2: Only poll for events if the user is interested in them
In reply to Sakari AilusAlso poll_wait() before checking the events since otherwise it's possible to go to sleep and not getting woken up if the event arrives between the two operations.
--- drivers/media/v4l2-core/videobuf2-core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/media/v4l2-core/videobuf2-core.c +4 -3
@@ -2003,13 +2003,14 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)unsigned int res =0 ;unsigned long flags; -if (test_bit (V4L2_FL_USES_V4L2_FH , &vfd->flags)) { +if (test_bit (V4L2_FL_USES_V4L2_FH , &vfd->flags) && + req_events &POLLPRI ) {struct v4l2_fh *fh = file->private_data; +poll_wait (file, &fh->wait, wait); +if (v4l2_event_pending (fh)) res =POLLPRI ; -else if (req_events &POLLPRI ) -poll_wait (file, &fh->wait, wait); }if (!V4L2_TYPE_IS_OUTPUT (q->type) && !(req_events & (POLLIN |POLLRDNORM )))
Signature · 1 line
1.8.3.2
Re: [RFC v2 2/4] v4l: vb2: Only poll for events if the user is interested in them
Quoted 14 lines · Sakari Ailus
Also
poll_wait()before checking the events since otherwise it's possible to go to sleep and not getting woken up if the event arrives between the two operations.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/videobuf2-core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/media/v4l2-core/videobuf2-core.c +0
-0
drivers/media/v4l2-core/videobuf2-core.c unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) · line 2007
Quoted 6 lines
drivers/media/v4l2-core/videobuf2-core.c +2
-1@@ -2003,13 +2003,14 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)unsigned int res =0 ;unsigned long flags; -if (test_bit (V4L2_FL_USES_V4L2_FH , &vfd->flags)) { +if (test_bit (V4L2_FL_USES_V4L2_FH , &vfd->flags) && + req_events &POLLPRI ) {
Can you add parenthesis around 'req_events & POLLPRI'? When mixing '&&' and '&' that makes it more readable.
drivers/media/v4l2-core/videobuf2-core.c unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) · line 2016
Quoted 12 lines
drivers/media/v4l2-core/videobuf2-core.c +2
-2struct v4l2_fh *fh = file->private_data; +poll_wait (file, &fh->wait, wait); +if (v4l2_event_pending (fh)) res =POLLPRI ; -else if (req_events &POLLPRI ) -poll_wait (file, &fh->wait, wait); }if (!V4L2_TYPE_IS_OUTPUT (q->type) && !(req_events & (POLLIN |POLLRDNORM )))
After making that change you can add my:
Regards,
Hans
RFC v2 3/4 v4l: vb2: Return POLLERR when polling for events and none are subscribed
In reply to Sakari AilusThe current implementation allowed polling for events even if none were subscribed. This may be troublesome in multi-threaded applications where the thread handling the subscription is different from the one that handles the events.
--- drivers/media/v4l2-core/videobuf2-core.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/media/v4l2-core/videobuf2-core.c +3 -0
@@ -2011,6 +2011,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)if (v4l2_event_pending (fh)) res =POLLPRI ; + +if (!v4l2_event_has_subscribed (fh)) +return POLLERR |POLLPRI ; }if (!V4L2_TYPE_IS_OUTPUT (q->type) && !(req_events & (POLLIN |POLLRDNORM )))
Signature · 1 line
1.8.3.2
Re: [RFC v2 3/4] v4l: vb2: Return POLLERR when polling for events and none are subscribed
Quoted 15 lines · Sakari Ailus
The current implementation allowed polling for events even if none were subscribed. This may be troublesome in multi-threaded applications where the thread handling the subscription is different from the one that handles the events.
Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/videobuf2-core.c | 3 +++ 1 file changed, 3 insertions(+)drivers/media/v4l2-core/videobuf2-core.c +0
-0
drivers/media/v4l2-core/videobuf2-core.c unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) · line 2016
Quoted 6 lines
drivers/media/v4l2-core/videobuf2-core.c +3
-0@@ -2011,6 +2011,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)if (v4l2_event_pending (fh)) res =POLLPRI ; + +if (!v4l2_event_has_subscribed (fh)) +return POLLERR |POLLPRI ;
What should happen if you poll for both POLLPRI and POLLIN and one of the two would normally return POLLERR? Should that error condition be ignored?
I'm not sure, frankly.
Regards,
Hans
Quoted 4 lines
drivers/media/v4l2-core/videobuf2-core.c +0
-0}if (!V4L2_TYPE_IS_OUTPUT (q->type) && !(req_events & (POLLIN |POLLRDNORM )))
Re: [RFC v2 3/4] v4l: vb2: Return POLLERR when polling for events and none are subscribed
Hi Hans,
Thanks for your comments.
Quoted 31 lines · Hans Verkuil
Quoted 18 lines · Sakari Ailus
The current implementation allowed polling for events even if none were subscribed. This may be troublesome in multi-threaded applications where the
thread handling the subscription is different from the one that handles the
events.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/videobuf2-core.c | 3 +++ 1 file changed, 3 insertions(+)drivers/media/v4l2-core/videobuf2-core.c +0
-0drivers/media/v4l2-core/videobuf2-core.c unsigned int vb2_poll(struct vb2_queue *q, · line 2017
Quoted 7 lines
drivers/media/v4l2-core/videobuf2-core.c +3
-0@@ -2011,6 +2011,9 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait) if (v4l2_event_pending(fh)) res =POLLPRI ; + +if (!v4l2_event_has_subscribed (fh)) +return POLLERR |POLLPRI ;What should happen if you poll for both POLLPRI and POLLIN and one of the two would normally return POLLERR? Should that error condition be ignored?
Quoted 1 line
I'm not sure, frankly.
I think you just need to go to see what does VIDIOC_DQBUF / VIDIOC_DQEVENT return. If you're using select(2) you won't know about POLLERR explicitly anyway: there's a bit for read, write and exceptions but not for errors.
Signature · 4 lines
Kind regards, Sakari Ailus [email protected]
RFC v2 4/4 v4l: events: Don't sleep in dequeue if none are subscribed
In reply to Sakari AilusDequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning -ENOENT when no events are subscribed.
--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/media/v4l2-core/v4l2-event.c +9 -2
@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);do { - ret =wait_event_interruptible (fh->wait, - fh->navailable !=0 ); +bool subscribed; + ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ; +break ; + } ret =__v4l2_event_dequeue (fh, event); }while (ret == -ENOENT );
Signature · 1 line
1.8.3.2
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Quoted 14 lines · Sakari Ailus
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/media/v4l2-core/v4l2-event.c +0
-0
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 80
Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c +1
-2@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock); do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed;
Can you add an empty line here?
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 8 lines
drivers/media/v4l2-core/v4l2-event.c +6
-0+ ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ;
Shouldn't this be -ENOENT?
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 93
Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c +2
-0+break ; + } ret =__v4l2_event_dequeue (fh, event); }while (ret == -ENOENT );
Regards,
Hans
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Hi Hans,
Thanks for the comments!
Quoted 19 lines · Hans Verkuil
Quoted 18 lines · Sakari Ailus
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/media/v4l2-core/v4l2-event.c +0
-0@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);
Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 80
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c +1
-2do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed;Can you add an empty line here?
Sure.
Quoted 10 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 8 lines
drivers/media/v4l2-core/v4l2-event.c +6
-0+ ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ;Shouldn't this be
-ENOENT?
If I use -ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.
-EIO makes that explicit --- this also mirrors the behaviour of VIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)
Signature · 4 lines
Kind regards, Sakari Ailus [email protected]
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Quoted 42 lines · Sakari Ailus
Hi Hans,
Thanks for the comments!
Quoted 19 lines · Hans Verkuil
Quoted 18 lines · Sakari Ailus
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/media/v4l2-core/v4l2-event.c +0
-0@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 80
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c +1
-2do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed;Can you add an empty line here?
Sure.
Quoted 9 lines
Quoted 8 lines
drivers/media/v4l2-core/v4l2-event.c +6
-0+ ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ;
Quoted 7 lines
Quoted 1 line
Shouldn't this be
-ENOENT?If I use
-ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.
OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT.
Quoted 3 lines
-EIOmakes that explicit --- this also mirrors the behaviour ofVIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)
I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about -ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone").
Regards,
Hans
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Hi Hans,
Quoted 45 lines · Hans Verkuil
Quoted 44 lines · Sakari Ailus
Hi Hans,
Thanks for the comments!
Quoted 19 lines · Hans Verkuil
Quoted 18 lines · Sakari Ailus
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/media/v4l2-core/v4l2-event.c +0
-0@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 80
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c +1
-2do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed;Can you add an empty line here?
Sure.
Quoted 10 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 8 lines
drivers/media/v4l2-core/v4l2-event.c +6
-0+ ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ;Shouldn't this be
-ENOENT?
Quoted 8 lines
Quoted 5 lines
If I use
-ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT.
Right. The error code was the last thing I changed before sending the patch, and I ignored it was also present in the commit message. :-P
Quoted 7 lines
Quoted 3 lines
-EIOmakes that explicit --- this also mirrors the behaviour ofVIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about
-ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone").
There is no pipe (or at least wasn't; it's a queue or rather is implemented as a fifo :)) so of the two I prefer -ENOMSG. What would you think of -ENODATA or -EPERM (which is used e.g. when writing read-only controls)?
Signature · 4 lines
Cheers, Sakari Ailus [email protected]
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Quoted 64 lines · Sakari Ailus
Hi Hans,
Quoted 45 lines · Hans Verkuil
Quoted 44 lines · Sakari Ailus
Hi Hans,
Thanks for the comments!
Quoted 19 lines · Hans Verkuil
Quoted 18 lines · Sakari Ailus
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/media/v4l2-core/v4l2-event.c +0
-0@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);Quoted 6 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 80
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c +1
-2do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed;Can you add an empty line here?
Sure.
Quoted 10 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 8 lines
drivers/media/v4l2-core/v4l2-event.c +6
-0+ ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ;Shouldn't this be
-ENOENT?Quoted 8 lines
Quoted 5 lines
If I use
-ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT.
Right. The error code was the last thing I changed before sending the patch, and I ignored it was also present in the commit message. :-P
Quoted 4 lines
Quoted 3 lines
-EIOmakes that explicit --- this also mirrors the behaviour ofVIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)
Quoted 9 lines
Quoted 3 lines
I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about
-ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone").There is no pipe (or at least wasn't; it's a queue or rather is implemented as a fifo :)) so of the two I prefer
-ENOMSG. What would you think of-ENODATAor-EPERM(which is used e.g. when writing read-only controls)?
I don't like ENODATA, mostly because it is so close in meaning to ENOENT. EPERM would work for me. It's probably a bit better than ENOMSG.
Regards,
Hans
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
In reply to Hans VerkuilHans Verkuil wrote:
...
Quoted 14 lines
Quoted 10 lines
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 2 lines
drivers/media/v4l2-core/v4l2-event.c +2
-0+if (!subscribed) { + ret = -EIO ;Shouldn't this be
-ENOENT?If I use
-ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT.
Quoted 7 lines
Quoted 3 lines
-EIOmakes that explicit --- this also mirrors the behaviour ofVIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about
-ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone").
Thinking about this some more, -ENOENT is probably what we should return. *But* when there are no events to dequeue, we should instead return -EAGAIN (i.e. EWOULDBLOCK) which VIDIOC_DQBUF also uses.
However I'm not sure if anything depends on -ENOENT currently (probably not really) so changing this might require some consideration. No error codes are currently defined for VIDIOC_DQEVENT; was planning to fix that while we're at this.
Signature · 4 lines
Cheers, Sakari Ailus [email protected]
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Quoted 24 lines · Sakari Ailus
Hans Verkuil wrote:
...Quoted 14 lines
Quoted 10 lines
Quoted 4 lines
drivers/media/v4l2-core/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, · line 88
Quoted 2 lines
drivers/media/v4l2-core/v4l2-event.c +2
-0+if (!subscribed) { + ret = -EIO ;Shouldn't this be
-ENOENT?If I use
-ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it.OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT.
Quoted 7 lines
Quoted 3 lines
-EIOmakes that explicit --- this also mirrors the behaviour ofVIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about
-ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone").
Quoted 9 lines
Thinking about this some more,
-ENOENTis probably what we should return. *But* when there are no events to dequeue, we should instead return-EAGAIN(i.e. EWOULDBLOCK) whichVIDIOC_DQBUFalso uses.However I'm not sure if anything depends on
-ENOENTcurrently (probably not really) so changing this might require some consideration. No error codes are currently defined forVIDIOC_DQEVENT; was planning to fix that while we're at this.
Urgh, this is messy. In non-blocking mode DQEVENT should indeed return -EAGAIN if you have subscribed events but no events are pending.
If you have no subscribed events, then -ENOENT would be IMHO the most suitable return value.
This means that DQEVENT's behavior changes in the non-blocking case. On the other hand, this is actually what you would expect based on the EAGAIN description in the spec: "It is also returned when the ioctl would need to wait for an event, but the device was opened in non-blocking mode."
That said, I don't think we can change it. It's been around for too long and you have no idea how it is used in embedded systems that are out there (and that's where you would see this used in practice).
I would just document the ENOENT error code (perhaps with a note that it should have been EAGAIN) and add a new error (EPERM?) for when no events are subscribed.
Regards,
Hans
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
In reply to Sakari AilusHi Sakari,
Quoted 1 line · On Wednesday 02 October 2013 16:45:16 Sakari Ailus wrote:
Dequeueing events was is entirely possible even if none are subscribed,
was or is ? :-)
Quoted 32 lines
leading to sleeping indefinitely. Fix this by returning
-ENOENTwhen no events are subscribed.Signed-off-by Sakari Ailus <[email protected]>--- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)(quoted diff) +0
-0
b/drivers/media/v4l2-core/v4l2-event.cindex b53897e..553a800 100644drivers/media/v4l2-core/v4l2-event.c +9
-2@@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,mutex_unlock (fh->vdev->lock);do { - ret =wait_event_interruptible (fh->wait, - fh->navailable !=0 ); +bool subscribed; + ret =wait_event_interruptible ( + fh->wait, + fh->navailable !=0 || + !(subscribed =v4l2_event_has_subscribed (fh)));if (ret <0 )break ; +if (!subscribed) { + ret = -EIO ; +break ; + } ret =__v4l2_event_dequeue (fh, event); }while (ret == -ENOENT );
Signature · 3 lines
Regards, Laurent Pinchart
Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed
Quoted 6 lines · Laurent Pinchart
Hi Sakari,
Quoted 1 line · On Wednesday 02 October 2013 16:45:16 Sakari Ailus wrote:
Dequeueing events was is entirely possible even if none are subscribed,
was or is ? :-)
Was. :-) I'll fix that.
Signature · 2 lines
Sakari Ailus [email protected]
Quoted 43 lines · Sakari Ailus
Hi all,
This is the second RFC set after the initial patch that makes poll return POLLERR if no events are subscribed. There are other issues as well which these patches address.
The original RFC patch is here:
<URL:http://www.spinics.net/lists/linux-media/msg68077.html>
poll(2) and select(2) can both be used for I/O multiplexing. While both provide slightly different semantics. man 2 select:
select()andpselect()allow a program to monitor multiple file descriptors, waiting until one or more of the file descriptors become "ready" for some class of I/O operation (e.g., input possible). A file descriptor is considered ready if it is possible to perform the corre‐ sponding I/O operation (e.g., read(2)) without blocking.The two system calls provide slightly different semantics: poll(2) can signal POLLERR related to a file handle but select(2) does not: instead, on POLLERR it sets a bit corresponding to a file handle in the read and write sets. This is somewhat confusing since with the original patch --- using select(2) would suggest that there's something to read or write instead of knowing no further exceptions are coming.
Thus, also denying polling a subdev file handle using select(2) will mean the POLLERR never gets through in any form.
So the meaningful alternatives I can think of are:
1) Use POLLERR | POLLPRI. When the last event subscription is gone and select(2) IOCTL is issued, all file descriptor sets are set for a file handle. Users of poll(2) will directly see both of the flags, making the case visible to the user immediately in some cases. On sub-devices this is obvious but on V4L2 devices the user should poll(2) (or select(2)) again to know whether there's I/O waiting to be read, written or whether buffers are ready.
2) Use POLLPRI only. While this does not differ from any regular event at the level of poll(2) or select(2), the POLLIN or POLLOUT flags are not adversely affected.
Quoted 7 lines
In each of the cases to ascertain oneself in a generic way of whether events cannot no longer be obtained one has to call
VIDIOC_DQEVENTIOCTL, which currently may block. A patch in the set makesVIDIOC_DQEVENTto signal EIO error code if no events are subscribed.The videobuf2 changes are untested at the moment since I didn't have a device using videobuf2 at hand right now.
You can use vivi for testing.
Hans
Hi Sakari,
Quoted 48 lines · On Wednesday 02 October 2013 16:45:12 Sakari Ailus wrote:
Hi all,
This is the second RFC set after the initial patch that makes poll return POLLERR if no events are subscribed. There are other issues as well which these patches address.
The original RFC patch is here:
<URL:http://www.spinics.net/lists/linux-media/msg68077.html>
poll(2) and select(2) can both be used for I/O multiplexing. While both provide slightly different semantics. man 2 select:
select()andpselect()allow a program to monitor multiple
file descriptors, waiting until one or more of the file descriptors become "ready" for some class of I/O operation (e.g., input possible). A file descriptor is considered ready if it is possible to perform the corre‐ sponding I/O operation (e.g., read(2)) without blocking.The two system calls provide slightly different semantics: poll(2) can signal POLLERR related to a file handle but select(2) does not: instead, on POLLERR it sets a bit corresponding to a file handle in the read and write sets. This is somewhat confusing since with the original patch --- using select(2) would suggest that there's something to read or write instead of knowing no further exceptions are coming.
Thus, also denying polling a subdev file handle using select(2) will mean the POLLERR never gets through in any form.
So the meaningful alternatives I can think of are:
1) Use POLLERR | POLLPRI. When the last event subscription is gone and select(2) IOCTL is issued, all file descriptor sets are set for a file handle. Users of poll(2) will directly see both of the flags, making the case visible to the user immediately in some cases. On sub-devices this is obvious but on V4L2 devices the user should poll(2) (or select(2)) again to know whether there's I/O waiting to be read, written or whether buffers are ready.
2) Use POLLPRI only. While this does not differ from any regular event at the level of poll(2) or select(2), the POLLIN or POLLOUT flags are not adversely affected.
In each of the cases to ascertain oneself in a generic way of whether events cannot no longer be obtained one has to call
VIDIOC_DQEVENTIOCTL, which currently may block. A patch in the set makesVIDIOC_DQEVENTto signal EIO error code if no events are subscribed.
Quoted 4 lines
The videobuf2 changes are untested at the moment since I didn't have a device using videobuf2 at hand right now.
Comments and questions are very welcome.
What's the behaviour of select(2) and poll(2) after this patch set when polling an fd for both read and events, when no event has been subscribed to ?
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent,
Quoted 49 lines · Laurent Pinchart
Quoted 48 lines · On Wednesday 02 October 2013 16:45:12 Sakari Ailus wrote:
Hi all,
This is the second RFC set after the initial patch that makes poll return POLLERR if no events are subscribed. There are other issues as well which these patches address.
The original RFC patch is here:
<URL:http://www.spinics.net/lists/linux-media/msg68077.html>
poll(2) and select(2) can both be used for I/O multiplexing. While both provide slightly different semantics. man 2 select:
select()andpselect()allow a program to monitor multiple
file descriptors, waiting until one or more of the file descriptors become "ready" for some class of I/O operation (e.g., input possible). A file descriptor is considered ready if it is possible to perform the corre‐ sponding I/O operation (e.g., read(2)) without blocking.The two system calls provide slightly different semantics: poll(2) can signal POLLERR related to a file handle but select(2) does not: instead, on POLLERR it sets a bit corresponding to a file handle in the read and write sets. This is somewhat confusing since with the original patch --- using select(2) would suggest that there's something to read or write instead of knowing no further exceptions are coming.
Thus, also denying polling a subdev file handle using select(2) will mean the POLLERR never gets through in any form.
So the meaningful alternatives I can think of are:
1) Use POLLERR | POLLPRI. When the last event subscription is gone and select(2) IOCTL is issued, all file descriptor sets are set for a file handle. Users of poll(2) will directly see both of the flags, making the case visible to the user immediately in some cases. On sub-devices this is obvious but on V4L2 devices the user should poll(2) (or select(2)) again to know whether there's I/O waiting to be read, written or whether buffers are ready.
2) Use POLLPRI only. While this does not differ from any regular event at the level of poll(2) or select(2), the POLLIN or POLLOUT flags are not adversely affected.
In each of the cases to ascertain oneself in a generic way of whether events cannot no longer be obtained one has to call
VIDIOC_DQEVENTIOCTL, which currently may block. A patch in the set makesVIDIOC_DQEVENTto signal EIO error code if no events are subscribed.
Quoted 7 lines
Quoted 4 lines
The videobuf2 changes are untested at the moment since I didn't have a device using videobuf2 at hand right now.
Comments and questions are very welcome.
What's the behaviour of select(2) and poll(2) after this patch set when polling an fd for both read and events, when no event has been subscribed to ?
The first one. If you're using select(2), you'll get the fd-specific bit set in all three sets. For poll(2), you'll get POLLERR and POLLPRI set for the fd.
No poll nor select can directly tell that there are no further events; instead they intend to say that the corresponding operations on the file descriptor wouldn't block. Events are a little funny in this respect; the difference of behaviour must be documented in select(2) V4L2 documentation which currently is missing entirely (I'll send a patch).
An alternative would be indeed use POLLPRI only. The only way the user would know there are no further events coming would be to use VIDIOC_DQEVENT then. As a matter of fact, this way the behaviour of select(2) would better conform to what the POSIX standard specifies but OTOH would not allow to tell about the situation using poll(2) only in any case since it'd look like the same as any event. But I don't think it'd be a problem either.
Signature · 4 lines
Regards, Sakari Ailus [email protected]