RFC: poll behavior

Message ID 201106291543.51271.hansverk@cisco.com (mailing list archive)
State RFC, archived
Headers

Commit Message

Hans Verkuil (hansverk) June 29, 2011, 1:43 p.m. UTC
  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:


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

Comments

Hans de Goede June 29, 2011, 2:35 p.m. UTC | #1
Hi,

On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:

<snip>

>> Ok, yet more reason to go with my proposal, but then simplified to:
>>
>> When streaming has not started return POLLIN or POLLOUT (or-ed with
>> POLLPRI if events are pending).
>
> So would this be what you are looking for:
>

Yes, although I do have some comments:

> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index 6ba1461..a3ce5a3 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -1371,35 +1371,37 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
>    */
>   unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>   {
> +	struct video_device *vfd = video_devdata(file);
>   	unsigned long flags;
>   	unsigned int ret;
>   	struct vb2_buffer *vb = NULL;
> +	bool have_events = false;
> +	unsigned int res = 0;
> +
> +	if (test_bit(V4L2_FL_USES_V4L2_FH,&vfd->flags)) {
> +		struct v4l2_fh *fh = file->private_data;
> +
> +		/* Is this file handle subscribed to any events? */
> +		have_events = fh->events != NULL;
> +		if (have_events && v4l2_event_pending(fh))
> +			res = POLLPRI;
> +	}
>
>   	/*
>   	 * Start file I/O emulator only if streaming API has not been used yet.
>   	 */

Comment needs to be changed to match the new code.

>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> -			ret = __vb2_init_fileio(q, 1);
> -			if (ret)
> -				return POLLERR;
> -		}
> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> -			ret = __vb2_init_fileio(q, 0);
> -			if (ret)
> -				return POLLERR;
> -			/*
> -			 * Write to OUTPUT queue can be done immediately.
> -			 */
> -			return POLLOUT | POLLWRNORM;
> -		}
> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> +			return res | POLLIN | POLLRDNORM;
> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> +			return res | POLLOUT | POLLWRNORM;
>   	}
>
>   	/*
>   	 * There is nothing to wait for if no buffers have already been queued.
>   	 */
>   	if (list_empty(&q->queued_list))
> -		return POLLERR;
> +		return have_events ? res : POLLERR;
>

This seems more accurate to me, given that in case of select the 2 influence
different fd sets:

		return res | POLLERR;

>   	poll_wait(file,&q->done_wq, wait);
>
> @@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>
>   	if (vb&&  (vb->state == VB2_BUF_STATE_DONE
>   			|| vb->state == VB2_BUF_STATE_ERROR)) {
> -		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> +		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
>   			POLLIN | POLLRDNORM;

I would prefer to see this as:
		res |= (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
			POLLIN | POLLRDNORM;


>   	}
> -	return 0;
> +	return res;
>   }
>   EXPORT_SYMBOL_GPL(vb2_poll);
>
>
> One note: the only time POLLERR is now returned is if no buffers have been queued
> and no events have been subscribed to. I think that qualifies as an error condition.
> I am not 100% certain, though.

I think it would be better to simply wait (iow return 0) then. I know that
gstreamer for example uses separate consumer and producer threads, so it is
possible for the producer thread to wait in select while all buffers have been
handed to the (lagging) consumer thread, once the consumer thread has consumed
a buffer it will queue it, and once filled the select will return it to
the producer thread, who shoves it into the pipeline again, etc.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil (hansverk) June 30, 2011, 1:46 p.m. UTC | #2
On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
> Hi,
> 
> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> > On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> 
> <snip>
> 
> >   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> > -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> > -			ret = __vb2_init_fileio(q, 1);
> > -			if (ret)
> > -				return POLLERR;
> > -		}
> > -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> > -			ret = __vb2_init_fileio(q, 0);
> > -			if (ret)
> > -				return POLLERR;
> > -			/*
> > -			 * Write to OUTPUT queue can be done immediately.
> > -			 */
> > -			return POLLOUT | POLLWRNORM;
> > -		}
> > +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> > +			return res | POLLIN | POLLRDNORM;
> > +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> > +			return res | POLLOUT | POLLWRNORM;
> >   	}
> >
> >   	/*
> >   	 * There is nothing to wait for if no buffers have already been 
queued.
> >   	 */
> >   	if (list_empty(&q->queued_list))
> > -		return POLLERR;
> > +		return have_events ? res : POLLERR;
> >
> 
> This seems more accurate to me, given that in case of select the 2 influence
> different fd sets:
> 
> 		return res | POLLERR;

Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
even if you only want to wait on POLLPRI. That's a perfectly valid thing to 
do. An alternative is to just not use POLLERR and return res|POLLIN or res|
POLLOUT depending on V4L2_TYPE_IS_OUTPUT().

