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

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

[Daniel Scally]

The current UVC gadget implementation hardcodes a single color matching
descriptor and includes it in the payload of USB descriptors 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 included 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 (7):
  usb: gadget: usb: Remove "default" from color matching attributes
  usb: uvc: Enumerate valid values for color matching
  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       |  19 +-
 drivers/usb/gadget/function/f_uvc.c           |   9 -
 drivers/usb/gadget/function/u_uvc.h           |   1 -
 drivers/usb/gadget/function/uvc_configfs.c    | 278 ++++++++++++++++--
 drivers/usb/gadget/function/uvc_configfs.h    |  22 +-
 include/uapi/linux/usb/video.h                |  30 ++
 6 files changed, 311 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- None
 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 76cb60d13049..e28becd435bf 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

[PATCH v2 2/7] usb: uvc: Enumerate valid values for color matching

[Daniel Scally]

The color matching descriptors defined in the UVC 1.5 Specification
contain 3 fields with discrete numeric values representing particular
settings. Enumerate those values so that later code setting them can
be more readable.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

  - New patch

 include/uapi/linux/usb/video.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index 6e8e572c2980..08606a52e1e2 100644
--- a/include/uapi/linux/usb/video.h
+++ b/include/uapi/linux/usb/video.h
@@ -179,6 +179,36 @@
 #define UVC_CONTROL_CAP_AUTOUPDATE			(1 << 3)
 #define UVC_CONTROL_CAP_ASYNCHRONOUS			(1 << 4)
 
+/* ref Color Matching Descriptor Values */
+enum uvc_color_primaries_values {
+	UVC_COLOR_PRIMARIES_UNSPECIFIED,
+	UVC_COLOR_PRIMARIES_BT_709_SRGB,
+	UVC_COLOR_PRIMARIES_BT_470_2_M,
+	UVC_COLOR_PRIMARIES_BT_470_2_B_G,
+	UVC_COLOR_PRIMARIES_SMPTE_170M,
+	UVC_COLOR_PRIMARIES_SMPTE_240M,
+};
+
+enum uvc_transfer_characteristics_values {
+	UVC_TRANSFER_CHARACTERISTICS_UNSPECIFIED,
+	UVC_TRANSFER_CHARACTERISTICS_BT_709,
+	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_M,
+	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_B_G,
+	UVC_TRANSFER_CHARACTERISTICS_SMPTE_170M,
+	UVC_TRANSFER_CHARACTERISTICS_SMPTE_240M,
+	UVC_TRANSFER_CHARACTERISTICS_LINEAR,
+	UVC_TRANSFER_CHARACTERISTICS_SRGB,
+};
+
+enum uvc_matrix_coefficients {
+	UVC_MATRIX_COEFFICIENTS_UNSPECIFIED,
+	UVC_MATRIX_COEFFICIENTS_BT_709,
+	UVC_MATRIX_COEFFICIENTS_FCC,
+	UVC_MATRIX_COEFFICIENTS_BT_470_2_B_G,
+	UVC_MATRIX_COEFFICIENTS_SMPTE_170M,
+	UVC_MATRIX_COEFFICIENTS_SMPTE_240M,
+};
+
 /* ------------------------------------------------------------------------
  * UVC structures
  */
-- 
2.34.1

Re: [PATCH v2 2/7] usb: uvc: Enumerate valid values for color matching

[Dan Scally]

On 19/12/2022 14:43, Daniel Scally wrote:
> The color matching descriptors defined in the UVC 1.5 Specification
> contain 3 fields with discrete numeric values representing particular
> settings. Enumerate those values so that later code setting them can
> be more readable.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
>
>    - New patch
>
>   include/uapi/linux/usb/video.h | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index 6e8e572c2980..08606a52e1e2 100644
> --- a/include/uapi/linux/usb/video.h
> +++ b/include/uapi/linux/usb/video.h
> @@ -179,6 +179,36 @@
>   #define UVC_CONTROL_CAP_AUTOUPDATE			(1 << 3)
>   #define UVC_CONTROL_CAP_ASYNCHRONOUS			(1 << 4)
>   
> +/* ref Color Matching Descriptor Values */


_Immediately_ noticed that I forgot to replace the placeholder with the 
actual reference to the document - sorry...I'll fix that in the v3

> +enum uvc_color_primaries_values {
> +	UVC_COLOR_PRIMARIES_UNSPECIFIED,
> +	UVC_COLOR_PRIMARIES_BT_709_SRGB,
> +	UVC_COLOR_PRIMARIES_BT_470_2_M,
> +	UVC_COLOR_PRIMARIES_BT_470_2_B_G,
> +	UVC_COLOR_PRIMARIES_SMPTE_170M,
> +	UVC_COLOR_PRIMARIES_SMPTE_240M,
> +};
> +
> +enum uvc_transfer_characteristics_values {
> +	UVC_TRANSFER_CHARACTERISTICS_UNSPECIFIED,
> +	UVC_TRANSFER_CHARACTERISTICS_BT_709,
> +	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_M,
> +	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_B_G,
> +	UVC_TRANSFER_CHARACTERISTICS_SMPTE_170M,
> +	UVC_TRANSFER_CHARACTERISTICS_SMPTE_240M,
> +	UVC_TRANSFER_CHARACTERISTICS_LINEAR,
> +	UVC_TRANSFER_CHARACTERISTICS_SRGB,
> +};
> +
> +enum uvc_matrix_coefficients {
> +	UVC_MATRIX_COEFFICIENTS_UNSPECIFIED,
> +	UVC_MATRIX_COEFFICIENTS_BT_709,
> +	UVC_MATRIX_COEFFICIENTS_FCC,
> +	UVC_MATRIX_COEFFICIENTS_BT_470_2_B_G,
> +	UVC_MATRIX_COEFFICIENTS_SMPTE_170M,
> +	UVC_MATRIX_COEFFICIENTS_SMPTE_240M,
> +};
> +
>   /* ------------------------------------------------------------------------
>    * UVC structures
>    */

Re: [PATCH v2 2/7] usb: uvc: Enumerate valid values for color matching

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Dec 19, 2022 at 02:45:07PM +0000, Dan Scally wrote:
> On 19/12/2022 14:43, Daniel Scally wrote:
> > The color matching descriptors defined in the UVC 1.5 Specification

Not just UVC 1.5, but also UVC 1.1. I would just say "UVC" gere.

> > contain 3 fields with discrete numeric values representing particular
> > settings. Enumerate those values so that later code setting them can
> > be more readable.
> >
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> >
> >    - New patch
> >
> >   include/uapi/linux/usb/video.h | 30 ++++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> > index 6e8e572c2980..08606a52e1e2 100644
> > --- a/include/uapi/linux/usb/video.h
> > +++ b/include/uapi/linux/usb/video.h
> > @@ -179,6 +179,36 @@
> >   #define UVC_CONTROL_CAP_AUTOUPDATE			(1 << 3)
> >   #define UVC_CONTROL_CAP_ASYNCHRONOUS			(1 << 4)
> >   
> > +/* ref Color Matching Descriptor Values */
> 
> _Immediately_ noticed that I forgot to replace the placeholder with the 
> actual reference to the document - sorry...I'll fix that in the v3

Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

with this fixed.

