Hi, On Mon, Oct 09, 2023, Jayant Chowdhary wrote: > > 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/8741b7cb-54ec-410b-caf5-697f81e8ad64@xxxxxxxxxx/T/__;!!A4F2R9G_pg!e3zVZGt-6Td6HJXqh8GaZAsUeKyvKBhOoyru9qzn3Vkw01Vdkwk7hFr_t5glBG2BYJlOYfFKEUpiH5H4gQ73n_-Y$ > >> > >> > >> 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://urldefense.com/v3/__https://lkml.org/lkml/diff/2023/5/8/1115/1__;!!A4F2R9G_pg!e3zVZGt-6Td6HJXqh8GaZAsUeKyvKBhOoyru9qzn3Vkw01Vdkwk7hFr_t5glBG2BYJlOYfFKEUpiH5H4gWbb9bvy$ 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. > I believe you're testing against dwc3 controller right? I may not be as familiar with UVC function driver, but based on the previous discussions, I think the driver should be able to handle this without the user input. 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. BR, Thinh