Another option is to just return res (which is your suggestion below as well).
I think this is also a reasonable approach. It would in fact allow one thread 
to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
filehandle. And the other thread would return from poll(2) as soon as the 
first frame becomes available.

This also leads to another ambiguity with poll(): what should poll do if 
another filehandle started streaming? So fh1 called STREAMON (and so becomes 
the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
should fh2 wake up? Is fh2 allowed to call DQBUF?

To be honest, I think vb2 should keep track of the filehandle that started 
streaming rather than leaving that to drivers, but that's a separate issue.

I really wonder whether we should ever use POLLERR at all: it is extremely
vague how it should be interpreted, and it doesn't actually tell you what is 
wrong. And is it really an error if you poll on a non-streaming node?

As shown by the use-case above, I don't think it is an error at all.

The default poll mask that is returned when the device doesn't support poll
is #define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM).

So if they don't return POLLERR for such devices, perhaps we shouldn't either.

Regards,

	Hans

> 
> >   	poll_wait(file,&q->done_wq, wait);
> >
> > @@ -1414,10 +1416,10 @@ unsigned int vb2_poll(struct vb2_queue *q, struct 
file *file, poll_table *wait)
> >
> >   	if (vb&&  (vb->state == VB2_BUF_STATE_DONE
> >   			|| vb->state == VB2_BUF_STATE_ERROR)) {
> > -		return (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> > +		return res | (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | 
POLLWRNORM :
> >   			POLLIN | POLLRDNORM;
> 
> I would prefer to see this as:
> 		res |= (V4L2_TYPE_IS_OUTPUT(q->type)) ? POLLOUT | POLLWRNORM :
> 			POLLIN | POLLRDNORM;
> 
> 
> >   	}
> > -	return 0;
> > +	return res;
> >   }
> >   EXPORT_SYMBOL_GPL(vb2_poll);
> >
> >
> > One note: the only time POLLERR is now returned is if no buffers have been 
queued
> > and no events have been subscribed to. I think that qualifies as an error 
condition.
> > I am not 100% certain, though.
> 
> I think it would be better to simply wait (iow return 0) then. I know that
> gstreamer for example uses separate consumer and producer threads, so it is
> possible for the producer thread to wait in select while all buffers have 
been
> handed to the (lagging) consumer thread, once the consumer thread has 
consumed
> a buffer it will queue it, and once filled the select will return it to
> the producer thread, who shoves it into the pipeline again, etc.
> 
> Regards,
> 
> Hans
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab June 30, 2011, 8:35 p.m. UTC | #3
Em 30-06-2011 10:46, Hans Verkuil escreveu:
> On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
>> Hi,
>>
>> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
>>> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
>>
>> <snip>
>>
>>>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
>>> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
>>> -			ret = __vb2_init_fileio(q, 1);
>>> -			if (ret)
>>> -				return POLLERR;
>>> -		}
>>> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
>>> -			ret = __vb2_init_fileio(q, 0);
>>> -			if (ret)
>>> -				return POLLERR;
>>> -			/*
>>> -			 * Write to OUTPUT queue can be done immediately.
>>> -			 */
>>> -			return POLLOUT | POLLWRNORM;
>>> -		}
>>> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
>>> +			return res | POLLIN | POLLRDNORM;
>>> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
>>> +			return res | POLLOUT | POLLWRNORM;

It is wrong to return POLLIN/POLLOUT if the stream hasn't started yet. You should
return it only when data is ready. Otherwise you should return 0.

>>>   	}
>>>
>>>   	/*
>>>   	 * There is nothing to wait for if no buffers have already been 
> queued.
>>>   	 */
>>>   	if (list_empty(&q->queued_list))
>>> -		return POLLERR;
>>> +		return have_events ? res : POLLERR;
>>>
>>
>> This seems more accurate to me, given that in case of select the 2 influence
>> different fd sets:
>>
>> 		return res | POLLERR;
> 
> Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
> even if you only want to wait on POLLPRI.

Yes, but this is the right thing to do: an error condition has happened. If you're
in doubt, think that poll() is being used for a text file or a socket: if the connection
has dropped, or there's a problem to access the file, poll() needs to return, as there is
a condition error that needs to be handled.

> That's a perfectly valid thing to 
> do. An alternative is to just not use POLLERR and return res|POLLIN or res|
> POLLOUT depending on V4L2_TYPE_IS_OUTPUT().

You should only rise POLLERR if a problem happened at the events delivery or at
the device streaming.

