Re: [PATCH] usb: gadget: uvc: reduce the request size to increase the throughput

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Fri, Nov 10, 2023, Thinh Nguyen wrote:
> Hi,
> 
> On Fri, Nov 10, 2023, Michael Grzeschik wrote:
> > One misconception of queueing request to the usb gadget controller is,
> > that the amount of data that one usb_request is representing is the same
> > as the hardware is able to transfer in one interval.
> > 
> > This exact idea applies to the uvc_video gadget that assumes that the
> > request needs to have the size as the gadgets bandwidth parameters are
> > configured with. (maxpacket, multiplier and burst)
> > 
> > Although it is actually no problem for the hardware to queue a big
> > request, in the case of isochronous transfers it is impractical to queue
> > big amount of data per request to the hardware. Especially if the
> > necessary bandwidth was increased on purpose to maintain high amounts of
> > data.
> > 
> > E.g. in the dwc3-controller it has the negative impact that the endpoint
> > FIFO will not be filled fast enough by the internal hardware engine.
> > Since each transfer buffer (TRB) represents one big request, the
> > hardware will need a long time to prefill the hardware FIFO. This can be
> > avoided by queueing more smaller requests which will then lead to
> > smaller TRBs which the hardware engine can keep up with.
> 
> Just want to clarify here to avoid any confusion, the hardware TX FIFO
> size is relatively small, usually can be smaller than the TRB. It should
> be fine whether the TRB is larger or smaller than the FIFO size. The
> hardware does not "prefill" the FIFO. It just fills whichever TRB it's
> currently processing (I think you may be mixing up with TRB cache).
> 
> The performance impact from this change is to reduce the USB bandwidth
> usage. The current calculation in uvc function can use 48KB/uframe for
> each request in SuperSpeed, the max size for isoc in SuperSpeed. I know
> many hosts can't handle this kind of transfer rate from their hardware.
> (e.g. It gets worse when scheduling transfers for multiple endpoints and
> under multiple tier hubs).
> 
> The bandwidth can be impacted by multiple factors and not just from the
> gadget side as noted in the discussion before.
> 
> > 
> > This patch is simply dropping the maxburst as an multiplier for the size
> > calculation and therefor decreases the overall maximum request size.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > This patch is created as an result from the discussion in the following thread:
> > 
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20231031231841.dbhtrgqounu2rvgn@synopsys.com/__;!!A4F2R9G_pg!fTaIo4tDljSbEvUY5SZLkNvKWcz0YeN0Ekzs0CPWyD73RGRmErRC2frODFgnMB1M4Nse0oKKgwxC65gePhGAtauKJq1Vnzlj$ 
> > 
> >  drivers/usb/gadget/function/uvc_queue.c | 1 -
> >  drivers/usb/gadget/function/uvc_video.c | 1 -
> >  2 files changed, 2 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> > index 0aa3d7e1f3cc32..1d3c3c09ff97cb 100644
> > --- a/drivers/usb/gadget/function/uvc_queue.c
> > +++ b/drivers/usb/gadget/function/uvc_queue.c
> > @@ -55,7 +55,6 @@ static int uvc_queue_setup(struct vb2_queue *vq,
> >  	sizes[0] = video->imagesize;
> >  
> >  	req_size = video->ep->maxpacket
> > -		 * max_t(unsigned int, video->ep->maxburst, 1)
> 
> I think you're reducing a bit too much here? Also, take advantage of
> burst. So, perhaps keep request size to max(16K, MPS * maxburst)?

I mean to potentially cap at 16K, it didn't translate correctly to
code.... I meant something like this:

max_size = mult * maxburst * MPS;
clamp(max_size, mult * MPS, 16K);

> 
> Can be more or less depending on how much video data is needed to
> transfer over a video frame.

Can the uvc gadget figure out the max video data over a video frame? Or
is it not available at this layer? I figure different setup requires
more video data payload.

If we have this info, then you can properly cap the request size.

Thanks,
Thinh