Hi Avichal,
(with a question for Dan below)
On Fri, May 05, 2023 at 03:39:41PM -0700, Avichal Rakesh wrote:
> 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.
No worries, I know how it feels :-)
> 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.
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.
> 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.
> 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.
> + 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;
> }
> -
--
Regards,
Laurent Pinchart