> Another option is to just return res (which is your suggestion below as well).
> I think this is also a reasonable approach. It would in fact allow one thread 
> to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
> filehandle. And the other thread would return from poll(2) as soon as the 
> first frame becomes available.
> 
> This also leads to another ambiguity with poll(): what should poll do if 
> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> should fh2 wake up? Is fh2 allowed to call DQBUF?

IMO, both fh's should get the same results. This is what happens if you're
writing into a file and two or more processes are selecting at the EOF.

Anyway, changing from the current behavior may break applications.

> To be honest, I think vb2 should keep track of the filehandle that started 
> streaming rather than leaving that to drivers, but that's a separate issue.
> 
> I really wonder whether we should ever use POLLERR at all: it is extremely
> vague how it should be interpreted, and it doesn't actually tell you what is 
> wrong. And is it really an error if you poll on a non-streaming node?

See above. You need to rise it if, for example, an error occurred, and no data
will be ready for read(), write() or DQEVENT. That's the reason why POLLERR
exists.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil July 1, 2011, 9:45 a.m. UTC | #4
On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> Em 30-06-2011 10:46, Hans Verkuil escreveu:
> > On Wednesday, June 29, 2011 16:35:04 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 06/29/2011 03:43 PM, Hans Verkuil wrote:
> >>> On Wednesday, June 29, 2011 15:07:14 Hans de Goede wrote:
> >>
> >> <snip>
> >>
> >>>   	if (q->num_buffers == 0&&  q->fileio == NULL) {
> >>> -		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ)) {
> >>> -			ret = __vb2_init_fileio(q, 1);
> >>> -			if (ret)
> >>> -				return POLLERR;
> >>> -		}
> >>> -		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE)) {
> >>> -			ret = __vb2_init_fileio(q, 0);
> >>> -			if (ret)
> >>> -				return POLLERR;
> >>> -			/*
> >>> -			 * Write to OUTPUT queue can be done immediately.
> >>> -			 */
> >>> -			return POLLOUT | POLLWRNORM;
> >>> -		}
> >>> +		if (!V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_READ))
> >>> +			return res | POLLIN | POLLRDNORM;
> >>> +		if (V4L2_TYPE_IS_OUTPUT(q->type)&&  (q->io_modes&  VB2_WRITE))
> >>> +			return res | POLLOUT | POLLWRNORM;
> 
> It is wrong to return POLLIN/POLLOUT if the stream hasn't started yet. You should
> return it only when data is ready. Otherwise you should return 0.
> 
> >>>   	}
> >>>
> >>>   	/*
> >>>   	 * There is nothing to wait for if no buffers have already been 
> > queued.
> >>>   	 */
> >>>   	if (list_empty(&q->queued_list))
> >>> -		return POLLERR;
> >>> +		return have_events ? res : POLLERR;
> >>>
> >>
> >> This seems more accurate to me, given that in case of select the 2 influence
> >> different fd sets:
> >>
> >> 		return res | POLLERR;
> > 
> > Hmm. The problem is that the poll(2) API will always return if POLLERR is set, 
> > even if you only want to wait on POLLPRI.
> 
> Yes, but this is the right thing to do: an error condition has happened. If you're
> in doubt, think that poll() is being used for a text file or a socket: if the connection
> has dropped, or there's a problem to access the file, poll() needs to return, as there is
> a condition error that needs to be handled.
> 
> > That's a perfectly valid thing to 
> > do. An alternative is to just not use POLLERR and return res|POLLIN or res|
> > POLLOUT depending on V4L2_TYPE_IS_OUTPUT().
> 
> You should only rise POLLERR if a problem happened at the events delivery or at
> the device streaming.
> 
> > Another option is to just return res (which is your suggestion below as well).
> > I think this is also a reasonable approach. It would in fact allow one thread 
> > to call poll(2) and another thread to call REQBUFS/QBUF/STREAMON on the same 
> > filehandle. And the other thread would return from poll(2) as soon as the 
> > first frame becomes available.
> > 
> > This also leads to another ambiguity with poll(): what should poll do if 
> > another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> > the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> > should fh2 wake up? Is fh2 allowed to call DQBUF?
> 
> IMO, both fh's should get the same results. This is what happens if you're
> writing into a file and two or more processes are selecting at the EOF.

Yes, but multiple filehandles are allowed to write/read from a file at the
same time. That's not true for V4L2. Only one filehandle can do I/O at a time.

I'm going to look into changing fs/select.c so that the poll driver function
can actually see the event mask provided by the application.

Regards,

	Hans

