On 22/10/2020 14:24, Alexandre Courbot wrote:
> do_poll()/do_select() seem to set the _qproc member of poll_table to
> NULL the first time they are called on a given table, making subsequent
> calls of poll_wait() on that table no-ops. This is a problem for mem2mem
> which calls poll_wait() on the V4L2 queues' waitqueues only when a
> queue-related event is requested, which may not necessarily be the case
> during the first poll.
>
> For instance, a stateful decoder is typically only interested in
> EPOLLPRI events when it starts, and will switch to listening to both
> EPOLLPRI and EPOLLIN after receiving the initial resolution change event
> and configuring the CAPTURE queue. However by the time that switch
> happens and v4l2_m2m_poll_for_data() is called for the first time,
> poll_wait() has become a no-op and the V4L2 queues waitqueues thus
> cannot be registered.
>
> Fix this by moving the registration to v4l2_m2m_poll() and do it whether
> or not one of the queue-related events are requested.
This looks good, but would it be possible to add a test for this to
v4l2-compliance? (Look for POLL_MODE_EPOLL in v4l2-test-buffers.cpp)
If I understand this right, calling EPOLL_CTL_ADD for EPOLLPRI, then
calling EPOLL_CTL_ADD for EPOLLIN/OUT would trigger this? Or does there
have to be an epoll_wait call in between?
Another reason for adding this test is that I wonder if regular capture
or output V4L2 devices don't have the same issue.
It's a very subtle bug and so adding a test for this to v4l2-compliance
would be very useful.
Regards,
Hans
>
> Signed-off-by: Alexandre Courbot <gnurou@xxxxxxxxx>
> ---
> I seem to be hitting all the polling corner cases recently! ^_^; This
> time I was wondering why epoll_wait() never returned after I received
> the first resolution change event from the vicodec stateful decoder.
> This is why - please take a look!
>
> drivers/media/v4l2-core/v4l2-mem2mem.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index b221b4e438a1..65476ef2879f 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -887,9 +887,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
> src_q = v4l2_m2m_get_src_vq(m2m_ctx);
> dst_q = v4l2_m2m_get_dst_vq(m2m_ctx);
>
> - poll_wait(file, &src_q->done_wq, wait);
> - poll_wait(file, &dst_q->done_wq, wait);
> -
> /*
> * There has to be at least one buffer queued on each queued_list, which
> * means either in driver already or waiting for driver to claim it
> @@ -922,9 +919,14 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> struct poll_table_struct *wait)
> {
> struct video_device *vfd = video_devdata(file);
> + struct vb2_queue *src_q = v4l2_m2m_get_src_vq(m2m_ctx);
> + struct vb2_queue *dst_q = v4l2_m2m_get_dst_vq(m2m_ctx);
> __poll_t req_events = poll_requested_events(wait);
> __poll_t rc = 0;
>
> + poll_wait(file, &src_q->done_wq, wait);
> + poll_wait(file, &dst_q->done_wq, wait);
> +
> if (req_events & (EPOLLOUT | EPOLLWRNORM | EPOLLIN | EPOLLRDNORM))
> rc = v4l2_m2m_poll_for_data(file, m2m_ctx, wait);
>
>
