Hi Everyone,
We had been seeing the UVC gadget driver receive isoc errors while
sending packets to the usb endpoint - resulting in glitches being shown
on linux hosts. My colleague Avichal Rakesh and others had a very
enlightening discussion at
https://lore.kernel.org/linux-usb/8741b7cb-54ec-410b-caf5-697f81e8ad64@xxxxxxxxxx/T/
The conclusion that we came to was : usb requests with actual uvc frame
data were missing their scheduled uframes in the usb controller. As a
mitigation, we started sending 0 length usb requests when there was no
uvc frame buffer available to get data from. Even with this mitigation,
we are seeing glitches - albeit at a lower frequency.
After some investigation, it is seen that we’re getting isoc errors when
the worker thread serving video_pump() work items, doesn’t get scheduled
for longer periods of time - than usual - in most cases > 6ms.
This is close enough to the 8ms limit that we have when the number of usb
requests in the queue is 64 (since we have a 125us uframe period). In order
to tolerate the scheduling delays better, it helps to increase the
number of
usb requests in the queue . In that case, we have more 0 length requests
given to the udc driver - and as a result we can wait longer for uvc
frames with valid data to get processed by video_pump(). I’m attaching a
patch which lets one configure the upper bound on the number of usb
requests allocated through configfs. Please let me know your thoughts.
I can formalize the patch if it looks okay.
Thank you
Jayant
---
.../ABI/testing/configfs-usb-gadget-uvc | 2 ++
Documentation/usb/gadget-testing.rst | 21 ++++++++++++-------
drivers/usb/gadget/function/f_uvc.c | 4 +++-
drivers/usb/gadget/function/u_uvc.h | 1 +
drivers/usb/gadget/function/uvc.h | 3 +++
drivers/usb/gadget/function/uvc_configfs.c | 2 ++
drivers/usb/gadget/function/uvc_queue.c | 5 ++++-
7 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 4feb692c4c1d..9bc58440e1b7 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -7,6 +7,8 @@ Description: UVC function directory
streaming_maxburst 0..15 (ss only)
streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
streaming_interval 1..16
+ streaming_max_usb_requests 64..256
+
function_name string [32]
=================== =============================
diff --git a/Documentation/usb/gadget-testing.rst
b/Documentation/usb/gadget-testing.rst
index 29072c166d23..24a62ba8e870 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -790,14 +790,19 @@ Function-specific configfs interface
The function name to use when creating the function directory is "uvc".
The uvc function provides these attributes in its function directory:
- =================== ================================================
- streaming_interval interval for polling endpoint for data transfers
- streaming_maxburst bMaxBurst for super speed companion descriptor
- streaming_maxpacket maximum packet size this endpoint is capable of
- sending or receiving when this configuration is
- selected
- function_name name of the interface
- =================== ================================================
+ =================== ===========================================
+ streaming_interval interval for polling endpoint for data
+ transfers
+ streaming_maxburst bMaxBurst for super speed companion
+ descriptor
+ streaming_maxpacket maximum packet size this endpoint is
+ capable of sending or receiving when
+ this configuration is selected
+ streaming_max_usb_requests upper bound for the number of usb
requests
+ the gadget driver will allocate for
+ sending to the endpoint.
+ function_name name of the interface
+ =================== ==================================================
There are also "control" and "streaming" subdirectories, each of which
contain
a number of their subdirectories. There are some sane defaults
provided, but
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index faa398109431..32a296ea37d7 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -659,7 +659,8 @@ uvc_function_bind(struct usb_configuration *c,
struct usb_function *f)
opts->streaming_interval = clamp(opts->streaming_interval, 1U, 16U);
opts->streaming_maxpacket = clamp(opts->streaming_maxpacket, 1U,
3072U);
opts->streaming_maxburst = min(opts->streaming_maxburst, 15U);
-
+ opts->streaming_max_usb_requests =
clamp(opts->streaming_max_usb_requests, 64U, 256U);
+ uvc->streaming_max_usb_requests = opts->streaming_max_usb_requests;
/* For SS, wMaxPacketSize has to be 1024 if bMaxBurst is not 0 */
if (opts->streaming_maxburst &&
(opts->streaming_maxpacket % 1024) != 0) {
@@ -934,6 +935,7 @@ static struct usb_function_instance
*uvc_alloc_inst(void)
opts->streaming_interval = 1;
opts->streaming_maxpacket = 1024;
+ opts->streaming_max_usb_requests = 64;
snprintf(opts->function_name, sizeof(opts->function_name), "UVC
Camera");
ret = uvcg_attach_configfs(opts);
diff --git a/drivers/usb/gadget/function/u_uvc.h
b/drivers/usb/gadget/function/u_uvc.h
index 1ce58f61253c..075fee178418 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -24,6 +24,7 @@ struct f_uvc_opts {
unsigned int streaming_interval;
unsigned int streaming_maxpacket;
unsigned int streaming_maxburst;
+ unsigned int streaming_max_usb_requests;
unsigned int control_interface;
unsigned int streaming_interface;
diff --git a/drivers/usb/gadget/function/uvc.h
b/drivers/usb/gadget/function/uvc.h
index 6751de8b63ad..943c074157fa 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -157,6 +157,9 @@ struct uvc_device {
/* Events */
unsigned int event_length;
unsigned int event_setup_out : 1;
+
+ /* Max number of usb requests allocated to send to the ep*/
+ unsigned int streaming_max_usb_requests;
};
static inline struct uvc_device *to_uvc(struct usb_function *f)
diff --git a/drivers/usb/gadget/function/uvc_configfs.c
b/drivers/usb/gadget/function/uvc_configfs.c
index 9bf0e985acfa..0ad9eae4845e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -3403,6 +3403,7 @@ UVC_ATTR(f_uvc_opts_, cname, cname)
UVCG_OPTS_ATTR(streaming_interval, streaming_interval, 16);
UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, 3072);
UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, 15);
+UVCG_OPTS_ATTR(streaming_max_usb_requests, streaming_max_usb_requests,
256);
#undef UVCG_OPTS_ATTR
@@ -3453,6 +3454,7 @@ static struct configfs_attribute *uvc_attrs[] = {
&f_uvc_opts_attr_streaming_maxpacket,
&f_uvc_opts_attr_streaming_maxburst,
&f_uvc_opts_string_attr_function_name,
+ &f_uvc_opts_attr_streaming_max_usb_requests,
NULL,
};
diff --git a/drivers/usb/gadget/function/uvc_queue.c
b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc..b81c2d8e5ef2 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -45,6 +45,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_video *video = container_of(queue, struct uvc_video,
queue);
unsigned int req_size;
+ unsigned int max_usb_requests;
unsigned int nreq;
if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
@@ -54,6 +55,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
sizes[0] = video->imagesize;
+ max_usb_requests = video->uvc->streaming_max_usb_requests;
+
req_size = video->ep->maxpacket
* max_t(unsigned int, video->ep->maxburst, 1)
* (video->ep->mult);
@@ -62,7 +65,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
* into fewer requests for smaller framesizes.
*/
nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
- nreq = clamp(nreq, 4U, 64U);
+ nreq = clamp(nreq, 4U, max_usb_requests);
video->uvc_num_requests = nreq;
return 0;
--
2.42.0.609.gbb76f46606-goog
