Re: [PATCH v3] usb: gadget: uvc: clean up comments and styling in video_pump

[Laurent Pinchart]

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