media: v4l2-mem2mem: fix mem order in last buf

  From: "Hsia-Jun(Randy) Li" <randy.li@synaptics.com>

The has_stopped property in struct v4l2_m2m_ctx is operated
without a lock protecction. Then the userspace calls to
v4l2_m2m_encoder_cmd()/v4l2_m2m_decoder_cmd() may lead to
a critical section issue.

Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Hi,

>  media: v4l2-mem2mem: fix mem order in last buf

mem order ? Did you mean call order ?

Le dimanche 11 février 2024 à 02:04 +0800, Hsia-Jun Li a écrit :
> From: "Hsia-Jun(Randy) Li" <randy.li@synaptics.com>
> 
> The has_stopped property in struct v4l2_m2m_ctx is operated
> without a lock protecction. Then the userspace calls to
                 protection   When ?                   ~~

> 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.

> 
> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.

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.

Nicolas

>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>
  

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@synaptics.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.
>> 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.

>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.
>
> Nicolas
>
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>>
  

On 15/02/2024 04:16, Randy Li wrote:
> 
> 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@synaptics.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.
>>> 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.
> 
>>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.

I think the root cause is something else.

Let me say first of all that swapping the order of the two calls does make sense:
before returning the buffer you want to mark the queue as stopped.

But the real problem is that for drivers using the mem2mem framework the streaming
ioctls can be serialized with a different lock than the VIDIOC_DE/ENCODER_CMD ioctls.

The reason for that is that those two ioctls are not marked with INFO_FL_QUEUE,
but I think they should. These ioctls are really part of the streaming ioctls
and should all use the same lock.

Note that for many drivers the same mutex is used for the streaming ioctls as for
all other ioctls, but it looks like at least the venus driver uses separate mutexes.

With that change in v4l2-core/v4l2-ioctl.c I don't believe any locking is needed,
since it should always be serialized by the same top-level mutex.

The v4l2_ioctl_get_lock() function in v4l2-ioctl.c is the one that selects which
mutex to use for a given ioctl.

Regards,

	Hans

>>
>> Nicolas
>>
>>>   }
>>>   EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>>>   
>
  

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@synaptics.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.

> 
> > > Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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
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.

Nicolas

> > 
> > Nicolas
> > 
> > >   }
> > >   EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
> > >   
>
  

Le jeudi 15 février 2024 à 09:41 +0100, Hans Verkuil a écrit :
> On 15/02/2024 04:16, Randy Li wrote:
> > 
> > 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@synaptics.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.
> > > > 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.
> > 
> > > > Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.
> 
> I think the root cause is something else.
> 
> Let me say first of all that swapping the order of the two calls does make sense:
> before returning the buffer you want to mark the queue as stopped.
> 
> But the real problem is that for drivers using the mem2mem framework the streaming
> ioctls can be serialized with a different lock than the VIDIOC_DE/ENCODER_CMD ioctls.
> 
> The reason for that is that those two ioctls are not marked with INFO_FL_QUEUE,
> but I think they should. These ioctls are really part of the streaming ioctls
> and should all use the same lock. 
> 
> Note that for many drivers the same mutex is used for the streaming ioctls as for
> all other ioctls, but it looks like at least the venus driver uses separate mutexes.
> 
> With that change in v4l2-core/v4l2-ioctl.c I don't believe any locking is needed,
> since it should always be serialized by the same top-level mutex.
> 
> The v4l2_ioctl_get_lock() function in v4l2-ioctl.c is the one that selects which
> mutex to use for a given ioctl.

There was no way to comply with the spec without accessing that state in the irq
thread in Wave5. In that case, we need to decide if we continue or cancel a
dynamic resolution change.


		if (!v4l2_m2m_has_stopped(m2m_ctx)) {
                        switch_state(inst, VPU_INST_STATE_STOP);

                        if (dec_info.sequence_changed)
                                handle_dynamic_resolution_change(inst);
                        else
                                send_eos_event(inst);

                        flag_last_buffer_done(inst);
                }

