epoll and vb2_poll: can't wake_up

epoll and vb2_poll: can't wake_up

[Yi Qingliang]

Hello, I encountered a "can't wake_up" problem when use camera on imx6.

if delay some time after 'streamon' the /dev/video0, then add fd
through epoll_ctl, then the process can't be waken_up after some time.

I checked both the epoll / vb2_poll(videobuf2_core.c) code.

epoll will pass 'poll_table' structure to vb2_poll, but it only
contain valid function pointer when inserting fd.

in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
after that, every call to vb2_poll will not contain valid poll_table,
which will result in all calling to poll_wait will not work.

so if app can process frames quickly, and found frame data when
inserting fd (i.e. poll_wait will not be called or not contain valid
function pointer), it will not found valid frame in 'vb2_poll' finally
at some time, then call 'poll_wait' to expect be waken up at following
vb2_buffer_done, but no good luck.

I also checked the 'videobuf-core.c', there is no this problem.

of course, both epoll and vb2_poll are right by itself side, but the
result is we can't get new frames.

I think by epoll's implementation, the user should always call poll_wait.

and it's better to split the two actions: 'wait' and 'poll' both for
epoll framework and all epoll users, for example, v4l2.

am I right?

Yi Qingliang

Re: epoll and vb2_poll: can't wake_up

[Hans Verkuil]

On 12/29/2018 03:10 AM, Yi Qingliang wrote:
> Hello, I encountered a "can't wake_up" problem when use camera on imx6.
> 
> if delay some time after 'streamon' the /dev/video0, then add fd
> through epoll_ctl, then the process can't be waken_up after some time.
> 
> I checked both the epoll / vb2_poll(videobuf2_core.c) code.
> 
> epoll will pass 'poll_table' structure to vb2_poll, but it only
> contain valid function pointer when inserting fd.
> 
> in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
> after that, every call to vb2_poll will not contain valid poll_table,
> which will result in all calling to poll_wait will not work.
> 
> so if app can process frames quickly, and found frame data when
> inserting fd (i.e. poll_wait will not be called or not contain valid
> function pointer), it will not found valid frame in 'vb2_poll' finally
> at some time, then call 'poll_wait' to expect be waken up at following
> vb2_buffer_done, but no good luck.
> 
> I also checked the 'videobuf-core.c', there is no this problem.
> 
> of course, both epoll and vb2_poll are right by itself side, but the
> result is we can't get new frames.
> 
> I think by epoll's implementation, the user should always call poll_wait.
> 
> and it's better to split the two actions: 'wait' and 'poll' both for
> epoll framework and all epoll users, for example, v4l2.
> 
> am I right?
> 
> Yi Qingliang
> 

Can you test this patch?

Looking at what other drivers/frameworks do it seems that calling
poll_wait() at the start of the poll function is the right approach.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 70e8c3366f9c..b1809628475d 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2273,6 +2273,8 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
 	struct vb2_buffer *vb = NULL;
 	unsigned long flags;

+	poll_wait(file, &q->done_wq, wait);
+
 	if (!q->is_output && !(req_events & (EPOLLIN | EPOLLRDNORM)))
 		return 0;
 	if (q->is_output && !(req_events & (EPOLLOUT | EPOLLWRNORM)))
@@ -2329,8 +2331,6 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
 		 */
 		if (q->last_buffer_dequeued)
 			return EPOLLIN | EPOLLRDNORM;
-
-		poll_wait(file, &q->done_wq, wait);
 	}

 	/*

Re: epoll and vb2_poll: can't wake_up

[Hans Verkuil]

On 01/07/2019 02:45 PM, Hans Verkuil wrote:
> On 12/29/2018 03:10 AM, Yi Qingliang wrote:
>> Hello, I encountered a "can't wake_up" problem when use camera on imx6.
>>
>> if delay some time after 'streamon' the /dev/video0, then add fd
>> through epoll_ctl, then the process can't be waken_up after some time.
>>
>> I checked both the epoll / vb2_poll(videobuf2_core.c) code.
>>
>> epoll will pass 'poll_table' structure to vb2_poll, but it only
>> contain valid function pointer when inserting fd.
>>
>> in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
>> after that, every call to vb2_poll will not contain valid poll_table,
>> which will result in all calling to poll_wait will not work.
>>
>> so if app can process frames quickly, and found frame data when
>> inserting fd (i.e. poll_wait will not be called or not contain valid
>> function pointer), it will not found valid frame in 'vb2_poll' finally
>> at some time, then call 'poll_wait' to expect be waken up at following
>> vb2_buffer_done, but no good luck.
>>
>> I also checked the 'videobuf-core.c', there is no this problem.
>>
>> of course, both epoll and vb2_poll are right by itself side, but the
>> result is we can't get new frames.
>>
>> I think by epoll's implementation, the user should always call poll_wait.
>>
>> and it's better to split the two actions: 'wait' and 'poll' both for
>> epoll framework and all epoll users, for example, v4l2.
>>
>> am I right?
>>
>> Yi Qingliang
>>
> 
> Can you test this patch?
> 
> Looking at what other drivers/frameworks do it seems that calling
> poll_wait() at the start of the poll function is the right approach.
> 
> Regards,
> 
> 	Hans

Here is a new patch that should do the right thing for all the media
frameworks. Please test!

If it works, then I'll make a proper patch submission.

	Hans

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 70e8c3366f9c..e37443c1461f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2329,8 +2329,6 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
 		 */
 		if (q->last_buffer_dequeued)
 			return EPOLLIN | EPOLLRDNORM;
