| Message ID | 20220608110339.141036-2-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 87d76b5f1d8eeb49efa16e2018e188864cbb9401 |
| Headers | show |
| Series | usb: gadget: uvc: fixes and improvements | expand |
Hi Michael, Thank you for the patch. On Wed, Jun 08, 2022 at 01:03:37PM +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. > > For a 1080p jpeg encoded video stream with a maximum imagesize of > e.g. 800kB with a maxburst of 8 and an multiplier of 1 the resulting > number of requests is calculated to 49. > > 800768 1 > nreqs = ------ * -------------- ~= 49 > 2 (1024 * 8 * 1) > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Tested-by: Dan Vacura <w36195@motorola.com> > > --- > v1 -> v2: - using clamp instead of min/max > - added calculation example to description > - commented the additional division by two in the code > > drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > index d25edc3d2174e1..eb9bd9d32cd056 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,16 @@ 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); No need for parentheses. > + > + /* We divide by two, to increase the chance to run > + * into fewer requests for smaller framesizes. > + */ Could you please change the comment style to the more standard /* * We divide by two, to increase the chance to run into fewer requests * for smaller framesizes. */ (with the text reflowed to 80 columns) ? I'm however now sure where the division by 2 come from. Furthermore, as far as I understand, the reason why the number of requests was increased for superspeed devices (by you ;-)) was to avoid underruns at higher speeds and keep the queue full. This is less of a concern at lower speeds. Is there any drawback in increasing it regardless of the speed ? Increased latency comes to mind, but is it a problem in practice ? > + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); > + nreq = clamp(nreq, 4U, 64U); > + video->uvc_num_requests = nreq; > > return 0; > }
Hi Laurent, On Wed, Jun 08, 2022 at 09:06:55PM +0300, Laurent Pinchart wrote: >On Wed, Jun 08, 2022 at 01:03:37PM +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. >> >> For a 1080p jpeg encoded video stream with a maximum imagesize of >> e.g. 800kB with a maxburst of 8 and an multiplier of 1 the resulting >> number of requests is calculated to 49. >> >> 800768 1 >> nreqs = ------ * -------------- ~= 49 >> 2 (1024 * 8 * 1) >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> Tested-by: Dan Vacura <w36195@motorola.com> >> >> --- >> v1 -> v2: - using clamp instead of min/max >> - added calculation example to description >> - commented the additional division by two in the code >> >> drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c >> index d25edc3d2174e1..eb9bd9d32cd056 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,16 @@ 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); > >No need for parentheses. > >> + >> + /* We divide by two, to increase the chance to run >> + * into fewer requests for smaller framesizes. >> + */ > >Could you please change the comment style to the more standard > > /* > * We divide by two, to increase the chance to run into fewer requests > * for smaller framesizes. > */ > >(with the text reflowed to 80 columns) ? These two things have to be fixed in a seperate patch. Greg seems to have taken it already, as is. >I'm however now sure where the division by 2 come from. > >Furthermore, as far as I understand, the reason why the number of >requests was increased for superspeed devices (by you ;-)) was to avoid >underruns at higher speeds and keep the queue full. This is less of a >concern at lower speeds. Is there any drawback in increasing it >regardless of the speed ? Increased latency comes to mind, but is it a >problem in practice ? This is a good question. Lets think through the case, where we set the number of requests to an high number, just to be safe. I think that for lower bandwidth USB you will probably not send high resolution video. Especially if they are uncompressed. So having an extra amount of requests available, they will probably unnecessary keep more data than one frame in memory. For the high bandwidth usb case we would by default also keep an extra amount of memory available, especially if the burst and mult is set in the upper values. We would have to think of an resonable value for both cases. This is why I think depending on the framesize is an good compromise. >> + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); >> + nreq = clamp(nreq, 4U, 64U); >> + 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 d25edc3d2174e1..eb9bd9d32cd056 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,16 @@ 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); + + /* We divide by two, to increase the chance to run + * into fewer requests for smaller framesizes. + */ + nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); + nreq = clamp(nreq, 4U, 64U); + 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. For a 1080p jpeg encoded video stream with a maximum imagesize of e.g. 800kB with a maxburst of 8 and an multiplier of 1 the resulting number of requests is calculated to 49. 800768 1 nreqs = ------ * -------------- ~= 49 2 (1024 * 8 * 1) Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Tested-by: Dan Vacura <w36195@motorola.com> --- v1 -> v2: - using clamp instead of min/max - added calculation example to description - commented the additional division by two in the code drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)