Hi Avichal, Thank you for the patch. On Fri, Jun 02, 2023 at 03:04:55PM -0700, Avichal Rakesh wrote: > This patch elaborates on some of the edge cases handled by > video_pump around setting no_interrupt flag, and brings the > code style in line with rest of the file. > > Link: https://lore.kernel.org/20230602151916.GH26944@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > Signed-off-by: Avichal Rakesh <arakesh@xxxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > --- > Changelog: > v2: > - Updated commit message to make it clear that userspace application is not > required to match the ISOC rate. > - Styling and comment revision based on review > v3: > - Rebased on to Greg's usb-next where v1 had already merged > - Updated commit message to match the actual changes after rebase. > > > drivers/usb/gadget/function/uvc_video.c | 38 ++++++++++++++++--------- > 1 file changed, 25 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index e81865978299..91af3b1ef0d4 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -382,13 +382,13 @@ static void uvcg_video_pump(struct work_struct *work) > { > struct uvc_video *video = container_of(work, struct uvc_video, pump); > struct uvc_video_queue *queue = &video->queue; > + /* video->max_payload_size is only set when using bulk transfer */ > + bool is_bulk = video->max_payload_size; > struct usb_request *req = NULL; > struct uvc_buffer *buf; > unsigned long flags; > + bool buf_done; > int ret; > - bool buf_int; > - /* video->max_payload_size is only set when using bulk transfer */ > - bool is_bulk = video->max_payload_size; > > while (video->ep->enabled) { > /* > @@ -414,20 +414,19 @@ static void uvcg_video_pump(struct work_struct *work) > > if (buf != NULL) { > video->encode(req, video, buf); > - /* Always interrupt for the last request of a video buffer */ > - buf_int = buf->state == UVC_BUF_STATE_DONE; > + buf_done = buf->state == UVC_BUF_STATE_DONE; > } else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) && !is_bulk) { > /* > * No video buffer available; the queue is still connected and > - * we're traferring over ISOC. Queue a 0 length request to > + * we're transferring over ISOC. Queue a 0 length request to > * prevent missed ISOC transfers. > */ > req->length = 0; > - buf_int = false; > + buf_done = false; > } else { > /* > - * Either queue has been disconnected or no video buffer > - * available to bulk transfer. Either way, stop processing > + * Either the queue has been disconnected or no video buffer > + * available for bulk transfer. Either way, stop processing > * further. > */ > spin_unlock_irqrestore(&queue->irqlock, flags); > @@ -435,11 +434,24 @@ static void uvcg_video_pump(struct work_struct *work) > } > > /* > - * With usb3 we have more requests. This will decrease the > - * interrupt load to a quarter but also catches the corner > - * cases, which needs to be handled. > + * With USB3 handling more requests at a higher speed, we can't > + * afford to generate an interrupt for every request. Decide to > + * interrupt: > + * > + * - When no more requests are available in the free queue, as > + * this may be our last chance to refill the endpoint's > + * request queue. > + * > + * - When this is request is the last request for the video > + * buffer, as we want to start sending the next video buffer > + * ASAP in case it doesn't get started already in the next > + * iteration of this loop. > + * > + * - Four times over the length of the requests queue (as > + * indicated by video->uvc_num_requests), as a trade-off > + * between latency and interrupt load. > */ > - if (list_empty(&video->req_free) || buf_int || > + if (list_empty(&video->req_free) || buf_done || > !(video->req_int_count % > DIV_ROUND_UP(video->uvc_num_requests, 4))) { > video->req_int_count = 0; -- Regards, Laurent Pinchart
