[v4,6/6] usb: gadget: uvc: add configfs option for sg support

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

Commit Message

Dan Vacura Oct. 18, 2022, 9:50 p.m. UTC
  The scatter gather support doesn't appear to work well with some UDC hw.
Add the ability to turn off the feature depending on the controller in
use or other platform quirks. The default is for the uvc gadget to
support sg as long as the UDC hw supports it.

The specific failure was with the dwc3 controller, but fixes and
improvements are pending for those failures. This capability is now
more intended for future unexpected failures or poor sg support on a
given platform.

Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- no change, new patch in series
V2 -> V3:
- default on, same as baseline
V3 -> V4:
- update comment and documentation, refactor use of opts->sg_supported 
  directly in uvc_queue

 Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
 Documentation/usb/gadget-testing.rst              | 4 ++++
 drivers/usb/gadget/function/f_uvc.c               | 1 +
 drivers/usb/gadget/function/u_uvc.h               | 1 +
 drivers/usb/gadget/function/uvc_configfs.c        | 2 ++
 drivers/usb/gadget/function/uvc_queue.c           | 4 +++-
 6 files changed, 12 insertions(+), 1 deletion(-)
  

Comments

Greg KH Oct. 22, 2022, 11:34 a.m. UTC | #1
On Tue, Oct 18, 2022 at 04:50:42PM -0500, Dan Vacura wrote:
> The scatter gather support doesn't appear to work well with some UDC hw.
> Add the ability to turn off the feature depending on the controller in
> use or other platform quirks. The default is for the uvc gadget to
> support sg as long as the UDC hw supports it.
> 
> The specific failure was with the dwc3 controller, but fixes and
> improvements are pending for those failures. This capability is now
> more intended for future unexpected failures or poor sg support on a
> given platform.
> 
> Signed-off-by: Dan Vacura <w36195@motorola.com>

Again, this should be dynamic.  Can't we detect this based on the packet
size and either do sg or not?

If the UDC hardware says it is supported, it should be supported.
Otherwise we need to fix the UDC hardware or it saying it is allowed.

> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -796,6 +796,10 @@ The uvc function provides these attributes in its function directory:
>  	function_name       name of the interface
>  	req_int_skip_div    divisor of total requests to aid in calculating
>  			    interrupt frequency, 0 indicates all interrupt
> +	sg_supported        allow for scatter gather to be used if the UDC
> +			    hw supports it, this is default on and only
> +			    intended to be temporally turned off if a given
> +			    platform doesn't work well with scatter gather

How do you know if it "doesn't work well"?

That's vague and not good and nothing we want to support for forever,
sorry.

thanks,

greg k-h
  

Patch

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 5dfaa3f7f6a4..839a75fc28ee 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -9,6 +9,7 @@  Description:	UVC function directory
 		streaming_interval	1..16
 		function_name		string [32]
 		req_int_skip_div	unsigned int
+		sg_supported		0..1
 		===================	=============================
 
 What:		/config/usb-gadget/gadget/functions/uvc.name/control
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index f9b5a09be1f4..be72577a03e9 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -796,6 +796,10 @@  The uvc function provides these attributes in its function directory:
 	function_name       name of the interface
 	req_int_skip_div    divisor of total requests to aid in calculating
 			    interrupt frequency, 0 indicates all interrupt
+	sg_supported        allow for scatter gather to be used if the UDC
+			    hw supports it, this is default on and only
+			    intended to be temporally turned off if a given
+			    platform doesn't work well with scatter gather
 	=================== ================================================
 
 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 e40ca26b9c55..83d96e81c05f 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -875,6 +875,7 @@  static struct usb_function_instance *uvc_alloc_inst(void)
 	opts->streaming_interval = 1;
 	opts->streaming_maxpacket = 1024;
 	opts->req_int_skip_div = 4;
+	opts->sg_supported = 1;
 	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 6f73bd5638ed..5ccced629925 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -25,6 +25,7 @@  struct f_uvc_opts {
 	unsigned int					streaming_maxpacket;
 	unsigned int					streaming_maxburst;
 	unsigned int					req_int_skip_div;
+	unsigned int					sg_supported;
 
 	unsigned int					control_interface;
 	unsigned int					streaming_interface;
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 419e926ab57e..3784c0e02d01 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2351,6 +2351,7 @@  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);
+UVCG_OPTS_ATTR(sg_supported, sg_supported, 1);
 
 #undef UVCG_OPTS_ATTR
 
@@ -2401,6 +2402,7 @@  static struct configfs_attribute *uvc_attrs[] = {
 	&f_uvc_opts_attr_streaming_maxpacket,
 	&f_uvc_opts_attr_streaming_maxburst,
 	&f_uvc_opts_attr_req_int_skip_div,
+	&f_uvc_opts_attr_sg_supported,
 	&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 02559906a55a..aee405663d6e 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -21,6 +21,7 @@ 
 #include <media/videobuf2-vmalloc.h>
 
 #include "uvc.h"
+#include "u_uvc.h"
 
 /* ------------------------------------------------------------------------
  * Video buffers queue management.
@@ -141,6 +142,7 @@  int uvcg_queue_init(struct uvc_video_queue *queue, struct device *dev, enum v4l2
 {
 	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
 	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;
+	struct f_uvc_opts *opts = fi_to_f_uvc_opts(video->uvc->func.fi);
 	int ret;
 
 	queue->queue.type = type;
@@ -149,7 +151,7 @@  int uvcg_queue_init(struct uvc_video_queue *queue, struct device *dev, enum v4l2
 	queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
 	queue->queue.ops = &uvc_queue_qops;
 	queue->queue.lock = lock;
-	if (cdev->gadget->sg_supported) {
+	if (opts->sg_supported && cdev->gadget->sg_supported) {
 		queue->queue.mem_ops = &vb2_dma_sg_memops;
 		queue->use_sg = 1;
 	} else {