Download raw body.
sys/uvideo: avoid using queue.h
On Mon, Mar 17, 2025 at 12:13:53AM GMT, Kirill A. Korinsky wrote:
> On Sun, 16 Mar 2025 23:07:18 +0100,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> >
> > Well, I'm not sure if we require a new queuing method in queue.h.
> > Probably there are other people who can judge better on this.
> >
>
> I think that we should keep that discussion for some time in the future.
>
> > 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.
> >
> > And the worry you had about the thread safeness of SIMPLEQ can also be
> > solved by using mutxes.
> >
> > Following a diff which demonstrates this, and works for me what I could
> > test so far.
> >
> > Though, I guess your method comes with a better performance. I had no
> > time to compare the performance between the two diffs yet, but I think
> > it would be interesting to know.
> >
> > Also, just to help on a decision making, I would like to understand
> > whether your diff is solving another issue which mine isn't, despite
> > the eventual performance improvements?
> >
>
> Well, I've just tried your diff and it doesn't work. Idea was to perform a
> stress test of your and my diff against uvideo.c,v 1.252 but the system had
> crashed before ffmpeg gets any frame from the device. OCRed stacktrace:
>
> panic: attempt to access user address 0x20 from EL1
> Stopped at panic+0x140: cmp w21, #0x0
> TID PID UID PRFLAGS PC PFLAGS CPU COMMAND
> 236976 22771 1000 0x3 0x4000000 1 ffmpeg
> db_enter() at panic+0x13c
> panic() at kdata_abort+0x180
> do_el0_sync() at handle_el1h_sync+0x68
> handle_el1h_sync() at uvideo_vs_cb+0xc0
> uvideo_vs_cb() at usb_transfer_complete+0x220
> usb_transfer_complete() at xhci_event_dequeue+0x10c
> xhci_event_dequeue() at xhci_softintr+0x34
> https://www.openbsd.org/ddb.html describes the minimum info required in bug
> reports. Insufficient info makes it difficult to find and fix bugs.
> ddb{0}>
>
> The test was to use Elgato webcam to stream 4K 60 fps stream to null for 100
> seconds as ffmpeg command:
>
> ffmpeg -input_format mjpeg -video_size 3840x2160 -framerate 60 -i /dev/video0 -t 100 -f null -
>
> Anyway, the same test works well on the same machine with the same device
> with and without my diff.
Ah crap, I had a left over in that diff from the first version, which
most probably caused this panic. Is this better by any chance?
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 18:55:18 -0000
@@ -24,6 +24,7 @@
#include <sys/stat.h>
#include <sys/kthread.h>
#include <sys/stdint.h>
+#include <sys/mutex.h>
#include <uvm/uvm_extern.h>
@@ -68,6 +69,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;
@@ -104,6 +106,7 @@ struct uvideo_softc {
void (*sc_decode_stream_header)
(struct uvideo_softc *,
uint8_t *, int);
+ struct mutex sc_queue_mtx;
};
int uvideo_open(void *, int, int *, uint8_t *, void (*)(void *),
@@ -172,6 +175,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,
@@ -546,6 +550,8 @@ uvideo_attach(struct device *parent, str
struct usbd_desc_iter iter;
int i;
+ mtx_init(&sc->sc_queue_mtx, IPL_USB);
+
sc->sc_udev = uaa->device;
/* Find the first unclaimed video interface. */
@@ -1949,6 +1955,8 @@ uvideo_vs_free_frame(struct uvideo_softc
{
struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
+ mtx_enter(&sc->sc_queue_mtx);
+
if (fb->buf != NULL) {
free(fb->buf, M_USBDEV, fb->buf_size);
fb->buf = NULL;
@@ -1964,6 +1972,8 @@ uvideo_vs_free_frame(struct uvideo_softc
SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
sc->sc_mmap_count = 0;
+
+ mtx_leave(&sc->sc_queue_mtx);
}
usbd_status
@@ -2498,22 +2508,46 @@ 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)
{
int i;
+ mtx_enter(&sc->sc_queue_mtx);
+
if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL)
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__);
+ mtx_leave(&sc->sc_queue_mtx);
return;
}
@@ -2541,6 +2575,8 @@ uvideo_mmap_queue(struct uvideo_softc *s
DPRINTF(2, "%s: %s: frame queued on index %d\n",
DEVNAME(sc), __func__, i);
+ mtx_leave(&sc->sc_queue_mtx);
+
wakeup(sc);
/*
@@ -3553,6 +3589,8 @@ uvideo_reqbufs(void *v, struct v4l2_requ
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;
@@ -3613,11 +3651,14 @@ uvideo_dqbuf(void *v, struct v4l2_buffer
dqb->memory != V4L2_MEMORY_MMAP)
return (EINVAL);
+ mtx_enter(&sc->sc_queue_mtx);
if (SIMPLEQ_EMPTY(&sc->sc_mmap_q)) {
+ mtx_leave(&sc->sc_queue_mtx);
/* mmap queue is empty, block until first frame is queued */
error = tsleep_nsec(sc, 0, "vid_mmap", SEC_TO_NSEC(10));
if (error)
return (EINVAL);
+ mtx_enter(&sc->sc_queue_mtx);
}
mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q);
@@ -3632,6 +3673,8 @@ uvideo_dqbuf(void *v, struct v4l2_buffer
DPRINTF(2, "%s: %s: frame dequeued from index %d\n",
DEVNAME(sc), __func__, mmap->v4l2_buf.index);
SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
+
+ mtx_leave(&sc->sc_queue_mtx);
return (0);
}
sys/uvideo: avoid using queue.h