Re: [PATCH v4 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated

Michael Grzeschik
2024-08-27T21:59:01Z

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

On Tue, Aug 13, 2024 at 12:41:10PM +0300, Laurent Pinchart wrote:
>On Tue, Aug 13, 2024 at 11:09:30AM +0200, Michael Grzeschik wrote:
>> The uvc gadget driver is calculating the req_size on every
>> video_enable/alloc_request and is based on the fixed configfs parameters
>> maxpacket, maxburst and mult.
>>
>> As those parameters can not be changed once the gadget is started and
>> the same calculation is done already early on the
>> vb2_streamon/queue_setup path its save to remove one extra calculation
>> and reuse the calculation from uvc_queue_setup for the allocation step.
>
>Avoiding double calculations is good, but then don't compute the value
>in uvc_queue_setup(). That will also be called multiple times, and its
>timing will be controlled by userspace. Move it to a better location.

I think I was unclear regarding the avoidance of double calculations.

Actually this is the right place to calculate this, since the
resulting req_size will be dependent on the expected imagesize
and the frameduration, we have to calculate it in every queue_setup.

Since this patch is an preperation for the next change, I will leave the
codebase as is but will add the details in the patch description.

Michael

>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v3 -> v4: -
>> v2 -> v3: -
>> v1 -> v2: -
>> ---
>> drivers/usb/gadget/function/uvc_queue.c | 2 ++
>> drivers/usb/gadget/function/uvc_video.c | 15 ++-------------
>> 2 files changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index 7995dd3fef184..2414d78b031f4 100644
>> --- a/drivers/usb/gadget/function/uvc_queue.c
>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>> @@ -63,6 +63,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>> */
>> nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
>> nreq = clamp(nreq, 4U, 64U);
>> +
>> + video->req_size = req_size;
>> video->uvc_num_requests = nreq;
>>
>> return 0;
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 259920ae36843..a6786beef91ad 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -480,7 +480,6 @@ uvc_video_free_requests(struct uvc_video *video)
>> INIT_LIST_HEAD(&video->ureqs);
>> INIT_LIST_HEAD(&video->req_free);
>> INIT_LIST_HEAD(&video->req_ready);
>> - video->req_size = 0;
>> return 0;
>> }
>>
>> @@ -488,16 +487,9 @@ static int
>> uvc_video_alloc_requests(struct uvc_video *video)
>> {
>> struct uvc_request *ureq;
>> - unsigned int req_size;
>> unsigned int i;
>> int ret = -ENOMEM;
>>
>> - BUG_ON(video->req_size);
>> -
>> - req_size = video->ep->maxpacket
>> - * max_t(unsigned int, video->ep->maxburst, 1)
>> - * (video->ep->mult);
>> -
>> for (i = 0; i < video->uvc_num_requests; i++) {
>> ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL);
>> if (ureq == NULL)
>> @@ -507,7 +499,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
>>
>> list_add_tail(&ureq->list, &video->ureqs);
>>
>> - ureq->req_buffer = kmalloc(req_size, GFP_KERNEL);
>> + ureq->req_buffer = kmalloc(video->req_size, GFP_KERNEL);
>> if (ureq->req_buffer == NULL)
>> goto error;
>>
>> @@ -525,12 +517,10 @@ uvc_video_alloc_requests(struct uvc_video *video)
>> list_add_tail(&ureq->req->list, &video->req_free);
>> /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
>> sg_alloc_table(&ureq->sgt,
>> - DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
>> + DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN,
>> PAGE_SIZE) + 2, GFP_KERNEL);
>> }
>>
>> - video->req_size = req_size;
>> -
>> return 0;
>>
>> error:
>> @@ -663,7 +653,6 @@ uvcg_video_disable(struct uvc_video *video)
>> INIT_LIST_HEAD(&video->ureqs);
>> INIT_LIST_HEAD(&video->req_free);
>> INIT_LIST_HEAD(&video->req_ready);
>> - video->req_size = 0;
>> spin_unlock_irqrestore(&video->req_lock, flags);
>>
>> /*
>
>--
>Regards,
>
>Laurent Pinchart
>

--
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 --]