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

[Dan Scally <dan.scally@ideasonboard.com>]

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 {