Re: [PATCH] usb: gadget: uvc: limit isoc_sg to super speed gadgets

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Michael Grzeschik <mgr@pengutronix.de>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"balbi@kernel.org" <balbi@kernel.org>,
	"laurent.pinchart@ideasonboard.com" 
	<laurent.pinchart@ideasonboard.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: Re: [PATCH] usb: gadget: uvc: limit isoc_sg to super speed gadgets
Date: Tue, 11 Oct 2022 23:41:59 +0000	[thread overview]
Message-ID: <20221011234145.xzy4466z4tl4ygp6@synopsys.com> (raw)
In-Reply-To: <20221011213256.GI27626@pengutronix.de>

On Tue, Oct 11, 2022, Michael Grzeschik wrote:
> On Tue, Oct 11, 2022 at 10:57:07PM +0200, Michael Grzeschik wrote:
> > The overhead of preparing sg data is high for transfers with limited
> > payload. When transferring isoc over high-speed usb the maximum payload
> > is rather small which is a good argument no to use sg. This patch is
> > changing the uvc_video_encode_isoc_sg encode function only to be used
> > for super speed gadgets.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> > index bb037fcc90e69e..5081eb3bc5484c 100644
> > --- a/drivers/usb/gadget/function/uvc_video.c
> > +++ b/drivers/usb/gadget/function/uvc_video.c
> > @@ -448,6 +448,9 @@ static void uvcg_video_pump(struct work_struct *work)
> >  */
> > int uvcg_video_enable(struct uvc_video *video, int enable)
> > {
> > +	struct uvc_device *uvc = video->uvc;
> > +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> > +	struct usb_gadget *gadget = cdev->gadget;
> > 	unsigned int i;
> > 	int ret;
> > 
> > @@ -479,9 +482,11 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
> > 	if (video->max_payload_size) {
> > 		video->encode = uvc_video_encode_bulk;
> > 		video->payload_size = 0;
> > -	} else
> > -		video->encode = video->queue.use_sg ?
> > +	} else {
> > +		video->encode = (video->queue.use_sg &&
> > +				 !(gadget->speed <= USB_SPEED_HIGH)) ?
> 
> I also came up with the following Idea:
> 
> -                                !(gadget->speed <= USB_SPEED_HIGH)) ?
> +                                video->req_size > 4096) ?
> 
> Would this threshold of 4096 make sense? What should be preferred?

Maybe req_size > PAGE_SIZE?

I'm not sure I understand why SG is being used here. It seems like we're
using a single contiguous buffer here right?

The uvc_video_encode_isoc_sg() only splits the single contiguous buffer
into SG entries for the video frame header + the remaining size of the
data? This seems to only add to the overhead, maybe I'm missing
something?

> 
> > 			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
> > +	}
> > 
> > 	video->req_int_count = 0;
> > 
> > -- 
> > 2.30.2
> > 

In dwc3 device mode, we need to handle the case similar to this for
host: 2017a1e58472 ("usb: xhci: Use temporary buffer to consolidate
SG"). Right now we haven't made this update to dwc3 driver yet.

So we should to be careful when splitting too many SG entries (more than
TRB_CACHE_SIZE) while adding up less than the endpoint's max packet
size. The current UVC implementation won't hit this scenario yet because
a PAGE_SIZE is generally greater than the endpoint MPS, but keep this in
mind for future changes.

Thanks,
Thinh