On Wed, Oct 25, 2023 at 04:09:27PM -0700, Jayant Chowdhary wrote:
On 10/24/23 05:33, Michael Grzeschik wrote:On Mon, Oct 23, 2023 at 11:13:03AM -0700, Jayant Chowdhary wrote: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.
Sounds good to me, but you missed your own Signed-off-by in the patch. Michael -- 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
