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@xxxxxxxxxxxxxx>
> Tested-by: Dan Vacura <w36195@xxxxxxxxxxxx>
>
> ---
> 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;
> }
--
Regards,
Laurent Pinchart
