Message-ID: <4e7640fc-5764-4fb0-aea1-8b26b9c1fede@synaptics.com>
Date: Sun, 18 Feb 2024 11:51:50 +0800
From: Hsia-Jun Li <Randy.Li@...aptics.com>
To: Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc: Randy Li <ayaka@...lik.info>, mchehab@...nel.org,
 hverkuil-cisco@...all.nl, sebastian.fricke@...labora.com,
 alexious@....edu.cn, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: v4l2-mem2mem: fix mem order in last buf



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@...aptics.com>
>>>>
>>>> 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@...aptics.com>
>>>> ---
>>>>    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

Powered by blists - more mailing lists