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
