| 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 |
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 --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;
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(-)