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:

quoted

On Mon, May 31, 2021 at 12:30:41AM +0200, Michael Grzeschik wrote:

quoted

With usb3 we handle much more requests. It only enables the interrupt on

s/much/many/

quoted

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."

quoted

Signed-off-by: Michael Grzeschik <redacted>

Other than that, looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

quoted

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

quoted

quoted

+
 	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.

quoted

quoted

+		if (list_empty(&video->req_free) ||
+		    (buf->state == UVC_BUF_STATE_DONE) ||

No need for parentheses here.

quoted

quoted

+		    (!(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

quoted

quoted

 	/* 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 |