Hi, On Mon, Jul 24, 2023 at 01:47:09PM +0100, Dan Scally wrote:
On 25/11/2022 15:34, Michael Grzeschik wrote:When calling uvc_video_encode_isoc_sg the function is preparing the sg payload by setting the sglist pointers of the videobuffer for the request. The usb gadget driver then is parsing the sg list and uses each sg entry to send in one urb to the host. Because of the unrelated buffer of the uvc header that buffer has to be send separately in an extra sg entry. When it comes to transfers with an limited payload (e.g. the maximum of 3kB for high-speed) this extra payload handling is not justified. A simple memcpy of the header and payload is usually faster and does not come with that extra runtime overhead.Sorry for the delay with this one. I don't suppose you benchmarked this at all? I'd be curious to see the effect.
No, I don't think that this was benchmarked. However since the last discussions about overall smaller request sizes to increase the performance on the fifo side of the usb-controller this whole topic needs to be rethought. I will look over this once we have a solution for the worker/queue_req situation as discussed in this thread: https://lore.kernel.org/linux-usb/ZToOJhyOFeGCGUFj@xxxxxxxxxxxxxx/T/#t
This patch is changing the uvc_video_encode_isoc_sg encode function only to be used for super speed gadgets. Cc: stable <stable@xxxxxxxxxx> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> --- v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg - rephrased the commit message drivers/usb/gadget/function/uvc_queue.c | 3 +-- drivers/usb/gadget/function/uvc_video.c | 9 +++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c index 0aa3d7e1f3cc32..0abb1763faf1b6 100644 --- a/drivers/usb/gadget/function/uvc_queue.c +++ b/drivers/usb/gadget/function/uvc_queue.c @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb) if (queue->use_sg) { buf->sgt = vb2_dma_sg_plane_desc(vb, 0); buf->sg = buf->sgt->sgl; - } else { - buf->mem = vb2_plane_vaddr(vb, 0); } + buf->mem = vb2_plane_vaddr(vb, 0); buf->length = vb2_plane_size(vb, 0); if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) buf->bytesused = 0; diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index dd1c6b2ca7c6f3..b6ea600b011185 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -459,6 +459,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; @@ -490,9 +493,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; + }I think it'd be better to just set video->queue.use_sg n uvcg_queue_init() by checking cdev->gadget->speed as well as cdev->gadget->sg_supported; can we do that?
Yes, this is the better place to check this. In case this patch will be send again, I will work this in sure.
video->req_int_count = 0;
Michael -- 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:
signature.asc
Description: PGP signature
