diff mbox series

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

Message ID 20221213083736.2284536-4-dan.scally@ideasonboard.com
State New
Headers show
Series UVC Gadget: Extend color matching support | expand

Commit Message

Dan Scally Dec. 13, 2022, 8:37 a.m. UTC 
As currently implemented the default color matching descriptor is
appended after _all_ the formats and frames that the gadget is
configured with. According to the UVC specifications however this
is supposed to be on a per-format basis (section 3.9.2.6):

"Only one instance is allowed for a given format and if present,
the Color Matching descriptor shall be placed following the Video
and Still Image Frame descriptors for that format."

Associate the default color matching descriptor with struct
uvcg_format and copy it once-per-format instead of once only.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
 drivers/usb/gadget/function/uvc_configfs.h |  1 +
 2 files changed, 58 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Dec. 18, 2022, 11:28 p.m. UTC | #1 
Hi Dan,

Thank you for the patch.

On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
> As currently implemented the default color matching descriptor is
> appended after _all_ the formats and frames that the gadget is
> configured with. According to the UVC specifications however this
> is supposed to be on a per-format basis (section 3.9.2.6):
> 
> "Only one instance is allowed for a given format and if present,
> the Color Matching descriptor shall be placed following the Video
> and Still Image Frame descriptors for that format."
> 
> Associate the default color matching descriptor with struct
> uvcg_format and copy it once-per-format instead of once only.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
>  drivers/usb/gadget/function/uvc_configfs.h |  1 +
>  2 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 9918e7b6a023..6f5932c9f09c 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
>  	"mjpeg",
>  };
>  
> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
> +{
> +	struct config_item *color_matching, *cm_default;
> +	struct uvcg_cmd *cmd;
> +
> +	color_matching = config_group_find_item(to_config_group(streaming),
> +						"color_matching");
> +	if (!color_matching)
> +		return NULL;
> +
> +	cm_default = config_group_find_item(to_config_group(color_matching),
> +					    "default");
> +	config_item_put(color_matching);
> +	if (!cm_default)
> +		return NULL;
> +
> +	cmd = to_uvcg_cmd(to_config_group(cm_default));
> +	config_item_put(cm_default);
> +
> +	return cmd;
> +}
> +
>  static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>  {
>  	struct f_uvc_opts *opts;
> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>  		'Y',  'U',  'Y',  '2', 0x00, 0x00, 0x10, 0x00,
>  		 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
>  	};
> +	struct config_item *streaming;
>  	struct uvcg_uncompressed *h;
> +	struct uvcg_cmd *cmd;
> +
> +	streaming = group->cg_item.ci_parent;
> +	cmd = uvcg_format_get_default_cmd(streaming);
> +	if (!cmd)
> +		return ERR_PTR(-EINVAL);
>  
>  	h = kzalloc(sizeof(*h), GFP_KERNEL);
>  	if (!h)
> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>  
>  	INIT_LIST_HEAD(&h->fmt.frames);
>  	h->fmt.type = UVCG_UNCOMPRESSED;
> +	h->fmt.color_matching = cmd;

Is there any need to reference-count cmd to make sure it doesn't get
deleted ? I don't expect that to be an issue for the default, but for
the user-defined color matching descriptors. This may be better
discussed in the context of other patches further in the series.

