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

[Jayant Chowdhary <jchowdhary@google.com>]

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@google.com/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