That forced us to introduce a spinlock to protect that state in that driver.

As for locking cmd_start, that might help, its certainly a behaviour change and
will have to be taken with care. How does the ioctl lock interact with blocking
QBUF notably ?

Nicolas

> 
> Regards,
> 
> 	Hans
> 
> > > 
> > > Nicolas
> > > 
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
> > > >   
> > 
> 
>
  

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

On 2/17/24 03:09, 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 à 09:41 +0100, Hans Verkuil a écrit :
>> On 15/02/2024 04:16, Randy Li wrote:
>>>
>>> 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@synaptics.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.
>>>>> 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.
>>>
>>>>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.
>>
>> I think the root cause is something else.
>>
>> Let me say first of all that swapping the order of the two calls does make sense:
>> before returning the buffer you want to mark the queue as stopped.
>>
>> But the real problem is that for drivers using the mem2mem framework the streaming
>> ioctls can be serialized with a different lock than the VIDIOC_DE/ENCODER_CMD ioctls.
>>
>> The reason for that is that those two ioctls are not marked with INFO_FL_QUEUE,
>> but I think they should. These ioctls are really part of the streaming ioctls
>> and should all use the same lock.
>>
>> Note that for many drivers the same mutex is used for the streaming ioctls as for
>> all other ioctls, but it looks like at least the venus driver uses separate mutexes.
>>

But I saw many drivers didn't assign that q_lock here. I am an one.
Since it is an optional mutex lock.

>> With that change in v4l2-core/v4l2-ioctl.c I don't believe any locking is needed,
>> since it should always be serialized by the same top-level mutex.
>>
>> The v4l2_ioctl_get_lock() function in v4l2-ioctl.c is the one that selects which
>> mutex to use for a given ioctl.
> 
> There was no way to comply with the spec without accessing that state in the irq
> thread in Wave5. In that case, we need to decide if we continue or cancel a
> dynamic resolution change.
> 
> 
>                  if (!v4l2_m2m_has_stopped(m2m_ctx)) {
>                          switch_state(inst, VPU_INST_STATE_STOP);
> 
>                          if (dec_info.sequence_changed)
>                                  handle_dynamic_resolution_change(inst);
>                          else
>                                  send_eos_event(inst);
> 
>                          flag_last_buffer_done(inst);
>                  }
> 
> That forced us to introduce a spinlock to protect that state in that driver.
> 
Usually we won't do buffer operation in the irq handler context. It 
causes too many times,

But that makes a point. Sometimes, I can't just introduce that a mutex, 
while most of the m2m context has acquired a spinlock.
> As for locking cmd_start, that might help, its certainly a behaviour change and
> will have to be taken with care. How does the ioctl lock interact with blocking
> QBUF notably ?
> 
> Nicolas
> 
>>
>> Regards,
>>
>>        Hans
>>
>>>>
>>>> Nicolas
>>>>
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>>>>>
>>>
>>
>>
>
  