-
-		poll_wait(file, &q->done_wq, wait);
 	}

 	/*
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 78a841b83d41..e657f0949641 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -848,13 +848,12 @@ __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	__poll_t req_events = poll_requested_events(wait);
 	__poll_t res = 0;

+	poll_wait(file, &q->done_wq, wait);
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
 		struct v4l2_fh *fh = file->private_data;

 		if (v4l2_event_pending(fh))
 			res = EPOLLPRI;
-		else if (req_events & EPOLLPRI)
-			poll_wait(file, &fh->wait, wait);
 	}

 	return res | vb2_core_poll(q, file, wait);
diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
index 6974f1731529..2fd60cef4ccf 100644
--- a/drivers/media/dvb-core/dvb_vb2.c
+++ b/drivers/media/dvb-core/dvb_vb2.c
@@ -437,6 +437,7 @@ __poll_t dvb_vb2_poll(struct dvb_vb2_ctx *ctx, struct file *file,
 		      poll_table *wait)
 {
 	dprintk(3, "[%s]\n", ctx->name);
+	poll_wait(file, &ctx->vb_q.done_wq, wait);
 	return vb2_core_poll(&ctx->vb_q, file, wait);
 }

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index c71a34ae6383..b5c984dd42df 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -97,6 +97,7 @@ static __poll_t media_request_poll(struct file *filp,
 	unsigned long flags;
 	__poll_t ret = 0;

+	poll_wait(filp, &req->poll_wait, wait);
 	if (!(poll_requested_events(wait) & EPOLLPRI))
 		return 0;

@@ -110,8 +111,6 @@ static __poll_t media_request_poll(struct file *filp,
 		goto unlock;
 	}

-	poll_wait(filp, &req->poll_wait, wait);
-
 unlock:
 	spin_unlock_irqrestore(&req->lock, flags);
 	return ret;
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5bbdec55b7d7..8803eab90b6e 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -617,13 +617,14 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	__poll_t rc = 0;
 	unsigned long flags;

+	poll_wait(file, &src_q->done_wq, wait);
+	poll_wait(file, &dst_q->done_wq, wait);
+
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
 		struct v4l2_fh *fh = file->private_data;

 		if (v4l2_event_pending(fh))
 			rc = EPOLLPRI;
-		else if (req_events & EPOLLPRI)
-			poll_wait(file, &fh->wait, wait);
 		if (!(req_events & (EPOLLOUT | EPOLLWRNORM | EPOLLIN | EPOLLRDNORM)))
 			return rc;
 	}
@@ -642,11 +643,6 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		goto end;
 	}

-	spin_lock_irqsave(&src_q->done_lock, flags);
-	if (list_empty(&src_q->done_list))
-		poll_wait(file, &src_q->done_wq, wait);
-	spin_unlock_irqrestore(&src_q->done_lock, flags);
-
 	spin_lock_irqsave(&dst_q->done_lock, flags);
 	if (list_empty(&dst_q->done_list)) {
 		/*
@@ -657,8 +653,6 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 			spin_unlock_irqrestore(&dst_q->done_lock, flags);
 			return rc | EPOLLIN | EPOLLRDNORM;
 		}
-
-		poll_wait(file, &dst_q->done_wq, wait);
 	}
 	spin_unlock_irqrestore(&dst_q->done_lock, flags);

Re: epoll and vb2_poll: can't wake_up

[Yi Qingliang]

Thanks! It should work now.
BTW, I don't know if we should think about the error case before
calling poll_wait, just like not streamon.
if poll return error, does epoll framework need and how to remove
waiter for client?
for epoll framework, does it have some requirements or some tutorial
for the implementation of client's poll?

and I think it's better to split the two operation: adding waiter and
polling, not only for epoll framework, and also for all clients.

Yi Qingliang

On Mon, Jan 7, 2019 at 1:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 12/29/2018 03:10 AM, Yi Qingliang wrote:
> > Hello, I encountered a "can't wake_up" problem when use camera on imx6.
> >
> > if delay some time after 'streamon' the /dev/video0, then add fd
> > through epoll_ctl, then the process can't be waken_up after some time.
> >
> > I checked both the epoll / vb2_poll(videobuf2_core.c) code.
> >
> > epoll will pass 'poll_table' structure to vb2_poll, but it only
> > contain valid function pointer when inserting fd.
> >
> > in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
> > after that, every call to vb2_poll will not contain valid poll_table,
> > which will result in all calling to poll_wait will not work.
> >
> > so if app can process frames quickly, and found frame data when
> > inserting fd (i.e. poll_wait will not be called or not contain valid
> > function pointer), it will not found valid frame in 'vb2_poll' finally
> > at some time, then call 'poll_wait' to expect be waken up at following
> > vb2_buffer_done, but no good luck.
> >
> > I also checked the 'videobuf-core.c', there is no this problem.
> >
> > of course, both epoll and vb2_poll are right by itself side, but the
> > result is we can't get new frames.
> >
> > I think by epoll's implementation, the user should always call poll_wait.
> >
> > and it's better to split the two actions: 'wait' and 'poll' both for
> > epoll framework and all epoll users, for example, v4l2.
> >
> > am I right?
> >
> > Yi Qingliang
> >
>
> Can you test this patch?
>
> Looking at what other drivers/frameworks do it seems that calling
> poll_wait() at the start of the poll function is the right approach.
>
> Regards,
>
>         Hans
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 70e8c3366f9c..b1809628475d 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2273,6 +2273,8 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
>         struct vb2_buffer *vb = NULL;
>         unsigned long flags;
>
> +       poll_wait(file, &q->done_wq, wait);
> +
>         if (!q->is_output && !(req_events & (EPOLLIN | EPOLLRDNORM)))
>                 return 0;
>         if (q->is_output && !(req_events & (EPOLLOUT | EPOLLWRNORM)))
> @@ -2329,8 +2331,6 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
>                  */
>                 if (q->last_buffer_dequeued)
>                         return EPOLLIN | EPOLLRDNORM;
> -
> -               poll_wait(file, &q->done_wq, wait);
>         }
>
>         /*

Re: epoll and vb2_poll: can't wake_up

[Yi Qingliang]

the first patch can work on freescale's 4.1.2 kernel ! My case maybe
can't cover other changes.

in file drivers/media/v4l2-core/videobuf-core.c, the function
'videobuf_poll_stream' may return POLLERR without calling 'poll_wait',
it looks like different with 'vb2_poll''s process, maybe need further
check.

Thanks!

Yi Qingliang


On Mon, Jan 7, 2019 at 2:29 PM Yi Qingliang <niqingliang2003@gmail.com> wrote:
>
> Thanks! It should work now.
> BTW, I don't know if we should think about the error case before
> calling poll_wait, just like not streamon.
> if poll return error, does epoll framework need and how to remove
> waiter for client?
> for epoll framework, does it have some requirements or some tutorial
> for the implementation of client's poll?
>
> and I think it's better to split the two operation: adding waiter and
> polling, not only for epoll framework, and also for all clients.
>
> Yi Qingliang
>
> On Mon, Jan 7, 2019 at 1:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 12/29/2018 03:10 AM, Yi Qingliang wrote:
> > > Hello, I encountered a "can't wake_up" problem when use camera on imx6.
> > >
> > > if delay some time after 'streamon' the /dev/video0, then add fd
> > > through epoll_ctl, then the process can't be waken_up after some time.
> > >
> > > I checked both the epoll / vb2_poll(videobuf2_core.c) code.
> > >
> > > epoll will pass 'poll_table' structure to vb2_poll, but it only
> > > contain valid function pointer when inserting fd.
> > >
> > > in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
> > > after that, every call to vb2_poll will not contain valid poll_table,
> > > which will result in all calling to poll_wait will not work.
> > >
> > > so if app can process frames quickly, and found frame data when
> > > inserting fd (i.e. poll_wait will not be called or not contain valid
> > > function pointer), it will not found valid frame in 'vb2_poll' finally
> > > at some time, then call 'poll_wait' to expect be waken up at following
> > > vb2_buffer_done, but no good luck.
> > >
> > > I also checked the 'videobuf-core.c', there is no this problem.
> > >
> > > of course, both epoll and vb2_poll are right by itself side, but the
> > > result is we can't get new frames.
> > >
> > > I think by epoll's implementation, the user should always call poll_wait.
> > >
> > > and it's better to split the two actions: 'wait' and 'poll' both for
> > > epoll framework and all epoll users, for example, v4l2.
> > >
> > > am I right?
> > >
> > > Yi Qingliang
> > >
> >
> > Can you test this patch?
> >
> > Looking at what other drivers/frameworks do it seems that calling
> > poll_wait() at the start of the poll function is the right approach.
> >
> > Regards,
> >
> >         Hans
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 70e8c3366f9c..b1809628475d 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -2273,6 +2273,8 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
> >         struct vb2_buffer *vb = NULL;
> >         unsigned long flags;
> >
> > +       poll_wait(file, &q->done_wq, wait);
> > +
> >         if (!q->is_output && !(req_events & (EPOLLIN | EPOLLRDNORM)))
> >                 return 0;
> >         if (q->is_output && !(req_events & (EPOLLOUT | EPOLLWRNORM)))
> > @@ -2329,8 +2331,6 @@ __poll_t vb2_core_poll(struct vb2_queue *q, struct file *file,
> >                  */
> >                 if (q->last_buffer_dequeued)
> >                         return EPOLLIN | EPOLLRDNORM;
> > -
> > -               poll_wait(file, &q->done_wq, wait);
> >         }
> >
> >         /*