| Message ID | 20221011183437.298437-2-w36195@motorola.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvc gadget performance issues | expand |
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 --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 {
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(-)