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: Mon, 17 Mar 2025 20:25:57 +0100

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?