Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] media: v4l2-mem2mem: always call poll_wait() on queues
To:     Alexandre Courbot <gnurou@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201022122421.133976-1-gnurou@gmail.com>
From:   Hans Verkuil <hverkuil-cisco@xs4all.nl>
Message-ID: <c6454292-935b-f14a-e743-838ccabc6590@xs4all.nl>
Date:   Fri, 30 Oct 2020 16:09:04 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20201022122421.133976-1-gnurou@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@gmail.com>
> ---
> 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);
>  
>