| Message ID | 20210530223041.15320-4-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: improve uvc gadget performance | expand |
Hi Michael, 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; > + > 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); > > + if (list_empty(&video->req_free) || > + (buf->state == UVC_BUF_STATE_DONE) || > + (!(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; > > /* Initialize the video buffers queue. */ > uvcg_queue_init(uvc->v4l2_dev.dev, &video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT, > -- > 2.29.2 >
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,
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 >
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; + 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); + if (list_empty(&video->req_free) || + (buf->state == UVC_BUF_STATE_DONE) || + (!(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; /* Initialize the video buffers queue. */ uvcg_queue_init(uvc->v4l2_dev.dev, &video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
With usb3 we handle much more requests. It only enables the interrupt on every quarter of the allocated requests. This patch decreases the interrupt load. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/gadget/function/uvc.h | 2 ++ drivers/usb/gadget/function/uvc_video.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)