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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> 
> ---
> 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;
>  

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux