| Message ID | 20220402233914.3625405-3-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: fixes and improvements | expand |
Hi Michael, Thank you for the patch. On Sun, Apr 03, 2022 at 01:39:11AM +0200, Michael Grzeschik wrote: > The current limitation of possible number of requests being handled is > dependent on the gadget speed. It makes more sense to depend on the > typical frame size when calculating the number of requests. This patch > is changing this and is using the previous limits as boundaries for > reasonable minimum and maximum number of requests. What are typical values you get for the number of requests in your use cases with this change ? Could you mention them in the commit message ? > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > index cfa0ac4adb04d5..2a091cf07981e1 100644 > --- a/drivers/usb/gadget/function/uvc_queue.c > +++ b/drivers/usb/gadget/function/uvc_queue.c > @@ -44,7 +44,8 @@ static int uvc_queue_setup(struct vb2_queue *vq, > { > struct uvc_video_queue *queue = vb2_get_drv_priv(vq); > struct uvc_video *video = container_of(queue, struct uvc_video, queue); > - struct usb_composite_dev *cdev = video->uvc->func.config->cdev; > + unsigned int req_size; > + unsigned int nreq; > > if (*nbuffers > UVC_MAX_VIDEO_BUFFERS) > *nbuffers = UVC_MAX_VIDEO_BUFFERS; > @@ -53,10 +54,14 @@ static int uvc_queue_setup(struct vb2_queue *vq, > > sizes[0] = video->imagesize; > > - if (cdev->gadget->speed < USB_SPEED_SUPER) > - video->uvc_num_requests = 4; > - else > - video->uvc_num_requests = 64; > + req_size = video->ep->maxpacket > + * max_t(unsigned int, video->ep->maxburst, 1) > + * (video->ep->mult); > + > + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); Where does the division by 2 come from ? > + nreq = min_t(unsigned int, nreq, 64); > + nreq = max_t(unsigned int, nreq, 4); You can use clamp(): video->uvc_num_requests = clamp(nreq, 4U, 64U); > + video->uvc_num_requests = nreq; > > return 0; > }
Hi Laurent, On Tue, Apr 19, 2022 at 11:46:44PM +0300, Laurent Pinchart wrote: >Thank you for the patch. > >On Sun, Apr 03, 2022 at 01:39:11AM +0200, Michael Grzeschik wrote: >> The current limitation of possible number of requests being handled is >> dependent on the gadget speed. It makes more sense to depend on the >> typical frame size when calculating the number of requests. This patch >> is changing this and is using the previous limits as boundaries for >> reasonable minimum and maximum number of requests. > >What are typical values you get for the number of requests in your use >cases with this change ? With this patch, for a 4k Video stream I get usually sizes of 3127808 bytes and 64 requests. With 1080p sizes is 800768 bytes and with this patch I get 56 request. >Could you mention them in the commit message ? Yes, I will add them in the comment of v2. >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c >> index cfa0ac4adb04d5..2a091cf07981e1 100644 >> --- a/drivers/usb/gadget/function/uvc_queue.c >> +++ b/drivers/usb/gadget/function/uvc_queue.c >> @@ -44,7 +44,8 @@ static int uvc_queue_setup(struct vb2_queue *vq, >> { >> struct uvc_video_queue *queue = vb2_get_drv_priv(vq); >> struct uvc_video *video = container_of(queue, struct uvc_video, queue); >> - struct usb_composite_dev *cdev = video->uvc->func.config->cdev; >> + unsigned int req_size; >> + unsigned int nreq; >> >> if (*nbuffers > UVC_MAX_VIDEO_BUFFERS) >> *nbuffers = UVC_MAX_VIDEO_BUFFERS; >> @@ -53,10 +54,14 @@ static int uvc_queue_setup(struct vb2_queue *vq, >> >> sizes[0] = video->imagesize; >> >> - if (cdev->gadget->speed < USB_SPEED_SUPER) >> - video->uvc_num_requests = 4; >> - else >> - video->uvc_num_requests = 64; >> + req_size = video->ep->maxpacket >> + * max_t(unsigned int, video->ep->maxburst, 1) >> + * (video->ep->mult); >> + >> + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); > >Where does the division by 2 come from ? That is a good question. I think that I used it as a tool to come into the range inbetween 4 and 64 requests for 1080p video streaming. Since the framesizes where more likely to run into the 64 requests to be allocated. >> + nreq = min_t(unsigned int, nreq, 64); >> + nreq = max_t(unsigned int, nreq, 4); > >You can use clamp(): > > video->uvc_num_requests = clamp(nreq, 4U, 64U); Thanks! I fixed that for v2. >> + video->uvc_num_requests = nreq; >> >> return 0; >> } Regards, Michael
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c index cfa0ac4adb04d5..2a091cf07981e1 100644 --- a/drivers/usb/gadget/function/uvc_queue.c +++ b/drivers/usb/gadget/function/uvc_queue.c @@ -44,7 +44,8 @@ static int uvc_queue_setup(struct vb2_queue *vq, { struct uvc_video_queue *queue = vb2_get_drv_priv(vq); struct uvc_video *video = container_of(queue, struct uvc_video, queue); - struct usb_composite_dev *cdev = video->uvc->func.config->cdev; + unsigned int req_size; + unsigned int nreq; if (*nbuffers > UVC_MAX_VIDEO_BUFFERS) *nbuffers = UVC_MAX_VIDEO_BUFFERS; @@ -53,10 +54,14 @@ static int uvc_queue_setup(struct vb2_queue *vq, sizes[0] = video->imagesize; - if (cdev->gadget->speed < USB_SPEED_SUPER) - video->uvc_num_requests = 4; - else - video->uvc_num_requests = 64; + req_size = video->ep->maxpacket + * max_t(unsigned int, video->ep->maxburst, 1) + * (video->ep->mult); + + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); + nreq = min_t(unsigned int, nreq, 64); + nreq = max_t(unsigned int, nreq, 4); + video->uvc_num_requests = nreq; return 0; }
The current limitation of possible number of requests being handled is dependent on the gadget speed. It makes more sense to depend on the typical frame size when calculating the number of requests. This patch is changing this and is using the previous limits as boundaries for reasonable minimum and maximum number of requests. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)