On Tue, Mar 18, 2025 at 12:50:38AM GMT, Kirill A. Korinsky wrote:

> > If the following diff works for your devices, I would propose to only
> > get this in, to fix the frame buffer utilization in uvideo.  Then as a
> > next step we review [1], and hopefully can bring that in next to fix
> > the detach-freeze issue.
> > 
> > [1] https://marc.info/?l=openbsd-tech&m=174180759929893&w=2
> > 
> 
> Both your diff doesn't crash system, so, I was able to make a performance
> testing of all 3 diffs. ffmpeg can read with ~35 fps from all three of them
> with 3840x2160 and 60 fps when I stream as 1920x1080. THe bottel neck is
> ffmpeg because it had hit almost 100% CPU on large stream.

Great!
 
> Regarding freezing on detach. I was able to reproduce it with your latest
> diff where you improved buffer utilization (like on snapshot), and with my
> diff and your mutex diff the system crashes with OCRed stacktrace:
> 
> 	panic: attempt to access user address 0x62d98 from ELI
> 	Stopped at  panic+0x114f  cmp   w21, #0x0
> 	TID   PID   UID   FLAGS   PFLAGS   CPU   COMMAND
> 	*1076599 3134  35    0x12    0       0     xorg
> 	338189  39674 0     0x10000 0       3     hotplugd
> 
> 	db_enter() at panic+0x1c
> 	panic() at kdata_abort+0x180
> 	do_el0_sync() at handle_el1h_sync+0x68
> 	handle_el1h_sync() at uvideo_vs_decode_stream_header+0x9c
> 	uvideo_vs_decode_stream_header() at uvideo_vs_cb+0x94
> 	uvideo_vs_cb() at usb_transfer_complete+0x220
> 	usb_transfer_complete() at xhci_event_dequeue+0x10c
> 
> Ok, next. Adding only my diff with device_lookup changes the crash to:
> 
>         panic: uvm_fault failed: fffff80002fd48 esr 96000004 far dead07fdeadbf37
> 	Stopped at  panic+0x140: cmp    w21, #0x0
> 	TID   PID   UID   FLAGS   PFLAGS   CPU   COMMAND
> 	304169 16111 35    0x12    0       7     xorg
> 
> 	db_enter() at panic+0x13c
> 	panic() at kdata_abort+0x170
> 	kdata_abort() at handle_el1h_sync+0x68
> 	handle_el1h_sync() at usbd_free_xfer+0x58
> 	usbd_free_xfer() at uvideo_vs_free_isoc+0x48
> 	uvideo_vs_free_isoc() at uvideo_close+0xe0
> 	uvideo_close() at videoclose+0x5c
> 
> but it only happens if ffplay doesn't quit on detach and consumes a lot of
> wired errors (really a lot), and I forced to kill it.
> 
> Yeah, non of diffs help with that, and I think that all consumer should be
> stop before device is detached.

In any case, a user-land application still shouldn't crash the kernel.
I'm still puzzled where exactly and why this happens.  I think we should
try to understand and tackle this specific crash next.

> Anyway, I'd like to point that I like your diff, and I think it should go
> because it improves behaviour and make it compatible with Linux.
>
> I also would like to point that I had discovered the freeze after nuke of
> the second bcopy was commited, and I recall that I can't reproduce it easy
> if I had enabled debug. I think that this issue may turn back when we
> removed bcopy again, but so far I agree that SIMPLEQ is the best option.

I'm willing to take OKs to get this peace in ;-)  Following the same
diff for reference.


Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -u -p -r1.252 uvideo.c
--- sys/dev/usb/uvideo.c	12 Mar 2025 14:08:51 -0000	1.252
+++ sys/dev/usb/uvideo.c	17 Mar 2025 20:35:40 -0000
@@ -68,6 +68,7 @@ struct uvideo_softc {
 	struct uvideo_mmap			 sc_mmap[UVIDEO_MAX_BUFFERS];
 	uint8_t					*sc_mmap_buffer;
 	size_t					 sc_mmap_buffer_size;
+	int					 sc_mmap_buffer_idx;
 	q_mmap					 sc_mmap_q;
 	int					 sc_mmap_count;
 	int					 sc_mmap_flag;
@@ -172,6 +173,7 @@ void		uvideo_vs_decode_stream_header(str
 		    uint8_t *, int);
 void		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
 		    uint8_t *, int);
+int		uvideo_get_buf(struct uvideo_softc *);
 void		uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int);
 void		uvideo_read(struct uvideo_softc *, uint8_t *, int);
 usbd_status	uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t,
@@ -2498,6 +2500,30 @@ uvideo_vs_decode_stream_header_isight(st
 	}
 }
 
+int
+uvideo_get_buf(struct uvideo_softc *sc)
+{
+	int i, idx = sc->sc_mmap_buffer_idx;
+
+	/* find a buffer which is ready for queueing */
+	for (i = 0; i < sc->sc_mmap_count; i++) {
+		if (sc->sc_mmap[sc->sc_mmap_buffer_idx].v4l2_buf.flags &
+		    V4L2_BUF_FLAG_QUEUED) {
+			idx = sc->sc_mmap_buffer_idx;
+			if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count)
+				sc->sc_mmap_buffer_idx = 0;
+			break;
+		}
+		if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count)
+			sc->sc_mmap_buffer_idx = 0;
+	}
+
+	if (i == sc->sc_mmap_count)
+		return -1;
+
+	return idx;
+}
+
 void
 uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
 {
@@ -2507,11 +2533,8 @@ uvideo_mmap_queue(struct uvideo_softc *s
 		panic("%s: mmap buffers not allocated", __func__);
 
 	/* find a buffer which is ready for queueing */
-	for (i = 0; i < sc->sc_mmap_count; i++) {
-		if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED)
-			break;
-	}
-	if (i == sc->sc_mmap_count) {
+	i = uvideo_get_buf(sc);
+	if (i == -1) {
 		DPRINTF(1, "%s: %s: mmap queue is full!\n",
 		    DEVNAME(sc), __func__);
 		return;
@@ -3552,6 +3575,8 @@ uvideo_reqbufs(void *v, struct v4l2_requ
 		    sc->sc_mmap[i].v4l2_buf.m.offset,
 		    sc->sc_mmap[i].v4l2_buf.length);
 	}
+
+	sc->sc_mmap_buffer_idx = 0;
 
 	/* tell how many buffers we have really allocated */
 	rb->count = sc->sc_mmap_count;