Le mercredi 21 février 2024 à 18:37 +0800, Hsia-Jun Li a écrit :
> 
> On 2/17/24 03:09, 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 à 09:41 +0100, Hans Verkuil a écrit :
> > > On 15/02/2024 04:16, Randy Li wrote:
> > > > 
> > > > 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@synaptics.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.
> > > > > > 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.
> > > > 
> > > > > > Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.
> > > 
> > > I think the root cause is something else.
> > > 
> > > Let me say first of all that swapping the order of the two calls does make sense:
> > > before returning the buffer you want to mark the queue as stopped.
> > > 
> > > But the real problem is that for drivers using the mem2mem framework the streaming
> > > ioctls can be serialized with a different lock than the VIDIOC_DE/ENCODER_CMD ioctls.
> > > 
> > > The reason for that is that those two ioctls are not marked with INFO_FL_QUEUE,
> > > but I think they should. These ioctls are really part of the streaming ioctls
> > > and should all use the same lock.
> > > 
> > > Note that for many drivers the same mutex is used for the streaming ioctls as for
> > > all other ioctls, but it looks like at least the venus driver uses separate mutexes.
> > > 
> 
> But I saw many drivers didn't assign that q_lock here. I am an one.
> Since it is an optional mutex lock.
> 
> > > With that change in v4l2-core/v4l2-ioctl.c I don't believe any locking is needed,
> > > since it should always be serialized by the same top-level mutex.
> > > 
> > > The v4l2_ioctl_get_lock() function in v4l2-ioctl.c is the one that selects which
> > > mutex to use for a given ioctl.
> > 
> > There was no way to comply with the spec without accessing that state in the irq
> > thread in Wave5. In that case, we need to decide if we continue or cancel a
> > dynamic resolution change.
> > 
> > 
> >                  if (!v4l2_m2m_has_stopped(m2m_ctx)) {
> >                          switch_state(inst, VPU_INST_STATE_STOP);
> > 
> >                          if (dec_info.sequence_changed)
> >                                  handle_dynamic_resolution_change(inst);
> >                          else
> >                                  send_eos_event(inst);
> > 
> >                          flag_last_buffer_done(inst);
> >                  }
> > 
> > That forced us to introduce a spinlock to protect that state in that driver.
> > 
> Usually we won't do buffer operation in the irq handler context. It 
> causes too many times,

I took this one out of context, but this is inside a threaded IRQ handle, we
indeed have too much work and state to match with how Wave5 firmware behave As
discuss on IRC, not being able to see the Synaptics driver you are referring to
has its limitation.

> 
> But that makes a point. Sometimes, I can't just introduce that a mutex, 
> while most of the m2m context has acquired a spinlock.

In wave5, we had to use a spinlock as the framework holds its own spinlock while
calling job_ready() (can't mix lock mutex while a spinlock is held), and we need
thread safety in that call in order to use that state to make the right
decisions. On agressive stress test, we were getting stalls due to decisions
being made with partially written state.

> > As for locking cmd_start, that might help, its certainly a behaviour change and
> > will have to be taken with care. How does the ioctl lock interact with blocking
> > QBUF notably ?
> > 
> > Nicolas
> > 
> > > 
> > > Regards,
> > > 
> > >        Hans
> > > 
> > > > > 
> > > > > Nicolas
> > > > > 
> > > > > >    }
> > > > > >    EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
> > > > > > 
> > > > 
> > > 
> > > 
> > 
>
  

