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

From: Michael Grzeschik
Date: Tue Oct 18 2022 - 11:28:48 EST


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@xxxxxxxxxxxx>
>
>
> Nitpick: I would call it use_sg everywhere, but either way:
>
>
> Reviewed-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
>
> Tested-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
>
> > ---
> > 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

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature