On Sat, May 6, 2023 at 5:53 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> > The first thing I tried was to split one video frame over 266 frames, without
> > changing the number of requests allocated. And it works! However, as Laurent
> > mentioned, it does add a fair amount of bookkeeping to split a video frame into
> > the required number of requests. I also hardcoded the number 266 from our
> > discussion, but I am not sure how to figure out that number dynamically. 266
> > also didn't work if the host started sending frames at more than 30fps :/, so
> > our dynamic calculation would need to take camera's real output fps into
> > account, which as far as I can tell is not known to the UVC driver.
>
> It would probably need to monitor how full the request queue is, and
> adapt the number of bytes it queues in each request accordingly. That's
> indeed quite a bit of work, for little gain compared to the option you
> describe below.
>
Agreed, especially if the hosts already handle 0 length packets.
As long as the usb controllers can keep up, the burst approach seems
more reasonable.
> > With those issues I tried what Laurent called the "burst" approach
> > (attached below), i.e. send the video frames in as few packets as possible,
> > and then queue up 0 length packets to keep the ISOC queue happy. This approach
> > works perfectly as far as I can tell. Locally I tried with a Linux, Window,
> > and MacOS host with no frame drops or ISOC failures on any of them!
> >
> > In the current patch, UVC gadget driver keeps the ISOC cadence by effectively
> > maintaining a back-pressure on the USB controller (at least to the best of its
> > capabilities). Any usb_request available to the UVC gadget gets immediately
> > queued back to the USB controller. If a video frame is available, the frame is
> > encoded, if not, the length is set to 0. The idea being that the host's polling
> > and the controller's 'complete' callback will result in a somewhat consistent
> > cadence for the uvc driver after the initial burst of packets.
> >
> > However this does mean that at worst, the new video frames are up to 63
> > usb_requests behind, but assuming a 125us per usb_request, that amounts to ~8ms
> > latency at the worst, which seems acceptable?
>
> There's a trade off between latency and the risk of underruns. We could
> decrease the number of queued requests to lower the latency, as long as
> we ensure the margin is high enough to avoid underruns in higher load
> conditions. We could also do so only when queuing 0-size requests, and
> queue the data in burst mode with a higher number of requests.
Would 8ms of latency be considered significant? Unless the host asks
for >125fps,
that amounts to less than a frame of latency, so frames should not be dropped
by the host for being "late". Admittedly, I don't know enough about UVC usage to
say if 8ms (at worst) will be problematic for certain usages. The
hosts don't seem to
have any issues when streaming at <=60fps.
> > Another concern I had was about how the back-pressure might affect other USB
> > controllers. DWC3 doesn't seem to be sweating and in local testing I saw no
> > EXDEVs or frame drops other than when the stream was being transitioned from
> > one configuration to another, but I don't know how this interaction might go for
> > other USB controllers. Would you have any insights into non-DWC3 controllers,
> > and if they might be negatively affected by having up to 64 requests queued at
> > once?
>
> Dan, do I recall correctly you have tested uvc-gadget with dwc2 too ?
> Could you test the patch below ? Testing with musb would be nice too.
>
> > Here's the patch, it doesn't currently handle bulk transfers, but I can upload a
> > formal patch with it if this approach seems acceptable!
> >
> > ---
> > drivers/usb/gadget/function/uvc_video.c | 24 ++++++++++++++++--------
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> > index dd1c6b2ca7c6..d7ad278709d4 100644
> > --- a/drivers/usb/gadget/function/uvc_video.c
> > +++ b/drivers/usb/gadget/function/uvc_video.c
> > @@ -386,6 +386,7 @@ static void uvcg_video_pump(struct work_struct *work)
> > struct uvc_buffer *buf;
> > unsigned long flags;
> > int ret;
> > + bool buf_int;
> >
> > while (video->ep->enabled) {
> > /*
> > @@ -408,20 +409,29 @@ static void uvcg_video_pump(struct work_struct *work)
> > */
> > spin_lock_irqsave(&queue->irqlock, flags);
> > buf = uvcg_queue_head(queue);
> > - if (buf == NULL) {
> > +
> > + if (buf != NULL) {
> > + // Encode video frame if we have one.
>
> C-style comments please.
>
Addressed this comment and uploaded a formal patch:
https://lore.kernel.org/20230508231103.1621375-1-arakesh@xxxxxxxxxx/
It is basically this patch with an extra flag to ensure that we don't
spam a bulk endpoint with 0-length requests.
I wrote a script to detect abrupt size changes in uvc frames on the
host. In my two hours of testing with the above patch, I've recorded
only one "short" frame.
```
[616677.453290] usb 1-6: frame 1 stats: 0/31/31 packets, 0/0/0 pts
(!early !initial), 30/31 scr, last pts/stc/sof 0/4055830784/1298
[617701.585256] usb 1-6: frame 30594 stats: 0/1/1 packets, 0/0/0 pts
(!early !initial), 0/1 scr, last pts/stc/sof 0/1677462368/2636
[617701.621240] usb 1-6: frame 30596 stats: 0/32/32 packets, 0/0/0 pts
(!early !initial), 31/32 scr, last pts/stc/sof 0/1679244656/2933
```
First log is when streaming starts (abrupt change from 0 to 31).
Frame 30594 had only 1 packet for some reason (I couldn't capture
gadget logs at the time :/), but the stream recovered after one
skipped frame.
Will leave the script running overnight, and report back if there's
anything significant there.
- Avi.