> 
> Anyway, changing from the current behavior may break applications.
> 
> > To be honest, I think vb2 should keep track of the filehandle that started 
> > streaming rather than leaving that to drivers, but that's a separate issue.
> > 
> > I really wonder whether we should ever use POLLERR at all: it is extremely
> > vague how it should be interpreted, and it doesn't actually tell you what is 
> > wrong. And is it really an error if you poll on a non-streaming node?
> 
> See above. You need to rise it if, for example, an error occurred, and no data
> will be ready for read(), write() or DQEVENT. That's the reason why POLLERR
> exists.
> 
> Cheers,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab July 1, 2011, 11:59 a.m. UTC | #5
Em 01-07-2011 06:45, Hans Verkuil escreveu:
> On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:

>>> This also leads to another ambiguity with poll(): what should poll do if 
>>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
>>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
>>> should fh2 wake up? Is fh2 allowed to call DQBUF?
>>
>> IMO, both fh's should get the same results. This is what happens if you're
>> writing into a file and two or more processes are selecting at the EOF.
> 
> Yes, but multiple filehandles are allowed to write/read from a file at the
> same time. That's not true for V4L2. Only one filehandle can do I/O at a time.

Actually, this is not quite true currently, as you could, for example use one fd
for QBUF, and another for DQBUF, with the current behavior, but, with luck,
no applications are doing weird things like that. Yet, tests are needed to avoid
breaking something, if we're willing to change it.

> I'm going to look into changing fs/select.c so that the poll driver function
> can actually see the event mask provided by the application.

Why? A POLLERR should be notified, whatever mask is there, as the application
may need to abort (for example, in cases like hardware removal).

Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab July 1, 2011, 12:03 p.m. UTC | #6
Em 01-07-2011 08:59, Mauro Carvalho Chehab escreveu:
> Em 01-07-2011 06:45, Hans Verkuil escreveu:
>> On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> 
>>>> This also leads to another ambiguity with poll(): what should poll do if 
>>>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
>>>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
>>>> should fh2 wake up? Is fh2 allowed to call DQBUF?
>>>
>>> IMO, both fh's should get the same results. This is what happens if you're
>>> writing into a file and two or more processes are selecting at the EOF.
>>
>> Yes, but multiple filehandles are allowed to write/read from a file at the
>> same time. That's not true for V4L2. Only one filehandle can do I/O at a time.
> 
> Actually, this is not quite true currently, as you could, for example use one fd
> for QBUF, and another for DQBUF, with the current behavior, but, with luck,
> no applications are doing weird things like that. Yet, tests are needed to avoid
> breaking something, if we're willing to change it.
> 
>> I'm going to look into changing fs/select.c so that the poll driver function
>> can actually see the event mask provided by the application.
> 
> Why? A POLLERR should be notified, whatever mask is there, as the application
> may need to abort (for example, in cases like hardware removal).

I was too quick on my last comment. Your patch is clear: you want to start it only
if the poll mask has "in" or "out" file descriptiors. Ok, this makes sense to me.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil July 1, 2011, 12:10 p.m. UTC | #7
On Friday, July 01, 2011 13:59:14 Mauro Carvalho Chehab wrote:
> Em 01-07-2011 06:45, Hans Verkuil escreveu:
> > On Thursday, June 30, 2011 22:35:15 Mauro Carvalho Chehab wrote:
> 
> >>> This also leads to another ambiguity with poll(): what should poll do if 
> >>> another filehandle started streaming? So fh1 called STREAMON (and so becomes 
> >>> the 'owner' of the stream), and you poll on fh2. If a frame becomes available, 
> >>> should fh2 wake up? Is fh2 allowed to call DQBUF?
> >>
> >> IMO, both fh's should get the same results. This is what happens if you're
> >> writing into a file and two or more processes are selecting at the EOF.
> > 
> > Yes, but multiple filehandles are allowed to write/read from a file at the
> > same time. That's not true for V4L2. Only one filehandle can do I/O at a time.
> 
> Actually, this is not quite true currently, as you could, for example use one fd
> for QBUF, and another for DQBUF, with the current behavior, but, with luck,
> no applications are doing weird things like that. Yet, tests are needed to avoid
> breaking something, if we're willing to change it.

Many drivers prevent such things. But it is very much up to the driver. My
gut-feeling is that it is a 50-50 split between drivers that allow it (whether
it actually works is another matter) and drivers that prevent this and return
-EBUSY.

> > I'm going to look into changing fs/select.c so that the poll driver function
> > can actually see the event mask provided by the application.
> 
> Why? A POLLERR should be notified, whatever mask is there, as the application
> may need to abort (for example, in cases like hardware removal).

If an application isn't interested in POLLIN or POLLOUT, but just POLLPRI, then
poll() doesn't need to start streaming.

It obviously makes no sense to start streaming if the application isn't polling
for input.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

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