diff mbox series

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

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

Commit Message

Dan Vacura Oct. 17, 2022, 8:54 p.m. UTC 
  The scatter gather support doesn't appear to work well with some UDC hw.
Add the ability to turn on the feature depending on the controller in
use.

Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- no change, new patch in serie
V2 -> V3:
- default on, same as baseline

 Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
 Documentation/usb/gadget-testing.rst              | 2 ++
 drivers/usb/gadget/function/f_uvc.c               | 2 ++
 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, 10 insertions(+), 2 deletions(-)

Comments

Dan Scally Oct. 18, 2022, 1:27 p.m. UTC | #1 
Hi Dan

On 17/10/2022 21:54, Dan Vacura wrote:
> The scatter gather support doesn't appear to work well with some UDC hw.
> Add the ability to turn on the feature depending on the controller in
> use.
>
> Signed-off-by: Dan Vacura <w36195@motorola.com>


Nitpick: I would call it use_sg everywhere, but either way:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
> V1 -> V2:
> - no change, new patch in serie
> V2 -> V3:
> - default on, same as baseline
>
>   Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>   Documentation/usb/gadget-testing.rst              | 2 ++
>   drivers/usb/gadget/function/f_uvc.c               | 2 ++
>   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, 10 insertions(+), 2 deletions(-)
>
> 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..8e3072d6a590 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -796,6 +796,8 @@ 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
>   	=================== ================================================
>   
>   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..d08ebe3ffeb2 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -656,6 +656,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>   			    (opts->streaming_maxburst + 1));
>   
>   	uvc->config_skip_int_div = opts->req_int_skip_div;
> +	uvc->video.queue.use_sg = opts->sg_supported;
>   
>   	/* Allocate endpoints. */
>   	ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
> @@ -875,6 +876,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..3c7aa5c4bba2 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -149,11 +149,11 @@ 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 (queue->use_sg && cdev->gadget->sg_supported) {
>   		queue->queue.mem_ops = &vb2_dma_sg_memops;
> -		queue->use_sg = 1;
>   	} else {
>   		queue->queue.mem_ops = &vb2_vmalloc_memops;
> +		queue->use_sg = false;
>   	}
>   
>   	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY
Michael Grzeschik Oct. 18, 2022, 2:04 p.m. UTC | #2 
Hi Dan!
Hi Dan!

On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
>Hi Dan
>
>On 17/10/2022 21:54, Dan Vacura wrote:
>>The scatter gather support doesn't appear to work well with some UDC hw.
>>Add the ability to turn on the feature depending on the controller in
>>use.
>>
>>Signed-off-by: Dan Vacura <w36195@motorola.com>
>
>
>Nitpick: I would call it use_sg everywhere, but either way:

Or even only "scatter_gather". How does that sound?

>
>Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>
>Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
>
>>---
>>V1 -> V2:
>>- no change, new patch in serie
>>V2 -> V3:
>>- default on, same as baseline
>>
>>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>>  Documentation/usb/gadget-testing.rst              | 2 ++
>>  drivers/usb/gadget/function/f_uvc.c               | 2 ++
>>  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, 10 insertions(+), 2 deletions(-)
>>
>>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..8e3072d6a590 100644
>>--- a/Documentation/usb/gadget-testing.rst
>>+++ b/Documentation/usb/gadget-testing.rst
>>@@ -796,6 +796,8 @@ 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
>>  	=================== ================================================
>>  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..d08ebe3ffeb2 100644
>>--- a/drivers/usb/gadget/function/f_uvc.c
>>+++ b/drivers/usb/gadget/function/f_uvc.c
>>@@ -656,6 +656,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>>  			    (opts->streaming_maxburst + 1));
>>  	uvc->config_skip_int_div = opts->req_int_skip_div;
>>+	uvc->video.queue.use_sg = opts->sg_supported;

Why do you set this here?

