[RFC v2 0/4]

[RFC v2 0/4]

[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() 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.

-- 
Kind regards,
Sakari

[RFC v2 1/4] v4l: return POLLERR on V4L2 sub-devices if no events are subscribed

[Sakari Ailus]

From: Teemu Tuominen <teemux.tuominen@intel.com>

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.

Signed-off-by: Teemu Tuominen <teemux.tuominen@intel.com>

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 <sakari.ailus@linux.intel.com>
Tested-by: Teemu Tuominen <teemux.tuominen@intel.com>
---
 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(+)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 86dcb54..b53897e 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -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);
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 996c248..7d72389 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -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;
 }
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index be05d01..a9ca2b5 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -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);
-- 
1.8.3.2

[RFC v2 2/4] v4l: vb2: Only poll for events if the user is interested in them

[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 <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 594c75e..79acf5e 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -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)))
-- 
1.8.3.2

[RFC v2 3/4] v4l: vb2: Return POLLERR when polling for events and none are subscribed

[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 <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 79acf5e..c5dc903 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -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)))
-- 
1.8.3.2

[RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Sakari Ailus]

Dequeueing events was is entirely possible even if none are subscribed,
leading to sleeping indefinitely. Fix this by returning -ENOENT when no
events are subscribed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index b53897e..553a800 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -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);
-- 
1.8.3.2

Re: [RFC v2 3/4] v4l: vb2: Return POLLERR when polling for events and none are subscribed

[Hans Verkuil]

On 10/02/13 15:45, Sakari Ailus wrote:
> 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 <sakari.ailus@linux.intel.com>
> ---
>   drivers/media/v4l2-core/videobuf2-core.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 79acf5e..c5dc903 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -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

>   	}
>
>   	if (!V4L2_TYPE_IS_OUTPUT(q->type) && !(req_events & (POLLIN | POLLRDNORM)))
>

Re: [RFC v2 0/4]

[Hans Verkuil]

On 10/02/13 15:45, 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() 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.

You can use vivi for testing.

	Hans

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Hans Verkuil]

On 10/02/13 15:45, Sakari Ailus wrote:
> Dequeueing events was is entirely possible even if none are subscribed,
> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
> events are subscribed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>   drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index b53897e..553a800 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -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?

> +		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?

> +			break;
> +		}
>
>   		ret = __v4l2_event_dequeue(fh, event);
>   	} while (ret == -ENOENT);
>

Regards,

	Hans

Re: [RFC v2 1/4] v4l: return POLLERR on V4L2 sub-devices if no events are subscribed

[Hans Verkuil]

On 10/02/13 15:45, Sakari Ailus wrote:
> From: Teemu Tuominen <teemux.tuominen@intel.com>
>
> 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.
>
> Signed-off-by: Teemu Tuominen <teemux.tuominen@intel.com>
>
> 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 <sakari.ailus@linux.intel.com>
> Tested-by: Teemu Tuominen <teemux.tuominen@intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>   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(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 86dcb54..b53897e 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -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);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 996c248..7d72389 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -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;
>   }
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index be05d01..a9ca2b5 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -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);
>

Re: [RFC v2 2/4] v4l: vb2: Only poll for events if the user is interested in them

[Hans Verkuil]

On 10/02/13 15:45, Sakari Ailus wrote:
> 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 <sakari.ailus@linux.intel.com>
> ---
>   drivers/media/v4l2-core/videobuf2-core.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 594c75e..79acf5e 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -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.

>   		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)))
>

After making that change you can add my:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Sakari Ailus]

Hi Hans,

Thanks for the comments!

Hans Verkuil wrote:
> On 10/02/13 15:45, Sakari Ailus wrote:
>> Dequeueing events was is entirely possible even if none are subscribed,
>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
>> events are subscribed.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>> b/drivers/media/v4l2-core/v4l2-event.c
>> index b53897e..553a800 100644
>> --- a/drivers/media/v4l2-core/v4l2-event.c
>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>> @@ -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?

Sure.

>> +        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.)

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 3/4] v4l: vb2: Return POLLERR when polling for events and none are subscribed

[Sakari Ailus]

Hi Hans,

Thanks for your comments.

Hans Verkuil wrote:
> On 10/02/13 15:45, Sakari Ailus wrote:
>> 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 <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/v4l2-core/videobuf2-core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 79acf5e..c5dc903 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -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.

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.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Hans Verkuil]

On 10/02/13 16:18, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the comments!
> 
> Hans Verkuil wrote:
>> On 10/02/13 15:45, Sakari Ailus wrote:
>>> Dequeueing events was is entirely possible even if none are subscribed,
>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
>>> events are subscribed.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>>> b/drivers/media/v4l2-core/v4l2-event.c
>>> index b53897e..553a800 100644
>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>> @@ -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?
> 
> Sure.
> 
>>> +        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.

OK, but then your commit message is out of sync with the actual patch since
the commit log says ENOENT.

> -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.)

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

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> On 10/02/13 16:18, Sakari Ailus wrote:
>> Hi Hans,
>>
>> Thanks for the comments!
>>
>> Hans Verkuil wrote:
>>> On 10/02/13 15:45, Sakari Ailus wrote:
>>>> Dequeueing events was is entirely possible even if none are subscribed,
>>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
>>>> events are subscribed.
>>>>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>    drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>>>> b/drivers/media/v4l2-core/v4l2-event.c
>>>> index b53897e..553a800 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>>> @@ -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?
>>
>> Sure.
>>
>>>> +        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.
>
> 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

>> -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.)
>
> 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)?

-- 
Cheers,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Sakari Ailus]

Hans Verkuil wrote:
...
>>>> +        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.
>
>> -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.)
>
> 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.

-- 
Cheers,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Laurent Pinchart]

Hi Sakari,

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 ? :-)

> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
> events are subscribed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c index b53897e..553a800 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -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);
-- 
Regards,

Laurent Pinchart

Re: [RFC v2 0/4]

[Laurent Pinchart]

Hi Sakari,

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() 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.

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 ?

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 0/4]

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> 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() 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.
>
> 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.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
>
> 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.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Hans Verkuil]

On 10/02/13 16:45, Sakari Ailus wrote:
> Hi Hans,
> 
> Hans Verkuil wrote:
>> On 10/02/13 16:18, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the comments!
>>>
>>> Hans Verkuil wrote:
>>>> On 10/02/13 15:45, Sakari Ailus wrote:
>>>>> Dequeueing events was is entirely possible even if none are subscribed,
>>>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
>>>>> events are subscribed.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> ---
>>>>>    drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>>>>> b/drivers/media/v4l2-core/v4l2-event.c
>>>>> index b53897e..553a800 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>>>> @@ -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?
>>>
>>> Sure.
>>>
>>>>> +        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.
>>
>> 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
> 
>>> -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.)
>>
>> 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)?
> 

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

[Hans Verkuil]

On 10/02/13 16:49, Sakari Ailus wrote:
> Hans Verkuil wrote:
> ...
>>>>> +        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.
>>
>>> -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.)
>>
>> 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.
> 

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