2022-04-04 04:51:56

by Dan Vacura

[permalink] [raw]
Subject: [PATCH v2] usb: gadget: uvc: allow changing interface name via configfs

Add a configfs entry, "function_name", to change the iInterface field
for VideoControl. This name is used on host devices for user selection,
useful when multiple cameras are present. The default will remain "UVC
Camera".

Signed-off-by: Dan Vacura <[email protected]>
---
Changes in v2:
- remove stable cc

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

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 889ed45be4ca..611b23e6488d 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -7,6 +7,7 @@ Description: UVC function directory
streaming_maxburst 0..15 (ss only)
streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
streaming_interval 1..16
+ function_name string [32]
=================== =============================

What: /config/usb-gadget/gadget/functions/uvc.name/control
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index c6d034abce3a..1c37159fa171 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -787,6 +787,7 @@ The uvc function provides these attributes in its function directory:
streaming_maxpacket maximum packet size this endpoint is capable of
sending or receiving when this configuration is
selected
+ function_name name of the interface
=================== ================================================

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 71bb5e477dba..50e6e7a58b41 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -44,7 +44,7 @@ MODULE_PARM_DESC(trace, "Trace level bitmask");
#define UVC_STRING_STREAMING_IDX 1

static struct usb_string uvc_en_us_strings[] = {
- [UVC_STRING_CONTROL_IDX].s = "UVC Camera",
+ /* [UVC_STRING_CONTROL_IDX].s = DYNAMIC, */
[UVC_STRING_STREAMING_IDX].s = "Video Streaming",
{ }
};
@@ -676,6 +676,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;

+ uvc_en_us_strings[UVC_STRING_CONTROL_IDX].s = opts->function_name;
us = usb_gstrings_attach(cdev, uvc_function_strings,
ARRAY_SIZE(uvc_en_us_strings));
if (IS_ERR(us)) {
@@ -866,6 +867,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)

opts->streaming_interval = 1;
opts->streaming_maxpacket = 1024;
+ snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");

