Message ID | 20221011205707.1603017-1-m.grzeschik@pengutronix.de (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1oiMJZ-006V66-Ej; Tue, 11 Oct 2022 20:57:27 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229621AbiJKU5P (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 11 Oct 2022 16:57:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56728 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229594AbiJKU5N (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 11 Oct 2022 16:57:13 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3E2EB9DD86 for <linux-media@vger.kernel.org>; Tue, 11 Oct 2022 13:57:12 -0700 (PDT) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <mgr@pengutronix.de>) id 1oiMJK-0005U8-Fp; Tue, 11 Oct 2022 22:57:10 +0200 Received: from [2a0a:edc0:0:1101:1d::ac] (helo=dude04.red.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.94.2) (envelope-from <mgr@pengutronix.de>) id 1oiMJJ-000xeC-Lb; Tue, 11 Oct 2022 22:57:09 +0200 Received: from mgr by dude04.red.stw.pengutronix.de with local (Exim 4.94.2) (envelope-from <mgr@pengutronix.de>) id 1oiMJI-006j2D-N1; Tue, 11 Oct 2022 22:57:08 +0200 From: Michael Grzeschik <m.grzeschik@pengutronix.de> To: linux-usb@vger.kernel.org Cc: linux-media@vger.kernel.org, balbi@kernel.org, laurent.pinchart@ideasonboard.com, kernel@pengutronix.de Subject: [PATCH] usb: gadget: uvc: limit isoc_sg to super speed gadgets Date: Tue, 11 Oct 2022 22:57:07 +0200 Message-Id: <20221011205707.1603017-1-m.grzeschik@pengutronix.de> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: mgr@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-media@vger.kernel.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.4 (--) X-LSpam-Report: No, score=-2.4 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
usb: gadget: uvc: limit isoc_sg to super speed gadgets
|
|
Commit Message
Michael Grzeschik
Oct. 11, 2022, 8:57 p.m. UTC
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(-)
Comments
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;