> > +enum uvc_color_primaries_values {
> > +	UVC_COLOR_PRIMARIES_UNSPECIFIED,
> > +	UVC_COLOR_PRIMARIES_BT_709_SRGB,
> > +	UVC_COLOR_PRIMARIES_BT_470_2_M,
> > +	UVC_COLOR_PRIMARIES_BT_470_2_B_G,
> > +	UVC_COLOR_PRIMARIES_SMPTE_170M,
> > +	UVC_COLOR_PRIMARIES_SMPTE_240M,
> > +};
> > +
> > +enum uvc_transfer_characteristics_values {
> > +	UVC_TRANSFER_CHARACTERISTICS_UNSPECIFIED,
> > +	UVC_TRANSFER_CHARACTERISTICS_BT_709,
> > +	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_M,
> > +	UVC_TRANSFER_CHARACTERISTICS_BT_470_2_B_G,
> > +	UVC_TRANSFER_CHARACTERISTICS_SMPTE_170M,
> > +	UVC_TRANSFER_CHARACTERISTICS_SMPTE_240M,
> > +	UVC_TRANSFER_CHARACTERISTICS_LINEAR,
> > +	UVC_TRANSFER_CHARACTERISTICS_SRGB,
> > +};
> > +
> > +enum uvc_matrix_coefficients {
> > +	UVC_MATRIX_COEFFICIENTS_UNSPECIFIED,
> > +	UVC_MATRIX_COEFFICIENTS_BT_709,
> > +	UVC_MATRIX_COEFFICIENTS_FCC,
> > +	UVC_MATRIX_COEFFICIENTS_BT_470_2_B_G,
> > +	UVC_MATRIX_COEFFICIENTS_SMPTE_170M,
> > +	UVC_MATRIX_COEFFICIENTS_SMPTE_240M,
> > +};
> > +
> >   /* ------------------------------------------------------------------------
> >    * UVC structures
> >    */

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/7] 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_color_matching. 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>
---
Changes in v2:

	- Renamed uvcg_cmd to uvcg_color_matching plus the associated
	  variables (Kieran, Laurent)
	- Added a refcnt member to struct uvcg_color_matching

 drivers/usb/gadget/function/uvc_configfs.c | 58 ++++++++++++++++------
 drivers/usb/gadget/function/uvc_configfs.h |  9 ++++
 2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index e28becd435bf..147d3def24dd 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -13,6 +13,7 @@
 #include "uvc_configfs.h"
 
 #include <linux/sort.h>
