Hi Michael, On 10/24/23 05:33, Michael Grzeschik wrote: > 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. > I sent out https://lore.kernel.org/linux-usb/99384044-0d14-4ebe-9109-8a5557e64449@xxxxxxxxxx/T/#u with a Signed-off-by crediting you and suggested by with Avichal and me. It has a few changes related to bulk end-points as well, but they're relatively minor. Thanks
