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@xxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx> Other than that, looks good to me. Reviewed-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx> > --- > 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:
signature.asc
Description: PGP signature
