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

From: Dan Vacura
Date: Wed Oct 12 2022 - 16:45:24 EST

Next message: Dmitry Baryshkov: "Re: [PATCH 03/20] phy: qcom-qmp-pcie: fix ipq6018 initialisation"
Previous message: Dmitry Baryshkov: "Re: [PATCH 02/20] phy: qcom-qmp-pcie: fix ipq8074-gen3 initialisation"
In reply to: Dan Vacura: "[PATCH v2 1/3] usb: gadget: uvc: make interrupt skip logic configurable"
Next in thread: Dan Vacura: "[PATCH v2 2/3] usb: gadget: uvc: fix sg handling in error case"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxx>
> Signed-off-by: Dan Vacura <w36195@xxxxxxxxxxxx>
> ---
> 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 message: Dmitry Baryshkov: "Re: [PATCH 03/20] phy: qcom-qmp-pcie: fix ipq6018 initialisation"
Previous message: Dmitry Baryshkov: "Re: [PATCH 02/20] phy: qcom-qmp-pcie: fix ipq8074-gen3 initialisation"
In reply to: Dan Vacura: "[PATCH v2 1/3] usb: gadget: uvc: make interrupt skip logic configurable"
Next in thread: Dan Vacura: "[PATCH v2 2/3] usb: gadget: uvc: fix sg handling in error case"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]