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

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

[Michael Grzeschik]

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)) ?
 			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
+	}
 
 	video->req_int_count = 0;
 
-- 
2.30.2

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

[Michael Grzeschik]

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

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?

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

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

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

[Thinh Nguyen]

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

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

[Greg KH]

On Tue, Oct 11, 2022 at 11:32:56PM +0200, 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?

Where did you pick 4096 from?  Even if you pick PAGE_SIZE, why?  Last
time I ran memcpy() tests vs. sg on a range of processors the benifit
did not kick in until the value was MUCH larger than just 4k, but I
guess that depends on the overall codepath involved here.

So please test, and don't just guess, and then document it really really
well why you picked that value.

For now, the SPEED_HIGH should be sufficient I think.

thanks,

greg k-h