Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 4 Jun 2023 10:54:04 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Avichal Rakesh <arakesh@google.com>
Cc:     dan.scally@ideasonboard.com, thinh.nguyen@synopsys.com,
        etalvala@google.com, gregkh@linuxfoundation.org,
        jchowdhary@google.com, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org
Subject: Re: [PATCH v3] usb: gadget: uvc: clean up comments and styling in
 video_pump
Message-ID: <20230604075404.GA28735@pendragon.ideasonboard.com>
References: <20230602211602.3b7rfa252wliiszp@synopsys.com>
 <20230602220455.313801-1-arakesh@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230602220455.313801-1-arakesh@google.com>
Precedence: bulk

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@pendragon.ideasonboard.com/
> Signed-off-by: Avichal Rakesh <arakesh@google.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 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