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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

On Mon, Dec 19, 2022 at 10:33:39AM +0000, Dan Scally wrote:
> On 18/12/2022 23:28, Laurent Pinchart wrote:
> > On Tue, Dec 13, 2022 at 08:37:33AM +0000, Daniel Scally wrote:
> >> As currently implemented the default color matching descriptor is
> >> appended after _all_ the formats and frames that the gadget is
> >> configured with. According to the UVC specifications however this
> >> is supposed to be on a per-format basis (section 3.9.2.6):
> >>
> >> "Only one instance is allowed for a given format and if present,
> >> the Color Matching descriptor shall be placed following the Video
> >> and Still Image Frame descriptors for that format."
> >>
> >> Associate the default color matching descriptor with struct
> >> uvcg_format and copy it once-per-format instead of once only.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >>   drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++--
> >>   drivers/usb/gadget/function/uvc_configfs.h |  1 +
> >>   2 files changed, 58 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index 9918e7b6a023..6f5932c9f09c 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -747,6 +747,28 @@ static const char * const uvcg_format_names[] = {
> >>   	"mjpeg",
> >>   };
> >>   
> >> +static struct uvcg_cmd *uvcg_format_get_default_cmd(struct config_item *streaming)
> >> +{
> >> +	struct config_item *color_matching, *cm_default;
> >> +	struct uvcg_cmd *cmd;
> >> +
> >> +	color_matching = config_group_find_item(to_config_group(streaming),
> >> +						"color_matching");
> >> +	if (!color_matching)
> >> +		return NULL;
> >> +
> >> +	cm_default = config_group_find_item(to_config_group(color_matching),
> >> +					    "default");
> >> +	config_item_put(color_matching);
> >> +	if (!cm_default)
> >> +		return NULL;
> >> +
> >> +	cmd = to_uvcg_cmd(to_config_group(cm_default));
> >> +	config_item_put(cm_default);
> >> +
> >> +	return cmd;
> >> +}
> >> +
> >>   static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> >>   {
> >>   	struct f_uvc_opts *opts;
> >> @@ -1560,7 +1582,14 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> >>   		'Y',  'U',  'Y',  '2', 0x00, 0x00, 0x10, 0x00,
> >>   		 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
> >>   	};
> >> +	struct config_item *streaming;
> >>   	struct uvcg_uncompressed *h;
> >> +	struct uvcg_cmd *cmd;
> >> +
> >> +	streaming = group->cg_item.ci_parent;
> >> +	cmd = uvcg_format_get_default_cmd(streaming);
> >> +	if (!cmd)
> >> +		return ERR_PTR(-EINVAL);
> >>   
> >>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
> >>   	if (!h)
> >> @@ -1579,6 +1608,7 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> >>   
> >>   	INIT_LIST_HEAD(&h->fmt.frames);
> >>   	h->fmt.type = UVCG_UNCOMPRESSED;
> >> +	h->fmt.color_matching = cmd;
> >
> > Is there any need to reference-count cmd to make sure it doesn't get
> > deleted ? I don't expect that to be an issue for the default, but for
> > the user-defined color matching descriptors. This may be better
> > discussed in the context of other patches further in the series.
> 
> configfs_symlink() holds references to the targets, so you can't remove 
> the color matching descriptors without first removing the symlink to them.
> 
> >>   	config_group_init_type_name(&h->fmt.group, name,
> >>   				    &uvcg_uncompressed_type);
> >>   
> >> @@ -1743,7 +1773,14 @@ static const struct config_item_type uvcg_mjpeg_type = {
> >>   static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> >>   						   const char *name)
> >>   {
> >> +	struct config_item *streaming;
> >>   	struct uvcg_mjpeg *h;
> >> +	struct uvcg_cmd *cmd;
> >> +
> >> +	streaming = group->cg_item.ci_parent;
> >> +	cmd = uvcg_format_get_default_cmd(streaming);
> >> +	if (!cmd)
> >> +		return ERR_PTR(-EINVAL);
> >>   
> >>   	h = kzalloc(sizeof(*h), GFP_KERNEL);
> >>   	if (!h)
> >> @@ -1760,6 +1797,7 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> >>   
> >>   	INIT_LIST_HEAD(&h->fmt.frames);
> >>   	h->fmt.type = UVCG_MJPEG;
> >> +	h->fmt.color_matching = cmd;
> >>   	config_group_init_type_name(&h->fmt.group, name,
> >>   				    &uvcg_mjpeg_type);
> >>   
> >> @@ -1906,7 +1944,8 @@ static inline struct uvc_descriptor_header
> >>   enum uvcg_strm_type {
> >>   	UVCG_HEADER = 0,
> >>   	UVCG_FORMAT,
> >> -	UVCG_FRAME
> >> +	UVCG_FRAME,
> >> +	UVCG_CMD,
> >>   };
> >>   
> >>   /*
> >> @@ -1956,6 +1995,10 @@ static int __uvcg_iter_strm_cls(struct uvcg_streaming_header *h,
> >>   			if (ret)
> >>   				return ret;
> >>   		}
> >> +
> >> +		ret = fun(f->fmt->color_matching, priv2, priv3, 0, UVCG_CMD);
> >> +		if (ret)
> >> +			return ret;
> >>   	}
> >>   
> >>   	return ret;
> >> @@ -2011,6 +2054,11 @@ static int __uvcg_cnt_strm(void *priv1, void *priv2, void *priv3, int n,
> >>   		*size += frm->frame.b_frame_interval_type * sz;
> >>   	}
> >>   	break;
> >> +	case UVCG_CMD: {
> >> +		struct uvcg_cmd *cmd = priv1;
> >
> > Missing blank line. Same below.
> 
> Sorry; where below?

Nowhere, my bad :-)

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

-- 
Regards,

Laurent Pinchart