From: Martin Pieuchot Subject: Re: sys/uvideo: avoid using queue.h To: Marcus Glocker Cc: "Kirill A. Korinsky" , tech@openbsd.org Date: Wed, 19 Mar 2025 11:35:09 +0100 On 18/03/25(Tue) 23:29, Marcus Glocker wrote: > 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 ... There are two issues here. The first one is that uvideo_detach() doesn't "close" the USB endpoints which should abort the transfers. I'd suggest uvideo_detach() calls uvideo_close(). The second issue is that uvideo_close() doesn't close the pipe due to the wrong isdying() check. This is what aborts transfers and stop the callbacks. Here's an untested diff that illustrates all of this. Note that the kernel might now call uvideo_close() twice and some pointers might have to be cleaned. The "dying" check should be avoided as much as possible. I don't think it is necessary in the callback if the pipe is aborted during the close operation. Btw usbd_close_pipe() implies usbd_abort_pipe(). Index: dev/usb/uvideo.c =================================================================== RCS file: /cvs/src/sys/dev/usb/uvideo.c,v diff -u -p -r1.252 uvideo.c --- dev/usb/uvideo.c 12 Mar 2025 14:08:51 -0000 1.252 +++ dev/usb/uvideo.c 19 Mar 2025 10:31:28 -0000 @@ -689,7 +689,7 @@ uvideo_detach(struct device *self, int f if (sc->sc_videodev != NULL) rv = config_detach(sc->sc_videodev, flags); - uvideo_vs_free_frame(sc); + uvideo_close(sc); return (rv); } @@ -2134,9 +2134,6 @@ skip_set_alt: void uvideo_vs_close(struct uvideo_softc *sc) { - if (usbd_is_dying(sc->sc_udev)) - return; - if (sc->sc_vs_cur->bulk_running == 1) { sc->sc_vs_cur->bulk_running = 0; @@ -2152,6 +2149,10 @@ uvideo_vs_close(struct uvideo_softc *sc) sc->sc_vs_cur->pipeh = NULL; } + /* No need to mess with HW if the device is gone. */ + if (usbd_is_dying(sc->sc_udev)) + return; + if (sc->sc_vs_cur->bulk_endpoint) { /* * UVC doesn't specify how to notify a bulk-based device @@ -2171,8 +2172,7 @@ uvideo_vs_close(struct uvideo_softc *sc) usbd_delay_ms(sc->sc_udev, 100); /* switch back to default interface (turns off cam LED) */ - if (!usbd_is_dying(sc->sc_udev)) - (void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0); + (void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0); } } @@ -2281,9 +2281,6 @@ uvideo_vs_start_isoc_ixfer(struct uvideo DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__); - if (usbd_is_dying(sc->sc_udev)) - return; - for (i = 0; i < sc->sc_nframes; i++) ixfer->size[i] = sc->sc_vs_cur->psize; @@ -2314,7 +2311,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__); - if (status != USBD_NORMAL_COMPLETION) { + if (status != USBD_NORMAL_COMPLETION/*|| usbd_is_dying(sc->sc_udev)*/){ DPRINTF(1, "%s: %s: %s\n", DEVNAME(sc), __func__, usbd_errstr(status)); return;