[v4,5/6] usb: gadget: uvc: make interrupt skip logic configurable

Message ID 20221018215044.765044-6-w36195@motorola.com
State New
Headers
Series uvc gadget performance issues |

Commit Message

Dan Vacura Oct. 18, 2022, 9:50 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.

Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- no change, new patch in series
V2 -> V3:
- default to baseline value of 4, fix storing the initial value
V3 -> V4:
- no change

 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                 | 2 ++
 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, 19 insertions(+), 1 deletion(-)
  

Comments

Greg KH Oct. 22, 2022, 11:32 a.m. UTC | #1
On Tue, Oct 18, 2022 at 04:50:41PM -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.
> 
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V2:
> - no change, new patch in series
> V2 -> V3:
> - default to baseline value of 4, fix storing the initial value
> V3 -> V4:
> - no change
> 
>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>  Documentation/usb/gadget-testing.rst              | 2 ++

Adding more tunable knobs always just adds complexity and fragility as
things do not get tested.

Can't we just make this dynamic and work properly on all systems?  What
UDC hardware types do not allow skipping interrupts?  Why can't we just
detect that and only do that if this is the case or not?

>  drivers/usb/gadget/function/f_uvc.c               | 3 +++
>  drivers/usb/gadget/function/u_uvc.h               | 1 +
>  drivers/usb/gadget/function/uvc.h                 | 2 ++
>  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, 19 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

This provides no information that would help to determine how to
configure this at all.  I wouldn't know what to do with this, so that's
a huge sign that this is not a good interface :(

thanks,

greg k-h
  

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..e40ca26b9c55 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->config_skip_int_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 = 4;
 	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..29f9477c92cc 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);
@@ -155,6 +156,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_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 0aa3d7e1f3cc..02559906a55a 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->uvc->config_skip_int_div) {
+		video->req_int_skip_div = nreq;
+	} else {
+		video->req_int_skip_div = min_t(unsigned int, nreq,
+				video->uvc->config_skip_int_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 dd1c6b2ca7c6..c1a80c5d1f63 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -423,7 +423,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 {