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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
