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
Date: Wed, 12 Oct 2022 15:44:37 -0500 [thread overview]
Message-ID: <Y0cnNSE+pHOQ0mMo@p1g3> (raw)
In-Reply-To: <20221011183437.298437-2-w36195@motorola.com>
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
>
next prev parent reply other threads:[~2022-10-12 20:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-11 18:34 [PATCH v2 0/3] uvc gadget performance issues Dan Vacura
2022-10-11 18:34 ` [PATCH v2 1/3] usb: gadget: uvc: make interrupt skip logic configurable Dan Vacura
2022-10-12 20:44 ` Dan Vacura [this message]
2022-10-11 18:34 ` [PATCH v2 2/3] usb: gadget: uvc: fix sg handling in error case Dan Vacura
2022-10-11 18:34 ` [PATCH v2 3/3] usb: gadget: uvc: add configfs option for sg support Dan Vacura
2022-10-11 19:48 ` [PATCH v2 0/3] uvc gadget performance issues Michael Grzeschik
2022-10-11 19:53 ` Greg Kroah-Hartman
2022-10-11 20:16 ` Dan Vacura
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y0cnNSE+pHOQ0mMo@p1g3 \
--to=w36195@motorola.com \
--cc=Thinh.Nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=corbet@lwn.net \
--cc=dan.scally@ideasonboard.com \
--cc=gregkh@linuxfoundation.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.grzeschik@pengutronix.de \
--cc=paul.elder@ideasonboard.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.