On 2/21/24 23:32, 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 mercredi 21 février 2024 à 18:37 +0800, Hsia-Jun Li a écrit :
>>
>> On 2/17/24 03:09, 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 à 09:41 +0100, Hans Verkuil a écrit :
>>>> On 15/02/2024 04:16, Randy Li wrote:
>>>>>
>>>>> 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@synaptics.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.
>>>>>>> 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.
>>>>>
>>>>>>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@synaptics.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.
>>>>
>>>> I think the root cause is something else.
>>>>
>>>> Let me say first of all that swapping the order of the two calls does make sense:
>>>> before returning the buffer you want to mark the queue as stopped.
>>>>
>>>> But the real problem is that for drivers using the mem2mem framework the streaming
>>>> ioctls can be serialized with a different lock than the VIDIOC_DE/ENCODER_CMD ioctls.
>>>>
>>>> The reason for that is that those two ioctls are not marked with INFO_FL_QUEUE,
>>>> but I think they should. These ioctls are really part of the streaming ioctls
>>>> and should all use the same lock.
>>>>
>>>> Note that for many drivers the same mutex is used for the streaming ioctls as for
>>>> all other ioctls, but it looks like at least the venus driver uses separate mutexes.
>>>>
>>
>> But I saw many drivers didn't assign that q_lock here. I am an one.
>> Since it is an optional mutex lock.
>>
>>>> With that change in v4l2-core/v4l2-ioctl.c I don't believe any locking is needed,
>>>> since it should always be serialized by the same top-level mutex.
>>>>
>>>> The v4l2_ioctl_get_lock() function in v4l2-ioctl.c is the one that selects which
>>>> mutex to use for a given ioctl.
>>>
>>> There was no way to comply with the spec without accessing that state in the irq
>>> thread in Wave5. In that case, we need to decide if we continue or cancel a
>>> dynamic resolution change.
>>>
>>>
>>>                   if (!v4l2_m2m_has_stopped(m2m_ctx)) {
>>>                           switch_state(inst, VPU_INST_STATE_STOP);
>>>
>>>                           if (dec_info.sequence_changed)
>>>                                   handle_dynamic_resolution_change(inst);
>>>                           else
>>>                                   send_eos_event(inst);
>>>
>>>                           flag_last_buffer_done(inst);
>>>                   }
>>>
>>> That forced us to introduce a spinlock to protect that state in that driver.
>>>
>> Usually we won't do buffer operation in the irq handler context. It
>> causes too many times,
> 
> I took this one out of context, but this is inside a threaded IRQ handle, we
> indeed have too much work and state to match with how Wave5 firmware behave. As
> discuss on IRC, not being able to see the Synaptics driver you are referring to
> has its limitation.
> 
I think it is common sense not sleep lock(mutex) inside irq handler.
Besides, it is not wrong to keep the runtime of the irq handler short. 
You are occupied a CPU core non-preempt.

I could show you a few slices of our driver while it is not formal 
released. The released version could be mediocre one.


static irqreturn_t syna_vpu_isr(int irq, void *dev_id) 

{ 

         struct syna_vpu_dev *vpu = dev_id; 

 

         vpu_srv_isr_done(vpu->srv); 

 

         return IRQ_HANDLED; 

}

The real work in a separated thread(work queue):
static void syna_vdec_v4g_worker(struct work_struct *work)
{
..
         if (ctrl->status.flags & BERLIN_VPU_STATUS_WAIT_INTERRUPTER) { 

                 ctrl->status.flags &= 
(~BERLIN_VPU_STATUS_WAIT_INTERRUPTER);
                 ret = vpu_srv_wait_isr(vpu->srv, 
DEC_V4G_TIMEOUT_DELAY);
                 if (ret) { 

                         ret = syna_vdec_hw_abort(ctx); 

                         /** 

                          * NOTE: if it failed, keep the device power 
here
                          * then we could dump registers from the 
device.
                          */ 

                         if (ret) 

                                 goto bail; 

                 } 

                 goto decoding; 

         } 

 

         pm_runtime_put(vpu->dev);
..
}

>>
>> But that makes a point. Sometimes, I can't just introduce that a mutex,
>> while most of the m2m context has acquired a spinlock.
> 
> In wave5, we had to use a spinlock as the framework holds its own spinlock while
> calling job_ready() (can't mix lock mutex while a spinlock is held), and we need
Yes, in __v4l2_m2m_try_queue()
> thread safety in that call in order to use that state to make the right
> decisions. On agressive stress test, we were getting stalls due to decisions
> being made with partially written state.
> 
For those three variables, only the has_stopped matters here.
I am not sure which case would need more lock acquired here?
CMD_STOP a context after it has been started?
They would always go through rdy_spinlock.
>>> As for locking cmd_start, that might help, its certainly a behaviour change and
>>> will have to be taken with care. How does the ioctl lock interact with blocking
>>> QBUF notably ?
QBUF? m2m would use rdy_spinlock here.

I didn't really get your case?
>>>
>>> Nicolas
>>>
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>>
>>>>>> Nicolas
>>>>>>
>>>>>>>     }
>>>>>>>     EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>