Re: [PATCH] media: v4l2-mem2mem: fix mem order in last buf
From: Hsia-Jun Li
Date: Sat Feb 17 2024 - 22:52:25 EST
On 2/17/24 02:56, Nicolas Dufresne wrote:
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
Le jeudi 15 février 2024 à 11:16 +0800, Randy Li a écrit :
On 2024/2/15 04:38, Nicolas Dufresne wrote:
Hi,
media: v4l2-mem2mem: fix mem order in last buf
mem order ? Did you mean call order ?
std::memory_order
Le dimanche 11 février 2024 à 02:04 +0800, Hsia-Jun Li a écrit :
From: "Hsia-Jun(Randy) Li" <randy.li@xxxxxxxxxxxxx>
The has_stopped property in struct v4l2_m2m_ctx is operated
without a lock protecction. Then the userspace calls to
protection When ? ~~
Access to those 3 booleans you mentioned later.
There were commenting you commit message typos, not a question.
v4l2_m2m_encoder_cmd()/v4l2_m2m_decoder_cmd() may lead to
a critical section issue.
As there is no locking, there is no critical section, perhaps a better phrasing
could help.
"concurrent accesses to shared resources can lead to unexpected or
erroneous behavior, so parts of the program where the shared resource is
accessed need to be protected in ways that avoid the concurrent access."
It didn't say we need a lock here.
I said it.
I mean, I think my description was correct. Because the critical section
don't need a lock invoked.
Signed-off-by: Hsia-Jun(Randy) Li <randy.li@xxxxxxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 75517134a5e9..f1de71031e02 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -635,9 +635,9 @@ void v4l2_m2m_last_buffer_done(struct v4l2_m2m_ctx *m2m_ctx,
struct vb2_v4l2_buffer *vbuf)
{
vbuf->flags |= V4L2_BUF_FLAG_LAST;
- vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_DONE);
-
v4l2_m2m_mark_stopped(m2m_ctx);
+
+ vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_DONE);
While it most likely fix the issue while testing, since userspace most likely
polls on that queue and don't touch the driver until the poll was signalled, I
strongly believe this is insufficient. When I look at vicodec and wave5, they
both add a layer of locking on top of the mem2mem framework to fix this issue.
Maybe a memory barrier is enough. Since vb2_buffer_done() itself would
invoke the (spin)lock operation.
When the poll() returns in userspace, the future operation for those
three boolean variables won't happen before the lock.
I think this is unfortunate, but v4l2_m2m_mark_stopped() is backed by 3 booleans
accessed in many places that aren't in any known atomic context. I think it
would be nice to remove the spurious locking in drivers and try and fix this
issue in the framework itself.
I tend to not introduce more locks here. There is a spinlock in m2m_ctx
which is a pain in the ass, something we could reuse it to save our CPU
but it just can't be access.
If you can find a way with memory barrier, but that is difficult to maintain and
I was thinking the spin lock which already existed in vb2_buffer_done()
is an implicit memory barrier. Anyway, the problem is a clear, the other
thread who would access those three 3 variables should happen after the
write operation here(v4l2_m2m_last_buffer_done()).
I would try to offer a possible memory barrier solution appended to this
version.
often breaks without noticing. I'm happy to review something that introduce
thread safety rather then depending on userspace call order. Can't disagree with
the spinlock, its been difficult in Wave5 and there is still a bug report of one
more case were we get that spinlock mixed with mutex.
I don't want to introduce a new spinlock either. But since the code here
has already used one, if we needed one, it would be a variant of spinlock.
Nicolas
Nicolas
}
EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
--
Hsia-Jun(Randy) Li