+#include <uapi/linux/usb/video.h>
 
 /* -----------------------------------------------------------------------------
  * Global Utility Structures and Macros
@@ -1788,20 +1789,21 @@ static ssize_t uvcg_color_matching_##cname##_show(			\
 	struct config_item *item, char *page)				\
 {									\
 	struct config_group *group = to_config_group(item);		\
+	struct uvcg_color_matching *color_match =			\
+		to_uvcg_color_matching(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(color_match->desc.aname));	\
 	mutex_unlock(&opts->lock);					\
 									\
 	mutex_unlock(su_mutex);						\
@@ -1823,29 +1825,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_color_matching *color_match =
+		to_uvcg_color_matching(to_config_group(item));
+
+	kfree(color_match);
+}
+
+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_color_matching *color_match;
+
+	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
+	if (!color_match)
+		return -ENOMEM;
+
+	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
+	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
+	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
+	color_match->desc.bColorPrimaries = UVC_COLOR_PRIMARIES_BT_709_SRGB;
+	color_match->desc.bTransferCharacteristics = UVC_TRANSFER_CHARACTERISTICS_BT_709;
+	color_match->desc.bMatrixCoefficients = UVC_MATRIX_COEFFICIENTS_SMPTE_170M;
+
+	config_group_init_type_name(&color_match->group, "default",
+				    &uvcg_color_matching_type);
+	configfs_add_default_group(&color_match->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..c7392c9b840e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -37,6 +37,15 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
 	return container_of(item, struct uvcg_control_header, item);
 }
 
+struct uvcg_color_matching {
+	struct config_group group;
+	struct uvc_color_matching_descriptor desc;
+	unsigned int refcnt;
+};
+
+#define to_uvcg_color_matching(group_ptr) \
+container_of(group_ptr, struct uvcg_color_matching, group)
+
 enum uvcg_format_type {
 	UVCG_UNCOMPRESSED = 0,
 	UVCG_MJPEG,
-- 
2.34.1

Re: [PATCH v2 3/7] usb: gadget: uvc: Add struct for color matching in configs

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Dec 19, 2022 at 02:43:12PM +0000, Daniel Scally wrote:
> 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_color_matching. 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>
> ---
> Changes in v2:
> 
> 	- Renamed uvcg_cmd to uvcg_color_matching plus the associated
> 	  variables (Kieran, Laurent)
> 	- Added a refcnt member to struct uvcg_color_matching
> 
>  drivers/usb/gadget/function/uvc_configfs.c | 58 ++++++++++++++++------
>  drivers/usb/gadget/function/uvc_configfs.h |  9 ++++
>  2 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index e28becd435bf..147d3def24dd 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -13,6 +13,7 @@
>  #include "uvc_configfs.h"
>  
>  #include <linux/sort.h>
> +#include <uapi/linux/usb/video.h>

Drop "uapi/", the kernel should handle that automatically.

>  
>  /* -----------------------------------------------------------------------------
>   * Global Utility Structures and Macros
> @@ -1788,20 +1789,21 @@ static ssize_t uvcg_color_matching_##cname##_show(			\
>  	struct config_item *item, char *page)				\
>  {									\
>  	struct config_group *group = to_config_group(item);		\
> +	struct uvcg_color_matching *color_match =			\
> +		to_uvcg_color_matching(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(color_match->desc.aname));	\
>  	mutex_unlock(&opts->lock);					\
>  									\
>  	mutex_unlock(su_mutex);						\
> @@ -1823,29 +1825,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_color_matching *color_match =
> +		to_uvcg_color_matching(to_config_group(item));
> +
> +	kfree(color_match);
> +}
> +
> +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_color_matching *color_match;
> +
> +	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
> +	if (!color_match)
> +		return -ENOMEM;
> +
> +	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> +	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> +	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> +	color_match->desc.bColorPrimaries = UVC_COLOR_PRIMARIES_BT_709_SRGB;
> +	color_match->desc.bTransferCharacteristics = UVC_TRANSFER_CHARACTERISTICS_BT_709;
> +	color_match->desc.bMatrixCoefficients = UVC_MATRIX_COEFFICIENTS_SMPTE_170M;
> +
> +	config_group_init_type_name(&color_match->group, "default",
> +				    &uvcg_color_matching_type);
> +	configfs_add_default_group(&color_match->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..c7392c9b840e 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -37,6 +37,15 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
>  	return container_of(item, struct uvcg_control_header, item);
>  }
>  
> +struct uvcg_color_matching {
> +	struct config_group group;
> +	struct uvc_color_matching_descriptor desc;
> +	unsigned int refcnt;

As refcnt isn't used in this patch, I would add it to the structure in
the patch that uses it.

Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

with these small issues fixed.

> +};
> +
> +#define to_uvcg_color_matching(group_ptr) \
> +container_of(group_ptr, struct uvcg_color_matching, group)
> +
>  enum uvcg_format_type {
>  	UVCG_UNCOMPRESSED = 0,
>  	UVCG_MJPEG,

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/7] 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>
---
Changes in v2:

	- Renamed uvcg_cmd and associated variables.
	- Formatting
	- Increased the refcnt variable for the color matching struct in
	the format make() functions

 drivers/usb/gadget/function/uvc_configfs.c | 64 +++++++++++++++++++++-
 drivers/usb/gadget/function/uvc_configfs.h | 13 +++--
 2 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 147d3def24dd..6fb7ac8366fe 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -748,6 +748,29 @@ static const char * const uvcg_format_names[] = {
 	"mjpeg",
 };
 
+static struct uvcg_color_matching *
+uvcg_format_get_default_color_match(struct config_item *streaming)
+{
+	struct config_item *color_matching_item, *cm_default;
+	struct uvcg_color_matching *color_match;
+
+	color_matching_item = config_group_find_item(to_config_group(streaming),
+						     "color_matching");
+	if (!color_matching_item)
+		return NULL;
+
+	cm_default = config_group_find_item(to_config_group(color_matching_item),
+					    "default");
+	config_item_put(color_matching_item);
+	if (!cm_default)
+		return NULL;
+
+	color_match = to_uvcg_color_matching(to_config_group(cm_default));
+	config_item_put(cm_default);
+
+	return color_match;
+}
+
 static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
 {
 	struct f_uvc_opts *opts;
@@ -1561,8 +1584,15 @@ 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 uvcg_color_matching *color_match;
+	struct config_item *streaming;
 	struct uvcg_uncompressed *h;
 
+	streaming = group->cg_item.ci_parent;
+	color_match = uvcg_format_get_default_color_match(streaming);
+	if (!color_match)
+		return ERR_PTR(-EINVAL);
+
 	h = kzalloc(sizeof(*h), GFP_KERNEL);
 	if (!h)
 		return ERR_PTR(-ENOMEM);
@@ -1580,6 +1610,8 @@ 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 = color_match;
+	color_match->refcnt++;
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_uncompressed_type);
 
@@ -1744,8 +1776,15 @@ static const struct config_item_type uvcg_mjpeg_type = {
 static struct config_group *uvcg_mjpeg_make(struct config_group *group,
 						   const char *name)
 {
+	struct uvcg_color_matching *color_match;
+	struct config_item *streaming;
 	struct uvcg_mjpeg *h;
 
+	streaming = group->cg_item.ci_parent;
+	color_match = uvcg_format_get_default_color_match(streaming);
+	if (!color_match)
+		return ERR_PTR(-EINVAL);
+
 	h = kzalloc(sizeof(*h), GFP_KERNEL);
 	if (!h)
 		return ERR_PTR(-ENOMEM);
@@ -1761,6 +1800,8 @@ 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 = color_match;
+	color_match->refcnt++;
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_mjpeg_type);
 
@@ -1909,7 +1950,8 @@ static inline struct uvc_descriptor_header
 enum uvcg_strm_type {
 	UVCG_HEADER = 0,
 	UVCG_FORMAT,
-	UVCG_FRAME
+	UVCG_FRAME,
+	UVCG_CMD,
 };
 
 /*
@@ -1959,6 +2001,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;
@@ -2014,6 +2060,12 @@ 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_color_matching *color_match = priv1;
+
+		*size += sizeof(color_match->desc);
+	}
+	break;
 	}
 
 	++*count;
@@ -2099,6 +2151,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_color_matching *color_match = priv1;
+
+		memcpy(*dest, &color_match->desc, sizeof(color_match->desc));
+		*dest += sizeof(color_match->desc);
+	}
+	break;
 	}
 
 	return 0;
@@ -2138,7 +2197,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;
@@ -2165,7 +2224,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 c7392c9b840e..174ee691302b 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -52,12 +52,13 @@ enum uvcg_format_type {
 };
 
 struct uvcg_format {
-	struct config_group	group;
-	enum uvcg_format_type	type;
-	unsigned		linked;
-	struct list_head	frames;
-	unsigned		num_frames;
-	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
+	struct config_group		group;
+	enum uvcg_format_type		type;
+	unsigned			linked;
+	struct list_head		frames;
+	unsigned			num_frames;
+	__u8				bmaControls[UVCG_STREAMING_CONTROL_SIZE];
+	struct uvcg_color_matching	*color_matching;
 };
 
 struct uvcg_format_ptr {
-- 
2.34.1

Re: [PATCH v2 4/7] usb: gadget: uvc: Copy color matching descriptor for each frame

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Dec 19, 2022 at 02:43:13PM +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>
> ---
> Changes in v2:
> 
> 	- Renamed uvcg_cmd and associated variables.
> 	- Formatting
> 	- Increased the refcnt variable for the color matching struct in
> 	the format make() functions
> 
>  drivers/usb/gadget/function/uvc_configfs.c | 64 +++++++++++++++++++++-
>  drivers/usb/gadget/function/uvc_configfs.h | 13 +++--
>  2 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 147d3def24dd..6fb7ac8366fe 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -748,6 +748,29 @@ static const char * const uvcg_format_names[] = {
>  	"mjpeg",
>  };
>  
> +static struct uvcg_color_matching *
> +uvcg_format_get_default_color_match(struct config_item *streaming)
> +{
> +	struct config_item *color_matching_item, *cm_default;
> +	struct uvcg_color_matching *color_match;
> +
> +	color_matching_item = config_group_find_item(to_config_group(streaming),
> +						     "color_matching");
> +	if (!color_matching_item)
> +		return NULL;
> +
> +	cm_default = config_group_find_item(to_config_group(color_matching_item),
> +					    "default");
> +	config_item_put(color_matching_item);
> +	if (!cm_default)
> +		return NULL;
> +
> +	color_match = to_uvcg_color_matching(to_config_group(cm_default));
> +	config_item_put(cm_default);
> +
> +	return color_match;
> +}
> +
>  static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>  {
>  	struct f_uvc_opts *opts;
> @@ -1561,8 +1584,15 @@ 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 uvcg_color_matching *color_match;
> +	struct config_item *streaming;
>  	struct uvcg_uncompressed *h;
>  
> +	streaming = group->cg_item.ci_parent;
> +	color_match = uvcg_format_get_default_color_match(streaming);
> +	if (!color_match)
> +		return ERR_PTR(-EINVAL);
> +
>  	h = kzalloc(sizeof(*h), GFP_KERNEL);
>  	if (!h)
>  		return ERR_PTR(-ENOMEM);
> @@ -1580,6 +1610,8 @@ 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 = color_match;
> +	color_match->refcnt++;

Does this need to be protected by a lock ? I suppose it may not matter
too much for the default as it will never be dropped, but it would still
be nice to get locking right overall.

I'm tempted to increase the refcnt in
uvcg_format_get_default_color_match(), as function that look up and
return a pointer to a refcounted object should take a reference. That's
not a hard requirement here if it makes the rest of the code too
complex.

>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_uncompressed_type);
>  
> @@ -1744,8 +1776,15 @@ static const struct config_item_type uvcg_mjpeg_type = {
>  static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  						   const char *name)
>  {
> +	struct uvcg_color_matching *color_match;
> +	struct config_item *streaming;
>  	struct uvcg_mjpeg *h;
>  
> +	streaming = group->cg_item.ci_parent;
> +	color_match = uvcg_format_get_default_color_match(streaming);
> +	if (!color_match)
> +		return ERR_PTR(-EINVAL);
> +
>  	h = kzalloc(sizeof(*h), GFP_KERNEL);
>  	if (!h)
>  		return ERR_PTR(-ENOMEM);
> @@ -1761,6 +1800,8 @@ 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 = color_match;
> +	color_match->refcnt++;
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_mjpeg_type);
>  
> @@ -1909,7 +1950,8 @@ static inline struct uvc_descriptor_header
>  enum uvcg_strm_type {
>  	UVCG_HEADER = 0,
>  	UVCG_FORMAT,
> -	UVCG_FRAME
> +	UVCG_FRAME,
> +	UVCG_CMD,

s/UVCG_CMD/UVCG_COLOR_MATCHING/

>  };
>  
>  /*
> @@ -1959,6 +2001,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;
> @@ -2014,6 +2060,12 @@ 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_color_matching *color_match = priv1;
> +
> +		*size += sizeof(color_match->desc);
> +	}
> +	break;
>  	}
>  
>  	++*count;
> @@ -2099,6 +2151,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_color_matching *color_match = priv1;
> +
> +		memcpy(*dest, &color_match->desc, sizeof(color_match->desc));
> +		*dest += sizeof(color_match->desc);
> +	}
> +	break;
>  	}
>  
>  	return 0;
> @@ -2138,7 +2197,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;
> @@ -2165,7 +2224,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 c7392c9b840e..174ee691302b 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -52,12 +52,13 @@ enum uvcg_format_type {
>  };
>  
>  struct uvcg_format {
> -	struct config_group	group;
> -	enum uvcg_format_type	type;
> -	unsigned		linked;
> -	struct list_head	frames;
> -	unsigned		num_frames;
> -	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +	struct config_group		group;
> +	enum uvcg_format_type		type;
> +	unsigned			linked;
> +	struct list_head		frames;
> +	unsigned			num_frames;
> +	__u8				bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +	struct uvcg_color_matching	*color_matching;
>  };
>  
>  struct uvcg_format_ptr {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/7] usb: gadget: uvc: Copy color matching descriptor for each frame

[Dan Scally]

Hi Laurent

On 28/12/2022 02:33, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Mon, Dec 19, 2022 at 02:43:13PM +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>
>> ---
>> Changes in v2:
>>
>> 	- Renamed uvcg_cmd and associated variables.
>> 	- Formatting
>> 	- Increased the refcnt variable for the color matching struct in
>> 	the format make() functions
>>
>>   drivers/usb/gadget/function/uvc_configfs.c | 64 +++++++++++++++++++++-
>>   drivers/usb/gadget/function/uvc_configfs.h | 13 +++--
>>   2 files changed, 68 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 147d3def24dd..6fb7ac8366fe 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -748,6 +748,29 @@ static const char * const uvcg_format_names[] = {
>>   	"mjpeg",
>>   };
>>   
>> +static struct uvcg_color_matching *
>> +uvcg_format_get_default_color_match(struct config_item *streaming)
>> +{
>> +	struct config_item *color_matching_item, *cm_default;
>> +	struct uvcg_color_matching *color_match;
>> +
>> +	color_matching_item = config_group_find_item(to_config_group(streaming),
>> +						     "color_matching");
>> +	if (!color_matching_item)
>> +		return NULL;
>> +
>> +	cm_default = config_group_find_item(to_config_group(color_matching_item),
>> +					    "default");
>> +	config_item_put(color_matching_item);
>> +	if (!cm_default)
>> +		return NULL;
>> +
>> +	color_match = to_uvcg_color_matching(to_config_group(cm_default));
>> +	config_item_put(cm_default);
>> +
>> +	return color_match;
>> +}
>> +
>>   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>>   {
>>   	struct f_uvc_opts *opts;
>> @@ -1561,8 +1584,15 @@ 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 uvcg_color_matching *color_match;
>> +	struct config_item *streaming;
>>   	struct uvcg_uncompressed *h;
>>   
>> +	streaming = group->cg_item.ci_parent;
>> +	color_match = uvcg_format_get_default_color_match(streaming);
>> +	if (!color_match)
>> +		return ERR_PTR(-EINVAL);
>> +
>>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
>>   	if (!h)
>>   		return ERR_PTR(-ENOMEM);
>> @@ -1580,6 +1610,8 @@ 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 = color_match;
>> +	color_match->refcnt++;
> Does this need to be protected by a lock ? I suppose it may not matter
> too much for the default as it will never be dropped, but it would still
> be nice to get locking right overall.


I don't think the locking is necessary here as in practice it's done by 
configfs core when you symlink between the config items. The refcnt here 
is only being used to protect a color matching descriptor's attributes 
from being changed when it's already been linked to a format, we rely on 
core to protect the descriptor from being dropped if it's in use somewhere.

>
> I'm tempted to increase the refcnt in
> uvcg_format_get_default_color_match(), as function that look up and
> return a pointer to a refcounted object should take a reference. That's
> not a hard requirement here if it makes the rest of the code too
> complex.
>
>>   	config_group_init_type_name(&h->fmt.group, name,
>>   				    &uvcg_uncompressed_type);
>>   
>> @@ -1744,8 +1776,15 @@ static const struct config_item_type uvcg_mjpeg_type = {
>>   static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>>   						   const char *name)
>>   {
>> +	struct uvcg_color_matching *color_match;
>> +	struct config_item *streaming;
>>   	struct uvcg_mjpeg *h;
>>   
>> +	streaming = group->cg_item.ci_parent;
>> +	color_match = uvcg_format_get_default_color_match(streaming);
>> +	if (!color_match)
>> +		return ERR_PTR(-EINVAL);
>> +
>>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
>>   	if (!h)
>>   		return ERR_PTR(-ENOMEM);
>> @@ -1761,6 +1800,8 @@ 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 = color_match;
>> +	color_match->refcnt++;
>>   	config_group_init_type_name(&h->fmt.group, name,
>>   				    &uvcg_mjpeg_type);
>>   
>> @@ -1909,7 +1950,8 @@ static inline struct uvc_descriptor_header
>>   enum uvcg_strm_type {
>>   	UVCG_HEADER = 0,
>>   	UVCG_FORMAT,
>> -	UVCG_FRAME
>> +	UVCG_FRAME,
>> +	UVCG_CMD,
> s/UVCG_CMD/UVCG_COLOR_MATCHING/
>
>>   };
>>   
>>   /*
>> @@ -1959,6 +2001,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;
>> @@ -2014,6 +2060,12 @@ 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_color_matching *color_match = priv1;
>> +
>> +		*size += sizeof(color_match->desc);
>> +	}
>> +	break;
>>   	}
>>   
>>   	++*count;
>> @@ -2099,6 +2151,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_color_matching *color_match = priv1;
>> +
>> +		memcpy(*dest, &color_match->desc, sizeof(color_match->desc));
>> +		*dest += sizeof(color_match->desc);
>> +	}
>> +	break;
>>   	}
>>   
>>   	return 0;
>> @@ -2138,7 +2197,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;
>> @@ -2165,7 +2224,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 c7392c9b840e..174ee691302b 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.h
>> +++ b/drivers/usb/gadget/function/uvc_configfs.h
>> @@ -52,12 +52,13 @@ enum uvcg_format_type {
>>   };
>>   
>>   struct uvcg_format {
>> -	struct config_group	group;
>> -	enum uvcg_format_type	type;
>> -	unsigned		linked;
>> -	struct list_head	frames;
>> -	unsigned		num_frames;
>> -	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>> +	struct config_group		group;
>> +	enum uvcg_format_type		type;
>> +	unsigned			linked;
>> +	struct list_head		frames;
>> +	unsigned			num_frames;
>> +	__u8				bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>> +	struct uvcg_color_matching	*color_matching;
>>   };
>>   
>>   struct uvcg_format_ptr {

Re: [PATCH v2 4/7] usb: gadget: uvc: Copy color matching descriptor for each frame

[Dan Scally]

On 30/01/2023 12:01, Dan Scally wrote:
> Hi Laurent
>
> On 28/12/2022 02:33, Laurent Pinchart wrote:
>> Hi Dan,
>>
>> Thank you for the patch.
>>
>> On Mon, Dec 19, 2022 at 02:43:13PM +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>
>>> ---
>>> Changes in v2:
>>>
>>>     - Renamed uvcg_cmd and associated variables.
>>>     - Formatting
>>>     - Increased the refcnt variable for the color matching struct in
>>>     the format make() functions
>>>
>>>   drivers/usb/gadget/function/uvc_configfs.c | 64 
>>> +++++++++++++++++++++-
>>>   drivers/usb/gadget/function/uvc_configfs.h | 13 +++--
>>>   2 files changed, 68 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
>>> b/drivers/usb/gadget/function/uvc_configfs.c
>>> index 147d3def24dd..6fb7ac8366fe 100644
>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>>> @@ -748,6 +748,29 @@ static const char * const uvcg_format_names[] = {
>>>       "mjpeg",
>>>   };
>>>   +static struct uvcg_color_matching *
>>> +uvcg_format_get_default_color_match(struct config_item *streaming)
>>> +{
>>> +    struct config_item *color_matching_item, *cm_default;
>>> +    struct uvcg_color_matching *color_match;
>>> +
>>> +    color_matching_item = 
>>> config_group_find_item(to_config_group(streaming),
>>> +                             "color_matching");
>>> +    if (!color_matching_item)
>>> +        return NULL;
>>> +
>>> +    cm_default = 
>>> config_group_find_item(to_config_group(color_matching_item),
>>> +                        "default");
>>> +    config_item_put(color_matching_item);
>>> +    if (!cm_default)
>>> +        return NULL;
>>> +
>>> +    color_match = to_uvcg_color_matching(to_config_group(cm_default));
>>> +    config_item_put(cm_default);
>>> +
>>> +    return color_match;
>>> +}
>>> +
>>>   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format 
>>> *f, char *page)
>>>   {
>>>       struct f_uvc_opts *opts;
>>> @@ -1561,8 +1584,15 @@ 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 uvcg_color_matching *color_match;
>>> +    struct config_item *streaming;
>>>       struct uvcg_uncompressed *h;
>>>   +    streaming = group->cg_item.ci_parent;
>>> +    color_match = uvcg_format_get_default_color_match(streaming);
>>> +    if (!color_match)
>>> +        return ERR_PTR(-EINVAL);
>>> +
>>>       h = kzalloc(sizeof(*h), GFP_KERNEL);
>>>       if (!h)
>>>           return ERR_PTR(-ENOMEM);
>>> @@ -1580,6 +1610,8 @@ 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 = color_match;
>>> +    color_match->refcnt++;
>> Does this need to be protected by a lock ? I suppose it may not matter
>> too much for the default as it will never be dropped, but it would still
>> be nice to get locking right overall.
>
>
> I don't think the locking is necessary here as in practice it's done 
> by configfs core when you symlink between the config items. The refcnt 
> here is only being used to protect a color matching descriptor's 
> attributes from being changed when it's already been linked to a 
> format, we rely on core to protect the descriptor from being dropped 
> if it's in use somewhere.
>
>>
>> I'm tempted to increase the refcnt in
>> uvcg_format_get_default_color_match(), as function that look up and
>> return a pointer to a refcounted object should take a reference. That's
>> not a hard requirement here if it makes the rest of the code too
>> complex.


Sorry forgot to respond here; in fact it does take a reference the the 
colour matching descriptors config_item, through 
config_group_find_item() in configfs core.

>>
>>> config_group_init_type_name(&h->fmt.group, name,
>>>                       &uvcg_uncompressed_type);
>>>   @@ -1744,8 +1776,15 @@ static const struct config_item_type 
>>> uvcg_mjpeg_type = {
>>>   static struct config_group *uvcg_mjpeg_make(struct config_group 
>>> *group,
>>>                              const char *name)
>>>   {
>>> +    struct uvcg_color_matching *color_match;
>>> +    struct config_item *streaming;
>>>       struct uvcg_mjpeg *h;
>>>   +    streaming = group->cg_item.ci_parent;
>>> +    color_match = uvcg_format_get_default_color_match(streaming);
>>> +    if (!color_match)
>>> +        return ERR_PTR(-EINVAL);
>>> +
>>>       h = kzalloc(sizeof(*h), GFP_KERNEL);
>>>       if (!h)
>>>           return ERR_PTR(-ENOMEM);
>>> @@ -1761,6 +1800,8 @@ 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 = color_match;
>>> +    color_match->refcnt++;
>>>       config_group_init_type_name(&h->fmt.group, name,
>>>                       &uvcg_mjpeg_type);
>>>   @@ -1909,7 +1950,8 @@ static inline struct uvc_descriptor_header
>>>   enum uvcg_strm_type {
>>>       UVCG_HEADER = 0,
>>>       UVCG_FORMAT,
>>> -    UVCG_FRAME
>>> +    UVCG_FRAME,
>>> +    UVCG_CMD,
>> s/UVCG_CMD/UVCG_COLOR_MATCHING/
>>
>>>   };
>>>     /*
>>> @@ -1959,6 +2001,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;
>>> @@ -2014,6 +2060,12 @@ 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_color_matching *color_match = priv1;
>>> +
>>> +        *size += sizeof(color_match->desc);
>>> +    }
>>> +    break;
>>>       }
>>>         ++*count;
>>> @@ -2099,6 +2151,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_color_matching *color_match = priv1;
>>> +
>>> +        memcpy(*dest, &color_match->desc, sizeof(color_match->desc));
>>> +        *dest += sizeof(color_match->desc);
>>> +    }
>>> +    break;
>>>       }
>>>         return 0;
>>> @@ -2138,7 +2197,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;
>>> @@ -2165,7 +2224,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 c7392c9b840e..174ee691302b 100644
>>> --- a/drivers/usb/gadget/function/uvc_configfs.h
>>> +++ b/drivers/usb/gadget/function/uvc_configfs.h
>>> @@ -52,12 +52,13 @@ enum uvcg_format_type {
>>>   };
>>>     struct uvcg_format {
>>> -    struct config_group    group;
>>> -    enum uvcg_format_type    type;
>>> -    unsigned        linked;
>>> -    struct list_head    frames;
>>> -    unsigned        num_frames;
>>> -    __u8            bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>>> +    struct config_group        group;
>>> +    enum uvcg_format_type        type;
>>> +    unsigned            linked;
>>> +    struct list_head        frames;
>>> +    unsigned            num_frames;
>>> +    __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>>> +    struct uvcg_color_matching    *color_matching;
>>>   };
>>>     struct uvcg_format_ptr {

[PATCH v2 5/7] 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- None

 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 32f2c1645467..fbb57a0df57f 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -803,7 +803,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;
 
@@ -852,14 +851,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

[PATCH v2 6/7] 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>
---
Changes in v2:

	- Check refcnt before allowing the change in .store()
	- Renamed uvcg_cmd to uvcg_color_matching

 .../ABI/testing/configfs-usb-gadget-uvc       |  2 +-
 drivers/usb/gadget/function/uvc_configfs.c    | 36 ++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index f00cff6d8c5c..53258b7c6f2d 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 6fb7ac8366fe..ef5d75942f24 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1851,7 +1851,41 @@ 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_color_matching *color_match =			\
+		to_uvcg_color_matching(group);				\
+	struct f_uvc_opts *opts;					\
+	struct config_item *opts_item;					\
+	int ret;							\
+	u##bits num;							\
+									\
+	if (color_match->refcnt)					\
+		return -EBUSY;						\
+									\
+	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);					\
+									\
+	color_match->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 v2 6/7] usb: gadget: uvc: Make color matching attributes read/write

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Dec 19, 2022 at 02:43:15PM +0000, Daniel Scally wrote:
> 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>
> ---
> Changes in v2:
> 
> 	- Check refcnt before allowing the change in .store()
> 	- Renamed uvcg_cmd to uvcg_color_matching
> 
>  .../ABI/testing/configfs-usb-gadget-uvc       |  2 +-
>  drivers/usb/gadget/function/uvc_configfs.c    | 36 ++++++++++++++++++-
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index f00cff6d8c5c..53258b7c6f2d 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 6fb7ac8366fe..ef5d75942f24 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1851,7 +1851,41 @@ 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_color_matching *color_match =			\
> +		to_uvcg_color_matching(group);				\
> +	struct f_uvc_opts *opts;					\
> +	struct config_item *opts_item;					\
> +	int ret;							\
> +	u##bits num;							\
> +									\
> +	if (color_match->refcnt)					\

Shouldn't this be protected by the same lock that you use to modify
refcnt in 7/7 ?

> +		return -EBUSY;						\
> +									\
> +	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);					\
> +									\
> +	color_match->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

[PATCH v2 7/7] 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>
---
Changes in v2:

	- New section of the ABI documentation
	- uvcg_format_allow_link() now checks to see if an existing link is 
	already there
	- .allow_link() and .drop_link() track color_matching->refcnt

 .../ABI/testing/configfs-usb-gadget-uvc       | 17 ++++
 drivers/usb/gadget/function/uvc_configfs.c    | 99 ++++++++++++++++++-
 2 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 53258b7c6f2d..e7753b2cb11b 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -177,6 +177,23 @@ Description:	Default color matching descriptors
 					  white
 		========================  ======================================
 
+What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
+Date:		Dec 2022
+KernelVersion:	6.3
+Description:	Additional color matching descriptors
+
+		All attributes read/write:
+
+		========================  ======================================
+		bMatrixCoefficients	  matrix used to compute luma and
+					  chroma values from the color primaries
+		bTransferCharacteristics  optoelectronic transfer
+					  characteristic of the source picture,
+					  also called the gamma function
+		bColorPrimaries		  color primaries and the reference
+					  white
+		========================  ======================================
+
 What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
 Date:		Dec 2014
 KernelVersion:	4.0
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index ef5d75942f24..3be6ca936851 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -771,6 +771,77 @@ uvcg_format_get_default_color_match(struct config_item *streaming)
 	return color_match;
 }
 
+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 uvcg_color_matching *color_matching_desc;
+	struct config_item *streaming, *color_matching;
+	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;
+	}
+
+	fmt = to_uvcg_format(src);
+
+	/*
+	 * There's always a color matching descriptor associated with the format
+	 * but without a symlink it should only ever be the default one. If it's
+	 * not the default, there's already a symlink and we should bail out.
+	 */
+	color_matching_desc = uvcg_format_get_default_color_match(streaming);
+	if (fmt->color_matching != color_matching_desc) {
+		ret = -EBUSY;
+		goto out_put_cm;
+	}
+
+	color_matching_desc->refcnt--;
+
+	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
+	fmt->color_matching = color_matching_desc;
+	color_matching_desc->refcnt++;
+
+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 mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
+	struct uvcg_color_matching *color_matching_desc;
+	struct config_item *streaming;
+	struct uvcg_format *fmt;
+
+	mutex_lock(su_mutex);
+
+	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
+	color_matching_desc->refcnt--;
+
+	streaming = src->ci_parent->ci_parent;
+	color_matching_desc = uvcg_format_get_default_color_match(streaming);
+
+	fmt = to_uvcg_format(src);
+	fmt->color_matching = color_matching_desc;
+	color_matching_desc->refcnt++;
+
+	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;
@@ -1571,7 +1642,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,
@@ -1767,7 +1838,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,
@@ -1922,6 +1993,29 @@ 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_color_matching *color_match;
+
+	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
+	if (!color_match)
+		return ERR_PTR(-ENOMEM);
+
+	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
+	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
+	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
+
+	config_group_init_type_name(&color_match->group, name,
+				    &uvcg_color_matching_type);
+
+	return &color_match->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_color_matching *color_match;
@@ -1947,6 +2041,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 v2 7/7] usb: gadget: uvc: Allow creating new color matching descriptors

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Mon, Dec 19, 2022 at 02:43:16PM +0000, Daniel Scally wrote:
> 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>
> ---
> Changes in v2:
> 
> 	- New section of the ABI documentation
> 	- uvcg_format_allow_link() now checks to see if an existing link is 
> 	already there
> 	- .allow_link() and .drop_link() track color_matching->refcnt
> 
>  .../ABI/testing/configfs-usb-gadget-uvc       | 17 ++++
>  drivers/usb/gadget/function/uvc_configfs.c    | 99 ++++++++++++++++++-
>  2 files changed, 114 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 53258b7c6f2d..e7753b2cb11b 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -177,6 +177,23 @@ Description:	Default color matching descriptors
>  					  white
>  		========================  ======================================
>  
> +What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> +Date:		Dec 2022
> +KernelVersion:	6.3
> +Description:	Additional color matching descriptors
> +
> +		All attributes read/write:
> +
> +		========================  ======================================
> +		bMatrixCoefficients	  matrix used to compute luma and
> +					  chroma values from the color primaries
> +		bTransferCharacteristics  optoelectronic transfer
> +					  characteristic of the source picture,
> +					  also called the gamma function
> +		bColorPrimaries		  color primaries and the reference
> +					  white
> +		========================  ======================================
> +

