Hi Thinh, Michael, On 10/20/23 16:30, Thinh Nguyen wrote: > Sorry for the delay response. > > On Sun, Oct 15, 2023, Jayant Chowdhary wrote: >> On 10/12/23 11:50, Thinh Nguyen wrote: >>> The frequency of the request submission should not depend on the >>> video_pump() work thread since it can vary. The frequency of request >>> submission should match with the request completion. We know that >>> request completion rate should be fixed (1 uframe/request + when you >>> don't set no_interrupt). Base on this you can do your calculation on how >>> often you should set no_interrupt and how many requests you must submit. >>> You don't have to wait for the video_pump() to submit 0-length requests. >>> >>> The only variable here is the completion handler delay or system >>> latency, which should not be much and should be within your calculation. >> >> Thanks for the suggestion. It indeed makes sense that we do not completely depend on >> video_pump() for sending 0 length requests. I was concerned about >> synchronization needed when we send requests to the dwc3 controller from >> different threads. I see that the dwc3 controller code does internally serialize >> queueing requests, can we expect this from other controllers as well ? > While it's not explicitly documented, when the gadget driver uses > usb_ep_queue(), the order in which the gadget recieves the request > should be maintained and serialized. Because the order the transfer go > out for the same endpoint can be critical, breaking this will cause > issue. > Thanks for clarifying this. Keeping this in mind - I made a slight modification to your test patch - I removed the uvc_video_pump() function call from uvc_v4l2_qbuf(). We just call it in uvcg_video_enable(). That should just queue 0 length requests till the first qbuf is called. There-after only the complete handler running uvcg_video_complete() calls video_pump(), which sends usb requests to the endpoint. While I do see that we hold the queue->irqlock while getting the uvc buffer to encode and sending it to the ep, I feel like its just logically safer for future changes if we can restrict the pumping of requests to one thread. Does that seem okay to you ? I can formalize it if it does. The patch would look something like (on top of: https://lore.kernel.org/linux-usb/20230930184821.310143-1-arakesh@xxxxxxxxxx/T/#t) --- drivers/usb/gadget/function/f_uvc.c | 4 ---- drivers/usb/gadget/function/uvc.h | 3 --- drivers/usb/gadget/function/uvc_v4l2.c | 3 --- drivers/usb/gadget/function/uvc_video.c | 19 +++++++------------ 4 files changed, 7 insertions(+), 22 deletions(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 44c36e40e943..7d78fc8c00c5 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -907,14 +907,10 @@ static void uvc_function_unbind(struct usb_configuration *c, { struct usb_composite_dev *cdev = c->cdev; struct uvc_device *uvc = to_uvc(f); - struct uvc_video *video = &uvc->video; long wait_ret = 1; uvcg_info(f, "%s()\n", __func__); - if (video->async_wq) - destroy_workqueue(video->async_wq); - /* * If we know we're connected via v4l2, then there should be a cleanup * of the device from userspace either via UVC_EVENT_DISCONNECT or diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index e8d4c87f1e09..b33211c92c02 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -88,9 +88,6 @@ struct uvc_video { struct uvc_device *uvc; struct usb_ep *ep; - struct work_struct pump; - struct workqueue_struct *async_wq; - /* Frame parameters */ u8 bpp; u32 fcc; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index 68bb18bdef81..ef4305f79cfe 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -421,9 +421,6 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b) if (ret < 0) return ret; - if (uvc->state == UVC_STATE_STREAMING) - queue_work(video->async_wq, &video->pump); - return ret; } diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 54a1c36e879e..35fb6a185b6e 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -24,6 +24,8 @@ * Video codecs */ +static void uvcg_video_pump(struct uvc_video *video); + static int uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf, u8 *data, int len) @@ -329,7 +331,9 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) */ if (video->is_enabled) { list_add_tail(&req->list, &video->req_free); - queue_work(video->async_wq, &video->pump); + spin_unlock_irqrestore(&video->req_lock, flags); + uvcg_video_pump(video); + return; } else { uvc_video_free_request(ureq, ep); } @@ -415,9 +419,8 @@ uvc_video_alloc_requests(struct uvc_video *video) * This function fills the available USB requests (listed in req_free) with * video data from the queued buffers. */ -static void uvcg_video_pump(struct work_struct *work) +static void uvcg_video_pump(struct uvc_video *video) { - struct uvc_video *video = container_of(work, struct uvc_video, pump); struct uvc_video_queue *queue = &video->queue; struct usb_request *req = NULL; struct uvc_buffer *buf; @@ -545,7 +548,6 @@ uvcg_video_disable(struct uvc_video *video) } spin_unlock_irqrestore(&video->req_lock, flags); - cancel_work_sync(&video->pump); uvcg_queue_cancel(&video->queue, 0); spin_lock_irqsave(&video->req_lock, flags); @@ -621,8 +623,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->req_int_count = 0; - queue_work(video->async_wq, &video->pump); - + uvcg_video_pump(video); return ret; } @@ -635,12 +636,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); spin_lock_init(&video->req_lock); - INIT_WORK(&video->pump, uvcg_video_pump); - - /* Allocate a work queue for asynchronous video pump handler. */ - video->async_wq = alloc_workqueue("uvcgadget", WQ_UNBOUND | WQ_HIGHPRI, 0); - if (!video->async_wq) - return -EINVAL; video->uvc = uvc; video->fcc = V4L2_PIX_FMT_YUYV; -- >> This brings me to another question for Michael - I see >> that we introduced a worker thread for pumping usb requests to the usb endpoint >> in https://urldefense.com/v3/__https://lore.kernel.org/all/20200427151614.10868-1-m.grzeschik@xxxxxxxxxxxxxx/__;!!A4F2R9G_pg!aAnzCopbTbZtUxBK6a6r6_QzV-b2Z2J5o5esPaartZ2jogKijmhqj6OyiKDg-BPhxq8pJHR3HS1Hf8z6YnqfWTon$ >> (I see multiple email addresses, so apologies if I used the incorrect one). >> >> Did we introduce the worker thread to solve some specific deadlock scenarios ? >> Or was it a general mitigation against racy usb request submission from v4l2 buffer >> queuing, stream enable and the video complete handler firing ? >> >> I was chatting with Avi about this, what if we submit requests to the endpoint >> only at two points in the streaming lifecycle - >> 1) The whole 64 (or however many usb requests are allocated) when >> uvcg_video_enable() is called - with 0 length usb_requests. >> 2) In the video complete handler - if a video buffer is available, we encode it >> and submit it to the endpoint. If not, we send a 0 length request. >> >> This way we're really maintaining back pressure and sending requests as soon >> as we can to the dwc3 controller. Encoding is mostly memcpys from what I see >> so hopefully not too heavy on the interrupt handler. I will work on prototyping >> this meanwhile. >> > That sounds good to me. I believe Michael already provided some test > patches and you've already done some preliminary tests for that right? Yes that is correct. I tested out the patch above as well and it seems to work for my setup. I haven't seen flickers in over an hour of streaming. Thanks, Jayant