ret = uvcg_attach_configfs(opts);
if (ret < 0) {
diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
index 9a01a7d4f17f..24b8681b0d6f 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -27,6 +27,7 @@ struct f_uvc_opts {

unsigned int control_interface;
unsigned int streaming_interface;
+ char function_name[32];

/*
* Control descriptors array pointers for full-/high-speed and
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 77d64031aa9c..63b8d3758b38 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2425,10 +2425,51 @@ UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, 15);

#undef UVCG_OPTS_ATTR

+#define UVCG_OPTS_STRING_ATTR(cname, aname) \
+static ssize_t f_uvc_opts_string_##cname##_show(struct config_item *item,\
+ char *page) \
+{ \
+ struct f_uvc_opts *opts = to_f_uvc_opts(item); \
+ int result; \
+ \
+ mutex_lock(&opts->lock); \
+ result = snprintf(page, sizeof(opts->aname), "%s", opts->aname);\
+ mutex_unlock(&opts->lock); \
+ \
+ return result; \
+} \
+ \
+static ssize_t f_uvc_opts_string_##cname##_store(struct config_item *item,\
+ const char *page, size_t len) \
+{ \
+ struct f_uvc_opts *opts = to_f_uvc_opts(item); \
+ int ret = 0; \
+ \
+ mutex_lock(&opts->lock); \
+ if (opts->refcnt) { \
+ ret = -EBUSY; \
+ goto end; \
+ } \
+ \
+ ret = snprintf(opts->aname, min(sizeof(opts->aname), len), \
+ "%s", page); \
+ \
+end: \
+ mutex_unlock(&opts->lock); \
+ return ret; \
+} \
+ \
+UVC_ATTR(f_uvc_opts_string_, cname, aname)
+
+UVCG_OPTS_STRING_ATTR(function_name, function_name);
+
+#undef UVCG_OPTS_STRING_ATTR
+
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_string_attr_function_name,
NULL,
};

--
2.32.0


2022-04-05 00:06:26

by Bagas Sanjaya

[permalink] [raw]
Subject: Re: [PATCH v2] usb: gadget: uvc: allow changing interface name via configfs

On 01/04/22 23.04, Dan Vacura wrote:
> Add a configfs entry, "function_name", to change the iInterface field
> for VideoControl. This name is used on host devices for user selection,
> useful when multiple cameras are present. The default will remain "UVC
> Camera".
>
> Signed-off-by: Dan Vacura <[email protected]>
> ---
> Changes in v2:
> - remove stable cc
>
> .../ABI/testing/configfs-usb-gadget-uvc | 1 +
> Documentation/usb/gadget-testing.rst | 1 +
> drivers/usb/gadget/function/f_uvc.c | 4 +-
> drivers/usb/gadget/function/u_uvc.h | 1 +
> drivers/usb/gadget/function/uvc_configfs.c | 41 +++++++++++++++++++
> 5 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 889ed45be4ca..611b23e6488d 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -7,6 +7,7 @@ Description: UVC function directory
> streaming_maxburst 0..15 (ss only)
> streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
> streaming_interval 1..16
> + function_name string [32]
> =================== =============================

Since you mention that default function_name is "UVC Camera", why don't you
mention it in the documentation?

--
An old man doll... just what I always wanted! - Clara

2022-04-05 00:25:36

by Dan Vacura

[permalink] [raw]
Subject: Re: [PATCH v2] usb: gadget: uvc: allow changing interface name via configfs

On Sun, Apr 03, 2022 at 01:13:02PM +0700, Bagas Sanjaya wrote:
> On 01/04/22 23.04, Dan Vacura wrote:
> > Add a configfs entry, "function_name", to change the iInterface field
> > for VideoControl. This name is used on host devices for user selection,
> > useful when multiple cameras are present. The default will remain "UVC
> > Camera".
> >
> > Signed-off-by: Dan Vacura <[email protected]>
> > ---
> > Changes in v2:
> > - remove stable cc
> >
> > .../ABI/testing/configfs-usb-gadget-uvc | 1 +
> > Documentation/usb/gadget-testing.rst | 1 +
> > drivers/usb/gadget/function/f_uvc.c | 4 +-
> > drivers/usb/gadget/function/u_uvc.h | 1 +
> > drivers/usb/gadget/function/uvc_configfs.c | 41 +++++++++++++++++++
> > 5 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > index 889ed45be4ca..611b23e6488d 100644
> > --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > @@ -7,6 +7,7 @@ Description: UVC function directory
> > streaming_maxburst 0..15 (ss only)
> > streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
> > streaming_interval 1..16
> > + function_name string [32]
> > =================== =============================
>
> Since you mention that default function_name is "UVC Camera", why don't you
> mention it in the documentation?

Good question. The rest of the file didn't contain any default values
and I wasn't sure what the process is for keeping things like this in
sync between source and documentation. Looking through most of the other
usb function files I don't see any mention of the hardcoded default
values either, so maybe it's intentional to not include.

>
> --
> An old man doll... just what I always wanted! - Clara

2022-04-05 02:30:25

by Dan Vacura

[permalink] [raw]
Subject: Re: [PATCH v2] usb: gadget: uvc: allow changing interface name via configfs

On Sat, Apr 02, 2022 at 12:24:23PM +0100, John Keeping wrote:
> On Fri, Apr 01, 2022 at 11:04:45AM -0500, Dan Vacura wrote:
> > Add a configfs entry, "function_name", to change the iInterface field
> > for VideoControl. This name is used on host devices for user selection,
> > useful when multiple cameras are present. The default will remain "UVC
> > Camera".
>
> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > index 71bb5e477dba..50e6e7a58b41 100644
> > --- a/drivers/usb/gadget/function/f_uvc.c
> > +++ b/drivers/usb/gadget/function/f_uvc.c
> > @@ -44,7 +44,7 @@ MODULE_PARM_DESC(trace, "Trace level bitmask");
> > #define UVC_STRING_STREAMING_IDX 1
> >
> > static struct usb_string uvc_en_us_strings[] = {
> > - [UVC_STRING_CONTROL_IDX].s = "UVC Camera",
> > + /* [UVC_STRING_CONTROL_IDX].s = DYNAMIC, */
> > [UVC_STRING_STREAMING_IDX].s = "Video Streaming",
> > { }
> > };
> > @@ -676,6 +676,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> > uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> > uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> >
> > + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].s = opts->function_name;
> > us = usb_gstrings_attach(cdev, uvc_function_strings,
> > ARRAY_SIZE(uvc_en_us_strings));
> > if (IS_ERR(us)) {
> > @@ -866,6 +867,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> >
> > opts->streaming_interval = 1;
> > opts->streaming_maxpacket = 1024;
> > + snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");
>
> This only allows a single language to be specified. I know that's what
> the existing string uses, but for other strings which can be set by
> userspace multiple languages are supported.
>
> Should we be making USB_CONFIG_STRINGS_LANG more generic so that it can
> be used by functions as well as the core configfs code?

Agree that adding support for more than one language would be ideal.
Looking through the gadget functions, most seem to be hardcoded to en-us
locale and don't provide a way to change the exposed names. Recently
this was just accepted, which I modeled my change after:
https://lore.kernel.org/all/[email protected]/
so at least making USB_CONFIG_STRINGS_LANG more generic would benefit
the uac and uvc gadgets.

2022-04-05 03:11:44

by John Keeping

[permalink] [raw]
Subject: Re: [PATCH v2] usb: gadget: uvc: allow changing interface name via configfs

On Fri, Apr 01, 2022 at 11:04:45AM -0500, Dan Vacura wrote:
> Add a configfs entry, "function_name", to change the iInterface field
> for VideoControl. This name is used on host devices for user selection,
> useful when multiple cameras are present. The default will remain "UVC
> Camera".

> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 71bb5e477dba..50e6e7a58b41 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -44,7 +44,7 @@ MODULE_PARM_DESC(trace, "Trace level bitmask");
> #define UVC_STRING_STREAMING_IDX 1
>
> static struct usb_string uvc_en_us_strings[] = {
> - [UVC_STRING_CONTROL_IDX].s = "UVC Camera",
> + /* [UVC_STRING_CONTROL_IDX].s = DYNAMIC, */
> [UVC_STRING_STREAMING_IDX].s = "Video Streaming",
> { }
> };
> @@ -676,6 +676,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>
> + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].s = opts->function_name;
> us = usb_gstrings_attach(cdev, uvc_function_strings,
> ARRAY_SIZE(uvc_en_us_strings));
> if (IS_ERR(us)) {
> @@ -866,6 +867,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>
> opts->streaming_interval = 1;
> opts->streaming_maxpacket = 1024;
> + snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");

This only allows a single language to be specified. I know that's what
the existing string uses, but for other strings which can be set by
userspace multiple languages are supported.

Should we be making USB_CONFIG_STRINGS_LANG more generic so that it can
be used by functions as well as the core configfs code?