Should the link also be documented somewhere ?

>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
>  Date:		Dec 2014
>  KernelVersion:	4.0
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index ef5d75942f24..3be6ca936851 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -771,6 +771,77 @@ uvcg_format_get_default_color_match(struct config_item *streaming)
>  	return color_match;
>  }
>  
> +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 uvcg_color_matching *color_matching_desc;
> +	struct config_item *streaming, *color_matching;
> +	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;
> +	}
> +
> +	fmt = to_uvcg_format(src);

It's been a long time since I worked with configfs, so I may be wrong,
but shouldn't we check the name of the source here to make sure it's
equal to "color_matching" ? Or do you want to allow the user to name the
source freely ? That would be a bit confusing I think.

> +
> +	/*
> +	 * There's always a color matching descriptor associated with the format
> +	 * but without a symlink it should only ever be the default one. If it's
> +	 * not the default, there's already a symlink and we should bail out.
> +	 */
> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
> +	if (fmt->color_matching != color_matching_desc) {

If you check the source link name, I suppose this could be dropped. Then
you coud just write

	fmt->color_matching->refcnt--;

and avoid the call to uvcg_format_get_default_color_match().

> +		ret = -EBUSY;
> +		goto out_put_cm;
> +	}
> +
> +	color_matching_desc->refcnt--;
> +
> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> +	fmt->color_matching = color_matching_desc;
> +	color_matching_desc->refcnt++;

