Re: uvc gadget: Making upper bound of number of usb requests allocated configurable through configfs

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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.

> 
> This brings me to another question for Michael - I see
> that we introduced a worker thread for pumping  usb requests to the usb endpoint
> in https://urldefense.com/v3/__https://lore.kernel.org/all/20200427151614.10868-1-m.grzeschik@pengutronix.de/__;!!A4F2R9G_pg!aAnzCopbTbZtUxBK6a6r6_QzV-b2Z2J5o5esPaartZ2jogKijmhqj6OyiKDg-BPhxq8pJHR3HS1Hf8z6YnqfWTon$ 
> (I see multiple email addresses, so apologies if I used the incorrect one).
> 
> Did we introduce the worker thread to solve some specific deadlock scenarios ?
> Or was it a general mitigation against racy usb request submission from v4l2 buffer
> queuing, stream enable and the video complete handler firing ?
> 
> I was chatting with Avi about this, what if we submit requests to the endpoint
> only at two points in the streaming lifecycle - 
> 1) The whole 64 (or however many usb requests are allocated) when
>    uvcg_video_enable() is called - with 0 length usb_requests.
> 2) In the video complete handler - if a video buffer is available, we encode it
>    and submit it to the endpoint. If not, we send a 0 length request.
> 
> This way we're really maintaining back pressure and sending requests as soon
> as we can to the dwc3 controller. Encoding is mostly memcpys from what I see
> so hopefully not too heavy on the interrupt handler. I will work on prototyping
> this meanwhile.
> 

That sounds good to me. I believe Michael already provided some test
patches and you've already done some preliminary tests for that right?

Thanks,
Thinh