Re: [PATCH v2 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,

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



[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