>>  	/* Allocate endpoints. */
>>  	ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
>>@@ -875,6 +876,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..3c7aa5c4bba2 100644
>>--- a/drivers/usb/gadget/function/uvc_queue.c
>>+++ b/drivers/usb/gadget/function/uvc_queue.c
>>@@ -149,11 +149,11 @@ 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 (queue->use_sg && cdev->gadget->sg_supported) {
>>  		queue->queue.mem_ops = &vb2_dma_sg_memops;
>>-		queue->use_sg = 1;
>>  	} else {
>>  		queue->queue.mem_ops = &vb2_vmalloc_memops;
>>+		queue->use_sg = false;

I am unsure, but can you actually not always use vb2_dma_sg_memops.

With my last patch we always set buf->mem to vb2_plane_vaddr(vb, 0);

https://lore.kernel.org/linux-usb/20221017221141.3134818-1-m.grzeschik@pengutronix.de/T/#u


The condition to decide if encode_isoc_sg or encode_isoc should then
remain the last place to switch between sg or not. I would hook the
userspace decision in here.

You can also directly get to opts->scatter_gather by using

	struct f_uvc_opts *opts = fi_to_f_uvc_opts(uvc->func.fi);

in the function uvcg_video_enable.

>>  	}
>>  	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY
>

Thanks,
Michael
Dan Scally Oct. 18, 2022, 2:09 p.m. UTC | #3 
Hi Michael

On 18/10/2022 15:04, Michael Grzeschik wrote:
> Hi Dan!
> Hi Dan!
>
> On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
>> Hi Dan
>>
>> On 17/10/2022 21:54, Dan Vacura wrote:
>>> The scatter gather support doesn't appear to work well with some UDC 
>>> hw.
>>> Add the ability to turn on the feature depending on the controller in
>>> use.
>>>
>>> Signed-off-by: Dan Vacura <w36195@motorola.com>
>>
>>
>> Nitpick: I would call it use_sg everywhere, but either way:
>
> Or even only "scatter_gather". How does that sound?
>
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>>> ---
>>> V1 -> V2:
>>> - no change, new patch in serie
>>> V2 -> V3:
>>> - default on, same as baseline
>>>
>>>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>>>  Documentation/usb/gadget-testing.rst              | 2 ++
>>>  drivers/usb/gadget/function/f_uvc.c               | 2 ++
>>>  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, 10 insertions(+), 2 deletions(-)
>>>
>>> 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..8e3072d6a590 100644
>>> --- a/Documentation/usb/gadget-testing.rst
>>> +++ b/Documentation/usb/gadget-testing.rst
>>> @@ -796,6 +796,8 @@ 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
>>>      =================== 
>>> ================================================
>>>  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..d08ebe3ffeb2 100644
>>> --- a/drivers/usb/gadget/function/f_uvc.c
>>> +++ b/drivers/usb/gadget/function/f_uvc.c
>>> @@ -656,6 +656,7 @@ uvc_function_bind(struct usb_configuration *c, 
>>> struct usb_function *f)
>>>                  (opts->streaming_maxburst + 1));
>>>      uvc->config_skip_int_div = opts->req_int_skip_div;
>>> +    uvc->video.queue.use_sg = opts->sg_supported;
>
> Why do you set this here?
>
>>>      /* Allocate endpoints. */
>>>      ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
>>> @@ -875,6 +876,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..3c7aa5c4bba2 100644
>>> --- a/drivers/usb/gadget/function/uvc_queue.c
>>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>>> @@ -149,11 +149,11 @@ 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 (queue->use_sg && cdev->gadget->sg_supported) {
>>>          queue->queue.mem_ops = &vb2_dma_sg_memops;
>>> -        queue->use_sg = 1;
>>>      } else {
>>>          queue->queue.mem_ops = &vb2_vmalloc_memops;
>>> +        queue->use_sg = false;
>
> I am unsure, but can you actually not always use vb2_dma_sg_memops.


We have problems with this on the imx8mp EVK, because it doesn't account 
for dma-range properties in device tree and so will attempt to allocate 
memory outside the 4GB range that a DWC3 can access. The allocation 
attempts to fall back on the swiotlb, but that's slow even if it 
works...and it can fail and return -ENOMEM if the swiotlb is too small.


Probably some future work needs to fix the vb2_dma_sg_memops to take 
those dma constraints into account, but for now I wouldn't force its use.

>
> With my last patch we always set buf->mem to vb2_plane_vaddr(vb, 0);
>
> https://lore.kernel.org/linux-usb/20221017221141.3134818-1-m.grzeschik@pengutronix.de/T/#u 
>
>
>
> The condition to decide if encode_isoc_sg or encode_isoc should then
> remain the last place to switch between sg or not. I would hook the
> userspace decision in here.
>
> You can also directly get to opts->scatter_gather by using
>
>     struct f_uvc_opts *opts = fi_to_f_uvc_opts(uvc->func.fi);
>
> in the function uvcg_video_enable.
>
>>>      }
>>>      queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY
>>
>
> Thanks,
> Michael
>
Dan Scally Oct. 18, 2022, 2:10 p.m. UTC | #4 
Hi Michael - again!

