From: Alexandre Courbot <gnurou@gmail.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media <linux-media@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] media: v4l2-mem2mem: always call poll_wait() on queues
Date: Fri, 23 Oct 2020 23:22:52 +0900 [thread overview]
Message-ID: <CAAVeFu+P-3pkQGZJgvoX6cRZj8keek70sNy5ZkRozeygiLCwKQ@mail.gmail.com> (raw)
In-Reply-To: <20201022122421.133976-1-gnurou@gmail.com>
On Thu, Oct 22, 2020 at 9:24 PM Alexandre Courbot <gnurou@gmail.com> 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.
>
> 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);
This should probably include a comment to not move this back to
v4l2_m2m_poll_for_data(). I'll add one and send a v2 unless someone
points out that the premise of this patch is a bad idea to begin with.
> +
> if (req_events & (EPOLLOUT | EPOLLWRNORM | EPOLLIN | EPOLLRDNORM))
> rc = v4l2_m2m_poll_for_data(file, m2m_ctx, wait);
>
> --
> 2.29.0
>
next prev parent reply other threads:[~2020-10-23 14:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-22 12:24 [PATCH] media: v4l2-mem2mem: always call poll_wait() on queues Alexandre Courbot
2020-10-23 14:22 ` Alexandre Courbot [this message]
2020-10-30 15:09 ` Hans Verkuil
2020-11-03 8:51 ` Alexandre Courbot
2020-11-03 9:48 ` Hans Verkuil
2020-11-05 12:21 ` Alexandre Courbot
2020-11-05 12:36 ` Hans Verkuil
2020-11-05 12:52 ` Alexandre Courbot
2020-11-05 13:12 ` Hans Verkuil
2020-11-05 14:05 ` Alexandre Courbot
2020-11-11 12:41 ` Alexandre Courbot
2020-11-11 12:47 ` Hans Verkuil
2020-11-11 12:58 ` Alexandre Courbot
2020-11-23 15:18 ` Alexandre Courbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAVeFu+P-3pkQGZJgvoX6cRZj8keek70sNy5ZkRozeygiLCwKQ@mail.gmail.com \
--to=gnurou@gmail.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).