On 17/03/25(Mon) 20:25, Marcus Glocker wrote:
> On Mon, Mar 17, 2025 at 10:34:22AM GMT, Martin Pieuchot wrote:
> 
> > We definitively don't need a new queueing method for the moment.
>  
> Thanks for the clarification!
> 
> > > In any case, the current issue of mostly re-using the first buffer isn't
> > > a SIMPLEQ issue as such.  The queue.h functions should also provide
> > > FIFO.  The problem is that we're currently using a pretty dumb loop to
> > > find a free buffer in uvideo(4).  It always starts from the top of the
> > > buffer list, and looks for the first buffer which is V4L2_BUF_FLAG_QUEUED
> > > flagged.  Obviously, if the first buffer gets processed and re-queued
> > > fast enough, which is usually the case, we will just select the first> buffer again, and so mostly use only one buffer.  This problem could be> also fixed by using a global index for the frame buffer, and make sure
> > > that it always iterates over the entire buffer.
> > 
> > What is the problem?
> > 
> > It's not clear to me what it is and using mutexes won't help as the code is
> > already running under KERNEL_LOCK().  The SIMPLEQ is already working as
> > a FIFO since descriptors are added at the tail and removed from the
> > head.
> 
> There is one clear problem, and one unclear problem which I tried to
> address by the last diff:
> 
> 1. The frame buffer utilization in uvideo sucks.  We know how to fix it.
>    Nothing to discuss on that.
> 
> 2. Kirill is facing some issues during uivdeo testing (system freezes
>    on uvideo device detach for example), where he suspects that it might
>    be related to SIMPLEQ operations aren't thread safe.  I think we
>    never could exactly prove that those issues are really caused by
>    threads messing up the frame buffer queue.
> 
> > > And the worry you had about the thread safeness of SIMPLEQ can also be
> > > solved by using mutxes.
> > 
> > I fail to see how this will be different knowing that the KERNEL_LOCK()
> > is held..  Please do not add locks without the corresponding
> > annotations that explains which data structure they are protecting.
> 
> The idea was to protect the frame buffer queue in uvideo in case that 
> the theory of the above point 2 would be valid.  But you are saying,
> that code anyway runs under the KERNEL_LOCK(), and hence using mutexes
> isn't required at all?

Exactly.  If there's a race it has to do with sleeping points.  The
problems is that uvideo_detach() doesn't synchronize with other threads
and instead does a sleep by calling usbd_delay_ms().  Sadly there's
nothing that guarantee that after the sleep it is safe to free memory.