On 18/10/2022 15:04, Michael Grzeschik wrote:
> Hi Dan!
> Hi Dan!
>
> On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
>> Hi Dan
>>
>> On 17/10/2022 21:54, Dan Vacura wrote:
>>> The scatter gather support doesn't appear to work well with some UDC 
>>> hw.
>>> Add the ability to turn on the feature depending on the controller in
>>> use.
>>>
>>> Signed-off-by: Dan Vacura <w36195@motorola.com>
>>
>>
>> Nitpick: I would call it use_sg everywhere, but either way:
>
> Or even only "scatter_gather". How does that sound?


I think I prefer use_sg actually, but I don't have a strong feeling 
either way.

>
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>>> ---
>>> V1 -> V2:
>>> - no change, new patch in serie
>>> V2 -> V3:
>>> - default on, same as baseline
>>>
>>>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>>>  Documentation/usb/gadget-testing.rst              | 2 ++
>>>  drivers/usb/gadget/function/f_uvc.c               | 2 ++
>>>  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, 10 insertions(+), 2 deletions(-)
>>>
>>> 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..8e3072d6a590 100644
>>> --- a/Documentation/usb/gadget-testing.rst
>>> +++ b/Documentation/usb/gadget-testing.rst
>>> @@ -796,6 +796,8 @@ 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
>>>      =================== 
>>> ================================================
>>>  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..d08ebe3ffeb2 100644
>>> --- a/drivers/usb/gadget/function/f_uvc.c
>>> +++ b/drivers/usb/gadget/function/f_uvc.c
>>> @@ -656,6 +656,7 @@ uvc_function_bind(struct usb_configuration *c, 
>>> struct usb_function *f)
>>>                  (opts->streaming_maxburst + 1));
>>>      uvc->config_skip_int_div = opts->req_int_skip_div;
>>> +    uvc->video.queue.use_sg = opts->sg_supported;
>
> Why do you set this here?
>
>>>      /* Allocate endpoints. */
>>>      ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
>>> @@ -875,6 +876,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..3c7aa5c4bba2 100644
>>> --- a/drivers/usb/gadget/function/uvc_queue.c
>>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>>> @@ -149,11 +149,11 @@ 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 (queue->use_sg && cdev->gadget->sg_supported) {
>>>          queue->queue.mem_ops = &vb2_dma_sg_memops;
>>> -        queue->use_sg = 1;
>>>      } else {
>>>          queue->queue.mem_ops = &vb2_vmalloc_memops;
>>> +        queue->use_sg = false;
>
> I am unsure, but can you actually not always use vb2_dma_sg_memops.
>
> With my last patch we always set buf->mem to vb2_plane_vaddr(vb, 0);
>
> https://lore.kernel.org/linux-usb/20221017221141.3134818-1-m.grzeschik@pengutronix.de/T/#u 
>
>
>
> The condition to decide if encode_isoc_sg or encode_isoc should then
> remain the last place to switch between sg or not. I would hook the
> userspace decision in here.
>
> You can also directly get to opts->scatter_gather by using
>
>     struct f_uvc_opts *opts = fi_to_f_uvc_opts(uvc->func.fi);
>
> in the function uvcg_video_enable.
>
>>>      }
>>>      queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY
>>
>
> Thanks,
> Michael
>
Alan Stern Oct. 18, 2022, 2:32 p.m. UTC | #5 
On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
> Hi Dan
> 
> On 17/10/2022 21:54, Dan Vacura wrote:
> > The scatter gather support doesn't appear to work well with some UDC hw.
> > Add the ability to turn on the feature depending on the controller in
> > use.
> > 
> > Signed-off-by: Dan Vacura <w36195@motorola.com>
> 
> 
> Nitpick: I would call it use_sg everywhere, but either way:
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> > ---
> > V1 -> V2:
> > - no change, new patch in serie
> > V2 -> V3:
> > - default on, same as baseline
> > 
> >   Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
> >   Documentation/usb/gadget-testing.rst              | 2 ++
> >   drivers/usb/gadget/function/f_uvc.c               | 2 ++
> >   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, 10 insertions(+), 2 deletions(-)
> > 
> > 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..8e3072d6a590 100644
> > --- a/Documentation/usb/gadget-testing.rst
> > +++ b/Documentation/usb/gadget-testing.rst
> > @@ -796,6 +796,8 @@ 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