And this could become

	fmt->color_matching = to_uvcg_color_matching(to_config_group(tgt));
	fmt->color_matching->refcnt++;

> +
> +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 mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> +	struct uvcg_color_matching *color_matching_desc;
> +	struct config_item *streaming;
> +	struct uvcg_format *fmt;
> +
> +	mutex_lock(su_mutex);
> +
> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> +	color_matching_desc->refcnt--;
> +
> +	streaming = src->ci_parent->ci_parent;
> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
> +
> +	fmt = to_uvcg_format(src);
> +	fmt->color_matching = color_matching_desc;
> +	color_matching_desc->refcnt++;

	fmt->color_matching = uvcg_format_get_default_color_match(streaming);
	fmt->color_matching->refcnt++;

although if you increase the refcnt in
uvcg_format_get_default_color_match() as I proposed in a previous patch
in this series, this would just be

	fmt->color_matching = uvcg_format_get_default_color_match(streaming);

> +
> +	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;
> @@ -1571,7 +1642,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,
> @@ -1767,7 +1838,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,
> @@ -1922,6 +1993,29 @@ 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_color_matching *color_match;
> +
> +	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
> +	if (!color_match)
> +		return ERR_PTR(-ENOMEM);
> +
> +	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> +	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> +	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> +
> +	config_group_init_type_name(&color_match->group, name,
> +				    &uvcg_color_matching_type);
> +
> +	return &color_match->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_color_matching *color_match;
> @@ -1947,6 +2041,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 v2 7/7] usb: gadget: uvc: Allow creating new color matching descriptors

[Dan Scally]

Hi Laurent - thanks for the review

On 28/12/2022 02:50, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Mon, Dec 19, 2022 at 02:43:16PM +0000, Daniel Scally wrote:
>> 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>
>> ---
>> Changes in v2:
>>
>> 	- New section of the ABI documentation
>> 	- uvcg_format_allow_link() now checks to see if an existing link is
>> 	already there
>> 	- .allow_link() and .drop_link() track color_matching->refcnt
>>
>>   .../ABI/testing/configfs-usb-gadget-uvc       | 17 ++++
>>   drivers/usb/gadget/function/uvc_configfs.c    | 99 ++++++++++++++++++-
>>   2 files changed, 114 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> index 53258b7c6f2d..e7753b2cb11b 100644
>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> @@ -177,6 +177,23 @@ Description:	Default color matching descriptors
>>   					  white
>>   		========================  ======================================
>>   
>> +What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
>> +Date:		Dec 2022
>> +KernelVersion:	6.3
>> +Description:	Additional color matching descriptors
>> +
>> +		All attributes read/write:
>> +
>> +		========================  ======================================
>> +		bMatrixCoefficients	  matrix used to compute luma and
>> +					  chroma values from the color primaries
>> +		bTransferCharacteristics  optoelectronic transfer
>> +					  characteristic of the source picture,
>> +					  also called the gamma function
>> +		bColorPrimaries		  color primaries and the reference
>> +					  white
>> +		========================  ======================================
>> +
> Should the link also be documented somewhere ?


I actually couldn't see that any of the links were described in this 
document, so I skipped it. I'm working on a more comprehensive piece of 
documentation which describes the UVC Gadget more fully, and my plan was 
to do it there.

>
>>   What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
>>   Date:		Dec 2014
>>   KernelVersion:	4.0
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index ef5d75942f24..3be6ca936851 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -771,6 +771,77 @@ uvcg_format_get_default_color_match(struct config_item *streaming)
>>   	return color_match;
>>   }
>>   
>> +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 uvcg_color_matching *color_matching_desc;
>> +	struct config_item *streaming, *color_matching;
>> +	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;
>> +	}
>> +
>> +	fmt = to_uvcg_format(src);
> It's been a long time since I worked with configfs, so I may be wrong,
> but shouldn't we check the name of the source here to make sure it's
> equal to "color_matching" ? Or do you want to allow the user to name the
> source freely ? That would be a bit confusing I think.


The source will be either streaming/uncompressed/<name> or 
streaming/mjpeg/<name>. I don't think we need to check that, as this 
function will only be called if that's where the user is attempting to 
link from. So it'll be a link from streaming/uncompressed/u to 
streaming/color_matching/yuy2 for example.

>
>> +
>> +	/*
>> +	 * There's always a color matching descriptor associated with the format
>> +	 * but without a symlink it should only ever be the default one. If it's
>> +	 * not the default, there's already a symlink and we should bail out.
>> +	 */
>> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
>> +	if (fmt->color_matching != color_matching_desc) {
> If you check the source link name, I suppose this could be dropped. Then
> you coud just write
>
> 	fmt->color_matching->refcnt--;
>
> and avoid the call to uvcg_format_get_default_color_match().


Not sure I follow here I'm afraid. As I see it, to retain the current 
functionality (sending the 1/1/4 descriptor when no other is specified) 
we need to link the default descriptor when none other is linked, so we 
need to check whether or not that's the one that's currently linked to 
know if there's another symlink hanging around already. This check is 
designed to avoid streaming/uncompressed/u being linked to both 
streaming/color_matching/yuy2 and streaming/color_matching/mjpeg for 
example.

>
>> +		ret = -EBUSY;
>> +		goto out_put_cm;
>> +	}
>> +
>> +	color_matching_desc->refcnt--;
>> +
>> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
>> +	fmt->color_matching = color_matching_desc;
>> +	color_matching_desc->refcnt++;
> And this could become
>
> 	fmt->color_matching = to_uvcg_color_matching(to_config_group(tgt));
> 	fmt->color_matching->refcnt++;
>
>> +
>> +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 mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
>> +	struct uvcg_color_matching *color_matching_desc;
>> +	struct config_item *streaming;
>> +	struct uvcg_format *fmt;
>> +
>> +	mutex_lock(su_mutex);
>> +
>> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
>> +	color_matching_desc->refcnt--;
>> +
>> +	streaming = src->ci_parent->ci_parent;
>> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
>> +
>> +	fmt = to_uvcg_format(src);
>> +	fmt->color_matching = color_matching_desc;
>> +	color_matching_desc->refcnt++;
> 	fmt->color_matching = uvcg_format_get_default_color_match(streaming);
> 	fmt->color_matching->refcnt++;
>
> although if you increase the refcnt in
> uvcg_format_get_default_color_match() as I proposed in a previous patch
> in this series, this would just be
>
> 	fmt->color_matching = uvcg_format_get_default_color_match(streaming);
>
>> +
>> +	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;
>> @@ -1571,7 +1642,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,
>> @@ -1767,7 +1838,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,
>> @@ -1922,6 +1993,29 @@ 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_color_matching *color_match;
>> +
>> +	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
>> +	if (!color_match)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
>> +	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
>> +	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
>> +
>> +	config_group_init_type_name(&color_match->group, name,
>> +				    &uvcg_color_matching_type);
>> +
>> +	return &color_match->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_color_matching *color_match;
>> @@ -1947,6 +2041,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 v2 7/7] usb: gadget: uvc: Allow creating new color matching descriptors

