From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: avoid using queue.h
To: Martin Pieuchot <mpi@grenadille.net>
Cc: "Kirill A. Korinsky" <kirill@korins.ky>, tech@openbsd.org
Date: Tue, 18 Mar 2025 23:29:07 +0100

On Tue, Mar 18, 2025 at 03:01:04PM GMT, Martin Pieuchot wrote:

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

Thanks for the explanation!

And yes, the main issue when detaching an uvideo device while the
stream is still running, is that uvideo_vs_cb() still accesses the
memory to process incoming frames, while uvideo_detach() and
uvideo_close() already free the memory.  At the moment I fail to find
a reliable way to make uvideo_detach() and uvideo_close() wait for
uvideo_vs_cb() to be completed ...