Why is a configuration option needed for this?  Why not always use SG 
when the UDC supports it?  Or at least, make the decision automatically 
(say, based on the amount of data to be transferred) with no need for 
any user input?

Is this because the SG support in some UDC drivers is buggy?  In that 
case the proper approach is to fix the UDC drivers, not add new options 
that users won't know when to use.

Or is it because the UDC hardware itself is buggy?  In that case the 
best approach is to fix the UDC drivers so that they don't advertise 
working SG support when the hardware is unable to handle it.

Alan Stern
Dan Vacura Oct. 18, 2022, 3 p.m. UTC | #6 
Hi Dan and Michael,

On Tue, Oct 18, 2022 at 03:10:38PM +0100, Dan Scally wrote:
> Hi Michael - again!
> 
> On 18/10/2022 15:04, Michael Grzeschik wrote:
> > Hi Dan!
> > Hi Dan!
> > 
> > On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
> > > Hi Dan
> > > 
> > > On 17/10/2022 21:54, Dan Vacura wrote:
> > > > The scatter gather support doesn't appear to work well with some
> > > > UDC hw.
> > > > Add the ability to turn on the feature depending on the controller in
> > > > use.
> > > > 
> > > > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > > 
> > > 
> > > Nitpick: I would call it use_sg everywhere, but either way:
> > 
> > Or even only "scatter_gather". How does that sound?
> 
> 
> I think I prefer use_sg actually, but I don't have a strong feeling either
> way.

I went with sg_supported since use_sg and scatter_gather may imply that
the feature will be guaranteed to be used if set to 1. I thought that
sg_supported conveyed that the driver supports sg ability and expressed
that it'll be used if the UDC driver supports it. Also, that name is
used in struct usb_gadget, with the similar wording: "true if we can
handle scatter-gather".

> 
> > 
> > > 
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > 
> > > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > 
> > > > ---
> > > > V1 -> V2:
> > > > - no change, new patch in serie
> > > > V2 -> V3:
> > > > - default on, same as baseline
> > > > 
> > > >  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
> > > >  Documentation/usb/gadget-testing.rst              | 2 ++
> > > >  drivers/usb/gadget/function/f_uvc.c               | 2 ++
> > > >  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, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > 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..8e3072d6a590 100644
> > > > --- a/Documentation/usb/gadget-testing.rst
> > > > +++ b/Documentation/usb/gadget-testing.rst
> > > > @@ -796,6 +796,8 @@ 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
> > > >      ===================
> > > > ================================================
> > > >  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..d08ebe3ffeb2 100644
> > > > --- a/drivers/usb/gadget/function/f_uvc.c
> > > > +++ b/drivers/usb/gadget/function/f_uvc.c
> > > > @@ -656,6 +656,7 @@ uvc_function_bind(struct usb_configuration
> > > > *c, struct usb_function *f)
> > > >                  (opts->streaming_maxburst + 1));
> > > >      uvc->config_skip_int_div = opts->req_int_skip_div;
> > > > +    uvc->video.queue.use_sg = opts->sg_supported;
> > 
> > Why do you set this here?

This is set here to enable or disable the support. I wasn't aware of
direct access to opts in uvcg_queue_init(). I'll update to use it
directly as that'll be more clear.

> > 
> > > >      /* Allocate endpoints. */
> > > >      ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
> > > > @@ -875,6 +876,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..3c7aa5c4bba2 100644
> > > > --- a/drivers/usb/gadget/function/uvc_queue.c
> > > > +++ b/drivers/usb/gadget/function/uvc_queue.c
> > > > @@ -149,11 +149,11 @@ 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 (queue->use_sg && cdev->gadget->sg_supported) {
> > > >          queue->queue.mem_ops = &vb2_dma_sg_memops;
> > > > -        queue->use_sg = 1;
> > > >      } else {
> > > >          queue->queue.mem_ops = &vb2_vmalloc_memops;
> > > > +        queue->use_sg = false;
> > 
> > I am unsure, but can you actually not always use vb2_dma_sg_memops.
> > 
> > With my last patch we always set buf->mem to vb2_plane_vaddr(vb, 0);
> > 
> > https://lore.kernel.org/linux-usb/20221017221141.3134818-1-m.grzeschik@pengutronix.de/T/#u
> > 
> > 
> > 
> > The condition to decide if encode_isoc_sg or encode_isoc should then
> > remain the last place to switch between sg or not. I would hook the
> > userspace decision in here.
> > 
> > You can also directly get to opts->scatter_gather by using
> > 
> >     struct f_uvc_opts *opts = fi_to_f_uvc_opts(uvc->func.fi);
> > 
> > in the function uvcg_video_enable.
> > 
> > > >      }
> > > >      queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY
> > > 
> > 
> > Thanks,
> > Michael
> >
Dan Vacura Oct. 18, 2022, 3:14 p.m. UTC | #7 
Hi Alan,

On Tue, Oct 18, 2022 at 10:32:33AM -0400, Alan Stern wrote:
> On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
> > Hi Dan
> > 
> > On 17/10/2022 21:54, Dan Vacura wrote:
> > > The scatter gather support doesn't appear to work well with some UDC hw.
> > > Add the ability to turn on the feature depending on the controller in
> > > use.
> > > 
> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > 
> > 
> > Nitpick: I would call it use_sg everywhere, but either way:
> > 
> > 
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > > ---
> > > V1 -> V2:
> > > - no change, new patch in serie
> > > V2 -> V3:
> > > - default on, same as baseline
> > > 
> > >   Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
> > >   Documentation/usb/gadget-testing.rst              | 2 ++
> > >   drivers/usb/gadget/function/f_uvc.c               | 2 ++
> > >   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, 10 insertions(+), 2 deletions(-)
> > > 
> > > 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..8e3072d6a590 100644
> > > --- a/Documentation/usb/gadget-testing.rst
> > > +++ b/Documentation/usb/gadget-testing.rst
> > > @@ -796,6 +796,8 @@ 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
> 
> Why is a configuration option needed for this?  Why not always use SG 
> when the UDC supports it?  Or at least, make the decision automatically 
> (say, based on the amount of data to be transferred) with no need for 
> any user input?

Patches for a fix and to select to use SG depending on amount of data
are already submitted and under review. I agree, ideally we don't need
this patch, but there have been several regressions uncovered with
enabling this support and it takes time to root cause these issues.

In my specific environment, Android GKI 2.0, changes need to get
upstreamed first here before they're pulled into Android device
software. Having this logic in place gives us the ability to turn off
this functionality without going through this process. A revert was also
considered until all the bugs are resolved, but the code is quite
entrenched now to take out, plus others seem to benefit from it being
enabled. Thus the configurability.

> 
> Is this because the SG support in some UDC drivers is buggy?  In that 
> case the proper approach is to fix the UDC drivers, not add new options 
> that users won't know when to use.
> 
> Or is it because the UDC hardware itself is buggy?  In that case the 
> best approach is to fix the UDC drivers so that they don't advertise 
> working SG support when the hardware is unable to handle it.
> 
> Alan Stern
Alan Stern Oct. 18, 2022, 3:23 p.m. UTC | #8 
On Tue, Oct 18, 2022 at 10:14:54AM -0500, Dan Vacura wrote:
> Hi Alan,
> 
> On Tue, Oct 18, 2022 at 10:32:33AM -0400, Alan Stern wrote:
> > On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
> > > Hi Dan

> > > > --- a/Documentation/usb/gadget-testing.rst
> > > > +++ b/Documentation/usb/gadget-testing.rst
> > > > @@ -796,6 +796,8 @@ 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
> > 
> > Why is a configuration option needed for this?  Why not always use SG 
> > when the UDC supports it?  Or at least, make the decision automatically 
> > (say, based on the amount of data to be transferred) with no need for 
> > any user input?
> 
> Patches for a fix and to select to use SG depending on amount of data
> are already submitted and under review. I agree, ideally we don't need
> this patch, but there have been several regressions uncovered with
> enabling this support and it takes time to root cause these issues.

Please put this information into the patch description, and maybe also 
into the documentation file.  For your readers' and reviewers' sake it's 
important -- probably _more_ important -- to explain why you're making a 
change than what that change is.

Alan Stern

> In my specific environment, Android GKI 2.0, changes need to get
> upstreamed first here before they're pulled into Android device
> software. Having this logic in place gives us the ability to turn off
> this functionality without going through this process. A revert was also
> considered until all the bugs are resolved, but the code is quite
> entrenched now to take out, plus others seem to benefit from it being
> enabled. Thus the configurability.
Michael Grzeschik Oct. 18, 2022, 3:28 p.m. UTC | #9 
Hi Dan!

On Tue, Oct 18, 2022 at 10:14:54AM -0500, Dan Vacura wrote:
>On Tue, Oct 18, 2022 at 10:32:33AM -0400, Alan Stern wrote:
>> On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
>> > On 17/10/2022 21:54, Dan Vacura wrote:
>> > > The scatter gather support doesn't appear to work well with some UDC hw.
>> > > Add the ability to turn on the feature depending on the controller in
>> > > use.
>> > >
>> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
>> >
>> >
>> > Nitpick: I would call it use_sg everywhere, but either way:
>> >
>> >
>> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>> >
>> > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
>> >
>> > > ---
>> > > V1 -> V2:
>> > > - no change, new patch in serie
>> > > V2 -> V3:
>> > > - default on, same as baseline
>> > >
>> > >   Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>> > >   Documentation/usb/gadget-testing.rst              | 2 ++
>> > >   drivers/usb/gadget/function/f_uvc.c               | 2 ++
>> > >   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, 10 insertions(+), 2 deletions(-)
>> > >
>> > > 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..8e3072d6a590 100644
>> > > --- a/Documentation/usb/gadget-testing.rst
>> > > +++ b/Documentation/usb/gadget-testing.rst
>> > > @@ -796,6 +796,8 @@ 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
>>
>> Why is a configuration option needed for this?  Why not always use SG
>> when the UDC supports it?  Or at least, make the decision automatically
>> (say, based on the amount of data to be transferred) with no need for
>> any user input?
>
>Patches for a fix and to select to use SG depending on amount of data
>are already submitted and under review. I agree, ideally we don't need
>this patch, but there have been several regressions uncovered with
>enabling this support and it takes time to root cause these issues.
>

>In my specific environment, Android GKI 2.0, changes need to get
>upstreamed first here before they're pulled into Android device
>software.

In fact this is actually a good policy, but adding workarounds mainline
to "hopefully" fix the real problems later are probably not what this
policy is about. Hopefully!

Michael
diff mbox series

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..8e3072d6a590 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -796,6 +796,8 @@  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
 	=================== ================================================
 
 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..d08ebe3ffeb2 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -656,6 +656,7 @@  uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 			    (opts->streaming_maxburst + 1));
 
 	uvc->config_skip_int_div = opts->req_int_skip_div;
+	uvc->video.queue.use_sg = opts->sg_supported;
 
 	/* Allocate endpoints. */
 	ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
@@ -875,6 +876,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..3c7aa5c4bba2 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -149,11 +149,11 @@  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 (queue->use_sg && cdev->gadget->sg_supported) {
 		queue->queue.mem_ops = &vb2_dma_sg_memops;
-		queue->use_sg = 1;
 	} else {
 		queue->queue.mem_ops = &vb2_vmalloc_memops;
+		queue->use_sg = false;
 	}
 
 	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY