Re: RFC: poll behavior

[Hans Verkuil <hansverk@cisco.com>]

On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> Hi,
> 
> On 06/29/2011 02:42 PM, Hans Verkuil wrote:
> > On Wednesday, June 29, 2011 14:10:44 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 06/29/2011 01:26 PM, Hans Verkuil wrote:
> 
> <Snip>
> 
> >>> 4) Proposal to change the poll behavior
> >>>
> >>> For the short term I propose that condition c is handled as follows:
> >>>
> >>> If for the filehandle passed to poll() no events have been subscribed, then
> >>> keep the old behavior (i.e. start streaming). If events have been subscribed,
> >>> however, then implement the new behavior (return POLLERR).
> >>>
> >>
> >> If events have been subscribed and no events are pending then the right
> >> behavior would be to return 0, not POLLERR, otherwise a waiting app
> >> will return from the poll immediately, or am I missing something?
> >
> > Yes and no. For select() POLLERR is ignored if you are only waiting for POLLPRI.
> >
> > But I see that that does not happen for the poll(2) API (see do_pollfd() in
> > fs/select.c). This means that POLLERR is indeed not a suitable event it
> > return. It will have to be POLLIN or POLLOUT instead.
> >
> > This is actually a real problem with poll(2): if there is no streaming in progress
> > and the driver does not support r/w, and you want to poll for just POLLPRI, then
> > POLLERR will be set, and poll(2) will always return. But select(2) will work fine.
> >
> > In other words, poll(2) and select(2) handle POLLPRI differently with respect to
> > POLLERR. What a mess. You can't really return POLLERR and support POLLPRI at the
> > same time.
> >
> 
> Ok, yet more reason to go with my proposal, but then simplified to:
> 
> When streaming has not started return POLLIN or POLLOUT (or-ed with
> POLLPRI if events are pending).

So would this be what you are looking for:

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 6ba1461..a3ce5a3 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -1371,35 +1371,37 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
  */
 unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 {
+	struct video_device *vfd = video_devdata(file);
 	unsigned long flags;
 	unsigned int ret;
 	struct vb2_buffer *vb = NULL;
+	bool have_events = false;
+	unsigned int res = 0;
+
+	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
+		struct v4l2_fh *fh = file->private_data;
+
+		/* Is this file handle subscribed to any events? */
+		have_events = fh->events != NULL;
+		if (have_events && v4l2_event_pending(fh))
+			res = POLLPRI;
+	} 
 
 	/*
 	 * Start file I/O emulator only if streaming API has not been used yet.
 	 */
 	if (q->num_buffers == 0 && q->fileio == NULL) {
-		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
-			ret = __vb2_init_fileio(q, 1);
-			if (ret)
-				return POLLERR;
-		}
-		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
-			ret = __vb2_init_fileio(q, 0);
-			if (ret)
-				return POLLERR;
-			/*
-			 * Write to OUTPUT queue can be done immediately.
-			 */
-			return POLLOUT | POLLWRNORM;
-		}
+		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
+			return res | POLLIN | POLLRDNORM;
+		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
+			return res | POLLOUT | POLLWRNORM;
 	}
 
 	/*
 	 * There is nothing to wait for if no buffers have already been queued.
 	 */
 	if (list_empty(&q->queued_list))
-		return POLLERR;
+		return have_events ? res : POLLERR;
 
 	poll_wait(file, &q->done_wq, wait);
 
@@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 
 	if (vb && (vb->state == VB2_BUF_STATE_DONE
 			|| vb->state == VB2_BUF_STATE_ERROR)) {
-		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
+		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
 			POLLIN | POLLRDNORM;
 	}
-	return 0;
+	return res;
 }
 EXPORT_SYMBOL_GPL(vb2_poll);
 

One note: the only time POLLERR is now returned is if no buffers have been queued
and no events have been subscribed to. I think that qualifies as an error condition.
I am not 100% certain, though.

Comments?

	Hans