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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 Hi Thinh,


On 10/12/23 11:50, Thinh Nguyen wrote:
> Hi,
>
> On Mon, Oct 09, 2023, Jayant Chowdhary wrote:
>>> On Fri, Oct 06, 2023 at 03:03:56PM -0700, Jayant Chowdhary wrote:
>>>> Hi Everyone,
>>>>
>>>> We had been seeing the UVC gadget driver receive isoc errors while
>>>> sending packets to the usb endpoint - resulting in glitches being shown
>>>> on linux hosts. My colleague Avichal Rakesh and others had a very
>>>> enlightening discussion at
>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/8741b7cb-54ec-410b-caf5-697f81e8ad64@xxxxxxxxxx/T/__;!!A4F2R9G_pg!e3zVZGt-6Td6HJXqh8GaZAsUeKyvKBhOoyru9qzn3Vkw01Vdkwk7hFr_t5glBG2BYJlOYfFKEUpiH5H4gQ73n_-Y$ 
>>>>
>>>>
>>>> The conclusion that we came to was : usb requests with actual uvc frame
>>>> data were missing their scheduled uframes in the usb controller. As a
>>>> mitigation, we started sending 0 length usb requests when there was no
>>>> uvc frame buffer available to get data from. Even with this mitigation,
>>>> we are seeing glitches - albeit at a lower frequency.
>>>>
>>>> After some investigation, it is seen that we’re getting isoc errors when
>>>> the worker thread serving video_pump() work items, doesn’t get scheduled
>>>> for longer periods of time - than usual - in most cases > 6ms.
>>>> This is close enough to the 8ms limit that we have when the number of usb
>>>> requests in the queue is 64 (since we have a 125us uframe period). In order
>>>> to tolerate the scheduling delays better, it helps to increase the number of
>>>> usb requests in the queue . In that case, we have more 0 length requests
>>>> given to the udc driver - and as a result we can wait longer for uvc
>>>> frames with valid data to get processed by video_pump(). I’m attaching a
>>>> patch which lets one configure the upper bound on the number of usb
>>>> requests allocated through configfs. Please let me know your thoughts.
>>>> I can formalize  the patch if it looks okay.
>>> Why do you want to limit the upper bound?  Why not just not submit so
>>> many requests from userspace as you control that, right?
>>
>> Userspace negotiates a video frame rate (typically 30/60fps) with the host that does
>> not necessarily correspond to the ISOC cadence. After the
>> patch at https://urldefense.com/v3/__https://lkml.org/lkml/diff/2023/5/8/1115/1__;!!A4F2R9G_pg!e3zVZGt-6Td6HJXqh8GaZAsUeKyvKBhOoyru9qzn3Vkw01Vdkwk7hFr_t5glBG2BYJlOYfFKEUpiH5H4gWbb9bvy$  was submitted, we are
>> maintaining back pressure on the usb controller even if we do not have uvc frame
>> data, by sending the controller 0 length requests (as long as usb_requests are
>> available). Also, even if the userspace application were to somehow produce
>> data to match the ISOC rate, it would  need to have information about USB
>> timing details - which I am not sure is available to userspace or is the right
>> thing to do here ?
>>
>> Here, we are trying to handle the scenario in which the video_pump() worker
>> thread does not get scheduled in time - by increasing the number of usb requests
>> allocated in the queue. This would send more usb requests to the usb controller,
>> when video_pump() does get scheduled - even if they're 0 length. This buys
>> the video_pump() worker thread scheduling time -since more usb requests
>> are with the controller, subsequent requests sent will not be 'stale' and
>> dropped by the usb controller.
>>
> I believe you're testing against dwc3 controller right? I may not be as
> familiar with UVC function driver, but based on the previous
> discussions, I think the driver should be able to handle this without
> the user input.

Yes we are testing against the dwc3 controller.

>
> 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 ? 

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://lore.kernel.org/all/20200427151614.10868-1-m.grzeschik@xxxxxxxxxxxxxx/
(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.

Thanks,
Jayant





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux