Re: uvc gadget: Making upper bound of number of usb requests allocated configurable through configfs

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 9050 bytes --]

On Mon, Oct 23, 2023 at 11:13:03AM -0700, Jayant Chowdhary wrote:
>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.

I tested this, and it looks good so far.

Since your changes are minimal you could send this with me as the author
and add your Suggested-by Tag. You should also add your Tested-by Tag in
that case.

Regards,
Michael

>The patch would look something like (on top of: https://lore.kernel.org/linux-usb/20230930184821.310143-1-arakesh@google.com/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@pengutronix.de/__;!!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
>
>
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]