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:   Wed, 12 Oct 2022 15:44:37 -0500
From:   Dan Vacura <w36195@motorola.com>
To:     linux-usb@vger.kernel.org
Cc:     Daniel Scally <dan.scally@ideasonboard.com>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        stable@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Felipe Balbi <balbi@kernel.org>,
        Paul Elder <paul.elder@ideasonboard.com>,
        Michael Grzeschik <m.grzeschik@pengutronix.de>,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v2 1/3] usb: gadget: uvc: make interrupt skip logic
 configurable
Message-ID: <Y0cnNSE+pHOQ0mMo@p1g3>
References: <20221011183437.298437-1-w36195@motorola.com>
 <20221011183437.298437-2-w36195@motorola.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221011183437.298437-2-w36195@motorola.com>
Precedence: bulk

It looks like configurability of interrupt throttling is not in favor,
but if we do proceed with this patch, I'll need to fix some logic. I
found a bug where req_int_skip_div will have a stale value used with
repeated resolution switches.

This fixes the bug:

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 003c2d610e61..b7a5681d5f85 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -649,7 +649,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
                cpu_to_le16(max_packet_size * max_packet_mult *
                            (opts->streaming_maxburst + 1));

-       uvc->video.req_int_skip_div = opts->req_int_skip_div;
+       uvc->config_skip_int_div = opts->req_int_skip_div;
        uvc->video.queue.use_sg = opts->sg_supported;

        /* Allocate endpoints. */
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index ddca23680c35..e7033cce0a43 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -153,6 +153,7 @@ struct uvc_device {
        /* Events */
        unsigned int event_length;
        unsigned int event_setup_out : 1;
+       unsigned int config_skip_int_div;
 };

 static inline struct uvc_device *to_uvc(struct usb_function *f)
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index c7b76ac36194..b6fada4eab12 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -63,11 +63,11 @@ static int uvc_queue_setup(struct vb2_queue *vq,
         */
        nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
        nreq = clamp(nreq, 4U, 64U);
-       if (0 == video->req_int_skip_div) {
+       if (0 == video->uvc->config_skip_int_div) {
                video->req_int_skip_div = nreq;
        } else {
-               video->req_int_skip_div =
-                       min_t(unsigned int, nreq, video->req_int_skip_div);
+               video->req_int_skip_div = min_t(unsigned int, nreq,
+                               video->uvc->config_skip_int_div);
        }
        video->uvc_num_requests = nreq;

On Tue, Oct 11, 2022 at 01:34:33PM -0500, Dan Vacura wrote:
> Some UDC hw may not support skipping interrupts, but still support the
> request. Allow the interrupt frequency to be configurable to the user.
> Default to not skip interrupts, a value of 0. This fixes a smmu panic
> that is occurring on dwc3 hw.
> 
> Fixes: fc78941d8169 ("usb: gadget: uvc: decrease the interrupt load to a quarter")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V2:
> - no change, new patch in series
> 
>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>  Documentation/usb/gadget-testing.rst              | 2 ++
>  drivers/usb/gadget/function/f_uvc.c               | 3 +++
>  drivers/usb/gadget/function/u_uvc.h               | 1 +
>  drivers/usb/gadget/function/uvc.h                 | 1 +
>  drivers/usb/gadget/function/uvc_configfs.c        | 2 ++
>  drivers/usb/gadget/function/uvc_queue.c           | 6 ++++++
>  drivers/usb/gadget/function/uvc_video.c           | 3 ++-
>  8 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 611b23e6488d..5dfaa3f7f6a4 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -8,6 +8,7 @@ Description:	UVC function directory
>  		streaming_maxpacket	1..1023 (fs), 1..3072 (hs/ss)
>  		streaming_interval	1..16
>  		function_name		string [32]
> +		req_int_skip_div	unsigned int
>  		===================	=============================
>  
>  What:		/config/usb-gadget/gadget/functions/uvc.name/control
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index 2278c9ffb74a..f9b5a09be1f4 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -794,6 +794,8 @@ The uvc function provides these attributes in its function directory:
>  			    sending or receiving when this configuration is
>  			    selected
>  	function_name       name of the interface
> +	req_int_skip_div    divisor of total requests to aid in calculating
> +			    interrupt frequency, 0 indicates all interrupt
>  	=================== ================================================
>  
>  There are also "control" and "streaming" subdirectories, each of which contain
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 6e196e06181e..75f524c83996 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -655,6 +655,8 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  		cpu_to_le16(max_packet_size * max_packet_mult *
>  			    (opts->streaming_maxburst + 1));
>  
> +	uvc->video.req_int_skip_div = opts->req_int_skip_div;
> +
>  	/* Allocate endpoints. */
>  	ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
>  	if (!ep) {
> @@ -872,6 +874,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>  
>  	opts->streaming_interval = 1;
>  	opts->streaming_maxpacket = 1024;
> +	opts->req_int_skip_div = 0;
>  	snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");
>  
>  	ret = uvcg_attach_configfs(opts);
> diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
> index 24b8681b0d6f..6f73bd5638ed 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -24,6 +24,7 @@ struct f_uvc_opts {
>  	unsigned int					streaming_interval;
>  	unsigned int					streaming_maxpacket;
>  	unsigned int					streaming_maxburst;
> +	unsigned int					req_int_skip_div;
>  
>  	unsigned int					control_interface;
>  	unsigned int					streaming_interface;
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 40226b1f7e14..53175cd564e5 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -107,6 +107,7 @@ struct uvc_video {
>  	spinlock_t req_lock;
>  
>  	unsigned int req_int_count;
> +	unsigned int req_int_skip_div;
>  
>  	void (*encode) (struct usb_request *req, struct uvc_video *video,
>  			struct uvc_buffer *buf);
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 4303a3283ba0..419e926ab57e 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -2350,6 +2350,7 @@ UVC_ATTR(f_uvc_opts_, cname, cname)
>  UVCG_OPTS_ATTR(streaming_interval, streaming_interval, 16);
>  UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, 3072);
>  UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, 15);
> +UVCG_OPTS_ATTR(req_int_skip_div, req_int_skip_div, UINT_MAX);
>  
>  #undef UVCG_OPTS_ATTR
>  
> @@ -2399,6 +2400,7 @@ static struct configfs_attribute *uvc_attrs[] = {
>  	&f_uvc_opts_attr_streaming_interval,
>  	&f_uvc_opts_attr_streaming_maxpacket,
>  	&f_uvc_opts_attr_streaming_maxburst,
> +	&f_uvc_opts_attr_req_int_skip_div,
>  	&f_uvc_opts_string_attr_function_name,
>  	NULL,
>  };
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ec500ee499ee..872d545838ee 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -63,6 +63,12 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  	 */
>  	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
>  	nreq = clamp(nreq, 4U, 64U);
> +	if (0 == video->req_int_skip_div) {
> +		video->req_int_skip_div = nreq;
> +	} else {
> +		video->req_int_skip_div =
> +			min_t(unsigned int, nreq, video->req_int_skip_div);
> +	}
>  	video->uvc_num_requests = nreq;
>  
>  	return 0;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index bb037fcc90e6..241df42ce0ae 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -413,7 +413,8 @@ static void uvcg_video_pump(struct work_struct *work)
>  		if (list_empty(&video->req_free) ||
>  		    buf->state == UVC_BUF_STATE_DONE ||
>  		    !(video->req_int_count %
> -		       DIV_ROUND_UP(video->uvc_num_requests, 4))) {
> +		       DIV_ROUND_UP(video->uvc_num_requests,
> +			       video->req_int_skip_div))) {
>  			video->req_int_count = 0;
>  			req->no_interrupt = 0;
>  		} else {
> -- 
> 2.34.1
>