From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Alexandre Courbot <gnurou@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
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: Thu, 5 Nov 2020 13:36:34 +0100 [thread overview]
Message-ID: <695e6163-7bdc-d120-cd02-0cff6efb53ef@xs4all.nl> (raw)
In-Reply-To: <CAAVeFuL8TaArTd_fOLSSE-854n9vwpob5LxdqgHNa-bTTn5Gxg@mail.gmail.com>
On 05/11/2020 13:21, Alexandre Courbot wrote:
> On Tue, Nov 3, 2020 at 6:48 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 03/11/2020 09:51, Alexandre Courbot wrote:
>>> Hi Hans,
>>>
>>> On Sat, Oct 31, 2020 at 12:09 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>>
>>>> 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?
>>>
>>> Even without an epoll_wait() in between the behavior is visible.
>>> v4l2_m2m_poll() will be called once during the initial EPOLL_CTL_ADD
>>> and this will trigger the bug.
>>>
>>>> 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.
>>>
>>> I fully agree, this is very counter-intuitive since what basically
>>> happens is that the kernel's poll_wait() function becomes a no-op
>>> after the poll() hook of a driver is called for the first time. There
>>> is no way one can expect this behavior just from browsing the code so
>>> this is likely to affect other drivers.
>>>
>>> As for the test itself, we can easily reproduce the conditions for
>>> failure in v4l2-test-buffers.cpp's captureBufs() function, but doing
>>> so will make the streaming tests fail without being specific about the
>>> cause. Or maybe we should add another pollmode to specifically test
>>> epoll in this setup? Can I get your thoughts?
>>
>> No, just keep it as part of the poll test. Just add comments at the place
>> where it fails describing this error.
>>
>> After all, it *is* a poll() bug, so it is only fair that it is tested as
>> part of the epoll test.
>>
>> Can you call EPOLL_CTL_ADD with ev.events set to 0? And then call it again
>> with the actual value that you need? If that triggers this issue as well,
>> then that is a nice test (but perhaps EPOLL_CTL_ADD won't call poll() if
>> ev.events is 0, but perhaps EPOLLERR would work instead of 0).
>
> Yup, actually the following is enough to make v4l2-compliance -s fail
> with vicodec:
Does it also fail with vivid? I am curious to know whether this issue is
m2m specific or a more general problem.
Regards,
Hans
>
> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> index 8000db23..b63326cd 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -903,6 +903,10 @@ static int captureBufs(struct node *node, struct
> node *node_m2m_cap, const cv4l_
> epollfd = epoll_create1(0);
>
> fail_on_test(epollfd < 0);
> +
> + ev.events = 0;
> + fail_on_test(epoll_ctl(epollfd, EPOLL_CTL_ADD,
> node->g_fd(), &ev));
> +
> if (node->is_m2m)
> ev.events = EPOLLIN | EPOLLOUT | EPOLLPRI;
> else if (v4l_type_is_output(q.g_type()))
> @@ -910,7 +914,7 @@ static int captureBufs(struct node *node, struct
> node *node_m2m_cap, const cv4l_
> else
> ev.events = EPOLLIN;
> ev.data.fd = node->g_fd();
> - fail_on_test(epoll_ctl(epollfd, EPOLL_CTL_ADD,
> node->g_fd(), &ev));
> + fail_on_test(epoll_ctl(epollfd, EPOLL_CTL_MOD,
> node->g_fd(), &ev));
> }
>
> if (pollmode)
>
>>
>> The epoll_wait() will fail when this issue hits, so that's a good place
>> to add comments explaining this problem.
>>
>> There is one other place where this needs to be tested: testEvents() in
>> v4l2-test-controls.cpp: currently this only tests select(), but there
>> should be a second epoll test here as well that just tests EPOLLPRI.
>>
>> This would catch drivers that do not stream (i.e. no EPOLLIN/OUT) but
>> that do have controls (so support EPOLLPRI).
>
> I'll take a look there as well, and think about a proper comment
> before sending a patch towards you.
>
> Cheers,
> Alex.
>
next prev parent reply other threads:[~2020-11-05 12:36 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
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 [this message]
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=695e6163-7bdc-d120-cd02-0cff6efb53ef@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=gnurou@gmail.com \
--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).