Hi Michael,
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.
That's a good question. Michael, do you have numbers ?
> > 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?
Agreed, otherwise use_sg will be a misnommer (it could be set to true
without actually using SG). Furthermore, we should not create a CPU
mapping of the buffer when using SG, as that's a waste of resources.
> >
> > video->req_int_count = 0;
> >
--
Regards,
Laurent Pinchart