>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_uncompressed_type);
>  
> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
>  static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  						   const char *name)
>  {
> +	struct config_item *streaming;
>  	struct uvcg_mjpeg *h;
> +	struct uvcg_cmd *cmd;
> +
> +	streaming = group->cg_item.ci_parent;
> +	cmd = uvcg_format_get_default_cmd(streaming);
> +	if (!cmd)
> +		return ERR_PTR(-EINVAL);
>  
>  	h = kzalloc(sizeof(*h), GFP_KERNEL);
>  	if (!h)
> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  
>  	INIT_LIST_HEAD(&h->fmt.frames);
>  	h->fmt.type = UVCG_MJPEG;
> +	h->fmt.color_matching = cmd;
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_mjpeg_type);
>  
> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
>  enum uvcg_strm_type {
>  	UVCG_HEADER = 0,
>  	UVCG_FORMAT,
> -	UVCG_FRAME
> +	UVCG_FRAME,
> +	UVCG_CMD,
>  };
>  
>  /*
> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
>  			if (ret)
>  				return ret;
>  		}
> +
> +		ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return ret;
> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
>  		*size += frm->frame.b_frame_interval_type * sz;
>  	}
>  	break;
> +	case UVCG_CMD: {
> +		struct uvcg_cmd *cmd = priv1;

Missing blank line. Same below.

> +		*size += sizeof(cmd->desc);
> +	}
> +	break;
>  	}
>  
>  	++*count;
> @@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
>  				frm->frame.b_frame_interval_type);
>  	}
>  	break;
> +	case UVCG_CMD: {
> +		struct uvcg_cmd *cmd = priv1;
> +
> +		memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
> +		*dest += sizeof(cmd->desc);
> +	}
> +	break;
>  	}
>  
>  	return 0;
> @@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>  	if (ret)
>  		goto unlock;
>  
> -	count += 2; /* color_matching, NULL */
> +	count += 1; /* NULL */
>  	*class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
>  	if (!*class_array) {
>  		ret = -ENOMEM;
> @@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>  		kfree(data_save);
>  		goto unlock;
>  	}
> -	*cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
>  
>  	++target_hdr->linked;
>  	ret = 0;
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index f990739838d5..6582d6c7b6f6 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -57,6 +57,7 @@ struct uvcg_format {
>  	struct list_head	frames;
>  	unsigned		num_frames;
>  	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +	struct uvcg_cmd		*color_matching;
>  };
>  
>  struct uvcg_format_ptr {
Dan Scally Dec. 19, 2022, 10:33 a.m. UTC | #2 
Hi Laurent

On 18/12/2022 23:28, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
>> As currently implemented the default color matching descriptor is
>> appended after _all_ the formats and frames that the gadget is
>> configured with. According to the UVC specifications however this
>> is supposed to be on a per-format basis (section 3.9.2.6):
>>
>> "Only one instance is allowed for a given format and if present,
>> the Color Matching descriptor shall be placed following the Video
>> and Still Image Frame descriptors for that format."
>>
>> Associate the default color matching descriptor with struct
>> uvcg_format and copy it once-per-format instead of once only.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
>>   drivers/usb/gadget/function/uvc_configfs.h |  1 +
>>   2 files changed, 58 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index 9918e7b6a023..6f5932c9f09c 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
>>   	"mjpeg",
>>   };
>>   
>> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
>> +{
>> +	struct config_item *color_matching, *cm_default;
>> +	struct uvcg_cmd *cmd;
>> +
>> +	color_matching = config_group_find_item(to_config_group(streaming),
>> +						"color_matching");
>> +	if (!color_matching)
>> +		return NULL;
>> +
>> +	cm_default = config_group_find_item(to_config_group(color_matching),
>> +					    "default");
>> +	config_item_put(color_matching);
>> +	if (!cm_default)
>> +		return NULL;
>> +
>> +	cmd = to_uvcg_cmd(to_config_group(cm_default));
>> +	config_item_put(cm_default);
>> +
>> +	return cmd;
>> +}
>> +
>>   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>>   {
>>   	struct f_uvc_opts *opts;
>> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>>   		'Y',  'U',  'Y',  '2', 0x00, 0x00, 0x10, 0x00,
>>   		 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
>>   	};
>> +	struct config_item *streaming;
>>   	struct uvcg_uncompressed *h;
>> +	struct uvcg_cmd *cmd;
>> +
>> +	streaming = group->cg_item.ci_parent;
>> +	cmd = uvcg_format_get_default_cmd(streaming);
>> +	if (!cmd)
>> +		return ERR_PTR(-EINVAL);
>>   
>>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
>>   	if (!h)
>> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>>   
>>   	INIT_LIST_HEAD(&h->fmt.frames);
>>   	h->fmt.type = UVCG_UNCOMPRESSED;
>> +	h->fmt.color_matching = cmd;
> Is there any need to reference-count cmd to make sure it doesn't get
> deleted ? I don't expect that to be an issue for the default, but for
> the user-defined color matching descriptors. This may be better
> discussed in the context of other patches further in the series.


