Re: [PATCH v2 3/3] usb: gadget: uvc: decrease the interrupt load to a quarter

[Michael Grzeschik <mgr@pengutronix.de>]

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

Hi Laurent!

On Tue, Jun 15, 2021 at 04:36:53AM +0300, Laurent Pinchart wrote:
>Hi Michael,
>
>Thank you for the patch.
>
>On Mon, Jun 14, 2021 at 07:35:58PM +0900, paul.elder@ideasonboard.com wrote:
>> On Mon, May 31, 2021 at 12:30:41AM +0200, Michael Grzeschik wrote:
>> > With usb3 we handle much more requests. It only enables the interrupt on
>>
>> s/much/many/
>>
>> > every quarter of the allocated requests. This patch decreases the
>> > interrupt load.
>>
>> The last two sentences might be better combined, like:
>>
>> "Decrease the interrupt load by only enabling the interrupt every
>> quarter of the allocated requests."
>>
>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> Other than that, looks good to me.
>>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>>
>> > ---
>> >  drivers/usb/gadget/function/uvc.h       |  2 ++
>> >  drivers/usb/gadget/function/uvc_video.c | 12 ++++++++++++
>> >  2 files changed, 14 insertions(+)
>> >
>> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> > index c1f06d9df5820..5a76e9351b530 100644
>> > --- a/drivers/usb/gadget/function/uvc.h
>> > +++ b/drivers/usb/gadget/function/uvc.h
>> > @@ -101,6 +101,8 @@ struct uvc_video {
>> >  	struct list_head req_free;
>> >  	spinlock_t req_lock;
>> >
>> > +	int req_int_count;
>
>unsigned int.
>
>> > +
>> >  	void (*encode) (struct usb_request *req, struct uvc_video *video,
>> >  			struct uvc_buffer *buf);
>> >
>> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> > index 240d361a45a44..66754687ce217 100644
>> > --- a/drivers/usb/gadget/function/uvc_video.c
>> > +++ b/drivers/usb/gadget/function/uvc_video.c
>> > @@ -357,6 +357,16 @@ static void uvcg_video_pump(struct work_struct *work)
>> >
>> >  		video->encode(req, video, buf);
>> >
>
>A comment to explain the logic would be useful.
>
>> > +		if (list_empty(&video->req_free) ||
>> > +		    (buf->state == UVC_BUF_STATE_DONE) ||
>
>No need for parentheses here.
>
>> > +		    (!(video->req_int_count %
>> > +		       DIV_ROUND_UP(video->uvc_num_requests, 4)))) {
>> > +			video->req_int_count = 0;
>> > +			req->no_interrupt = 0;
>> > +		} else {
>> > +			req->no_interrupt = 1;
>> > +		}
>> > +
>> >  		/* Queue the USB request */
>> >  		ret = uvcg_video_ep_queue(video, req);
>> >  		spin_unlock_irqrestore(&queue->irqlock, flags);
>> > @@ -365,6 +375,7 @@ static void uvcg_video_pump(struct work_struct *work)
>> >  			uvcg_queue_cancel(queue, 0);
>> >  			break;
>> >  		}
>> > +		video->req_int_count++;
>> >  	}
>> >
>> >  	spin_lock_irqsave(&video->req_lock, flags);
>> > @@ -437,6 +448,7 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>> >  	video->width = 320;
>> >  	video->height = 240;
>> >  	video->imagesize = 320 * 240 * 2;
>> > +	video->req_int_count = 0;
>
>Should this be initialized to 0 in uvcg_video_enable() instead of
>uvcg_video_init(), to ensure that stop/start cycles will operate in a
>predictable way ?

This makes total sense. I don't see why it should not start by 0 on
every enable. I worked in yours and Paul's feedback and moved the
req_int_count initialization to uvcg_video_enable.

Thanks!

Michael Grzeschik

>> >
>> >  	/* Initialize the video buffers queue. */
>> >  	uvcg_queue_init(uvc->v4l2_dev.dev, &video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
>
>-- 
>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 --]