> On Fri, Oct 06, 2023 at 03:03:56PM -0700, Jayant Chowdhary wrote: >> 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. > Why do you want to limit the upper bound? Why not just not submit so > many requests from userspace as you control that, right? Userspace negotiates a video frame rate (typically 30/60fps) with the host that does not necessarily correspond to the ISOC cadence. After the patch at https://lkml.org/lkml/diff/2023/5/8/1115/1 was submitted, we are maintaining back pressure on the usb controller even if we do not have uvc frame data, by sending the controller 0 length requests (as long as usb_requests are available). Also, even if the userspace application were to somehow produce data to match the ISOC rate, it would need to have information about USB timing details - which I am not sure is available to userspace or is the right thing to do here ? Here, we are trying to handle the scenario in which the video_pump() worker thread does not get scheduled in time - by increasing the number of usb requests allocated in the queue. This would send more usb requests to the usb controller, when video_pump() does get scheduled - even if they're 0 length. This buys the video_pump() worker thread scheduling time -since more usb requests are with the controller, subsequent requests sent will not be 'stale' and dropped by the usb controller. > >> 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 > Note, your patch is whitespace damaged and line-wrapped, making it > really really hard to read and impossible to apply. My apologies and thank you for your patience. I have attached the patch again for discussion. (Also fixed a mailing list typo I had made in the previous e-mail) Thanks, 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; --