[Laurent Pinchart]

Hi Dan,

On Sun, Jan 01, 2023 at 08:55:43PM +0000, Dan Scally wrote:
> On 28/12/2022 02:50, Laurent Pinchart wrote:
> > On Mon, Dec 19, 2022 at 02:43:16PM +0000, Daniel Scally wrote:
> >> 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>
> >> ---
> >> Changes in v2:
> >>
> >> 	- New section of the ABI documentation
> >> 	- uvcg_format_allow_link() now checks to see if an existing link is
> >> 	already there
> >> 	- .allow_link() and .drop_link() track color_matching->refcnt
> >>
> >>   .../ABI/testing/configfs-usb-gadget-uvc       | 17 ++++
> >>   drivers/usb/gadget/function/uvc_configfs.c    | 99 ++++++++++++++++++-
> >>   2 files changed, 114 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> index 53258b7c6f2d..e7753b2cb11b 100644
> >> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> @@ -177,6 +177,23 @@ Description:	Default color matching descriptors
> >>   					  white
> >>   		========================  ======================================
> >>   
> >> +What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/color_matching/name
> >> +Date:		Dec 2022
> >> +KernelVersion:	6.3
> >> +Description:	Additional color matching descriptors
> >> +
> >> +		All attributes read/write:
> >> +
> >> +		========================  ======================================
> >> +		bMatrixCoefficients	  matrix used to compute luma and
> >> +					  chroma values from the color primaries
> >> +		bTransferCharacteristics  optoelectronic transfer
> >> +					  characteristic of the source picture,
> >> +					  also called the gamma function
> >> +		bColorPrimaries		  color primaries and the reference
> >> +					  white
> >> +		========================  ======================================
> >> +
> >
> > Should the link also be documented somewhere ?
> 
> I actually couldn't see that any of the links were described in this 
> document, so I skipped it. I'm working on a more comprehensive piece of 
> documentation which describes the UVC Gadget more fully, and my plan was 
> to do it there.
> 
> >
> >>   What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
> >>   Date:		Dec 2014
> >>   KernelVersion:	4.0
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index ef5d75942f24..3be6ca936851 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -771,6 +771,77 @@ uvcg_format_get_default_color_match(struct config_item *streaming)
> >>   	return color_match;
> >>   }
> >>   
> >> +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 uvcg_color_matching *color_matching_desc;
> >> +	struct config_item *streaming, *color_matching;
> >> +	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;
> >> +	}
> >> +
> >> +	fmt = to_uvcg_format(src);
> >
> > It's been a long time since I worked with configfs, so I may be wrong,
> > but shouldn't we check the name of the source here to make sure it's
> > equal to "color_matching" ? Or do you want to allow the user to name the
> > source freely ? That would be a bit confusing I think.
> 
> The source will be either streaming/uncompressed/<name> or 
> streaming/mjpeg/<name>. I don't think we need to check that, as this 
> function will only be called if that's where the user is attempting to 
> link from. So it'll be a link from streaming/uncompressed/u to 
> streaming/color_matching/yuy2 for example.

Ah so the source is the directory in which the link is created, not the
link itself ? In that case you indeed can't check the link name as that
information isn't available.

> >> +
> >> +	/*
> >> +	 * There's always a color matching descriptor associated with the format
> >> +	 * but without a symlink it should only ever be the default one. If it's
> >> +	 * not the default, there's already a symlink and we should bail out.
> >> +	 */
> >> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
> >> +	if (fmt->color_matching != color_matching_desc) {
> >
> > If you check the source link name, I suppose this could be dropped. Then
> > you coud just write
> >
> > 	fmt->color_matching->refcnt--;
> >
> > and avoid the call to uvcg_format_get_default_color_match().
> 
> Not sure I follow here I'm afraid. As I see it, to retain the current 
> functionality (sending the 1/1/4 descriptor when no other is specified) 
> we need to link the default descriptor when none other is linked, so we 
> need to check whether or not that's the one that's currently linked to 
> know if there's another symlink hanging around already. This check is 
> designed to avoid streaming/uncompressed/u being linked to both 
> streaming/color_matching/yuy2 and streaming/color_matching/mjpeg for 
> example.

The idea is that if we could restrict the link name to "color_matching",
we could only reach this point in the code when no link exists yet as
the configfs core wouldn't allow creating a second link with the same
name. 

> >> +		ret = -EBUSY;
> >> +		goto out_put_cm;
> >> +	}
> >> +
> >> +	color_matching_desc->refcnt--;
> >> +
> >> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> >> +	fmt->color_matching = color_matching_desc;
> >> +	color_matching_desc->refcnt++;
> >
> > And this could become
> >
> > 	fmt->color_matching = to_uvcg_color_matching(to_config_group(tgt));
> > 	fmt->color_matching->refcnt++;
> >
> >> +
> >> +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 mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> +	struct uvcg_color_matching *color_matching_desc;
> >> +	struct config_item *streaming;
> >> +	struct uvcg_format *fmt;
> >> +
> >> +	mutex_lock(su_mutex);
> >> +
> >> +	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
> >> +	color_matching_desc->refcnt--;
> >> +
> >> +	streaming = src->ci_parent->ci_parent;
> >> +	color_matching_desc = uvcg_format_get_default_color_match(streaming);
> >> +
> >> +	fmt = to_uvcg_format(src);
> >> +	fmt->color_matching = color_matching_desc;
> >> +	color_matching_desc->refcnt++;
> >
> > 	fmt->color_matching = uvcg_format_get_default_color_match(streaming);
> > 	fmt->color_matching->refcnt++;
> >
> > although if you increase the refcnt in
> > uvcg_format_get_default_color_match() as I proposed in a previous patch
> > in this series, this would just be
> >
> > 	fmt->color_matching = uvcg_format_get_default_color_match(streaming);
> >
> >> +
> >> +	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;
> >> @@ -1571,7 +1642,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,
> >> @@ -1767,7 +1838,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,
> >> @@ -1922,6 +1993,29 @@ 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_color_matching *color_match;
> >> +
> >> +	color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
> >> +	if (!color_match)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> >> +	color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> >> +	color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> >> +
> >> +	config_group_init_type_name(&color_match->group, name,
> >> +				    &uvcg_color_matching_type);
> >> +
> >> +	return &color_match->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_color_matching *color_match;
> >> @@ -1947,6 +2041,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