diff mbox series

[RESEND,2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

Message ID 20221026184212.366897-2-m.grzeschik@pengutronix.de
State Superseded
Headers show
Series [1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks | expand

Commit Message

Michael Grzeschik Oct. 26, 2022, 6:42 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.

Cc: stable <stable@kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
Since the last time the patch was send, I saw some issues regarding
the use of the vaddr. I thought there was a fix needed in this code.

But the problem was to be found in the videobuf2-dma-sg vmap/vunmap
implementation. So this patch stays unchanged and is save to be applied,
if the corresponding videobuf2-dma-sg patch is applied before it.

 drivers/usb/gadget/function/uvc_queue.c | 9 +++------
 drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
 2 files changed, 10 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart Nov. 6, 2022, 7:01 p.m. UTC | #1
Hi Michael,

Thank you for the patch.

On Wed, Oct 26, 2022 at 08:42:12PM +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

What exactly is causing a high overhead, and how big is it ?

> 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.
> 
> Cc: stable <stable@kernel.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> Since the last time the patch was send, I saw some issues regarding
> the use of the vaddr. I thought there was a fix needed in this code.
> 
> But the problem was to be found in the videobuf2-dma-sg vmap/vunmap
> implementation. So this patch stays unchanged and is save to be applied,
> if the corresponding videobuf2-dma-sg patch is applied before it.
> 
>  drivers/usb/gadget/function/uvc_queue.c | 9 +++------
>  drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ec500ee499eed1..31c50ba1774f0d 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -84,12 +84,9 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>  		return -ENODEV;
>  
>  	buf->state = UVC_BUF_STATE_QUEUED;
> -	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->sgt = vb2_dma_sg_plane_desc(vb, 0);
> +	buf->sg = buf->sgt->sgl;

vb2_dma_sg_plane_desc() is defined as

static inline struct sg_table *vb2_dma_sg_plane_desc(
		struct vb2_buffer *vb, unsigned int plane_no)
{
	return (struct sg_table *)vb2_plane_cookie(vb, plane_no);
}

and vb2_plane_cookie() as

void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
{
	if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
		return NULL;

	return call_ptr_memop(cookie, vb, vb->planes[plane_no].mem_priv);
}

If the queue isn't using scatter-gather (use_sg == false), the cookie
operation will not be defined, and buf->sgt will be NULL. Dereferencing
it will thus segfault.

> +	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 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;
>
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499eed1..31c50ba1774f0d 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -84,12 +84,9 @@  static int uvc_buffer_prepare(struct vb2_buffer *vb)
 		return -ENODEV;
 
 	buf->state = UVC_BUF_STATE_QUEUED;
-	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->sgt = vb2_dma_sg_plane_desc(vb, 0);
+	buf->sg = buf->sgt->sgl;
+	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 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;