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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> >  
> >  	/* Initialize the video buffers queue. */
> >  	uvcg_queue_init(uvc->v4l2_dev.dev, &video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,

-- 
Regards,

Laurent Pinchart