Re: [PATCH v2] usb:gadget:uvc Do not use worker thread to pump usb requests

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Fri, Oct 27, 2023 at 10:58:11AM -0400, Alan Stern wrote:
>On Fri, Oct 27, 2023 at 03:39:44PM +0200, Michael Grzeschik wrote:
>> On Fri, Oct 27, 2023 at 02:47:52PM +0300, Laurent Pinchart wrote:
>> > On Fri, Oct 27, 2023 at 01:10:21PM +0200, Michael Grzeschik wrote:
>> > > On Fri, Oct 27, 2023 at 10:51:17AM +0300, Laurent Pinchart wrote:
>> > > > On Thu, Oct 26, 2023 at 09:56:35PM +0000, Jayant Chowdhary wrote:
>> > > >> This patch is based on top of
>> > > >> https://lore.kernel.org/linux-usb/20230930184821.310143-1-arakesh@google.com/T/#t:
>> > > >>
>> > > >> When we use an async work queue to perform the function of pumping
>> > > >> usb requests to the usb controller, it is possible that thread scheduling
>> > > >> affects at what cadence we're able to pump requests. This could mean usb
>> > > >> requests miss their uframes - resulting in video stream flickers on the host
>> > > >> device.
>> > > >>
>> > > >> In this patch, we move the pumping of usb requests to
>> > > >> 1) uvcg_video_complete() complete handler for both isoc + bulk
>> > > >>    endpoints. We still send 0 length requests when there is no uvc buffer
>> > > >>    available to encode.
>> > > >
>> > > > This means you will end up copying large amounts of data in interrupt
>> > > > context. The work queue was there to avoid exactly that, as it will
>> > > > introduce delays that can affect other parts of the system. I think this
>> > > > is a problem.
>> > >
>> > > Regarding Thin's argument about possible scheduling latency that is already
>> > > introducing real errors, this seemed like a good solution.
>> > >
>> > > But sure, this potential latency introduced in the interrupt context can
>> > > trigger other side effects.
>> > >
>> > > However I think we need some compromise since both arguments are very valid.
>> >
>> > Agreed.
>> >
>> > > Any ideas, how to solve this?
>> >
>> > I'm afraid not.
>>
>> We discussed this and came to the conclusion that we could make use of
>> kthread_create and sched_setattr with an attr->sched_policy = SCHED_DEADLINE
>> here instead of the workqueue. This way we would ensure that the worker
>> would be triggered with hard definitions.
>>
>> Since the SG case is not that heavy on the completion handler, we could
>> also make this kthread conditionaly to the memcpy case.
>
>If you don't mind a naive suggestion from someone who knows nothing
>about the driver...
>
>An attractive possibility is to have the work queue (or kthread) do the
>time-consuming copying, but leave the submission up to the completion
>handler.  If the data isn't ready (or there's no data to send) when the
>handler runs, then queue a 0-length request.
>
>That will give you the best of both worlds: low latency while in
>interrupt context and a steady, constant flow of USB transfers at all
>times.  The question of how to schedule the work queue or kthread is a
>separate matter, not directly relevant to this design decision.

That's it. This is probably the best way to tackle the overall problem.

So we leave the call of the encode callback to the worker, that will
probably still can be a workqueue. The complete callback is calling
the explicit uvcg_video_ep_queue when prepared requests are available
and if there is nothing pending it will just enqueue zero requests.

Thank you Alan, this makes so much sense!

Jayant, Laurent: Do you agree?
If yes, Jayant will you change the patch accordingly?

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