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

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3439 bytes --]

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@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.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]