diff mbox series

[v2,1/3] usb: gadget: uvc: make interrupt skip logic configurable

Message ID 20221011183437.298437-2-w36195@motorola.com (mailing list archive)
State Superseded
Headers show
Series uvc gadget performance issues | expand

Commit Message

Dan Vacura Oct. 11, 2022, 6:34 p.m. UTC
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(-)

Comments

Dan Vacura Oct. 12, 2022, 8:44 p.m. UTC | #1
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
>
diff mbox series

Patch

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 {