configfs_symlink() holds references to the targets, so you can't remove 
the color matching descriptors without first removing the symlink to them.

>
>>   	config_group_init_type_name(&h->fmt.group, name,
>>   				    &uvcg_uncompressed_type);
>>   
>> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
>>   static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>>   						   const char *name)
>>   {
>> +	struct config_item *streaming;
>>   	struct uvcg_mjpeg *h;
>> +	struct uvcg_cmd *cmd;
>> +
>> +	streaming = group->cg_item.ci_parent;
>> +	cmd = uvcg_format_get_default_cmd(streaming);
>> +	if (!cmd)
>> +		return ERR_PTR(-EINVAL);
>>   
>>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
>>   	if (!h)
>> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>>   
>>   	INIT_LIST_HEAD(&h->fmt.frames);
>>   	h->fmt.type = UVCG_MJPEG;
>> +	h->fmt.color_matching = cmd;
>>   	config_group_init_type_name(&h->fmt.group, name,
>>   				    &uvcg_mjpeg_type);
>>   
>> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
>>   enum uvcg_strm_type {
>>   	UVCG_HEADER = 0,
>>   	UVCG_FORMAT,
>> -	UVCG_FRAME
>> +	UVCG_FRAME,
>> +	UVCG_CMD,
>>   };
>>   
>>   /*
>> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
>>   			if (ret)
>>   				return ret;
>>   		}
>> +
>> +		ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
>> +		if (ret)
>> +			return ret;
>>   	}
>>   
>>   	return ret;
>> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
>>   		*size += frm->frame.b_frame_interval_type * sz;
>>   	}
>>   	break;
>> +	case UVCG_CMD: {
>> +		struct uvcg_cmd *cmd = priv1;
> Missing blank line. Same below.


Sorry; where below?

>
>> +		*size += sizeof(cmd->desc);
>> +	}
>> +	break;
>>   	}
>>   
>>   	++*count;
>> @@ -2096,6 +2144,13 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
>>   				frm->frame.b_frame_interval_type);
>>   	}
>>   	break;
>> +	case UVCG_CMD: {
>> +		struct uvcg_cmd *cmd = priv1;
>> +
>> +		memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
>> +		*dest += sizeof(cmd->desc);
>> +	}
>> +	break;
>>   	}
>>   
>>   	return 0;
>> @@ -2135,7 +2190,7 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>>   	if (ret)
>>   		goto unlock;
>>   
>> -	count += 2; /* color_matching, NULL */
>> +	count += 1; /* NULL */
>>   	*class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
>>   	if (!*class_array) {
>>   		ret = -ENOMEM;
>> @@ -2162,7 +2217,6 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
>>   		kfree(data_save);
>>   		goto unlock;
>>   	}
>> -	*cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
>>   
>>   	++target_hdr->linked;
>>   	ret = 0;
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
>> index f990739838d5..6582d6c7b6f6 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.h
>> +++ b/drivers/usb/gadget/function/uvc_configfs.h
>> @@ -57,6 +57,7 @@ struct uvcg_format {
>>   	struct list_head	frames;
>>   	unsigned		num_frames;
>>   	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>> +	struct uvcg_cmd		*color_matching;
>>   };
>>   
>>   struct uvcg_format_ptr {
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 9918e7b6a023..6f5932c9f09c 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -747,6 +747,28 @@  static const char * const uvcg_format_names[] = {
 	"mjpeg",
 };
 
+static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
+{
+	struct config_item *color_matching, *cm_default;
+	struct uvcg_cmd *cmd;
+
+	color_matching = config_group_find_item(to_config_group(streaming),
+						"color_matching");
+	if (!color_matching)
+		return NULL;
+
+	cm_default = config_group_find_item(to_config_group(color_matching),
+					    "default");
+	config_item_put(color_matching);
+	if (!cm_default)
+		return NULL;
+
+	cmd = to_uvcg_cmd(to_config_group(cm_default));
+	config_item_put(cm_default);
+
+	return cmd;
+}
+
 static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
 {
 	struct f_uvc_opts *opts;
@@ -1560,7 +1582,14 @@  static struct config_group *uvcg_uncompressed_make(struct config_group *group,
 		'Y',  'U',  'Y',  '2', 0x00, 0x00, 0x10, 0x00,
 		 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
 	};
+	struct config_item *streaming;
 	struct uvcg_uncompressed *h;
+	struct uvcg_cmd *cmd;
+
+	streaming = group->cg_item.ci_parent;
+	cmd = uvcg_format_get_default_cmd(streaming);
+	if (!cmd)
+		return ERR_PTR(-EINVAL);
 
 	h = kzalloc(sizeof(*h), GFP_KERNEL);
 	if (!h)
@@ -1579,6 +1608,7 @@  static struct config_group *uvcg_uncompressed_make(struct config_group *group,
 
 	INIT_LIST_HEAD(&h->fmt.frames);
 	h->fmt.type = UVCG_UNCOMPRESSED;
+	h->fmt.color_matching = cmd;
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_uncompressed_type);
 
@@ -1743,7 +1773,14 @@  static const struct config_item_type uvcg_mjpeg_type = {
 static struct config_group *uvcg_mjpeg_make(struct config_group *group,
 						   const char *name)
 {
+	struct config_item *streaming;
 	struct uvcg_mjpeg *h;
+	struct uvcg_cmd *cmd;
+
+	streaming = group->cg_item.ci_parent;
+	cmd = uvcg_format_get_default_cmd(streaming);
+	if (!cmd)
+		return ERR_PTR(-EINVAL);
 
 	h = kzalloc(sizeof(*h), GFP_KERNEL);
 	if (!h)
@@ -1760,6 +1797,7 @@  static struct config_group *uvcg_mjpeg_make(struct config_group *group,
 
 	INIT_LIST_HEAD(&h->fmt.frames);
 	h->fmt.type = UVCG_MJPEG;
+	h->fmt.color_matching = cmd;
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_mjpeg_type);
 
@@ -1906,7 +1944,8 @@  static inline struct uvc_descriptor_header
 enum uvcg_strm_type {
 	UVCG_HEADER = 0,
 	UVCG_FORMAT,
-	UVCG_FRAME
+	UVCG_FRAME,
+	UVCG_CMD,
 };
 
 /*
@@ -1956,6 +1995,10 @@  static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
 			if (ret)
 				return ret;
 		}
+
+		ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
+		if (ret)
+			return ret;
 	}
 
 	return ret;
@@ -2011,6 +2054,11 @@  static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
 		*size += frm->frame.b_frame_interval_type * sz;
 	}
 	break;
+	case UVCG_CMD: {
+		struct uvcg_cmd *cmd = priv1;
+		*size += sizeof(cmd->desc);
+	}
+	break;
 	}
 
 	++*count;
@@ -2096,6 +2144,13 @@  static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n,
 				frm->frame.b_frame_interval_type);
 	}
 	break;
+	case UVCG_CMD: {
+		struct uvcg_cmd *cmd = priv1;
+
+		memcpy(*dest, &cmd->desc, sizeof(cmd->desc));
+		*dest += sizeof(cmd->desc);
+	}
+	break;
 	}
 
 	return 0;
@@ -2135,7 +2190,7 @@  static int uvcg_streaming_class_allow_link(struct config_item *src,
 	if (ret)
 		goto unlock;
 
-	count += 2; /* color_matching, NULL */
+	count += 1; /* NULL */
 	*class_array = kcalloc(count, sizeof(void *), GFP_KERNEL);
 	if (!*class_array) {
 		ret = -ENOMEM;
@@ -2162,7 +2217,6 @@  static int uvcg_streaming_class_allow_link(struct config_item *src,
 		kfree(data_save);
 		goto unlock;
 	}
-	*cl_arr = (struct uvc_descriptor_header *)&opts->uvc_color_matching;
 
 	++target_hdr->linked;
 	ret = 0;
diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
index f990739838d5..6582d6c7b6f6 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -57,6 +57,7 @@  struct uvcg_format {
 	struct list_head	frames;
 	unsigned		num_frames;
 	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
+	struct uvcg_cmd		*color_matching;
 };
 
 struct uvcg_format_ptr {