Re: [PATCH 3/5] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI

[Michael Grzeschik <mgr@pengutronix.de>]

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

Hi Dan,
Hi Laurent,

On Fri, Apr 29, 2022 at 01:51:48PM -0500, Dan Vacura wrote:
>Thanks for this change it improves the performance with the DWC3
>controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
>scatter/gather path, so memcpy was used here via
>uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
>on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.
>
>On Tue, Apr 19, 2022 at 11:46:57PM +0300, Laurent Pinchart wrote:
>> Thank you for the patch.
>>
>> On Sun, Apr 03, 2022 at 01:39:12AM +0200, Michael Grzeschik wrote:
>> > Likewise to the uvcvideo hostside driver, this patch is changing the
>> > simple workqueue to an async_wq with higher priority. This ensures that
>> > the worker will not be scheduled away while the video stream is handled.
>> >
>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> > +	video->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI, 0);
>>
>> Unless I'm mistaken, an unbound work queue means that multiple CPUs will
>> handle tasks in parallel. Is that safe ?
>
>I found that with the WQ_UNBOUND flag I didn't see any performance
>improvement to the baseline, perhaps related to cpu caching or
>scheduling delays. I didn't notice any stability problems or concurrent
>execution. Do you see any benefit to keeping the WQ_UNBOUND flag?

I actually copied this from drivers/media/usb/uvc/uvc_driver.c ,
which is also allocating the workqueue with WQ_UNBOUND.

Look into drivers/media/usb/uvc/uvc_driver.c + 486

	stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI,

In my tests, continous streaming did not trigger any errors. In fact if
this would be unsafe, the issue would probably trigger early, numerous
and obvious on multicore cpus.

However, some users seem to have seen recent issues on unplugging the
cable while streaming. I have to check if this could be related.

>> > +	if (!video->async_wq)
>> > +		return -EINVAL;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>
>Thanks,
>
>Dan
>

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