| Message ID | 20221011205707.1603017-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: uvc: limit isoc_sg to super speed gadgets | expand |
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 > > >
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
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
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;
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(-)