From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/uvideo: avoid using queue.h
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Mon, 17 Mar 2025 00:13:53 +0100

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.

-- 
wbr, Kirill