[PATCH 0/6] UVC Gadget: Extend color matching support

[PATCH 0/6] UVC Gadget: Extend color matching support

[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

[PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes

[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

Re: [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes

[Laurent Pinchart]

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

Re: [PATCH 1/6] usb: gadget: usb: Remove "default" from color matching attributes

[Kieran Bingham]

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

[PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs

[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

Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs

[Kieran Bingham]

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
>

Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs

[Dan Scally]

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
>>

Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs

[Laurent Pinchart]

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

[PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame

[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

Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame

[Laurent Pinchart]

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

Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame

[Dan Scally]

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 {

Re: [PATCH 3/6] usb: gadget: uvc: Copy color matching descriptor for each frame

[Laurent Pinchart]

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

[PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching

[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

Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching

[Kieran Bingham]

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
>

Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching

[Dan Scally]

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
>>

Re: [PATCH 4/6] usb: gadget: uvc: Remove the hardcoded default color matching

[Laurent Pinchart]

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

[PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[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

Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Kieran Bingham]

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
>

Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Dan Scally]

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
>>

Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Laurent Pinchart]

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

Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Dan Scally]

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);

[PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[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

Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Kieran Bingham]

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
>

Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Dan Scally]

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
>>

Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Laurent Pinchart]

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

Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Dan Scally]

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",

Re: [PATCH 6/6] usb: gadget: uvc: Allow creating new color matching descriptors

[Laurent Pinchart]

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

Re: [PATCH 0/6] UVC Gadget: Extend color matching support

[Laurent Pinchart]

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

Re: [PATCH 0/6] UVC Gadget: Extend color matching support

[Dan Scally]

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(-)