On 4/20/23 10:20, Laurent Pinchart wrote:
>
> As far as I understand, we have two ways forward here to avoid running
> out of requests to send: sending data as quickly as possible (maximizing
> the number of bytes sent in each packet) and filling up with 0-length
> requests in-between, and spreading the data across packets. I'll call
> the first one burst mode for lack of a better term.
>
Hey all,
Apologies for the late reply. My not-so-long work took longer than expected.
I tried the 2 (technically 3, but I'll go over it in a bit) approaches we had
discussed above and the "burst" approach works pretty well. I'll attach that
to this email, and send another email out with the other patch.
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.
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?
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?
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.
+ video->encode(req, video, buf);
+ // Always interrupt for the last usb_request of a video frame
+ buf_int = buf->state == UVC_BUF_STATE_DONE;
+ } else if (!(queue->flags & UVC_QUEUE_DISCONNECTED)) {
+ // No video frame, but the queue is still connected
+ // Send an empty USB request to keep ISOC contract...
+ req->length = 0;
+ buf_int = false;
+ } else {
+ // queue no longer connected. Stop processing further.
spin_unlock_irqrestore(&queue->irqlock, flags);
break;
}
- video->encode(req, video, buf);
-
/*
* With usb3 we have more requests. This will decrease the
* interrupt load to a quarter but also catches the corner
* cases, which needs to be handled.
*/
- if (list_empty(&video->req_free) ||
- buf->state == UVC_BUF_STATE_DONE ||
+ if (list_empty(&video->req_free) || buf_int ||
!(video->req_int_count %
DIV_ROUND_UP(video->uvc_num_requests, 4))) {
video->req_int_count = 0;
@@ -441,8 +451,7 @@ static void uvcg_video_pump(struct work_struct *work)
/* Endpoint now owns the request */
req = NULL;
- if (buf->state != UVC_BUF_STATE_DONE)
- video->req_int_count++;
+ video->req_int_count++;
}
if (!req)
@@ -527,4 +536,3 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
V4L2_BUF_TYPE_VIDEO_OUTPUT, &video->mutex);
return 0;
}
-
--
