* [PATCH 0/6] UVC Gadget: Extend color matching support
@ 2022-12-13 8:37 Daniel Scally
2022-12-13 8:37 ` [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes Daniel Scally
` (6 more replies)
0 siblings, 7 replies; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
The current UVC gadget implementation hardcodes a single color matching
descriptor and transmits it a single time following all the format and frame
descriptors. This is inflexible, and additionally applies only to the _last_
format in the array of descriptors.
This series extends the support such that the default descriptor can be amended
and is transmitted once-per-format instead of once-only, it then adds the ability
to create new color matching descriptors and associate them with particular formats.
The default color matching descriptor is retained and used where the user does not
link a new color matching descriptor to the format, so the default interaction
with userspace is unchanged from the current implementation.
Daniel Scally (6):
usb: gadget: usb: Remove "default" from color matching attributes
usb: gadget: uvc: Add struct for color matching in configs
usb: gadget: uvc: Copy color matching descriptor for each frame
usb: gadget: uvc: Remove the hardcoded default color matching
usb: gadget: uvc: Make color matching attributes read/write
usb: gadget: uvc: Allow creating new color matching descriptors
.../ABI/testing/configfs-usb-gadget-uvc | 6 +-
drivers/usb/gadget/function/f_uvc.c | 9 -
drivers/usb/gadget/function/u_uvc.h | 1 -
drivers/usb/gadget/function/uvc_configfs.c | 247 +++++++++++++++---
drivers/usb/gadget/function/uvc_configfs.h | 9 +
5 files changed, 228 insertions(+), 44 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-18 23:29 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs Daniel Scally
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
Color matching attributes in the configfs for UVC are named with the
phrase "default". The implication of that is that they will only be
used _with_ the default color matching descriptor, and that will
shortly no longer be the case.
Remove the "default" from the color matching descriptor attribute
variables.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/uvc_configfs.c | 29 +++++++++++-----------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 4303a3283ba0..26d092790f12 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1783,8 +1783,8 @@ static const struct uvcg_config_group_type uvcg_mjpeg_grp_type = {
* streaming/color_matching/default
*/
-#define UVCG_DEFAULT_COLOR_MATCHING_ATTR(cname, aname, bits) \
-static ssize_t uvcg_default_color_matching_##cname##_show( \
+#define UVCG_COLOR_MATCHING_ATTR(cname, aname, bits) \
+static ssize_t uvcg_color_matching_##cname##_show( \
struct config_item *item, char *page) \
{ \
struct config_group *group = to_config_group(item); \
@@ -1808,26 +1808,25 @@ static ssize_t uvcg_default_color_matching_##cname##_show( \
return result; \
} \
\
-UVC_ATTR_RO(uvcg_default_color_matching_, cname, aname)
+UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
-UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
-UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_transfer_characteristics,
- bTransferCharacteristics, 8);
-UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
+UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
+UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
+UVCG_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
-#undef UVCG_DEFAULT_COLOR_MATCHING_ATTR
+#undef UVCG_COLOR_MATCHING_ATTR
-static struct configfs_attribute *uvcg_default_color_matching_attrs[] = {
- &uvcg_default_color_matching_attr_b_color_primaries,
- &uvcg_default_color_matching_attr_b_transfer_characteristics,
- &uvcg_default_color_matching_attr_b_matrix_coefficients,
+static struct configfs_attribute *uvcg_color_matching_attrs[] = {
+ &uvcg_color_matching_attr_b_color_primaries,
+ &uvcg_color_matching_attr_b_transfer_characteristics,
+ &uvcg_color_matching_attr_b_matrix_coefficients,
NULL,
};
-static const struct uvcg_config_group_type uvcg_default_color_matching_type = {
+static const struct uvcg_config_group_type uvcg_color_matching_type = {
.type = {
.ct_item_ops = &uvcg_config_item_ops,
- .ct_attrs = uvcg_default_color_matching_attrs,
+ .ct_attrs = uvcg_color_matching_attrs,
.ct_owner = THIS_MODULE,
},
.name = "default",
@@ -1844,7 +1843,7 @@ static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
},
.name = "color_matching",
.children = (const struct uvcg_config_group_type*[]) {
- &uvcg_default_color_matching_type,
+ &uvcg_color_matching_type,
NULL,
},
};
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
2022-12-13 8:37 ` [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-15 11:45 ` Kieran Bingham
2022-12-13 8:37 ` [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame Daniel Scally
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
Color matching descriptors are meant to be a per-format piece of data
and we need to be able to support different descriptors for different
formats. As a preliminary step towards that goal, switch the default
color matching configfs functionality to point to an instance of a
new struct uvcg_cmd (for "color matching descriptor"). Use the same
default values for its attributes as the currently hard-coded ones so
that the interface to userspace is consistent.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/uvc_configfs.c | 55 ++++++++++++++++------
drivers/usb/gadget/function/uvc_configfs.h | 8 ++++
2 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 26d092790f12..9918e7b6a023 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1788,20 +1788,19 @@ static ssize_t uvcg_color_matching_##cname##_show( \
struct config_item *item, char *page) \
{ \
struct config_group *group = to_config_group(item); \
+ struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
struct f_uvc_opts *opts; \
struct config_item *opts_item; \
struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
- struct uvc_color_matching_descriptor *cd; \
int result; \
\
mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
\
opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
opts = to_f_uvc_opts(opts_item); \
- cd = &opts->uvc_color_matching; \
\
mutex_lock(&opts->lock); \
- result = sprintf(page, "%u\n", le##bits##_to_cpu(cd->aname)); \
+ result = sprintf(page, "%u\n", le##bits##_to_cpu(cmd->desc.aname));\
mutex_unlock(&opts->lock); \
\
mutex_unlock(su_mutex); \
@@ -1823,29 +1822,57 @@ static struct configfs_attribute *uvcg_color_matching_attrs[] = {
NULL,
};
-static const struct uvcg_config_group_type uvcg_color_matching_type = {
- .type = {
- .ct_item_ops = &uvcg_config_item_ops,
- .ct_attrs = uvcg_color_matching_attrs,
- .ct_owner = THIS_MODULE,
- },
- .name = "default",
+static void uvcg_color_matching_release(struct config_item *item)
+{
+ struct uvcg_cmd *cmd;
+
+ cmd = to_uvcg_cmd(to_config_group(item));
+ kfree(cmd);
+}
+
+static struct configfs_item_operations uvcg_color_matching_item_ops = {
+ .release = uvcg_color_matching_release,
+};
+
+static const struct config_item_type uvcg_color_matching_type = {
+ .ct_item_ops = &uvcg_color_matching_item_ops,
+ .ct_attrs = uvcg_color_matching_attrs,
+ .ct_owner = THIS_MODULE,
};
/* -----------------------------------------------------------------------------
* streaming/color_matching
*/
+static int uvcg_color_matching_create_children(struct config_group *parent)
+{
+ struct uvcg_cmd *cmd;
+
+ cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd)
+ return -ENOMEM;
+
+ cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
+ cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
+ cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
+ cmd->desc.bColorPrimaries = 1;
+ cmd->desc.bTransferCharacteristics = 1;
+ cmd->desc.bMatrixCoefficients = 4;
+
+ config_group_init_type_name(&cmd->group, "default",
+ &uvcg_color_matching_type);
+ configfs_add_default_group(&cmd->group, parent);
+
+ return 0;
+}
+
static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
.type = {
.ct_item_ops = &uvcg_config_item_ops,
.ct_owner = THIS_MODULE,
},
.name = "color_matching",
- .children = (const struct uvcg_config_group_type*[]) {
- &uvcg_color_matching_type,
- NULL,
- },
+ .create_children = uvcg_color_matching_create_children,
};
/* -----------------------------------------------------------------------------
diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
index ad2ec8c4c78c..f990739838d5 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -37,6 +37,14 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
return container_of(item, struct uvcg_control_header, item);
}
+struct uvcg_cmd {
+ struct config_group group;
+ struct uvc_color_matching_descriptor desc;
+};
+
+#define to_uvcg_cmd(group_ptr) \
+container_of(group_ptr, struct uvcg_cmd, group)
+
enum uvcg_format_type {
UVCG_UNCOMPRESSED = 0,
UVCG_MJPEG,
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
2022-12-13 8:37 ` [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes Daniel Scally
2022-12-13 8:37 ` [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-18 23:28 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching Daniel Scally
` (3 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
As currently implemented the default color matching descriptor is
appended after _all_ the formats and frames that the gadget is
configured with. According to the UVC specifications however this
is supposed to be on a per-format basis (section 3.9.2.6):
"Only one instance is allowed for a given format and if present,
the Color Matching descriptor shall be placed following the Video
and Still Image Frame descriptors for that format."
Associate the default color matching descriptor with struct
uvcg_format and copy it once-per-format instead of once only.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
drivers/usb/gadget/function/uvc_configfs.h | 1 +
2 files changed, 58 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 9918e7b6a023..6f5932c9f09c 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
"mjpeg",
};
+static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
+{
+ struct config_item *color_matching, *cm_default;
+ struct uvcg_cmd *cmd;
+
+ color_matching = config_group_find_item(to_config_group(streaming),
+ "color_matching");
+ if (!color_matching)
+ return NULL;
+
+ cm_default = config_group_find_item(to_config_group(color_matching),
+ "default");
+ config_item_put(color_matching);
+ if (!cm_default)
+ return NULL;
+
+ cmd = to_uvcg_cmd(to_config_group(cm_default));
+ config_item_put(cm_default);
+
+ return cmd;
+}
+
static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
{
struct f_uvc_opts *opts;
@@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
'Y', 'U', 'Y', '2', 0x00, 0x00, 0x10, 0x00,
0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
};
+ struct config_item *streaming;
struct uvcg_uncompressed *h;
+ struct uvcg_cmd *cmd;
+
+ streaming = group->cg_item.ci_parent;
+ cmd = uvcg_format_get_default_cmd(streaming);
+ if (!cmd)
+ return ERR_PTR(-EINVAL);
h = kzalloc(sizeof(*h), GFP_KERNEL);
if (!h)
@@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
INIT_LIST_HEAD(&h->fmt.frames);
h->fmt.type = UVCG_UNCOMPRESSED;
+ h->fmt.color_matching = cmd;
config_group_init_type_name(&h->fmt.group, name,
&uvcg_uncompressed_type);
@@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
static struct config_group *uvcg_mjpeg_make(struct config_group *group,
const char *name)
{
+ struct config_item *streaming;
struct uvcg_mjpeg *h;
+ struct uvcg_cmd *cmd;
+
+ streaming = group->cg_item.ci_parent;
+ cmd = uvcg_format_get_default_cmd(streaming);
+ if (!cmd)
+ return ERR_PTR(-EINVAL);
h = kzalloc(sizeof(*h), GFP_KERNEL);
if (!h)
@@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
INIT_LIST_HEAD(&h->fmt.frames);
h->fmt.type = UVCG_MJPEG;
+ h->fmt.color_matching = cmd;
config_group_init_type_name(&h->fmt.group, name,
&uvcg_mjpeg_type);
@@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
enum uvcg_strm_type {
UVCG_HEADER = 0,
UVCG_FORMAT,
- UVCG_FRAME
+ UVCG_FRAME,
+ UVCG_CMD,
};
/*
@@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
if (ret)
return ret;
}
+
+ ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
+ if (ret)
+ return ret;
}
return ret;
@@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
*size += frm->frame.b_frame_interval_type * sz;
}
break;
+ case UVCG_CMD: {
+ struct uvcg_cmd *cmd = priv1;
+ *size += sizeof(cmd->desc);
+ }
+ break;
}
++*count;
@@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
frm->frame.b_frame_interval_type);
}
break;
+ case UVCG_CMD: {
+ struct uvcg_cmd *cmd = priv1;
+
+ memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
+ *dest += sizeof(cmd->desc);
+ }
+ break;
}
return 0;
@@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
if (ret)
goto unlock;
- count += 2; /* color_matching, NULL */
+ count += 1; /* NULL */
*class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
if (!*class_array) {
ret = -ENOMEM;
@@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
kfree(data_save);
goto unlock;
}
- *cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
++target_hdr->linked;
ret = 0;
diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
index f990739838d5..6582d6c7b6f6 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -57,6 +57,7 @@ struct uvcg_format {
struct list_head frames;
unsigned num_frames;
__u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
+ struct uvcg_cmd *color_matching;
};
struct uvcg_format_ptr {
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
` (2 preceding siblings ...)
2022-12-13 8:37 ` [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-15 11:48 ` Kieran Bingham
2022-12-18 22:52 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write Daniel Scally
` (2 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
A hardcoded default color matching descriptor is embedded in struct
f_uvc_opts but no longer has any use - remove it.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/f_uvc.c | 9 ---------
drivers/usb/gadget/function/u_uvc.h | 1 -
2 files changed, 10 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 6e196e06181e..46bdea73cdeb 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -793,7 +793,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
struct uvc_camera_terminal_descriptor *cd;
struct uvc_processing_unit_descriptor *pd;
struct uvc_output_terminal_descriptor *od;
- struct uvc_color_matching_descriptor *md;
struct uvc_descriptor_header **ctl_cls;
int ret;
@@ -842,14 +841,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
od->bSourceID = 2;
od->iTerminal = 0;
- md = &opts->uvc_color_matching;
- md->bLength = UVC_DT_COLOR_MATCHING_SIZE;
- md->bDescriptorType = USB_DT_CS_INTERFACE;
- md->bDescriptorSubType = UVC_VS_COLORFORMAT;
- md->bColorPrimaries = 1;
- md->bTransferCharacteristics = 1;
- md->bMatrixCoefficients = 4;
-
/* Prepare fs control class descriptors for configfs-based gadgets */
ctl_cls = opts->uvc_fs_control_cls;
ctl_cls[0] = NULL; /* assigned elsewhere by configfs */
diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
index 24b8681b0d6f..577c1c48ca4a 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -52,7 +52,6 @@ struct f_uvc_opts {
struct uvc_camera_terminal_descriptor uvc_camera_terminal;
struct uvc_processing_unit_descriptor uvc_processing;
struct uvc_output_terminal_descriptor uvc_output_terminal;
- struct uvc_color_matching_descriptor uvc_color_matching;
/*
* Control descriptors pointers arrays for full-/high-speed and
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
` (3 preceding siblings ...)
2022-12-13 8:37 ` [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-15 11:51 ` Kieran Bingham
2022-12-13 8:37 ` [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors Daniel Scally
2022-12-18 18:12 ` [PATCH 0/6] UVC Gadget: Extend color matching support Laurent Pinchart
6 siblings, 1 reply; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
In preparation for allowing more than the default color matching
descriptor, make the color matching attributes writeable.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
.../ABI/testing/configfs-usb-gadget-uvc | 2 +-
drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 611b23e6488d..3512f4899fe3 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -165,7 +165,7 @@ Date: Dec 2014
KernelVersion: 4.0
Description: Default color matching descriptors
- All attributes read only:
+ All attributes read/write:
======================== ======================================
bMatrixCoefficients matrix used to compute luma and
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 6f5932c9f09c..4fbc42d738a4 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
return result; \
} \
\
-UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
+static ssize_t uvcg_color_matching_##cname##_store( \
+ struct config_item *item, const char *page, size_t len) \
+{ \
+ struct config_group *group = to_config_group(item); \
+ struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
+ struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
+ struct f_uvc_opts *opts; \
+ struct config_item *opts_item; \
+ int ret; \
+ u##bits num; \
+ \
+ ret = kstrtou##bits(page, 0, &num); \
+ if (ret) \
+ return ret; \
+ \
+ mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
+ \
+ opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
+ opts = to_f_uvc_opts(opts_item); \
+ \
+ mutex_lock(&opts->lock); \
+ \
+ cmd->desc.aname = num; \
+ ret = len; \
+ \
+ mutex_unlock(&opts->lock); \
+ mutex_unlock(su_mutex); \
+ \
+ return ret; \
+} \
+UVC_ATTR(uvcg_color_matching_, cname, aname)
UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
` (4 preceding siblings ...)
2022-12-13 8:37 ` [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write Daniel Scally
@ 2022-12-13 8:37 ` Daniel Scally
2022-12-15 12:00 ` Kieran Bingham
2022-12-18 18:12 ` [PATCH 0/6] UVC Gadget: Extend color matching support Laurent Pinchart
6 siblings, 1 reply; 29+ messages in thread
From: Daniel Scally @ 2022-12-13 8:37 UTC (permalink / raw)
To: linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, kieran.bingham,
torleiv, Daniel Scally
Allow users to create new color matching descriptors in addition to
the default one. These must be associated with a UVC format in order
to be transmitted to the host, which is achieved by symlinking from
the format to the newly created color matching descriptor - extend
the uncompressed and mjpeg formats to support that linking operation.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
.../ABI/testing/configfs-usb-gadget-uvc | 4 +-
drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
2 files changed, 79 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 3512f4899fe3..ce629f0880a9 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -160,10 +160,10 @@ Date: Dec 2014
KernelVersion: 4.0
Description: Color matching descriptors
-What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
+What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
Date: Dec 2014
KernelVersion: 4.0
-Description: Default color matching descriptors
+Description: color matching descriptors
All attributes read/write:
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 4fbc42d738a4..82c10f0dab71 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
return cmd;
}
+static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
+{
+ struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
+ struct config_item *streaming, *color_matching;
+ struct uvcg_cmd *color_matching_desc;
+ struct uvcg_format *fmt;
+ int ret = 0;
+
+ mutex_lock(su_mutex);
+
+ streaming = src->ci_parent->ci_parent;
+ color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
+ if (!color_matching || color_matching != tgt->ci_parent) {
+ ret = -EINVAL;
+ goto out_put_cm;
+ }
+
+ color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
+ fmt = to_uvcg_format(src);
+ fmt->color_matching = color_matching_desc;
+
+out_put_cm:
+ config_item_put(color_matching);
+ mutex_unlock(su_mutex);
+
+ return ret;
+}
+
+static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
+{
+ struct config_item *streaming;
+ struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
+ struct uvcg_format *fmt;
+ struct uvcg_cmd *cmd;
+
+ mutex_lock(su_mutex);
+
+ streaming = src->ci_parent->ci_parent;
+ cmd = uvcg_format_get_default_cmd(streaming);
+
+ fmt = to_uvcg_format(src);
+ fmt->color_matching = cmd;
+
+ mutex_unlock(su_mutex);
+}
+
+static struct configfs_item_operations uvcg_format_item_operations = {
+ .release = uvcg_config_item_release,
+ .allow_link = uvcg_format_allow_link,
+ .drop_link = uvcg_format_drop_link,
+};
+
static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
{
struct f_uvc_opts *opts;
@@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
};
static const struct config_item_type uvcg_uncompressed_type = {
- .ct_item_ops = &uvcg_config_item_ops,
+ .ct_item_ops = &uvcg_format_item_operations,
.ct_group_ops = &uvcg_uncompressed_group_ops,
.ct_attrs = uvcg_uncompressed_attrs,
.ct_owner = THIS_MODULE,
@@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
};
static const struct config_item_type uvcg_mjpeg_type = {
- .ct_item_ops = &uvcg_config_item_ops,
+ .ct_item_ops = &uvcg_format_item_operations,
.ct_group_ops = &uvcg_mjpeg_group_ops,
.ct_attrs = uvcg_mjpeg_attrs,
.ct_owner = THIS_MODULE,
@@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
* streaming/color_matching
*/
+static struct config_group *uvcg_color_matching_make(struct config_group *group,
+ const char *name)
+{
+ struct uvcg_cmd *cmd;
+
+ cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd)
+ return ERR_PTR(-ENOMEM);
+
+ cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
+ cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
+ cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
+
+ config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
+
+ return &cmd->group;
+}
+
+static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
+ .make_group = uvcg_color_matching_make,
+};
+
static int uvcg_color_matching_create_children(struct config_group *parent)
{
struct uvcg_cmd *cmd;
@@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
.type = {
.ct_item_ops = &uvcg_config_item_ops,
+ .ct_group_ops = &uvcg_color_matching_grp_group_ops,
.ct_owner = THIS_MODULE,
},
.name = "color_matching",
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs
2022-12-13 8:37 ` [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs Daniel Scally
@ 2022-12-15 11:45 ` Kieran Bingham
2022-12-16 14:06 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2022-12-15 11:45 UTC (permalink / raw)
To: Daniel Scally, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv, Daniel Scally
Quoting Daniel Scally (2022-12-13 08:37:32)
> Color matching descriptors are meant to be a per-format piece of data
> and we need to be able to support different descriptors for different
> formats. As a preliminary step towards that goal, switch the default
> color matching configfs functionality to point to an instance of a
> new struct uvcg_cmd (for "color matching descriptor"). Use the same
Hrm .. I can't see 'cmd' and not think 'command' ... but longer names
are longer ...
> default values for its attributes as the currently hard-coded ones so
> that the interface to userspace is consistent.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 55 ++++++++++++++++------
> drivers/usb/gadget/function/uvc_configfs.h | 8 ++++
> 2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 26d092790f12..9918e7b6a023 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1788,20 +1788,19 @@ static ssize_t uvcg_color_matching_##cname##_show( \
> struct config_item *item, char *page) \
> { \
> struct config_group *group = to_config_group(item); \
> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
> struct f_uvc_opts *opts; \
> struct config_item *opts_item; \
> struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
> - struct uvc_color_matching_descriptor *cd; \
> int result; \
> \
> mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> \
> opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> opts = to_f_uvc_opts(opts_item); \
> - cd = &opts->uvc_color_matching; \
> \
> mutex_lock(&opts->lock); \
> - result = sprintf(page, "%u\n", le##bits##_to_cpu(cd->aname)); \
> + result = sprintf(page, "%u\n", le##bits##_to_cpu(cmd->desc.aname));\
> mutex_unlock(&opts->lock); \
> \
> mutex_unlock(su_mutex); \
> @@ -1823,29 +1822,57 @@ static struct configfs_attribute *uvcg_color_matching_attrs[] = {
> NULL,
> };
>
> -static const struct uvcg_config_group_type uvcg_color_matching_type = {
> - .type = {
> - .ct_item_ops = &uvcg_config_item_ops,
> - .ct_attrs = uvcg_color_matching_attrs,
> - .ct_owner = THIS_MODULE,
> - },
> - .name = "default",
> +static void uvcg_color_matching_release(struct config_item *item)
> +{
> + struct uvcg_cmd *cmd;
> +
> + cmd = to_uvcg_cmd(to_config_group(item));
> + kfree(cmd);
> +}
> +
> +static struct configfs_item_operations uvcg_color_matching_item_ops = {
> + .release = uvcg_color_matching_release,
> +};
> +
> +static const struct config_item_type uvcg_color_matching_type = {
> + .ct_item_ops = &uvcg_color_matching_item_ops,
> + .ct_attrs = uvcg_color_matching_attrs,
> + .ct_owner = THIS_MODULE,
> };
>
> /* -----------------------------------------------------------------------------
> * streaming/color_matching
> */
>
> +static int uvcg_color_matching_create_children(struct config_group *parent)
> +{
> + struct uvcg_cmd *cmd;
> +
> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> + if (!cmd)
> + return -ENOMEM;
> +
> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> + cmd->desc.bColorPrimaries = 1;
> + cmd->desc.bTransferCharacteristics = 1;
> + cmd->desc.bMatrixCoefficients = 4;
I realise these values were taken directly as existing code, but
particularly in regards to how these values will be set from userspace -
is it easy enough to have some common definitions in a preceeding patch
that state the supported values here from the spec, to avoid 'magic
values' here ...
A header with defines or an enum isn't going to be usable from a bash
script configuring configfs, but at least a compiled program could use
the definitions.
> +
> + config_group_init_type_name(&cmd->group, "default",
> + &uvcg_color_matching_type);
> + configfs_add_default_group(&cmd->group, parent);
> +
> + return 0;
> +}
> +
> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> .type = {
> .ct_item_ops = &uvcg_config_item_ops,
> .ct_owner = THIS_MODULE,
> },
> .name = "color_matching",
> - .children = (const struct uvcg_config_group_type*[]) {
> - &uvcg_color_matching_type,
> - NULL,
> - },
> + .create_children = uvcg_color_matching_create_children,
> };
>
> /* -----------------------------------------------------------------------------
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index ad2ec8c4c78c..f990739838d5 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -37,6 +37,14 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
> return container_of(item, struct uvcg_control_header, item);
> }
>
> +struct uvcg_cmd {
> + struct config_group group;
> + struct uvc_color_matching_descriptor desc;
> +};
> +
> +#define to_uvcg_cmd(group_ptr) \
> +container_of(group_ptr, struct uvcg_cmd, group)
> +
> enum uvcg_format_type {
> UVCG_UNCOMPRESSED = 0,
> UVCG_MJPEG,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching
2022-12-13 8:37 ` [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching Daniel Scally
@ 2022-12-15 11:48 ` Kieran Bingham
2022-12-16 15:32 ` Dan Scally
2022-12-18 22:52 ` Laurent Pinchart
1 sibling, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2022-12-15 11:48 UTC (permalink / raw)
To: Daniel Scally, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv, Daniel Scally
Quoting Daniel Scally (2022-12-13 08:37:34)
> A hardcoded default color matching descriptor is embedded in struct
> f_uvc_opts but no longer has any use - remove it.
Does this affect the legacy g_webcam, or is this part independent ?
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> drivers/usb/gadget/function/f_uvc.c | 9 ---------
> drivers/usb/gadget/function/u_uvc.h | 1 -
> 2 files changed, 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 6e196e06181e..46bdea73cdeb 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -793,7 +793,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> struct uvc_camera_terminal_descriptor *cd;
> struct uvc_processing_unit_descriptor *pd;
> struct uvc_output_terminal_descriptor *od;
> - struct uvc_color_matching_descriptor *md;
> struct uvc_descriptor_header **ctl_cls;
> int ret;
>
> @@ -842,14 +841,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> od->bSourceID = 2;
> od->iTerminal = 0;
>
> - md = &opts->uvc_color_matching;
> - md->bLength = UVC_DT_COLOR_MATCHING_SIZE;
> - md->bDescriptorType = USB_DT_CS_INTERFACE;
> - md->bDescriptorSubType = UVC_VS_COLORFORMAT;
> - md->bColorPrimaries = 1;
> - md->bTransferCharacteristics = 1;
> - md->bMatrixCoefficients = 4;
> -
> /* Prepare fs control class descriptors for configfs-based gadgets */
> ctl_cls = opts->uvc_fs_control_cls;
> ctl_cls[0] = NULL; /* assigned elsewhere by configfs */
> diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
> index 24b8681b0d6f..577c1c48ca4a 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -52,7 +52,6 @@ struct f_uvc_opts {
> struct uvc_camera_terminal_descriptor uvc_camera_terminal;
> struct uvc_processing_unit_descriptor uvc_processing;
> struct uvc_output_terminal_descriptor uvc_output_terminal;
> - struct uvc_color_matching_descriptor uvc_color_matching;
>
> /*
> * Control descriptors pointers arrays for full-/high-speed and
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write
2022-12-13 8:37 ` [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write Daniel Scally
@ 2022-12-15 11:51 ` Kieran Bingham
2022-12-16 15:53 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2022-12-15 11:51 UTC (permalink / raw)
To: Daniel Scally, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv, Daniel Scally
Quoting Daniel Scally (2022-12-13 08:37:35)
> In preparation for allowing more than the default color matching
> descriptor, make the color matching attributes writeable.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> .../ABI/testing/configfs-usb-gadget-uvc | 2 +-
> drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 611b23e6488d..3512f4899fe3 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -165,7 +165,7 @@ Date: Dec 2014
> KernelVersion: 4.0
> Description: Default color matching descriptors
>
> - All attributes read only:
> + All attributes read/write:
Do we need to specify here what acceptable values can now be written at
all?
>
> ======================== ======================================
> bMatrixCoefficients matrix used to compute luma and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 6f5932c9f09c..4fbc42d738a4 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
> return result; \
> } \
> \
> -UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
> +static ssize_t uvcg_color_matching_##cname##_store( \
> + struct config_item *item, const char *page, size_t len) \
> +{ \
> + struct config_group *group = to_config_group(item); \
> + struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
> + struct f_uvc_opts *opts; \
> + struct config_item *opts_item; \
> + int ret; \
> + u##bits num; \
> + \
> + ret = kstrtou##bits(page, 0, &num); \
> + if (ret) \
> + return ret; \
I don't know how horrible it would be - or if there's any other
precendence, but I'm weary that setting '1', or '4' in here from
userspace is fairly meaningless.
Of course - the user doing so would have to know from the spec perhaps
what they are configuring - but it makes me wonder if we should support
string matching in here to also convert say "BT.709" to the appropriate
integer value (if a non-integer was set).
It may depend on how 'most' other configfs entries that would be similar
to this would expect to operate.
> + \
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> + \
> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> + opts = to_f_uvc_opts(opts_item); \
> + \
> + mutex_lock(&opts->lock); \
> + \
> + cmd->desc.aname = num; \
> + ret = len; \
> + \
> + mutex_unlock(&opts->lock); \
> + mutex_unlock(su_mutex); \
> + \
> + return ret; \
> +} \
> +UVC_ATTR(uvcg_color_matching_, cname, aname)
>
> UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-13 8:37 ` [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors Daniel Scally
@ 2022-12-15 12:00 ` Kieran Bingham
2022-12-15 12:03 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2022-12-15 12:00 UTC (permalink / raw)
To: Daniel Scally, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv, Daniel Scally
Quoting Daniel Scally (2022-12-13 08:37:36)
> Allow users to create new color matching descriptors in addition to
> the default one. These must be associated with a UVC format in order
> to be transmitted to the host, which is achieved by symlinking from
> the format to the newly created color matching descriptor - extend
> the uncompressed and mjpeg formats to support that linking operation.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> .../ABI/testing/configfs-usb-gadget-uvc | 4 +-
> drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
> 2 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 3512f4899fe3..ce629f0880a9 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -160,10 +160,10 @@ Date: Dec 2014
> KernelVersion: 4.0
> Description: Color matching descriptors
>
> -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
Hrm... do we need to keep 'default' (and keep it available in patch
1/6?) so that the default is still kept accesible?
That would mean others are added as siblings to the default, and then
the one that gets linked is the one that is used perhaps?
Feels like that would overcomplicate 'the default case' maybe ... but
I'm weary that this is 'removing' the default from ABI...?
> Date: Dec 2014
> KernelVersion: 4.0
Hrm ... and this would leave the documentation stating that you could
provide custom color matching descriptors from v4.0 onwards, which would
be inaccurate ?
So I'm not sure we can just rename this documentation segment eitherway.
> -Description: Default color matching descriptors
> +Description: color matching descriptors
>
> All attributes read/write:
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 4fbc42d738a4..82c10f0dab71 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
> return cmd;
> }
>
> +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
> +{
> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> + struct config_item *streaming, *color_matching;
> + struct uvcg_cmd *color_matching_desc;
> + struct uvcg_format *fmt;
> + int ret = 0;
> +
> + mutex_lock(su_mutex);
> +
> + streaming = src->ci_parent->ci_parent;
> + color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
> + if (!color_matching || color_matching != tgt->ci_parent) {
> + ret = -EINVAL;
> + goto out_put_cm;
> + }
> +
> + color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
> + fmt = to_uvcg_format(src);
> + fmt->color_matching = color_matching_desc;
> +
> +out_put_cm:
> + config_item_put(color_matching);
> + mutex_unlock(su_mutex);
> +
> + return ret;
> +}
> +
> +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
> +{
> + struct config_item *streaming;
> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> + struct uvcg_format *fmt;
> + struct uvcg_cmd *cmd;
> +
> + mutex_lock(su_mutex);
> +
> + streaming = src->ci_parent->ci_parent;
> + cmd = uvcg_format_get_default_cmd(streaming);
> +
> + fmt = to_uvcg_format(src);
> + fmt->color_matching = cmd;
> +
> + mutex_unlock(su_mutex);
> +}
> +
> +static struct configfs_item_operations uvcg_format_item_operations = {
> + .release = uvcg_config_item_release,
> + .allow_link = uvcg_format_allow_link,
> + .drop_link = uvcg_format_drop_link,
> +};
> +
> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> {
> struct f_uvc_opts *opts;
> @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
> };
>
> static const struct config_item_type uvcg_uncompressed_type = {
> - .ct_item_ops = &uvcg_config_item_ops,
> + .ct_item_ops = &uvcg_format_item_operations,
> .ct_group_ops = &uvcg_uncompressed_group_ops,
> .ct_attrs = uvcg_uncompressed_attrs,
> .ct_owner = THIS_MODULE,
> @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
> };
>
> static const struct config_item_type uvcg_mjpeg_type = {
> - .ct_item_ops = &uvcg_config_item_ops,
> + .ct_item_ops = &uvcg_format_item_operations,
> .ct_group_ops = &uvcg_mjpeg_group_ops,
> .ct_attrs = uvcg_mjpeg_attrs,
> .ct_owner = THIS_MODULE,
> @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
> * streaming/color_matching
> */
>
> +static struct config_group *uvcg_color_matching_make(struct config_group *group,
> + const char *name)
> +{
> + struct uvcg_cmd *cmd;
> +
> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> + if (!cmd)
> + return ERR_PTR(-ENOMEM);
> +
> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> +
> + config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
> +
> + return &cmd->group;
> +}
> +
> +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
> + .make_group = uvcg_color_matching_make,
> +};
> +
> static int uvcg_color_matching_create_children(struct config_group *parent)
> {
> struct uvcg_cmd *cmd;
> @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> .type = {
> .ct_item_ops = &uvcg_config_item_ops,
> + .ct_group_ops = &uvcg_color_matching_grp_group_ops,
> .ct_owner = THIS_MODULE,
> },
> .name = "color_matching",
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-15 12:00 ` Kieran Bingham
@ 2022-12-15 12:03 ` Dan Scally
2022-12-18 23:17 ` Laurent Pinchart
0 siblings, 1 reply; 29+ messages in thread
From: Dan Scally @ 2022-12-15 12:03 UTC (permalink / raw)
To: Kieran Bingham, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv
Hi Kieran
On 15/12/2022 12:00, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:36)
>> Allow users to create new color matching descriptors in addition to
>> the default one. These must be associated with a UVC format in order
>> to be transmitted to the host, which is achieved by symlinking from
>> the format to the newly created color matching descriptor - extend
>> the uncompressed and mjpeg formats to support that linking operation.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> .../ABI/testing/configfs-usb-gadget-uvc | 4 +-
>> drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
>> 2 files changed, 79 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> index 3512f4899fe3..ce629f0880a9 100644
>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> @@ -160,10 +160,10 @@ Date: Dec 2014
>> KernelVersion: 4.0
>> Description: Color matching descriptors
>>
>> -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
>> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> Hrm... do we need to keep 'default' (and keep it available in patch
> 1/6?) so that the default is still kept accesible?
It's still there - it's just no longer necessarily the only entry
>
> That would mean others are added as siblings to the default, and then
> the one that gets linked is the one that is used perhaps?
Yep that's how it's implemented
>
> Feels like that would overcomplicate 'the default case' maybe ... but
> I'm weary that this is 'removing' the default from ABI...?
>
>> Date: Dec 2014
>> KernelVersion: 4.0
> Hrm ... and this would leave the documentation stating that you could
> provide custom color matching descriptors from v4.0 onwards, which would
> be inaccurate ?
Ah, good point...fair enough, I'll add a new entry for the custom named
ones.
>
> So I'm not sure we can just rename this documentation segment eitherway.
>
>
>> -Description: Default color matching descriptors
>> +Description: color matching descriptors
>>
>> All attributes read/write:
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 4fbc42d738a4..82c10f0dab71 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
>> return cmd;
>> }
>>
>> +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
>> +{
>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>> + struct config_item *streaming, *color_matching;
>> + struct uvcg_cmd *color_matching_desc;
>> + struct uvcg_format *fmt;
>> + int ret = 0;
>> +
>> + mutex_lock(su_mutex);
>> +
>> + streaming = src->ci_parent->ci_parent;
>> + color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
>> + if (!color_matching || color_matching != tgt->ci_parent) {
>> + ret = -EINVAL;
>> + goto out_put_cm;
>> + }
>> +
>> + color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
>> + fmt = to_uvcg_format(src);
>> + fmt->color_matching = color_matching_desc;
>> +
>> +out_put_cm:
>> + config_item_put(color_matching);
>> + mutex_unlock(su_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
>> +{
>> + struct config_item *streaming;
>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>> + struct uvcg_format *fmt;
>> + struct uvcg_cmd *cmd;
>> +
>> + mutex_lock(su_mutex);
>> +
>> + streaming = src->ci_parent->ci_parent;
>> + cmd = uvcg_format_get_default_cmd(streaming);
>> +
>> + fmt = to_uvcg_format(src);
>> + fmt->color_matching = cmd;
>> +
>> + mutex_unlock(su_mutex);
>> +}
>> +
>> +static struct configfs_item_operations uvcg_format_item_operations = {
>> + .release = uvcg_config_item_release,
>> + .allow_link = uvcg_format_allow_link,
>> + .drop_link = uvcg_format_drop_link,
>> +};
>> +
>> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>> {
>> struct f_uvc_opts *opts;
>> @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
>> };
>>
>> static const struct config_item_type uvcg_uncompressed_type = {
>> - .ct_item_ops = &uvcg_config_item_ops,
>> + .ct_item_ops = &uvcg_format_item_operations,
>> .ct_group_ops = &uvcg_uncompressed_group_ops,
>> .ct_attrs = uvcg_uncompressed_attrs,
>> .ct_owner = THIS_MODULE,
>> @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
>> };
>>
>> static const struct config_item_type uvcg_mjpeg_type = {
>> - .ct_item_ops = &uvcg_config_item_ops,
>> + .ct_item_ops = &uvcg_format_item_operations,
>> .ct_group_ops = &uvcg_mjpeg_group_ops,
>> .ct_attrs = uvcg_mjpeg_attrs,
>> .ct_owner = THIS_MODULE,
>> @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
>> * streaming/color_matching
>> */
>>
>> +static struct config_group *uvcg_color_matching_make(struct config_group *group,
>> + const char *name)
>> +{
>> + struct uvcg_cmd *cmd;
>> +
>> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> + if (!cmd)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
>> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
>> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
>> +
>> + config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
>> +
>> + return &cmd->group;
>> +}
>> +
>> +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
>> + .make_group = uvcg_color_matching_make,
>> +};
>> +
>> static int uvcg_color_matching_create_children(struct config_group *parent)
>> {
>> struct uvcg_cmd *cmd;
>> @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
>> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
>> .type = {
>> .ct_item_ops = &uvcg_config_item_ops,
>> + .ct_group_ops = &uvcg_color_matching_grp_group_ops,
>> .ct_owner = THIS_MODULE,
>> },
>> .name = "color_matching",
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs
2022-12-15 11:45 ` Kieran Bingham
@ 2022-12-16 14:06 ` Dan Scally
2022-12-18 23:28 ` Laurent Pinchart
0 siblings, 1 reply; 29+ messages in thread
From: Dan Scally @ 2022-12-16 14:06 UTC (permalink / raw)
To: Kieran Bingham, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv
Hi Kieran
On 15/12/2022 11:45, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:32)
>> Color matching descriptors are meant to be a per-format piece of data
>> and we need to be able to support different descriptors for different
>> formats. As a preliminary step towards that goal, switch the default
>> color matching configfs functionality to point to an instance of a
>> new struct uvcg_cmd (for "color matching descriptor"). Use the same
> Hrm .. I can't see 'cmd' and not think 'command' ... but longer names
> are longer ...
Yeah. Naming things was never my strong suit...I couldn't think of a
name of intermediate length that wasn't rubbish so it was either this or
"uvcg_color_matching_descriptor" which is loooong.
>
>
>> default values for its attributes as the currently hard-coded ones so
>> that the interface to userspace is consistent.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> drivers/usb/gadget/function/uvc_configfs.c | 55 ++++++++++++++++------
>> drivers/usb/gadget/function/uvc_configfs.h | 8 ++++
>> 2 files changed, 49 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 26d092790f12..9918e7b6a023 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -1788,20 +1788,19 @@ static ssize_t uvcg_color_matching_##cname##_show( \
>> struct config_item *item, char *page) \
>> { \
>> struct config_group *group = to_config_group(item); \
>> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
>> struct f_uvc_opts *opts; \
>> struct config_item *opts_item; \
>> struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
>> - struct uvc_color_matching_descriptor *cd; \
>> int result; \
>> \
>> mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
>> \
>> opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
>> opts = to_f_uvc_opts(opts_item); \
>> - cd = &opts->uvc_color_matching; \
>> \
>> mutex_lock(&opts->lock); \
>> - result = sprintf(page, "%u\n", le##bits##_to_cpu(cd->aname)); \
>> + result = sprintf(page, "%u\n", le##bits##_to_cpu(cmd->desc.aname));\
>> mutex_unlock(&opts->lock); \
>> \
>> mutex_unlock(su_mutex); \
>> @@ -1823,29 +1822,57 @@ static struct configfs_attribute *uvcg_color_matching_attrs[] = {
>> NULL,
>> };
>>
>> -static const struct uvcg_config_group_type uvcg_color_matching_type = {
>> - .type = {
>> - .ct_item_ops = &uvcg_config_item_ops,
>> - .ct_attrs = uvcg_color_matching_attrs,
>> - .ct_owner = THIS_MODULE,
>> - },
>> - .name = "default",
>> +static void uvcg_color_matching_release(struct config_item *item)
>> +{
>> + struct uvcg_cmd *cmd;
>> +
>> + cmd = to_uvcg_cmd(to_config_group(item));
>> + kfree(cmd);
>> +}
>> +
>> +static struct configfs_item_operations uvcg_color_matching_item_ops = {
>> + .release = uvcg_color_matching_release,
>> +};
>> +
>> +static const struct config_item_type uvcg_color_matching_type = {
>> + .ct_item_ops = &uvcg_color_matching_item_ops,
>> + .ct_attrs = uvcg_color_matching_attrs,
>> + .ct_owner = THIS_MODULE,
>> };
>>
>> /* -----------------------------------------------------------------------------
>> * streaming/color_matching
>> */
>>
>> +static int uvcg_color_matching_create_children(struct config_group *parent)
>> +{
>> + struct uvcg_cmd *cmd;
>> +
>> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> + if (!cmd)
>> + return -ENOMEM;
>> +
>> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
>> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
>> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
>> + cmd->desc.bColorPrimaries = 1;
>> + cmd->desc.bTransferCharacteristics = 1;
>> + cmd->desc.bMatrixCoefficients = 4;
> I realise these values were taken directly as existing code, but
> particularly in regards to how these values will be set from userspace -
> is it easy enough to have some common definitions in a preceeding patch
> that state the supported values here from the spec, to avoid 'magic
> values' here ...
>
> A header with defines or an enum isn't going to be usable from a bash
> script configuring configfs, but at least a compiled program could use
> the definitions.
Yes, I think probably it's a candidate for
include/uapi/linux/usb/video.h...unless anyone thinks it's better elsewhere
>
>
>> +
>> + config_group_init_type_name(&cmd->group, "default",
>> + &uvcg_color_matching_type);
>> + configfs_add_default_group(&cmd->group, parent);
>> +
>> + return 0;
>> +}
>> +
>> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
>> .type = {
>> .ct_item_ops = &uvcg_config_item_ops,
>> .ct_owner = THIS_MODULE,
>> },
>> .name = "color_matching",
>> - .children = (const struct uvcg_config_group_type*[]) {
>> - &uvcg_color_matching_type,
>> - NULL,
>> - },
>> + .create_children = uvcg_color_matching_create_children,
>> };
>>
>> /* -----------------------------------------------------------------------------
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
>> index ad2ec8c4c78c..f990739838d5 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.h
>> +++ b/drivers/usb/gadget/function/uvc_configfs.h
>> @@ -37,6 +37,14 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
>> return container_of(item, struct uvcg_control_header, item);
>> }
>>
>> +struct uvcg_cmd {
>> + struct config_group group;
>> + struct uvc_color_matching_descriptor desc;
>> +};
>> +
>> +#define to_uvcg_cmd(group_ptr) \
>> +container_of(group_ptr, struct uvcg_cmd, group)
>> +
>> enum uvcg_format_type {
>> UVCG_UNCOMPRESSED = 0,
>> UVCG_MJPEG,
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching
2022-12-15 11:48 ` Kieran Bingham
@ 2022-12-16 15:32 ` Dan Scally
0 siblings, 0 replies; 29+ messages in thread
From: Dan Scally @ 2022-12-16 15:32 UTC (permalink / raw)
To: Kieran Bingham, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv
Hi Kieran
On 15/12/2022 11:48, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:34)
>> A hardcoded default color matching descriptor is embedded in struct
>> f_uvc_opts but no longer has any use - remove it.
> Does this affect the legacy g_webcam, or is this part independent ?
It's not independent, but the legacy gadget doesn't use the
uvc_color_matching member of f_uvc_opts. Instead that file has a static
definition of the same thing [1], so this is safe to remove here.
The legacy version does actually have the same issue with just a single
color matching descriptor trailing all the format/frame descriptors
rather than once-per-format...I'll patch that too.
>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> drivers/usb/gadget/function/f_uvc.c | 9 ---------
>> drivers/usb/gadget/function/u_uvc.h | 1 -
>> 2 files changed, 10 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index 6e196e06181e..46bdea73cdeb 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -793,7 +793,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>> struct uvc_camera_terminal_descriptor *cd;
>> struct uvc_processing_unit_descriptor *pd;
>> struct uvc_output_terminal_descriptor *od;
>> - struct uvc_color_matching_descriptor *md;
>> struct uvc_descriptor_header **ctl_cls;
>> int ret;
>>
>> @@ -842,14 +841,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>> od->bSourceID = 2;
>> od->iTerminal = 0;
>>
>> - md = &opts->uvc_color_matching;
>> - md->bLength = UVC_DT_COLOR_MATCHING_SIZE;
>> - md->bDescriptorType = USB_DT_CS_INTERFACE;
>> - md->bDescriptorSubType = UVC_VS_COLORFORMAT;
>> - md->bColorPrimaries = 1;
>> - md->bTransferCharacteristics = 1;
>> - md->bMatrixCoefficients = 4;
>> -
>> /* Prepare fs control class descriptors for configfs-based gadgets */
>> ctl_cls = opts->uvc_fs_control_cls;
>> ctl_cls[0] = NULL; /* assigned elsewhere by configfs */
>> diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
>> index 24b8681b0d6f..577c1c48ca4a 100644
>> --- a/drivers/usb/gadget/function/u_uvc.h
>> +++ b/drivers/usb/gadget/function/u_uvc.h
>> @@ -52,7 +52,6 @@ struct f_uvc_opts {
>> struct uvc_camera_terminal_descriptor uvc_camera_terminal;
>> struct uvc_processing_unit_descriptor uvc_processing;
>> struct uvc_output_terminal_descriptor uvc_output_terminal;
>> - struct uvc_color_matching_descriptor uvc_color_matching;
>>
>> /*
>> * Control descriptors pointers arrays for full-/high-speed and
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write
2022-12-15 11:51 ` Kieran Bingham
@ 2022-12-16 15:53 ` Dan Scally
2022-12-18 23:04 ` Laurent Pinchart
0 siblings, 1 reply; 29+ messages in thread
From: Dan Scally @ 2022-12-16 15:53 UTC (permalink / raw)
To: Kieran Bingham, linux-usb
Cc: laurent.pinchart, gregkh, w36195, m.grzeschik, torleiv
On 15/12/2022 11:51, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:35)
>> In preparation for allowing more than the default color matching
>> descriptor, make the color matching attributes writeable.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> .../ABI/testing/configfs-usb-gadget-uvc | 2 +-
>> drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> index 611b23e6488d..3512f4899fe3 100644
>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> @@ -165,7 +165,7 @@ Date: Dec 2014
>> KernelVersion: 4.0
>> Description: Default color matching descriptors
>>
>> - All attributes read only:
>> + All attributes read/write:
> Do we need to specify here what acceptable values can now be written at
> all?
Yes I guess so...we probably need better documentation for this
somewhere. I don't think this file is the right place to describe it
fully, it's more of an enumeration. We probably need something like
Documentation/usb/gadget_serial.rst
>
>>
>> ======================== ======================================
>> bMatrixCoefficients matrix used to compute luma and
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 6f5932c9f09c..4fbc42d738a4 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
>> return result; \
>> } \
>> \
>> -UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
>> +static ssize_t uvcg_color_matching_##cname##_store( \
>> + struct config_item *item, const char *page, size_t len) \
>> +{ \
>> + struct config_group *group = to_config_group(item); \
>> + struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
>> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
>> + struct f_uvc_opts *opts; \
>> + struct config_item *opts_item; \
>> + int ret; \
>> + u##bits num; \
>> + \
>> + ret = kstrtou##bits(page, 0, &num); \
>> + if (ret) \
>> + return ret; \
> I don't know how horrible it would be - or if there's any other
> precendence, but I'm weary that setting '1', or '4' in here from
> userspace is fairly meaningless.
>
> Of course - the user doing so would have to know from the spec perhaps
> what they are configuring - but it makes me wonder if we should support
> string matching in here to also convert say "BT.709" to the appropriate
> integer value (if a non-integer was set).
>
> It may depend on how 'most' other configfs entries that would be similar
> to this would expect to operate.
This might be abusing configfs slightly, though I did something similar
for the custom string descriptors (see [1] and ctrl-f for
"uvcg_language_strings_show") and I think it's a worthwhile thing. What
about an "enumerate options" attribute that's read only and simply
enumerates the settings with their corresponding descriptions? The
problem with string parsing is you've still got to know the exact string
to pass (and googling "BT.709" tells me it can also be called "Rec.709",
"Rec. 709" and "ITU 709" for example) so you'd have to look it up
anyway. I'm thinking something like:
$ cat enumerate_options
bColorPrimaries - This defines the color primaries and the reference white
value desc
0 Unspecified (Image characteristics unknown)
1 BT.709 (sRGB)
2 BT.470-2 (M)
3 BT.470 (B, G)
4 SMPTE 170M
5 SMPTE 240M
6-255 Reserved
bTransferCharacteristics - This field defines the opto-electronic transfer
characteristics of the
source picture, also
called the gamma function
value desc
0 Unspecified (Image characteristics unknown)
1 BT.7-0
2 BT.470-2 M
... ...
... and so on. What do you think?
[1]
https://lore.kernel.org/linux-usb/20221121092517.225242-6-dan.scally@ideasonboard.com/
>
>> + \
>> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
>> + \
>> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
>> + opts = to_f_uvc_opts(opts_item); \
>> + \
>> + mutex_lock(&opts->lock); \
>> + \
>> + cmd->desc.aname = num; \
>> + ret = len; \
>> + \
>> + mutex_unlock(&opts->lock); \
>> + mutex_unlock(su_mutex); \
>> + \
>> + return ret; \
>> +} \
>> +UVC_ATTR(uvcg_color_matching_, cname, aname)
>>
>> UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
>> UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] UVC Gadget: Extend color matching support
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
` (5 preceding siblings ...)
2022-12-13 8:37 ` [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors Daniel Scally
@ 2022-12-18 18:12 ` Laurent Pinchart
2022-12-19 7:30 ` Dan Scally
6 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 18:12 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Dan,
Thank you for the series.
On Tue, Dec 13, 2022 at 08:37:30AM +0000, Daniel Scally wrote:
> The current UVC gadget implementation hardcodes a single color matching
> descriptor and transmits it a single time following all the format and frame
I'm not sure I would use "transmits" in this context. Descriptors are
for sure transmitted over the wire, but all in one go, not as individual
units (at least within a configuration descriptor). Maybe "includes"
would be a better term ? This is nitpicking for the cover letter, but
the comment applies more importantly to commit messages and code for the
whole series.
> descriptors. This is inflexible, and additionally applies only to the _last_
> format in the array of descriptors.
>
> This series extends the support such that the default descriptor can be amended
> and is transmitted once-per-format instead of once-only, it then adds the ability
> to create new color matching descriptors and associate them with particular formats.
> The default color matching descriptor is retained and used where the user does not
> link a new color matching descriptor to the format, so the default interaction
> with userspace is unchanged from the current implementation.
I wonder if we shouldn't drop the default descriptor. If userspace
doesn't specify one, then we really can't know what colorimetry data
applies to the frames. Instead of providing a default to the host, not
providing any colorimetry information would be better.
> Daniel Scally (6):
> usb: gadget: usb: Remove "default" from color matching attributes
> usb: gadget: uvc: Add struct for color matching in configs
> usb: gadget: uvc: Copy color matching descriptor for each frame
> usb: gadget: uvc: Remove the hardcoded default color matching
> usb: gadget: uvc: Make color matching attributes read/write
> usb: gadget: uvc: Allow creating new color matching descriptors
>
> .../ABI/testing/configfs-usb-gadget-uvc | 6 +-
> drivers/usb/gadget/function/f_uvc.c | 9 -
> drivers/usb/gadget/function/u_uvc.h | 1 -
> drivers/usb/gadget/function/uvc_configfs.c | 247 +++++++++++++++---
> drivers/usb/gadget/function/uvc_configfs.h | 9 +
> 5 files changed, 228 insertions(+), 44 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching
2022-12-13 8:37 ` [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching Daniel Scally
2022-12-15 11:48 ` Kieran Bingham
@ 2022-12-18 22:52 ` Laurent Pinchart
1 sibling, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 22:52 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Dan,
Thank you for the patch.
On Tue, Dec 13, 2022 at 08:37:34AM +0000, Daniel Scally wrote:
> A hardcoded default color matching descriptor is embedded in struct
> f_uvc_opts but no longer has any use - remove it.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/usb/gadget/function/f_uvc.c | 9 ---------
> drivers/usb/gadget/function/u_uvc.h | 1 -
> 2 files changed, 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 6e196e06181e..46bdea73cdeb 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -793,7 +793,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> struct uvc_camera_terminal_descriptor *cd;
> struct uvc_processing_unit_descriptor *pd;
> struct uvc_output_terminal_descriptor *od;
> - struct uvc_color_matching_descriptor *md;
> struct uvc_descriptor_header **ctl_cls;
> int ret;
>
> @@ -842,14 +841,6 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> od->bSourceID = 2;
> od->iTerminal = 0;
>
> - md = &opts->uvc_color_matching;
> - md->bLength = UVC_DT_COLOR_MATCHING_SIZE;
> - md->bDescriptorType = USB_DT_CS_INTERFACE;
> - md->bDescriptorSubType = UVC_VS_COLORFORMAT;
> - md->bColorPrimaries = 1;
> - md->bTransferCharacteristics = 1;
> - md->bMatrixCoefficients = 4;
> -
> /* Prepare fs control class descriptors for configfs-based gadgets */
> ctl_cls = opts->uvc_fs_control_cls;
> ctl_cls[0] = NULL; /* assigned elsewhere by configfs */
> diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
> index 24b8681b0d6f..577c1c48ca4a 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -52,7 +52,6 @@ struct f_uvc_opts {
> struct uvc_camera_terminal_descriptor uvc_camera_terminal;
> struct uvc_processing_unit_descriptor uvc_processing;
> struct uvc_output_terminal_descriptor uvc_output_terminal;
> - struct uvc_color_matching_descriptor uvc_color_matching;
>
> /*
> * Control descriptors pointers arrays for full-/high-speed and
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write
2022-12-16 15:53 ` Dan Scally
@ 2022-12-18 23:04 ` Laurent Pinchart
2022-12-19 9:21 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 23:04 UTC (permalink / raw)
To: Dan Scally
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
On Fri, Dec 16, 2022 at 03:53:05PM +0000, Dan Scally wrote:
> On 15/12/2022 11:51, Kieran Bingham wrote:
> > Quoting Daniel Scally (2022-12-13 08:37:35)
> >> In preparation for allowing more than the default color matching
> >> descriptor, make the color matching attributes writeable.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >> .../ABI/testing/configfs-usb-gadget-uvc | 2 +-
> >> drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
> >> 2 files changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> index 611b23e6488d..3512f4899fe3 100644
> >> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> @@ -165,7 +165,7 @@ Date: Dec 2014
> >> KernelVersion: 4.0
> >> Description: Default color matching descriptors
> >>
> >> - All attributes read only:
> >> + All attributes read/write:
> >
> > Do we need to specify here what acceptable values can now be written at
> > all?
>
> Yes I guess so...we probably need better documentation for this
> somewhere. I don't think this file is the right place to describe it
> fully, it's more of an enumeration. We probably need something like
> Documentation/usb/gadget_serial.rst
That would make sense. I think you can also heavily quote or reference
the USB video class documentation.
> >> ======================== ======================================
> >> bMatrixCoefficients matrix used to compute luma and
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index 6f5932c9f09c..4fbc42d738a4 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
> >> return result; \
> >> } \
> >> \
> >> -UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
> >> +static ssize_t uvcg_color_matching_##cname##_store( \
> >> + struct config_item *item, const char *page, size_t len) \
> >> +{ \
> >> + struct config_group *group = to_config_group(item); \
> >> + struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
> >> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
> >> + struct f_uvc_opts *opts; \
> >> + struct config_item *opts_item; \
> >> + int ret; \
I'd make this one last.
> >> + u##bits num; \
In other places we use
typeof(cmd->desc.aname) num;
Up to you.
> >> + \
> >> + ret = kstrtou##bits(page, 0, &num); \
> >> + if (ret) \
> >> + return ret; \
> >
> > I don't know how horrible it would be - or if there's any other
> > precendence, but I'm weary that setting '1', or '4' in here from
> > userspace is fairly meaningless.
> >
> > Of course - the user doing so would have to know from the spec perhaps
> > what they are configuring - but it makes me wonder if we should support
> > string matching in here to also convert say "BT.709" to the appropriate
> > integer value (if a non-integer was set).
> >
> > It may depend on how 'most' other configfs entries that would be similar
> > to this would expect to operate.
>
> This might be abusing configfs slightly, though I did something similar
> for the custom string descriptors (see [1] and ctrl-f for
> "uvcg_language_strings_show") and I think it's a worthwhile thing. What
> about an "enumerate options" attribute that's read only and simply
> enumerates the settings with their corresponding descriptions? The
> problem with string parsing is you've still got to know the exact string
> to pass (and googling "BT.709" tells me it can also be called "Rec.709",
> "Rec. 709" and "ITU 709" for example) so you'd have to look it up
> anyway. I'm thinking something like:
>
> $ cat enumerate_options
> bColorPrimaries - This defines the color primaries and the reference white
> value desc
> 0 Unspecified (Image characteristics unknown)
> 1 BT.709 (sRGB)
> 2 BT.470-2 (M)
> 3 BT.470 (B, G)
> 4 SMPTE 170M
> 5 SMPTE 240M
> 6-255 Reserved
>
> bTransferCharacteristics - This field defines the opto-electronic transfer
> characteristics of the
> source picture, also
> called the gamma function
> value desc
> 0 Unspecified (Image characteristics unknown)
> 1 BT.7-0
> 2 BT.470-2 M
> ... ...
>
>
> ... and so on. What do you think?
I think it's overkill. The userspace side of the UVC gadget is expected
to have good knowledge of UVC. This isn't an interface meant for users.
> [1] https://lore.kernel.org/linux-usb/20221121092517.225242-6-dan.scally@ideasonboard.com/
>
> >> + \
> >> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> >> + \
> >> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> >> + opts = to_f_uvc_opts(opts_item); \
> >> + \
> >> + mutex_lock(&opts->lock); \
Don't you need a similar linked check as in uvcg_frame_##cname##_store()
?
> >> + \
> >> + cmd->desc.aname = num; \
> >> + ret = len; \
> >> + \
> >> + mutex_unlock(&opts->lock); \
> >> + mutex_unlock(su_mutex); \
> >> + \
> >> + return ret; \
> >> +} \
> >> +UVC_ATTR(uvcg_color_matching_, cname, aname)
> >>
> >> UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> >> UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-15 12:03 ` Dan Scally
@ 2022-12-18 23:17 ` Laurent Pinchart
2022-12-19 9:44 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 23:17 UTC (permalink / raw)
To: Dan Scally
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
On Thu, Dec 15, 2022 at 12:03:03PM +0000, Dan Scally wrote:
> On 15/12/2022 12:00, Kieran Bingham wrote:
> > Quoting Daniel Scally (2022-12-13 08:37:36)
> >> Allow users to create new color matching descriptors in addition to
> >> the default one. These must be associated with a UVC format in order
> >> to be transmitted to the host, which is achieved by symlinking from
> >> the format to the newly created color matching descriptor - extend
> >> the uncompressed and mjpeg formats to support that linking operation.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >> .../ABI/testing/configfs-usb-gadget-uvc | 4 +-
> >> drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
> >> 2 files changed, 79 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> index 3512f4899fe3..ce629f0880a9 100644
> >> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> @@ -160,10 +160,10 @@ Date: Dec 2014
> >> KernelVersion: 4.0
> >> Description: Color matching descriptors
> >>
> >> -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
> >> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> > Hrm... do we need to keep 'default' (and keep it available in patch
> > 1/6?) so that the default is still kept accesible?
>
> It's still there - it's just no longer necessarily the only entry
>
> > That would mean others are added as siblings to the default, and then
> > the one that gets linked is the one that is used perhaps?
>
> Yep that's how it's implemented
>
> > Feels like that would overcomplicate 'the default case' maybe ... but
> > I'm weary that this is 'removing' the default from ABI...?
> >
> >> Date: Dec 2014
> >> KernelVersion: 4.0
> >
> > Hrm ... and this would leave the documentation stating that you could
> > provide custom color matching descriptors from v4.0 onwards, which would
> > be inaccurate ?
>
> Ah, good point...fair enough, I'll add a new entry for the custom named
> ones.
>
> > So I'm not sure we can just rename this documentation segment eitherway.
> >
> >> -Description: Default color matching descriptors
> >> +Description: color matching descriptors
> >>
> >> All attributes read/write:
> >>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index 4fbc42d738a4..82c10f0dab71 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
> >> return cmd;
> >> }
> >>
> >> +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
> >> +{
> >> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> + struct config_item *streaming, *color_matching;
> >> + struct uvcg_cmd *color_matching_desc;
> >> + struct uvcg_format *fmt;
> >> + int ret = 0;
> >> +
> >> + mutex_lock(su_mutex);
> >> +
> >> + streaming = src->ci_parent->ci_parent;
> >> + color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
> >> + if (!color_matching || color_matching != tgt->ci_parent) {
> >> + ret = -EINVAL;
> >> + goto out_put_cm;
> >> + }
> >> +
> >> + color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
> >> + fmt = to_uvcg_format(src);
> >> + fmt->color_matching = color_matching_desc;
As you store a pointer to the target, don't you need to keep a reference
to it somehow, to avoid the target being deleted ? Or is that handled by
configfs itself ?
It also looks like you need to refcount the number of links to the
target, to disallow changes to the color matching attributes when the
descriptor is linked.
This being said, are links the best option here ? Why can't we create
color matching descriptors as children of format descriptors the same
way that frame descriptors are handled ?
> >> +
> >> +out_put_cm:
> >> + config_item_put(color_matching);
> >> + mutex_unlock(su_mutex);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
> >> +{
> >> + struct config_item *streaming;
> >> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> + struct uvcg_format *fmt;
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + mutex_lock(su_mutex);
> >> +
> >> + streaming = src->ci_parent->ci_parent;
> >> + cmd = uvcg_format_get_default_cmd(streaming);
> >> +
> >> + fmt = to_uvcg_format(src);
> >> + fmt->color_matching = cmd;
> >> +
> >> + mutex_unlock(su_mutex);
> >> +}
> >> +
> >> +static struct configfs_item_operations uvcg_format_item_operations = {
Not a candidate for this patch (or this series), but I wonder if this
could be made const. It would involve making several pointers in struct
config_item_type const, which may or may not be straightforward.
> >> + .release = uvcg_config_item_release,
> >> + .allow_link = uvcg_format_allow_link,
> >> + .drop_link = uvcg_format_drop_link,
> >> +};
> >> +
> >> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> >> {
> >> struct f_uvc_opts *opts;
> >> @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
> >> };
> >>
> >> static const struct config_item_type uvcg_uncompressed_type = {
> >> - .ct_item_ops = &uvcg_config_item_ops,
> >> + .ct_item_ops = &uvcg_format_item_operations,
> >> .ct_group_ops = &uvcg_uncompressed_group_ops,
> >> .ct_attrs = uvcg_uncompressed_attrs,
> >> .ct_owner = THIS_MODULE,
> >> @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
> >> };
> >>
> >> static const struct config_item_type uvcg_mjpeg_type = {
> >> - .ct_item_ops = &uvcg_config_item_ops,
> >> + .ct_item_ops = &uvcg_format_item_operations,
> >> .ct_group_ops = &uvcg_mjpeg_group_ops,
> >> .ct_attrs = uvcg_mjpeg_attrs,
> >> .ct_owner = THIS_MODULE,
> >> @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
> >> * streaming/color_matching
> >> */
> >>
> >> +static struct config_group *uvcg_color_matching_make(struct config_group *group,
> >> + const char *name)
> >> +{
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> >> + if (!cmd)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> >> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> >> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> >> +
> >> + config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
> >> +
> >> + return &cmd->group;
> >> +}
> >> +
> >> +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
> >> + .make_group = uvcg_color_matching_make,
> >> +};
> >> +
> >> static int uvcg_color_matching_create_children(struct config_group *parent)
> >> {
> >> struct uvcg_cmd *cmd;
> >> @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
> >> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> >> .type = {
> >> .ct_item_ops = &uvcg_config_item_ops,
> >> + .ct_group_ops = &uvcg_color_matching_grp_group_ops,
> >> .ct_owner = THIS_MODULE,
> >> },
> >> .name = "color_matching",
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame
2022-12-13 8:37 ` [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame Daniel Scally
@ 2022-12-18 23:28 ` Laurent Pinchart
2022-12-19 10:33 ` Dan Scally
0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 23:28 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Dan,
Thank you for the patch.
On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
> As currently implemented the default color matching descriptor is
> appended after _all_ the formats and frames that the gadget is
> configured with. According to the UVC specifications however this
> is supposed to be on a per-format basis (section 3.9.2.6):
>
> "Only one instance is allowed for a given format and if present,
> the Color Matching descriptor shall be placed following the Video
> and Still Image Frame descriptors for that format."
>
> Associate the default color matching descriptor with struct
> uvcg_format and copy it once-per-format instead of once only.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
> drivers/usb/gadget/function/uvc_configfs.h | 1 +
> 2 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 9918e7b6a023..6f5932c9f09c 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
> "mjpeg",
> };
>
> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
> +{
> + struct config_item *color_matching, *cm_default;
> + struct uvcg_cmd *cmd;
> +
> + color_matching = config_group_find_item(to_config_group(streaming),
> + "color_matching");
> + if (!color_matching)
> + return NULL;
> +
> + cm_default = config_group_find_item(to_config_group(color_matching),
> + "default");
> + config_item_put(color_matching);
> + if (!cm_default)
> + return NULL;
> +
> + cmd = to_uvcg_cmd(to_config_group(cm_default));
> + config_item_put(cm_default);
> +
> + return cmd;
> +}
> +
> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> {
> struct f_uvc_opts *opts;
> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> 'Y', 'U', 'Y', '2', 0x00, 0x00, 0x10, 0x00,
> 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
> };
> + struct config_item *streaming;
> struct uvcg_uncompressed *h;
> + struct uvcg_cmd *cmd;
> +
> + streaming = group->cg_item.ci_parent;
> + cmd = uvcg_format_get_default_cmd(streaming);
> + if (!cmd)
> + return ERR_PTR(-EINVAL);
>
> h = kzalloc(sizeof(*h), GFP_KERNEL);
> if (!h)
> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>
> INIT_LIST_HEAD(&h->fmt.frames);
> h->fmt.type = UVCG_UNCOMPRESSED;
> + h->fmt.color_matching = cmd;
Is there any need to reference-count cmd to make sure it doesn't get
deleted ? I don't expect that to be an issue for the default, but for
the user-defined color matching descriptors. This may be better
discussed in the context of other patches further in the series.
> config_group_init_type_name(&h->fmt.group, name,
> &uvcg_uncompressed_type);
>
> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
> static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> const char *name)
> {
> + struct config_item *streaming;
> struct uvcg_mjpeg *h;
> + struct uvcg_cmd *cmd;
> +
> + streaming = group->cg_item.ci_parent;
> + cmd = uvcg_format_get_default_cmd(streaming);
> + if (!cmd)
> + return ERR_PTR(-EINVAL);
>
> h = kzalloc(sizeof(*h), GFP_KERNEL);
> if (!h)
> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>
> INIT_LIST_HEAD(&h->fmt.frames);
> h->fmt.type = UVCG_MJPEG;
> + h->fmt.color_matching = cmd;
> config_group_init_type_name(&h->fmt.group, name,
> &uvcg_mjpeg_type);
>
> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
> enum uvcg_strm_type {
> UVCG_HEADER = 0,
> UVCG_FORMAT,
> - UVCG_FRAME
> + UVCG_FRAME,
> + UVCG_CMD,
> };
>
> /*
> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
> if (ret)
> return ret;
> }
> +
> + ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
> + if (ret)
> + return ret;
> }
>
> return ret;
> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
> *size += frm->frame.b_frame_interval_type * sz;
> }
> break;
> + case UVCG_CMD: {
> + struct uvcg_cmd *cmd = priv1;
Missing blank line. Same below.
> + *size += sizeof(cmd->desc);
> + }
> + break;
> }
>
> ++*count;
> @@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
> frm->frame.b_frame_interval_type);
> }
> break;
> + case UVCG_CMD: {
> + struct uvcg_cmd *cmd = priv1;
> +
> + memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
> + *dest += sizeof(cmd->desc);
> + }
> + break;
> }
>
> return 0;
> @@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
> if (ret)
> goto unlock;
>
> - count += 2; /* color_matching, NULL */
> + count += 1; /* NULL */
> *class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
> if (!*class_array) {
> ret = -ENOMEM;
> @@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
> kfree(data_save);
> goto unlock;
> }
> - *cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
>
> ++target_hdr->linked;
> ret = 0;
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index f990739838d5..6582d6c7b6f6 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -57,6 +57,7 @@ struct uvcg_format {
> struct list_head frames;
> unsigned num_frames;
> __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> + struct uvcg_cmd *color_matching;
> };
>
> struct uvcg_format_ptr {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs
2022-12-16 14:06 ` Dan Scally
@ 2022-12-18 23:28 ` Laurent Pinchart
0 siblings, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 23:28 UTC (permalink / raw)
To: Dan Scally
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
Hello,
On Fri, Dec 16, 2022 at 02:06:48PM +0000, Dan Scally wrote:
> On 15/12/2022 11:45, Kieran Bingham wrote:
> > Quoting Daniel Scally (2022-12-13 08:37:32)
> >> Color matching descriptors are meant to be a per-format piece of data
> >> and we need to be able to support different descriptors for different
> >> formats. As a preliminary step towards that goal, switch the default
> >> color matching configfs functionality to point to an instance of a
> >> new struct uvcg_cmd (for "color matching descriptor"). Use the same
> >
> > Hrm .. I can't see 'cmd' and not think 'command' ... but longer names
> > are longer ...
>
> Yeah. Naming things was never my strong suit...I couldn't think of a
> name of intermediate length that wasn't rubbish so it was either this or
> "uvcg_color_matching_descriptor" which is loooong.
I agree with Kieran, uvcg_cmd isn't good. uvcg_color_matching ?
> >> default values for its attributes as the currently hard-coded ones so
> >> that the interface to userspace is consistent.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >> drivers/usb/gadget/function/uvc_configfs.c | 55 ++++++++++++++++------
> >> drivers/usb/gadget/function/uvc_configfs.h | 8 ++++
> >> 2 files changed, 49 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index 26d092790f12..9918e7b6a023 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -1788,20 +1788,19 @@ static ssize_t uvcg_color_matching_##cname##_show( \
> >> struct config_item *item, char *page) \
> >> { \
> >> struct config_group *group = to_config_group(item); \
> >> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
And you could name the variable color_match.
> >> struct f_uvc_opts *opts; \
> >> struct config_item *opts_item; \
> >> struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
> >> - struct uvc_color_matching_descriptor *cd; \
> >> int result; \
> >> \
> >> mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> >> \
> >> opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> >> opts = to_f_uvc_opts(opts_item); \
> >> - cd = &opts->uvc_color_matching; \
> >> \
> >> mutex_lock(&opts->lock); \
> >> - result = sprintf(page, "%u\n", le##bits##_to_cpu(cd->aname)); \
> >> + result = sprintf(page, "%u\n", le##bits##_to_cpu(cmd->desc.aname));\
> >> mutex_unlock(&opts->lock); \
> >> \
> >> mutex_unlock(su_mutex); \
> >> @@ -1823,29 +1822,57 @@ static struct configfs_attribute *uvcg_color_matching_attrs[] = {
> >> NULL,
> >> };
> >>
> >> -static const struct uvcg_config_group_type uvcg_color_matching_type = {
> >> - .type = {
> >> - .ct_item_ops = &uvcg_config_item_ops,
> >> - .ct_attrs = uvcg_color_matching_attrs,
> >> - .ct_owner = THIS_MODULE,
> >> - },
> >> - .name = "default",
> >> +static void uvcg_color_matching_release(struct config_item *item)
> >> +{
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + cmd = to_uvcg_cmd(to_config_group(item));
You can write
struct uvcg_cmd *cmd = to_uvcg_cmd(to_config_group(item));
although it would need to become
struct uvcg_color_matching *color_match =
to_uvcg_color_matching(to_config_group(item));
> >> + kfree(cmd);
> >> +}
> >> +
> >> +static struct configfs_item_operations uvcg_color_matching_item_ops = {
> >> + .release = uvcg_color_matching_release,
> >> +};
> >> +
> >> +static const struct config_item_type uvcg_color_matching_type = {
> >> + .ct_item_ops = &uvcg_color_matching_item_ops,
> >> + .ct_attrs = uvcg_color_matching_attrs,
> >> + .ct_owner = THIS_MODULE,
> >> };
> >>
> >> /* -----------------------------------------------------------------------------
> >> * streaming/color_matching
> >> */
> >>
> >> +static int uvcg_color_matching_create_children(struct config_group *parent)
> >> +{
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> >> + if (!cmd)
> >> + return -ENOMEM;
> >> +
> >> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> >> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> >> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> >> + cmd->desc.bColorPrimaries = 1;
> >> + cmd->desc.bTransferCharacteristics = 1;
> >> + cmd->desc.bMatrixCoefficients = 4;
> >
> > I realise these values were taken directly as existing code, but
> > particularly in regards to how these values will be set from userspace -
> > is it easy enough to have some common definitions in a preceeding patch
> > that state the supported values here from the spec, to avoid 'magic
> > values' here ...
> >
> > A header with defines or an enum isn't going to be usable from a bash
> > script configuring configfs, but at least a compiled program could use
> > the definitions.
>
> Yes, I think probably it's a candidate for
> include/uapi/linux/usb/video.h...unless anyone thinks it's better elsewhere
Sounds good to me.
> >> +
> >> + config_group_init_type_name(&cmd->group, "default",
> >> + &uvcg_color_matching_type);
> >> + configfs_add_default_group(&cmd->group, parent);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> >> .type = {
> >> .ct_item_ops = &uvcg_config_item_ops,
> >> .ct_owner = THIS_MODULE,
> >> },
> >> .name = "color_matching",
> >> - .children = (const struct uvcg_config_group_type*[]) {
> >> - &uvcg_color_matching_type,
> >> - NULL,
> >> - },
> >> + .create_children = uvcg_color_matching_create_children,
> >> };
> >>
> >> /* -----------------------------------------------------------------------------
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> >> index ad2ec8c4c78c..f990739838d5 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.h
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> >> @@ -37,6 +37,14 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
> >> return container_of(item, struct uvcg_control_header, item);
> >> }
> >>
> >> +struct uvcg_cmd {
> >> + struct config_group group;
> >> + struct uvc_color_matching_descriptor desc;
> >> +};
> >> +
> >> +#define to_uvcg_cmd(group_ptr) \
> >> +container_of(group_ptr, struct uvcg_cmd, group)
> >> +
> >> enum uvcg_format_type {
> >> UVCG_UNCOMPRESSED = 0,
> >> UVCG_MJPEG,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes
2022-12-13 8:37 ` [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes Daniel Scally
@ 2022-12-18 23:29 ` Laurent Pinchart
2022-12-19 9:53 ` Kieran Bingham
0 siblings, 1 reply; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-18 23:29 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Dan,
Thank you for the patch.
On Tue, Dec 13, 2022 at 08:37:31AM +0000, Daniel Scally wrote:
> Color matching attributes in the configfs for UVC are named with the
> phrase "default". The implication of that is that they will only be
> used _with_ the default color matching descriptor, and that will
> shortly no longer be the case.
>
> Remove the "default" from the color matching descriptor attribute
> variables.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 29 +++++++++++-----------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 4303a3283ba0..26d092790f12 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1783,8 +1783,8 @@ static const struct uvcg_config_group_type uvcg_mjpeg_grp_type = {
> * streaming/color_matching/default
> */
>
> -#define UVCG_DEFAULT_COLOR_MATCHING_ATTR(cname, aname, bits) \
> -static ssize_t uvcg_default_color_matching_##cname##_show( \
> +#define UVCG_COLOR_MATCHING_ATTR(cname, aname, bits) \
> +static ssize_t uvcg_color_matching_##cname##_show( \
> struct config_item *item, char *page) \
> { \
> struct config_group *group = to_config_group(item); \
> @@ -1808,26 +1808,25 @@ static ssize_t uvcg_default_color_matching_##cname##_show( \
> return result; \
> } \
> \
> -UVC_ATTR_RO(uvcg_default_color_matching_, cname, aname)
> +UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
>
> -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_transfer_characteristics,
> - bTransferCharacteristics, 8);
> -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
> +UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> +UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
> +UVCG_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
>
> -#undef UVCG_DEFAULT_COLOR_MATCHING_ATTR
> +#undef UVCG_COLOR_MATCHING_ATTR
>
> -static struct configfs_attribute *uvcg_default_color_matching_attrs[] = {
> - &uvcg_default_color_matching_attr_b_color_primaries,
> - &uvcg_default_color_matching_attr_b_transfer_characteristics,
> - &uvcg_default_color_matching_attr_b_matrix_coefficients,
> +static struct configfs_attribute *uvcg_color_matching_attrs[] = {
> + &uvcg_color_matching_attr_b_color_primaries,
> + &uvcg_color_matching_attr_b_transfer_characteristics,
> + &uvcg_color_matching_attr_b_matrix_coefficients,
> NULL,
> };
>
> -static const struct uvcg_config_group_type uvcg_default_color_matching_type = {
> +static const struct uvcg_config_group_type uvcg_color_matching_type = {
> .type = {
> .ct_item_ops = &uvcg_config_item_ops,
> - .ct_attrs = uvcg_default_color_matching_attrs,
> + .ct_attrs = uvcg_color_matching_attrs,
> .ct_owner = THIS_MODULE,
> },
> .name = "default",
> @@ -1844,7 +1843,7 @@ static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> },
> .name = "color_matching",
> .children = (const struct uvcg_config_group_type*[]) {
> - &uvcg_default_color_matching_type,
> + &uvcg_color_matching_type,
> NULL,
> },
> };
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] UVC Gadget: Extend color matching support
2022-12-18 18:12 ` [PATCH 0/6] UVC Gadget: Extend color matching support Laurent Pinchart
@ 2022-12-19 7:30 ` Dan Scally
0 siblings, 0 replies; 29+ messages in thread
From: Dan Scally @ 2022-12-19 7:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Morning Laurent
On 18/12/2022 18:12, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the series.
>
> On Tue, Dec 13, 2022 at 08:37:30AM +0000, Daniel Scally wrote:
>> The current UVC gadget implementation hardcodes a single color matching
>> descriptor and transmits it a single time following all the format and frame
> I'm not sure I would use "transmits" in this context. Descriptors are
> for sure transmitted over the wire, but all in one go, not as individual
> units (at least within a configuration descriptor). Maybe "includes"
> would be a better term ? This is nitpicking for the cover letter, but
> the comment applies more importantly to commit messages and code for the
> whole series.
I've used the same term I think in the series yes. No problem; I'll swap
to includes.
>
>> descriptors. This is inflexible, and additionally applies only to the _last_
>> format in the array of descriptors.
>>
>> This series extends the support such that the default descriptor can be amended
>> and is transmitted once-per-format instead of once-only, it then adds the ability
>> to create new color matching descriptors and associate them with particular formats.
>> The default color matching descriptor is retained and used where the user does not
>> link a new color matching descriptor to the format, so the default interaction
>> with userspace is unchanged from the current implementation.
> I wonder if we shouldn't drop the default descriptor. If userspace
> doesn't specify one, then we really can't know what colorimetry data
> applies to the frames. Instead of providing a default to the host, not
> providing any colorimetry information would be better.
According to the spec:
"In the absence of this descriptor, or in the case of "Unspecified"
values within the descriptor, color matching defaults will be assumed.
The color matching defaults are compliant with sRGB since the BT.709
transfer function and the sRGB transfer function are very similar"
And it goes on to identify the default values for each of the
descriptor's fields...which happen to be the values that are set in our
default descriptor. So I think that including that default descriptor
shouldn't change the host's behaviour, but does give userspace an easy
way to see what's set...I think it's fine to keep.
>
>> Daniel Scally (6):
>> usb: gadget: usb: Remove "default" from color matching attributes
>> usb: gadget: uvc: Add struct for color matching in configs
>> usb: gadget: uvc: Copy color matching descriptor for each frame
>> usb: gadget: uvc: Remove the hardcoded default color matching
>> usb: gadget: uvc: Make color matching attributes read/write
>> usb: gadget: uvc: Allow creating new color matching descriptors
>>
>> .../ABI/testing/configfs-usb-gadget-uvc | 6 +-
>> drivers/usb/gadget/function/f_uvc.c | 9 -
>> drivers/usb/gadget/function/u_uvc.h | 1 -
>> drivers/usb/gadget/function/uvc_configfs.c | 247 +++++++++++++++---
>> drivers/usb/gadget/function/uvc_configfs.h | 9 +
>> 5 files changed, 228 insertions(+), 44 deletions(-)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write
2022-12-18 23:04 ` Laurent Pinchart
@ 2022-12-19 9:21 ` Dan Scally
0 siblings, 0 replies; 29+ messages in thread
From: Dan Scally @ 2022-12-19 9:21 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
Morning Laurent
On 18/12/2022 23:04, Laurent Pinchart wrote:
> On Fri, Dec 16, 2022 at 03:53:05PM +0000, Dan Scally wrote:
>> On 15/12/2022 11:51, Kieran Bingham wrote:
>>> Quoting Daniel Scally (2022-12-13 08:37:35)
>>>> In preparation for allowing more than the default color matching
>>>> descriptor, make the color matching attributes writeable.
>>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> ---
>>>> .../ABI/testing/configfs-usb-gadget-uvc | 2 +-
>>>> drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
>>>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> index 611b23e6488d..3512f4899fe3 100644
>>>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> @@ -165,7 +165,7 @@ Date: Dec 2014
>>>> KernelVersion: 4.0
>>>> Description: Default color matching descriptors
>>>>
>>>> - All attributes read only:
>>>> + All attributes read/write:
>>> Do we need to specify here what acceptable values can now be written at
>>> all?
>> Yes I guess so...we probably need better documentation for this
>> somewhere. I don't think this file is the right place to describe it
>> fully, it's more of an enumeration. We probably need something like
>> Documentation/usb/gadget_serial.rst
> That would make sense. I think you can also heavily quote or reference
> the USB video class documentation.
Ack. I've started this.
>
>>>> ======================== ======================================
>>>> bMatrixCoefficients matrix used to compute luma and
>>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>>>> index 6f5932c9f09c..4fbc42d738a4 100644
>>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>>>> @@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
>>>> return result; \
>>>> } \
>>>> \
>>>> -UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
>>>> +static ssize_t uvcg_color_matching_##cname##_store( \
>>>> + struct config_item *item, const char *page, size_t len) \
>>>> +{ \
>>>> + struct config_group *group = to_config_group(item); \
>>>> + struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
>>>> + struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
>>>> + struct f_uvc_opts *opts; \
>>>> + struct config_item *opts_item; \
>>>> + int ret; \
> I'd make this one last.
Hah I overthought this quite a lot because following the preprocessing
num might be shorter. No problem though, I'll switch it to last.
>
>>>> + u##bits num; \
> In other places we use
>
> typeof(cmd->desc.aname) num;
>
> Up to you.
>
>>>> + \
>>>> + ret = kstrtou##bits(page, 0, &num); \
>>>> + if (ret) \
>>>> + return ret; \
>>> I don't know how horrible it would be - or if there's any other
>>> precendence, but I'm weary that setting '1', or '4' in here from
>>> userspace is fairly meaningless.
>>>
>>> Of course - the user doing so would have to know from the spec perhaps
>>> what they are configuring - but it makes me wonder if we should support
>>> string matching in here to also convert say "BT.709" to the appropriate
>>> integer value (if a non-integer was set).
>>>
>>> It may depend on how 'most' other configfs entries that would be similar
>>> to this would expect to operate.
>> This might be abusing configfs slightly, though I did something similar
>> for the custom string descriptors (see [1] and ctrl-f for
>> "uvcg_language_strings_show") and I think it's a worthwhile thing. What
>> about an "enumerate options" attribute that's read only and simply
>> enumerates the settings with their corresponding descriptions? The
>> problem with string parsing is you've still got to know the exact string
>> to pass (and googling "BT.709" tells me it can also be called "Rec.709",
>> "Rec. 709" and "ITU 709" for example) so you'd have to look it up
>> anyway. I'm thinking something like:
>>
>> $ cat enumerate_options
>> bColorPrimaries - This defines the color primaries and the reference white
>> value desc
>> 0 Unspecified (Image characteristics unknown)
>> 1 BT.709 (sRGB)
>> 2 BT.470-2 (M)
>> 3 BT.470 (B, G)
>> 4 SMPTE 170M
>> 5 SMPTE 240M
>> 6-255 Reserved
>>
>> bTransferCharacteristics - This field defines the opto-electronic transfer
>> characteristics of the
>> source picture, also
>> called the gamma function
>> value desc
>> 0 Unspecified (Image characteristics unknown)
>> 1 BT.7-0
>> 2 BT.470-2 M
>> ... ...
>>
>>
>> ... and so on. What do you think?
> I think it's overkill. The userspace side of the UVC gadget is expected
> to have good knowledge of UVC. This isn't an interface meant for users.
Fair enough
>
>> [1] https://lore.kernel.org/linux-usb/20221121092517.225242-6-dan.scally@ideasonboard.com/
>>
>>>> + \
>>>> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
>>>> + \
>>>> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
>>>> + opts = to_f_uvc_opts(opts_item); \
>>>> + \
>>>> + mutex_lock(&opts->lock); \
> Don't you need a similar linked check as in uvcg_frame_##cname##_store()
> ?
Yes, actually. I was halfway through explaining why I had considered it
unnecessary and skipped it, and then realised I was wrong :)
>
>>>> + \
>>>> + cmd->desc.aname = num; \
>>>> + ret = len; \
>>>> + \
>>>> + mutex_unlock(&opts->lock); \
>>>> + mutex_unlock(su_mutex); \
>>>> + \
>>>> + return ret; \
>>>> +} \
>>>> +UVC_ATTR(uvcg_color_matching_, cname, aname)
>>>>
>>>> UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
>>>> UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-18 23:17 ` Laurent Pinchart
@ 2022-12-19 9:44 ` Dan Scally
2022-12-19 16:05 ` Laurent Pinchart
0 siblings, 1 reply; 29+ messages in thread
From: Dan Scally @ 2022-12-19 9:44 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
Hi Laurent
On 18/12/2022 23:17, Laurent Pinchart wrote:
> On Thu, Dec 15, 2022 at 12:03:03PM +0000, Dan Scally wrote:
>> On 15/12/2022 12:00, Kieran Bingham wrote:
>>> Quoting Daniel Scally (2022-12-13 08:37:36)
>>>> Allow users to create new color matching descriptors in addition to
>>>> the default one. These must be associated with a UVC format in order
>>>> to be transmitted to the host, which is achieved by symlinking from
>>>> the format to the newly created color matching descriptor - extend
>>>> the uncompressed and mjpeg formats to support that linking operation.
>>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> ---
>>>> .../ABI/testing/configfs-usb-gadget-uvc | 4 +-
>>>> drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
>>>> 2 files changed, 79 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> index 3512f4899fe3..ce629f0880a9 100644
>>>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>>>> @@ -160,10 +160,10 @@ Date: Dec 2014
>>>> KernelVersion: 4.0
>>>> Description: Color matching descriptors
>>>>
>>>> -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
>>>> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
>>> Hrm... do we need to keep 'default' (and keep it available in patch
>>> 1/6?) so that the default is still kept accesible?
>> It's still there - it's just no longer necessarily the only entry
>>
>>> That would mean others are added as siblings to the default, and then
>>> the one that gets linked is the one that is used perhaps?
>> Yep that's how it's implemented
>>
>>> Feels like that would overcomplicate 'the default case' maybe ... but
>>> I'm weary that this is 'removing' the default from ABI...?
>>>
>>>> Date: Dec 2014
>>>> KernelVersion: 4.0
>>> Hrm ... and this would leave the documentation stating that you could
>>> provide custom color matching descriptors from v4.0 onwards, which would
>>> be inaccurate ?
>> Ah, good point...fair enough, I'll add a new entry for the custom named
>> ones.
>>
>>> So I'm not sure we can just rename this documentation segment eitherway.
>>>
>>>> -Description: Default color matching descriptors
>>>> +Description: color matching descriptors
>>>>
>>>> All attributes read/write:
>>>>
>>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>>>> index 4fbc42d738a4..82c10f0dab71 100644
>>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>>>> @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
>>>> return cmd;
>>>> }
>>>>
>>>> +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
>>>> +{
>>>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>>>> + struct config_item *streaming, *color_matching;
>>>> + struct uvcg_cmd *color_matching_desc;
>>>> + struct uvcg_format *fmt;
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(su_mutex);
>>>> +
>>>> + streaming = src->ci_parent->ci_parent;
>>>> + color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
>>>> + if (!color_matching || color_matching != tgt->ci_parent) {
>>>> + ret = -EINVAL;
>>>> + goto out_put_cm;
>>>> + }
>>>> +
>>>> + color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
>>>> + fmt = to_uvcg_format(src);
>>>> + fmt->color_matching = color_matching_desc;
> As you store a pointer to the target, don't you need to keep a reference
> to it somehow, to avoid the target being deleted ? Or is that handled by
> configfs itself ?
configfs is doing it in configfs_symlink()
> It also looks like you need to refcount the number of links to the
> target, to disallow changes to the color matching attributes when the
> descriptor is linked.
Yes; I had thought that that was unnecessary so skipped it, but turns
out I'm wrong - I'll add that in.
> This being said, are links the best option here ? Why can't we create
> color matching descriptors as children of format descriptors the same
> way that frame descriptors are handled ?
We can do it that way, but I thought that it made more sense to expand
the current implementation because
1) I'm reluctant to get rid of the default descriptor and I wouldn't
want to have them in multiple places.
2) We're planning to expand the uncompressed format to support more than
just yuy2, which for some cameras might result in _a lot_ of available
formats (vivid has 80!). It seems easier to have color matching
descriptors that you can set once and simply link to than have to
recreate them for each format.
>
>>>> +
>>>> +out_put_cm:
>>>> + config_item_put(color_matching);
>>>> + mutex_unlock(su_mutex);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
>>>> +{
>>>> + struct config_item *streaming;
>>>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>>>> + struct uvcg_format *fmt;
>>>> + struct uvcg_cmd *cmd;
>>>> +
>>>> + mutex_lock(su_mutex);
>>>> +
>>>> + streaming = src->ci_parent->ci_parent;
>>>> + cmd = uvcg_format_get_default_cmd(streaming);
>>>> +
>>>> + fmt = to_uvcg_format(src);
>>>> + fmt->color_matching = cmd;
>>>> +
>>>> + mutex_unlock(su_mutex);
>>>> +}
>>>> +
>>>> +static struct configfs_item_operations uvcg_format_item_operations = {
> Not a candidate for this patch (or this series), but I wonder if this
> could be made const. It would involve making several pointers in struct
> config_item_type const, which may or may not be straightforward.
>
>>>> + .release = uvcg_config_item_release,
>>>> + .allow_link = uvcg_format_allow_link,
>>>> + .drop_link = uvcg_format_drop_link,
>>>> +};
>>>> +
>>>> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>>>> {
>>>> struct f_uvc_opts *opts;
>>>> @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
>>>> };
>>>>
>>>> static const struct config_item_type uvcg_uncompressed_type = {
>>>> - .ct_item_ops = &uvcg_config_item_ops,
>>>> + .ct_item_ops = &uvcg_format_item_operations,
>>>> .ct_group_ops = &uvcg_uncompressed_group_ops,
>>>> .ct_attrs = uvcg_uncompressed_attrs,
>>>> .ct_owner = THIS_MODULE,
>>>> @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
>>>> };
>>>>
>>>> static const struct config_item_type uvcg_mjpeg_type = {
>>>> - .ct_item_ops = &uvcg_config_item_ops,
>>>> + .ct_item_ops = &uvcg_format_item_operations,
>>>> .ct_group_ops = &uvcg_mjpeg_group_ops,
>>>> .ct_attrs = uvcg_mjpeg_attrs,
>>>> .ct_owner = THIS_MODULE,
>>>> @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
>>>> * streaming/color_matching
>>>> */
>>>>
>>>> +static struct config_group *uvcg_color_matching_make(struct config_group *group,
>>>> + const char *name)
>>>> +{
>>>> + struct uvcg_cmd *cmd;
>>>> +
>>>> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>>>> + if (!cmd)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
>>>> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
>>>> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
>>>> +
>>>> + config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
>>>> +
>>>> + return &cmd->group;
>>>> +}
>>>> +
>>>> +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
>>>> + .make_group = uvcg_color_matching_make,
>>>> +};
>>>> +
>>>> static int uvcg_color_matching_create_children(struct config_group *parent)
>>>> {
>>>> struct uvcg_cmd *cmd;
>>>> @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
>>>> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
>>>> .type = {
>>>> .ct_item_ops = &uvcg_config_item_ops,
>>>> + .ct_group_ops = &uvcg_color_matching_grp_group_ops,
>>>> .ct_owner = THIS_MODULE,
>>>> },
>>>> .name = "color_matching",
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes
2022-12-18 23:29 ` Laurent Pinchart
@ 2022-12-19 9:53 ` Kieran Bingham
0 siblings, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2022-12-19 9:53 UTC (permalink / raw)
To: Daniel Scally, Laurent Pinchart
Cc: linux-usb, gregkh, w36195, m.grzeschik, torleiv
Quoting Laurent Pinchart (2022-12-18 23:29:22)
> Hi Dan,
>
> Thank you for the patch.
>
> On Tue, Dec 13, 2022 at 08:37:31AM +0000, Daniel Scally wrote:
> > Color matching attributes in the configfs for UVC are named with the
> > phrase "default". The implication of that is that they will only be
> > used _with_ the default color matching descriptor, and that will
> > shortly no longer be the case.
> >
> > Remove the "default" from the color matching descriptor attribute
> > variables.
> >
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Yup.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > drivers/usb/gadget/function/uvc_configfs.c | 29 +++++++++++-----------
> > 1 file changed, 14 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > index 4303a3283ba0..26d092790f12 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -1783,8 +1783,8 @@ static const struct uvcg_config_group_type uvcg_mjpeg_grp_type = {
> > * streaming/color_matching/default
> > */
> >
> > -#define UVCG_DEFAULT_COLOR_MATCHING_ATTR(cname, aname, bits) \
> > -static ssize_t uvcg_default_color_matching_##cname##_show( \
> > +#define UVCG_COLOR_MATCHING_ATTR(cname, aname, bits) \
> > +static ssize_t uvcg_color_matching_##cname##_show( \
> > struct config_item *item, char *page) \
> > { \
> > struct config_group *group = to_config_group(item); \
> > @@ -1808,26 +1808,25 @@ static ssize_t uvcg_default_color_matching_##cname##_show( \
> > return result; \
> > } \
> > \
> > -UVC_ATTR_RO(uvcg_default_color_matching_, cname, aname)
> > +UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
> >
> > -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> > -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_transfer_characteristics,
> > - bTransferCharacteristics, 8);
> > -UVCG_DEFAULT_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
> > +UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
> > +UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
> > +UVCG_COLOR_MATCHING_ATTR(b_matrix_coefficients, bMatrixCoefficients, 8);
> >
> > -#undef UVCG_DEFAULT_COLOR_MATCHING_ATTR
> > +#undef UVCG_COLOR_MATCHING_ATTR
> >
> > -static struct configfs_attribute *uvcg_default_color_matching_attrs[] = {
> > - &uvcg_default_color_matching_attr_b_color_primaries,
> > - &uvcg_default_color_matching_attr_b_transfer_characteristics,
> > - &uvcg_default_color_matching_attr_b_matrix_coefficients,
> > +static struct configfs_attribute *uvcg_color_matching_attrs[] = {
> > + &uvcg_color_matching_attr_b_color_primaries,
> > + &uvcg_color_matching_attr_b_transfer_characteristics,
> > + &uvcg_color_matching_attr_b_matrix_coefficients,
> > NULL,
> > };
> >
> > -static const struct uvcg_config_group_type uvcg_default_color_matching_type = {
> > +static const struct uvcg_config_group_type uvcg_color_matching_type = {
> > .type = {
> > .ct_item_ops = &uvcg_config_item_ops,
> > - .ct_attrs = uvcg_default_color_matching_attrs,
> > + .ct_attrs = uvcg_color_matching_attrs,
> > .ct_owner = THIS_MODULE,
> > },
> > .name = "default",
> > @@ -1844,7 +1843,7 @@ static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> > },
> > .name = "color_matching",
> > .children = (const struct uvcg_config_group_type*[]) {
> > - &uvcg_default_color_matching_type,
> > + &uvcg_color_matching_type,
> > NULL,
> > },
> > };
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame
2022-12-18 23:28 ` Laurent Pinchart
@ 2022-12-19 10:33 ` Dan Scally
2022-12-19 15:52 ` Laurent Pinchart
0 siblings, 1 reply; 29+ messages in thread
From: Dan Scally @ 2022-12-19 10:33 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Laurent
On 18/12/2022 23:28, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
>> As currently implemented the default color matching descriptor is
>> appended after _all_ the formats and frames that the gadget is
>> configured with. According to the UVC specifications however this
>> is supposed to be on a per-format basis (section 3.9.2.6):
>>
>> "Only one instance is allowed for a given format and if present,
>> the Color Matching descriptor shall be placed following the Video
>> and Still Image Frame descriptors for that format."
>>
>> Associate the default color matching descriptor with struct
>> uvcg_format and copy it once-per-format instead of once only.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
>> drivers/usb/gadget/function/uvc_configfs.h | 1 +
>> 2 files changed, 58 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 9918e7b6a023..6f5932c9f09c 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
>> "mjpeg",
>> };
>>
>> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
>> +{
>> + struct config_item *color_matching, *cm_default;
>> + struct uvcg_cmd *cmd;
>> +
>> + color_matching = config_group_find_item(to_config_group(streaming),
>> + "color_matching");
>> + if (!color_matching)
>> + return NULL;
>> +
>> + cm_default = config_group_find_item(to_config_group(color_matching),
>> + "default");
>> + config_item_put(color_matching);
>> + if (!cm_default)
>> + return NULL;
>> +
>> + cmd = to_uvcg_cmd(to_config_group(cm_default));
>> + config_item_put(cm_default);
>> +
>> + return cmd;
>> +}
>> +
>> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>> {
>> struct f_uvc_opts *opts;
>> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>> 'Y', 'U', 'Y', '2', 0x00, 0x00, 0x10, 0x00,
>> 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
>> };
>> + struct config_item *streaming;
>> struct uvcg_uncompressed *h;
>> + struct uvcg_cmd *cmd;
>> +
>> + streaming = group->cg_item.ci_parent;
>> + cmd = uvcg_format_get_default_cmd(streaming);
>> + if (!cmd)
>> + return ERR_PTR(-EINVAL);
>>
>> h = kzalloc(sizeof(*h), GFP_KERNEL);
>> if (!h)
>> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>>
>> INIT_LIST_HEAD(&h->fmt.frames);
>> h->fmt.type = UVCG_UNCOMPRESSED;
>> + h->fmt.color_matching = cmd;
> Is there any need to reference-count cmd to make sure it doesn't get
> deleted ? I don't expect that to be an issue for the default, but for
> the user-defined color matching descriptors. This may be better
> discussed in the context of other patches further in the series.
configfs_symlink() holds references to the targets, so you can't remove
the color matching descriptors without first removing the symlink to them.
>
>> config_group_init_type_name(&h->fmt.group, name,
>> &uvcg_uncompressed_type);
>>
>> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
>> static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>> const char *name)
>> {
>> + struct config_item *streaming;
>> struct uvcg_mjpeg *h;
>> + struct uvcg_cmd *cmd;
>> +
>> + streaming = group->cg_item.ci_parent;
>> + cmd = uvcg_format_get_default_cmd(streaming);
>> + if (!cmd)
>> + return ERR_PTR(-EINVAL);
>>
>> h = kzalloc(sizeof(*h), GFP_KERNEL);
>> if (!h)
>> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>>
>> INIT_LIST_HEAD(&h->fmt.frames);
>> h->fmt.type = UVCG_MJPEG;
>> + h->fmt.color_matching = cmd;
>> config_group_init_type_name(&h->fmt.group, name,
>> &uvcg_mjpeg_type);
>>
>> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
>> enum uvcg_strm_type {
>> UVCG_HEADER = 0,
>> UVCG_FORMAT,
>> - UVCG_FRAME
>> + UVCG_FRAME,
>> + UVCG_CMD,
>> };
>>
>> /*
>> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
>> if (ret)
>> return ret;
>> }
>> +
>> + ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
>> + if (ret)
>> + return ret;
>> }
>>
>> return ret;
>> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
>> *size += frm->frame.b_frame_interval_type * sz;
>> }
>> break;
>> + case UVCG_CMD: {
>> + struct uvcg_cmd *cmd = priv1;
> Missing blank line. Same below.
Sorry; where below?
>
>> + *size += sizeof(cmd->desc);
>> + }
>> + break;
>> }
>>
>> ++*count;
>> @@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
>> frm->frame.b_frame_interval_type);
>> }
>> break;
>> + case UVCG_CMD: {
>> + struct uvcg_cmd *cmd = priv1;
>> +
>> + memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
>> + *dest += sizeof(cmd->desc);
>> + }
>> + break;
>> }
>>
>> return 0;
>> @@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>> if (ret)
>> goto unlock;
>>
>> - count += 2; /* color_matching, NULL */
>> + count += 1; /* NULL */
>> *class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
>> if (!*class_array) {
>> ret = -ENOMEM;
>> @@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>> kfree(data_save);
>> goto unlock;
>> }
>> - *cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
>>
>> ++target_hdr->linked;
>> ret = 0;
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
>> index f990739838d5..6582d6c7b6f6 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.h
>> +++ b/drivers/usb/gadget/function/uvc_configfs.h
>> @@ -57,6 +57,7 @@ struct uvcg_format {
>> struct list_head frames;
>> unsigned num_frames;
>> __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>> + struct uvcg_cmd *color_matching;
>> };
>>
>> struct uvcg_format_ptr {
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame
2022-12-19 10:33 ` Dan Scally
@ 2022-12-19 15:52 ` Laurent Pinchart
0 siblings, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-19 15:52 UTC (permalink / raw)
To: Dan Scally
Cc: linux-usb, gregkh, w36195, m.grzeschik, kieran.bingham, torleiv
Hi Dan,
On Mon, Dec 19, 2022 at 10:33:39AM +0000, Dan Scally wrote:
> On 18/12/2022 23:28, Laurent Pinchart wrote:
> > On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
> >> As currently implemented the default color matching descriptor is
> >> appended after _all_ the formats and frames that the gadget is
> >> configured with. According to the UVC specifications however this
> >> is supposed to be on a per-format basis (section 3.9.2.6):
> >>
> >> "Only one instance is allowed for a given format and if present,
> >> the Color Matching descriptor shall be placed following the Video
> >> and Still Image Frame descriptors for that format."
> >>
> >> Associate the default color matching descriptor with struct
> >> uvcg_format and copy it once-per-format instead of once only.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >> drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
> >> drivers/usb/gadget/function/uvc_configfs.h | 1 +
> >> 2 files changed, 58 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index 9918e7b6a023..6f5932c9f09c 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
> >> "mjpeg",
> >> };
> >>
> >> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
> >> +{
> >> + struct config_item *color_matching, *cm_default;
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + color_matching = config_group_find_item(to_config_group(streaming),
> >> + "color_matching");
> >> + if (!color_matching)
> >> + return NULL;
> >> +
> >> + cm_default = config_group_find_item(to_config_group(color_matching),
> >> + "default");
> >> + config_item_put(color_matching);
> >> + if (!cm_default)
> >> + return NULL;
> >> +
> >> + cmd = to_uvcg_cmd(to_config_group(cm_default));
> >> + config_item_put(cm_default);
> >> +
> >> + return cmd;
> >> +}
> >> +
> >> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> >> {
> >> struct f_uvc_opts *opts;
> >> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> >> 'Y', 'U', 'Y', '2', 0x00, 0x00, 0x10, 0x00,
> >> 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
> >> };
> >> + struct config_item *streaming;
> >> struct uvcg_uncompressed *h;
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + streaming = group->cg_item.ci_parent;
> >> + cmd = uvcg_format_get_default_cmd(streaming);
> >> + if (!cmd)
> >> + return ERR_PTR(-EINVAL);
> >>
> >> h = kzalloc(sizeof(*h), GFP_KERNEL);
> >> if (!h)
> >> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> >>
> >> INIT_LIST_HEAD(&h->fmt.frames);
> >> h->fmt.type = UVCG_UNCOMPRESSED;
> >> + h->fmt.color_matching = cmd;
> >
> > Is there any need to reference-count cmd to make sure it doesn't get
> > deleted ? I don't expect that to be an issue for the default, but for
> > the user-defined color matching descriptors. This may be better
> > discussed in the context of other patches further in the series.
>
> configfs_symlink() holds references to the targets, so you can't remove
> the color matching descriptors without first removing the symlink to them.
>
> >> config_group_init_type_name(&h->fmt.group, name,
> >> &uvcg_uncompressed_type);
> >>
> >> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
> >> static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> >> const char *name)
> >> {
> >> + struct config_item *streaming;
> >> struct uvcg_mjpeg *h;
> >> + struct uvcg_cmd *cmd;
> >> +
> >> + streaming = group->cg_item.ci_parent;
> >> + cmd = uvcg_format_get_default_cmd(streaming);
> >> + if (!cmd)
> >> + return ERR_PTR(-EINVAL);
> >>
> >> h = kzalloc(sizeof(*h), GFP_KERNEL);
> >> if (!h)
> >> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> >>
> >> INIT_LIST_HEAD(&h->fmt.frames);
> >> h->fmt.type = UVCG_MJPEG;
> >> + h->fmt.color_matching = cmd;
> >> config_group_init_type_name(&h->fmt.group, name,
> >> &uvcg_mjpeg_type);
> >>
> >> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
> >> enum uvcg_strm_type {
> >> UVCG_HEADER = 0,
> >> UVCG_FORMAT,
> >> - UVCG_FRAME
> >> + UVCG_FRAME,
> >> + UVCG_CMD,
> >> };
> >>
> >> /*
> >> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
> >> if (ret)
> >> return ret;
> >> }
> >> +
> >> + ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
> >> + if (ret)
> >> + return ret;
> >> }
> >>
> >> return ret;
> >> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
> >> *size += frm->frame.b_frame_interval_type * sz;
> >> }
> >> break;
> >> + case UVCG_CMD: {
> >> + struct uvcg_cmd *cmd = priv1;
> >
> > Missing blank line. Same below.
>
> Sorry; where below?
Nowhere, my bad :-)
> >> + *size += sizeof(cmd->desc);
> >> + }
> >> + break;
> >> }
> >>
> >> ++*count;
> >> @@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
> >> frm->frame.b_frame_interval_type);
> >> }
> >> break;
> >> + case UVCG_CMD: {
> >> + struct uvcg_cmd *cmd = priv1;
> >> +
> >> + memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
> >> + *dest += sizeof(cmd->desc);
> >> + }
> >> + break;
> >> }
> >>
> >> return 0;
> >> @@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
> >> if (ret)
> >> goto unlock;
> >>
> >> - count += 2; /* color_matching, NULL */
> >> + count += 1; /* NULL */
> >> *class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
> >> if (!*class_array) {
> >> ret = -ENOMEM;
> >> @@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
> >> kfree(data_save);
> >> goto unlock;
> >> }
> >> - *cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
> >>
> >> ++target_hdr->linked;
> >> ret = 0;
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> >> index f990739838d5..6582d6c7b6f6 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.h
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> >> @@ -57,6 +57,7 @@ struct uvcg_format {
> >> struct list_head frames;
> >> unsigned num_frames;
> >> __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> >> + struct uvcg_cmd *color_matching;
> >> };
> >>
> >> struct uvcg_format_ptr {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors
2022-12-19 9:44 ` Dan Scally
@ 2022-12-19 16:05 ` Laurent Pinchart
0 siblings, 0 replies; 29+ messages in thread
From: Laurent Pinchart @ 2022-12-19 16:05 UTC (permalink / raw)
To: Dan Scally
Cc: Kieran Bingham, linux-usb, gregkh, w36195, m.grzeschik, torleiv
Hi Dan,
On Mon, Dec 19, 2022 at 09:44:53AM +0000, Dan Scally wrote:
> On 18/12/2022 23:17, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 12:03:03PM +0000, Dan Scally wrote:
> >> On 15/12/2022 12:00, Kieran Bingham wrote:
> >>> Quoting Daniel Scally (2022-12-13 08:37:36)
> >>>> Allow users to create new color matching descriptors in addition to
> >>>> the default one. These must be associated with a UVC format in order
> >>>> to be transmitted to the host, which is achieved by symlinking from
> >>>> the format to the newly created color matching descriptor - extend
> >>>> the uncompressed and mjpeg formats to support that linking operation.
> >>>>
> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >>>> ---
> >>>> .../ABI/testing/configfs-usb-gadget-uvc | 4 +-
> >>>> drivers/usb/gadget/function/uvc_configfs.c | 79 ++++++++++++++++++-
> >>>> 2 files changed, 79 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>>> index 3512f4899fe3..ce629f0880a9 100644
> >>>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>>> @@ -160,10 +160,10 @@ Date: Dec 2014
> >>>> KernelVersion: 4.0
> >>>> Description: Color matching descriptors
> >>>>
> >>>> -What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/default
> >>>> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> >>>
> >>> Hrm... do we need to keep 'default' (and keep it available in patch
> >>> 1/6?) so that the default is still kept accesible?
> >>
> >> It's still there - it's just no longer necessarily the only entry
> >>
> >>> That would mean others are added as siblings to the default, and then
> >>> the one that gets linked is the one that is used perhaps?
> >>
> >> Yep that's how it's implemented
> >>
> >>> Feels like that would overcomplicate 'the default case' maybe ... but
> >>> I'm weary that this is 'removing' the default from ABI...?
> >>>
> >>>> Date: Dec 2014
> >>>> KernelVersion: 4.0
> >>>
> >>> Hrm ... and this would leave the documentation stating that you could
> >>> provide custom color matching descriptors from v4.0 onwards, which would
> >>> be inaccurate ?
> >>
> >> Ah, good point...fair enough, I'll add a new entry for the custom named
> >> ones.
> >>
> >>> So I'm not sure we can just rename this documentation segment eitherway.
> >>>
> >>>> -Description: Default color matching descriptors
> >>>> +Description: color matching descriptors
> >>>>
> >>>> All attributes read/write:
> >>>>
> >>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >>>> index 4fbc42d738a4..82c10f0dab71 100644
> >>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >>>> @@ -769,6 +769,58 @@ static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streamin
> >>>> return cmd;
> >>>> }
> >>>>
> >>>> +static int uvcg_format_allow_link(struct config_item *src, struct config_item *tgt)
> >>>> +{
> >>>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >>>> + struct config_item *streaming, *color_matching;
> >>>> + struct uvcg_cmd *color_matching_desc;
> >>>> + struct uvcg_format *fmt;
> >>>> + int ret = 0;
> >>>> +
> >>>> + mutex_lock(su_mutex);
> >>>> +
> >>>> + streaming = src->ci_parent->ci_parent;
> >>>> + color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
> >>>> + if (!color_matching || color_matching != tgt->ci_parent) {
> >>>> + ret = -EINVAL;
> >>>> + goto out_put_cm;
> >>>> + }
> >>>> +
> >>>> + color_matching_desc = to_uvcg_cmd(to_config_group(tgt));
> >>>> + fmt = to_uvcg_format(src);
> >>>> + fmt->color_matching = color_matching_desc;
> >
> > As you store a pointer to the target, don't you need to keep a reference
> > to it somehow, to avoid the target being deleted ? Or is that handled by
> > configfs itself ?
>
> configfs is doing it in configfs_symlink()
>
> > It also looks like you need to refcount the number of links to the
> > target, to disallow changes to the color matching attributes when the
> > descriptor is linked.
>
> Yes; I had thought that that was unnecessary so skipped it, but turns
> out I'm wrong - I'll add that in.
>
> > This being said, are links the best option here ? Why can't we create
> > color matching descriptors as children of format descriptors the same
> > way that frame descriptors are handled ?
>
> We can do it that way, but I thought that it made more sense to expand
> the current implementation because
>
>
> 1) I'm reluctant to get rid of the default descriptor and I wouldn't
> want to have them in multiple places.
>
> 2) We're planning to expand the uncompressed format to support more than
> just yuy2, which for some cameras might result in _a lot_ of available
> formats (vivid has 80!). It seems easier to have color matching
> descriptors that you can set once and simply link to than have to
> recreate them for each format.
The first point doesn't convince me, as I think we should remove the
default. As the UVC specification doesn't mandate a color matching
descriptor, I think it should be left to the system integrator whether
to include one or not.
I had considered the second point as well. In practice I expect we'll
have few formats, but still more than one. I also expect them to have
identical color matching data. Links could avoid recreating the same
descriptors, but the gain isn't very big. Consider the format
descriptors as well, we could have identifical format descriptors for
different formats, and we don't use links.
I thus still think links are overkill, but if the general consensus is
that they're better, I'm fine with that.
> >>>> +
> >>>> +out_put_cm:
> >>>> + config_item_put(color_matching);
> >>>> + mutex_unlock(su_mutex);
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static void uvcg_format_drop_link(struct config_item *src, struct config_item *tgt)
> >>>> +{
> >>>> + struct config_item *streaming;
> >>>> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >>>> + struct uvcg_format *fmt;
> >>>> + struct uvcg_cmd *cmd;
> >>>> +
> >>>> + mutex_lock(su_mutex);
> >>>> +
> >>>> + streaming = src->ci_parent->ci_parent;
> >>>> + cmd = uvcg_format_get_default_cmd(streaming);
> >>>> +
> >>>> + fmt = to_uvcg_format(src);
> >>>> + fmt->color_matching = cmd;
> >>>> +
> >>>> + mutex_unlock(su_mutex);
> >>>> +}
> >>>> +
> >>>> +static struct configfs_item_operations uvcg_format_item_operations = {
> >
> > Not a candidate for this patch (or this series), but I wonder if this
> > could be made const. It would involve making several pointers in struct
> > config_item_type const, which may or may not be straightforward.
> >
> >>>> + .release = uvcg_config_item_release,
> >>>> + .allow_link = uvcg_format_allow_link,
> >>>> + .drop_link = uvcg_format_drop_link,
> >>>> +};
> >>>> +
> >>>> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> >>>> {
> >>>> struct f_uvc_opts *opts;
> >>>> @@ -1569,7 +1621,7 @@ static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
> >>>> };
> >>>>
> >>>> static const struct config_item_type uvcg_uncompressed_type = {
> >>>> - .ct_item_ops = &uvcg_config_item_ops,
> >>>> + .ct_item_ops = &uvcg_format_item_operations,
> >>>> .ct_group_ops = &uvcg_uncompressed_group_ops,
> >>>> .ct_attrs = uvcg_uncompressed_attrs,
> >>>> .ct_owner = THIS_MODULE,
> >>>> @@ -1764,7 +1816,7 @@ static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
> >>>> };
> >>>>
> >>>> static const struct config_item_type uvcg_mjpeg_type = {
> >>>> - .ct_item_ops = &uvcg_config_item_ops,
> >>>> + .ct_item_ops = &uvcg_format_item_operations,
> >>>> .ct_group_ops = &uvcg_mjpeg_group_ops,
> >>>> .ct_attrs = uvcg_mjpeg_attrs,
> >>>> .ct_owner = THIS_MODULE,
> >>>> @@ -1912,6 +1964,28 @@ static const struct config_item_type uvcg_color_matching_type = {
> >>>> * streaming/color_matching
> >>>> */
> >>>>
> >>>> +static struct config_group *uvcg_color_matching_make(struct config_group *group,
> >>>> + const char *name)
> >>>> +{
> >>>> + struct uvcg_cmd *cmd;
> >>>> +
> >>>> + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> >>>> + if (!cmd)
> >>>> + return ERR_PTR(-ENOMEM);
> >>>> +
> >>>> + cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> >>>> + cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> >>>> + cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> >>>> +
> >>>> + config_group_init_type_name(&cmd->group, name, &uvcg_color_matching_type);
> >>>> +
> >>>> + return &cmd->group;
> >>>> +}
> >>>> +
> >>>> +static struct configfs_group_operations uvcg_color_matching_grp_group_ops = {
> >>>> + .make_group = uvcg_color_matching_make,
> >>>> +};
> >>>> +
> >>>> static int uvcg_color_matching_create_children(struct config_group *parent)
> >>>> {
> >>>> struct uvcg_cmd *cmd;
> >>>> @@ -1937,6 +2011,7 @@ static int uvcg_color_matching_create_children(struct config_group *parent)
> >>>> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> >>>> .type = {
> >>>> .ct_item_ops = &uvcg_config_item_ops,
> >>>> + .ct_group_ops = &uvcg_color_matching_grp_group_ops,
> >>>> .ct_owner = THIS_MODULE,
> >>>> },
> >>>> .name = "color_matching",
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2022-12-19 16:06 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 8:37 [PATCH 0/6] UVC Gadget: Extend color matching support Daniel Scally
2022-12-13 8:37 ` [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes Daniel Scally
2022-12-18 23:29 ` Laurent Pinchart
2022-12-19 9:53 ` Kieran Bingham
2022-12-13 8:37 ` [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs Daniel Scally
2022-12-15 11:45 ` Kieran Bingham
2022-12-16 14:06 ` Dan Scally
2022-12-18 23:28 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame Daniel Scally
2022-12-18 23:28 ` Laurent Pinchart
2022-12-19 10:33 ` Dan Scally
2022-12-19 15:52 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching Daniel Scally
2022-12-15 11:48 ` Kieran Bingham
2022-12-16 15:32 ` Dan Scally
2022-12-18 22:52 ` Laurent Pinchart
2022-12-13 8:37 ` [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write Daniel Scally
2022-12-15 11:51 ` Kieran Bingham
2022-12-16 15:53 ` Dan Scally
2022-12-18 23:04 ` Laurent Pinchart
2022-12-19 9:21 ` Dan Scally
2022-12-13 8:37 ` [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors Daniel Scally
2022-12-15 12:00 ` Kieran Bingham
2022-12-15 12:03 ` Dan Scally
2022-12-18 23:17 ` Laurent Pinchart
2022-12-19 9:44 ` Dan Scally
2022-12-19 16:05 ` Laurent Pinchart
2022-12-18 18:12 ` [PATCH 0/6] UVC Gadget: Extend color matching support Laurent Pinchart
2022-12-19 7:30 ` Dan Scally
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.