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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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




--
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: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux