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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Michael,

On Fri, Apr 29, 2022 at 10:01:37PM +0200, Michael Grzeschik wrote:
> 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,

Just for the record, as a newer version of this patch has been merged,
the host-side uvcvideo driver is specifically made to handle multiple
work items in parallel. Each work item will essentially perform one or
multiple memcpy operations, with the size and offset calculated by the
code that dispatches the work items.

As Lucas separately commented, the UVC gadget driver has a single
work_struct, so there can't be any concurrency. We seem to be safe for
now.

> 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