Re: [PATCH 2/5] usb: gadget: uvc: calculate the number of request depending on framesize

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]