[PATCH 0/3] usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS

[PATCH 0/3] usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS

[Michael Grzeschik]

This series improves the uvc video gadget by parsing the configfs
entries. With the configfs data, the driver now is able to negotiate the
format with the usb host in the kernel and also exports the supported
frames/formats/intervals via the v4l2 VIDIOC interface.

Michael Grzeschik (3):
  usb: gadget: uvc: move structs to common header
  usb: gadget: uvc: add VIDIOC function
  usb: gadget: uvc: add format/frame handling code

 drivers/usb/gadget/function/f_uvc.c        | 324 +++++++++++++++++++-
 drivers/usb/gadget/function/uvc.h          |  32 +-
 drivers/usb/gadget/function/uvc_configfs.c | 116 +------
 drivers/usb/gadget/function/uvc_configfs.h | 121 ++++++++
 drivers/usb/gadget/function/uvc_queue.c    |   4 +-
 drivers/usb/gadget/function/uvc_v4l2.c     | 335 ++++++++++++++++++---
 drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
 drivers/usb/gadget/function/uvc_video.c    |  23 +-
 8 files changed, 781 insertions(+), 175 deletions(-)

-- 
2.29.2

[PATCH 1/3] usb: gadget: uvc: move structs to common header

[Michael Grzeschik]

The functions and structs of the configfs interface should also be used
by the uvc gadget driver. This patch prepares the stack by moving the
common structs and functions to the common header file.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_configfs.c | 111 -------------------
 drivers/usb/gadget/function/uvc_configfs.h | 119 +++++++++++++++++++++
 2 files changed, 119 insertions(+), 111 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 77d64031aa9c2..86463bb2639ed 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -19,8 +19,6 @@
  * Global Utility Structures and Macros
  */
 
-#define UVCG_STREAMING_CONTROL_SIZE	1
-
 #define UVC_ATTR(prefix, cname, aname) \
 static struct configfs_attribute prefix##attr_##cname = { \
 	.ca_name	= __stringify(aname),				\
@@ -49,12 +47,6 @@ static int uvcg_config_compare_u32(const void *l, const void *r)
 	return li < ri ? -1 : li == ri ? 0 : 1;
 }
 
-static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
-{
-	return container_of(to_config_group(item), struct f_uvc_opts,
-			    func_inst.group);
-}
-
 struct uvcg_config_group_type {
 	struct config_item_type type;
 	const char *name;
@@ -125,19 +117,6 @@ static void uvcg_config_remove_children(struct config_group *group)
  * control/header
  */
 
-DECLARE_UVC_HEADER_DESCRIPTOR(1);
-
-struct uvcg_control_header {
-	struct config_item		item;
-	struct UVC_HEADER_DESCRIPTOR(1)	desc;
-	unsigned			linked;
-};
-
-static struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
-{
-	return container_of(item, struct uvcg_control_header, item);
-}
-
 #define UVCG_CTRL_HDR_ATTR(cname, aname, bits, limit)			\
 static ssize_t uvcg_control_header_##cname##_show(			\
 	struct config_item *item, char *page)				\
@@ -764,29 +743,6 @@ static const struct uvcg_config_group_type uvcg_control_grp_type = {
  * streaming/mjpeg
  */
 
-static const char * const uvcg_format_names[] = {
-	"uncompressed",
-	"mjpeg",
-};
-
-enum uvcg_format_type {
-	UVCG_UNCOMPRESSED = 0,
-	UVCG_MJPEG,
-};
-
-struct uvcg_format {
-	struct config_group	group;
-	enum uvcg_format_type	type;
-	unsigned		linked;
-	unsigned		num_frames;
-	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
-};
-
-static struct uvcg_format *to_uvcg_format(struct config_item *item)
-{
-	return container_of(to_config_group(item), struct uvcg_format, group);
-}
-
 static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
 {
 	struct f_uvc_opts *opts;
@@ -845,29 +801,11 @@ static ssize_t uvcg_format_bma_controls_store(struct uvcg_format *ch,
 	return ret;
 }
 
-struct uvcg_format_ptr {
-	struct uvcg_format	*fmt;
-	struct list_head	entry;
-};
-
 /* -----------------------------------------------------------------------------
  * streaming/header/<NAME>
  * streaming/header
  */
 
-struct uvcg_streaming_header {
-	struct config_item				item;
-	struct uvc_input_header_descriptor		desc;
-	unsigned					linked;
-	struct list_head				formats;
-	unsigned					num_fmt;
-};
-
-static struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
-{
-	return container_of(item, struct uvcg_streaming_header, item);
-}
-
 static void uvcg_format_set_indices(struct config_group *fmt);
 
 static int uvcg_streaming_header_allow_link(struct config_item *src,
@@ -1059,31 +997,6 @@ static const struct uvcg_config_group_type uvcg_streaming_header_grp_type = {
  * streaming/<mode>/<format>/<NAME>
  */
 
-struct uvcg_frame {
-	struct config_item	item;
-	enum uvcg_format_type	fmt_type;
-	struct {
-		u8	b_length;
-		u8	b_descriptor_type;
-		u8	b_descriptor_subtype;
-		u8	b_frame_index;
-		u8	bm_capabilities;
-		u16	w_width;
-		u16	w_height;
-		u32	dw_min_bit_rate;
-		u32	dw_max_bit_rate;
-		u32	dw_max_video_frame_buffer_size;
-		u32	dw_default_frame_interval;
-		u8	b_frame_interval_type;
-	} __attribute__((packed)) frame;
-	u32 *dw_frame_interval;
-};
-
-static struct uvcg_frame *to_uvcg_frame(struct config_item *item)
-{
-	return container_of(item, struct uvcg_frame, item);
-}
-
 #define UVCG_FRAME_ATTR(cname, aname, bits) \
 static ssize_t uvcg_frame_##cname##_show(struct config_item *item, char *page)\
 {									\
@@ -1420,18 +1333,6 @@ static void uvcg_format_set_indices(struct config_group *fmt)
  * streaming/uncompressed/<NAME>
  */
 
-struct uvcg_uncompressed {
-	struct uvcg_format		fmt;
-	struct uvc_format_uncompressed	desc;
-};
-
-static struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
-{
-	return container_of(
-		container_of(to_config_group(item), struct uvcg_format, group),
-		struct uvcg_uncompressed, fmt);
-}
-
 static struct configfs_group_operations uvcg_uncompressed_group_ops = {
 	.make_item		= uvcg_frame_make,
 	.drop_item		= uvcg_frame_drop,
@@ -1669,18 +1570,6 @@ static const struct uvcg_config_group_type uvcg_uncompressed_grp_type = {
  * streaming/mjpeg/<NAME>
  */
 
-struct uvcg_mjpeg {
-	struct uvcg_format		fmt;
-	struct uvc_format_mjpeg		desc;
-};
-
-static struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
-{
-	return container_of(
-		container_of(to_config_group(item), struct uvcg_format, group),
-		struct uvcg_mjpeg, fmt);
-}
-
 static struct configfs_group_operations uvcg_mjpeg_group_ops = {
 	.make_item		= uvcg_frame_make,
 	.drop_item		= uvcg_frame_drop,
diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
index 7e1d7ca29bf21..f905d29570eb4 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -16,4 +16,123 @@ struct f_uvc_opts;
 
 int uvcg_attach_configfs(struct f_uvc_opts *opts);
 
+static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct f_uvc_opts,
+			    func_inst.group);
+}
+
+#define UVCG_STREAMING_CONTROL_SIZE	1
+
+DECLARE_UVC_HEADER_DESCRIPTOR(1);
+
+struct uvcg_control_header {
+	struct config_item		item;
+	struct UVC_HEADER_DESCRIPTOR(1)	desc;
+	unsigned			linked;
+};
+
+static inline struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
+{
+	return container_of(item, struct uvcg_control_header, item);
+}
+
+static const char * const uvcg_format_names[] = {
+	"uncompressed",
+	"mjpeg",
+};
+
+enum uvcg_format_type {
+	UVCG_UNCOMPRESSED = 0,
+	UVCG_MJPEG,
+};
+
+struct uvcg_format {
+	struct config_group	group;
+	enum uvcg_format_type	type;
+	unsigned		linked;
+	unsigned		num_frames;
+	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
+};
+
+struct uvcg_format_ptr {
+	struct uvcg_format	*fmt;
+	struct list_head	entry;
+};
+
+static inline struct uvcg_format *to_uvcg_format(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct uvcg_format, group);
+}
+
+struct uvcg_streaming_header {
+	struct config_item				item;
+	struct uvc_input_header_descriptor		desc;
+	unsigned					linked;
+	struct list_head				formats;
+	unsigned					num_fmt;
+};
+
+static inline struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
+{
+	return container_of(item, struct uvcg_streaming_header, item);
+}
+
+struct uvcg_frame {
+	struct config_item	item;
+	enum uvcg_format_type	fmt_type;
+	struct {
+		u8	b_length;
+		u8	b_descriptor_type;
+		u8	b_descriptor_subtype;
+		u8	b_frame_index;
+		u8	bm_capabilities;
+		u16	w_width;
+		u16	w_height;
+		u32	dw_min_bit_rate;
+		u32	dw_max_bit_rate;
+		u32	dw_max_video_frame_buffer_size;
+		u32	dw_default_frame_interval;
+		u8	b_frame_interval_type;
+	} __attribute__((packed)) frame;
+	u32 *dw_frame_interval;
+};
+
+static inline struct uvcg_frame *to_uvcg_frame(struct config_item *item)
+{
+	return container_of(item, struct uvcg_frame, item);
+}
+
+/* -----------------------------------------------------------------------------
+ * streaming/uncompressed/<NAME>
+ */
+
+struct uvcg_uncompressed {
+	struct uvcg_format		fmt;
+	struct uvc_format_uncompressed	desc;
+};
+
+static inline struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
+{
+	return container_of(
+		container_of(to_config_group(item), struct uvcg_format, group),
+		struct uvcg_uncompressed, fmt);
+}
+
+/* -----------------------------------------------------------------------------
+ * streaming/mjpeg/<NAME>
+ */
+
+struct uvcg_mjpeg {
+	struct uvcg_format		fmt;
+	struct uvc_format_mjpeg		desc;
+};
+
+static inline struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
+{
+	return container_of(
+		container_of(to_config_group(item), struct uvcg_format, group),
+		struct uvcg_mjpeg, fmt);
+}
+
 #endif /* UVC_CONFIGFS_H */
-- 
2.29.2

[PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[Michael Grzeschik]

This patch adds support to the v4l2 VIDIOC for enum_format,
enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
setup gets parsed and this configfs data is used in the v4l2 interface
functions.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c        |  54 ++++
 drivers/usb/gadget/function/uvc.h          |  18 +-
 drivers/usb/gadget/function/uvc_configfs.c |   5 +
 drivers/usb/gadget/function/uvc_configfs.h |   2 +
 drivers/usb/gadget/function/uvc_queue.c    |   4 +-
 drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
 drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
 drivers/usb/gadget/function/uvc_video.c    |  10 +-
 8 files changed, 369 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index f48a00e497945..7945ea93a775a 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -410,6 +410,44 @@ static ssize_t function_name_show(struct device *dev,
 
 static DEVICE_ATTR_RO(function_name);
 
+static int
+uvc_analyze_configfs(struct uvc_device *uvc)
+{
+	struct uvcg_streaming_header *src_hdr = uvc->h;
+	struct config_item *item;
+	struct config_group *grp;
+	struct uvcg_format_ptr *f;
+	int i = 0, j = 0;
+
+	if (!src_hdr->linked)
+		return -EBUSY;
+
+	list_for_each_entry(f, &src_hdr->formats, entry)
+		uvc->nframes += f->fmt->num_frames;
+
+	uvc->nformats = src_hdr->num_fmt;
+
+	uvc->frm = kcalloc(uvc->nframes, sizeof(struct uvcg_frame *), GFP_KERNEL);
+	if (!uvc->frm)
+		return -ENOMEM;
+
+	uvc->fmt = kcalloc(uvc->nformats, sizeof(struct uvcg_format *), GFP_KERNEL);
+	if (!uvc->fmt) {
+		kfree(uvc->frm);
+		return -ENOMEM;
+	}
+
+	list_for_each_entry(f, &src_hdr->formats, entry) {
+		uvc->fmt[i++] = f->fmt;
+		grp = &f->fmt->group;
+		list_for_each_entry(item, &grp->cg_children, ci_entry) {
+			uvc->frm[j++] = to_uvcg_frame(item);
+		}
+	}
+
+	return 0;
+}
+
 static int
 uvc_register_video(struct uvc_device *uvc)
 {
@@ -742,6 +780,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 		goto error;
 	}
 
+	/* Register configfs formats and frames. */
+	ret = uvc_analyze_configfs(uvc);
+	if (ret < 0) {
+		uvcg_err(f, "failed to read configfs\n");
+		goto v4l2_error;
+	}
+
 	/* Initialise video. */
 	ret = uvcg_video_init(&uvc->video, uvc);
 	if (ret < 0)
@@ -905,6 +950,8 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 	struct uvc_device *uvc;
 	struct f_uvc_opts *opts;
 	struct uvc_descriptor_header **strm_cls;
+	struct config_item *item;
+	struct config_group *grp;
 
 	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
 	if (uvc == NULL)
@@ -936,6 +983,13 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 	uvc->desc.fs_streaming = opts->fs_streaming;
 	uvc->desc.hs_streaming = opts->hs_streaming;
 	uvc->desc.ss_streaming = opts->ss_streaming;
+
+	grp = &opts->func_inst.group;
+	item = config_group_find_item(grp, "streaming");
+	item = config_group_find_item(to_config_group(item), "header");
+	item = config_group_find_item(to_config_group(item), "h");
+	uvc->h = to_uvcg_streaming_header(item);
+
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 23ee25383c1f7..62d7420a25666 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -80,11 +80,10 @@ struct uvc_video {
 	struct work_struct pump;
 
 	/* Frame parameters */
-	u8 bpp;
-	u32 fcc;
-	unsigned int width;
-	unsigned int height;
-	unsigned int imagesize;
+	struct uvcg_format *def_format;
+	struct uvcg_format *cur_format;
+	struct uvcg_frame *cur_frame;
+
 	struct mutex mutex;	/* protects frame parameters */
 
 	/* Requests */
@@ -118,6 +117,14 @@ struct uvc_device {
 	struct usb_function func;
 	struct uvc_video video;
 
+	struct uvcg_streaming_header *h;
+
+	struct uvcg_frame **frm;
+	int nframes;
+
+	struct uvcg_format **fmt;
+	int nformats;
+
 	/* Descriptors */
 	struct {
 		const struct uvc_descriptor_header * const *fs_control;
@@ -162,4 +169,5 @@ extern void uvc_endpoint_stream(struct uvc_device *dev);
 extern void uvc_function_connect(struct uvc_device *uvc);
 extern void uvc_function_disconnect(struct uvc_device *uvc);
 
+extern int uvc_frame_default(struct uvcg_format *ufmt);
 #endif /* _UVC_GADGET_H_ */
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 86463bb2639ed..009c80d0e1780 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/sort.h>
+#include <linux/videodev2.h>
 
 #include "u_uvc.h"
 #include "uvc_configfs.h"
@@ -1547,6 +1548,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
 	h->desc.bCopyProtect		= 0;
 
 	h->fmt.type = UVCG_UNCOMPRESSED;
+	h->fmt.fcc = V4L2_PIX_FMT_YUYV;
+	h->fmt.name = "YUV 4:2:2 (YUYV)";
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_uncompressed_type);
 
@@ -1721,6 +1724,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
 	h->desc.bCopyProtect		= 0;
 
 	h->fmt.type = UVCG_MJPEG;
+	h->fmt.fcc = V4L2_PIX_FMT_MJPEG;
+	h->fmt.name = "MJPEG";
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_mjpeg_type);
 
diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
index f905d29570eb4..8ed966275f838 100644
--- a/drivers/usb/gadget/function/uvc_configfs.h
+++ b/drivers/usb/gadget/function/uvc_configfs.h
@@ -52,6 +52,8 @@ struct uvcg_format {
 	enum uvcg_format_type	type;
 	unsigned		linked;
 	unsigned		num_frames;
+	char			*name;
+	u32			fcc;
 	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
 };
 
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 61e2c94cc0b0c..6afc4b79adfe9 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -20,6 +20,8 @@
 #include <media/videobuf2-vmalloc.h>
 
 #include "uvc.h"
+#include "u_uvc.h"
+#include "uvc_configfs.h"
 
 /* ------------------------------------------------------------------------
  * Video buffers queue management.
@@ -49,7 +51,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	*nplanes = 1;
 
-	sizes[0] = video->imagesize;
+	sizes[0] = video->cur_frame->frame.dw_max_video_frame_buffer_size;
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 4ca89eab61590..83830b8864a6e 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -24,6 +24,127 @@
 #include "uvc_queue.h"
 #include "uvc_video.h"
 #include "uvc_v4l2.h"
+#include "u_uvc.h"
+#include "uvc_configfs.h"
+
+u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm)
+{
+	struct uvcg_uncompressed *u;
+
+	switch (fmt->type) {
+	case UVCG_UNCOMPRESSED:
+		u = to_uvcg_uncompressed(&fmt->group.cg_item);
+		if (!u)
+			return 0;
+
+		return u->desc.bBitsPerPixel * frm->frame.w_width / 8;
+	case UVCG_MJPEG:
+		return frm->frame.w_width;
+	}
+
+	return 0;
+}
+
+struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
+					      struct uvcg_format *ufmt,
+					      int index)
+{
+	int i;
+
+	for (i = 0; i < uvc->nframes; i++) {
+		if (uvc->frm[i]->fmt_type != ufmt->type)
+			continue;
+
+		if (index == uvc->frm[i]->frame.b_frame_index)
+			break;
+	}
+
+	if (i == uvc->nframes)
+		return NULL;
+
+	return uvc->frm[i];
+}
+
+static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
+						    u32 pixelformat)
+{
+	int i;
+
+	for (i = 0; i < uvc->nformats; i++)
+		if (uvc->fmt[i]->fcc == pixelformat)
+			break;
+
+	if (i == uvc->nformats)
+		return NULL;
+
+	return uvc->fmt[i];
+}
+
+int uvc_frame_default(struct uvcg_format *ufmt)
+{
+	struct uvcg_uncompressed *m;
+	struct uvcg_uncompressed *u;
+	int ret = 1;
+
+	switch (ufmt->type) {
+	case UVCG_UNCOMPRESSED:
+		u = to_uvcg_uncompressed(&ufmt->group.cg_item);
+		if (u)
+			ret = u->desc.bDefaultFrameIndex;
+		break;
+	case UVCG_MJPEG:
+		m = to_uvcg_uncompressed(&ufmt->group.cg_item);
+		if (m)
+			ret = m->desc.bDefaultFrameIndex;
+		break;
+	}
+
+	if (!ret)
+		ret = 1;
+
+	return ret;
+}
+
+static struct uvcg_frame *find_frm_by_size(struct uvc_device *uvc,
+					   struct uvcg_format *ufmt,
+					   u16 rw, u16 rh)
+{
+	struct uvc_video *video = &uvc->video;
+	struct uvcg_frame *ufrm = NULL;
+	unsigned int d, maxd;
+	int i;
+
+	/* Find the closest image size. The distance between image sizes is
+	 * the size in pixels of the non-overlapping regions between the
+	 * requested size and the frame-specified size.
+	 */
+	maxd = (unsigned int)-1;
+
+	for (i = 0; i < uvc->nframes; i++) {
+		u16 w, h;
+
+		if (uvc->frm[i]->fmt_type != ufmt->type)
+			continue;
+
+		w = uvc->frm[i]->frame.w_width;
+		h = uvc->frm[i]->frame.w_height;
+
+		d = min(w, rw) * min(h, rh);
+		d = w*h + rw*rh - 2*d;
+		if (d < maxd) {
+			maxd = d;
+			ufrm = uvc->frm[i];
+		}
+
+		if (maxd == 0)
+			break;
+	}
+
+	if (ufrm == NULL)
+		uvcg_dbg(&video->uvc->func, "Unsupported size %ux%u\n", rw, rh);
+
+	return ufrm;
+}
 
 /* --------------------------------------------------------------------------
  * Requests handling
@@ -50,16 +171,6 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
  * V4L2 ioctls
  */
 
-struct uvc_format {
-	u8 bpp;
-	u32 fcc;
-};
-
-static struct uvc_format uvc_formats[] = {
-	{ 16, V4L2_PIX_FMT_YUYV  },
-	{ 0,  V4L2_PIX_FMT_MJPEG },
-};
-
 static int
 uvc_v4l2_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
 {
@@ -81,55 +192,187 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
 
-	fmt->fmt.pix.pixelformat = video->fcc;
-	fmt->fmt.pix.width = video->width;
-	fmt->fmt.pix.height = video->height;
+	fmt->fmt.pix.pixelformat = video->cur_format->fcc;
+	fmt->fmt.pix.width = video->cur_frame->frame.w_width;
+	fmt->fmt.pix.height = video->cur_frame->frame.w_height;
+	fmt->fmt.pix.field = V4L2_FIELD_NONE;
+	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format, video->cur_frame);
+	fmt->fmt.pix.sizeimage = video->cur_frame->frame.dw_max_video_frame_buffer_size;
+	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->fmt.pix.priv = 0;
+
+	return 0;
+}
+
+static int _uvc_v4l2_try_fmt(struct uvc_video *video,
+	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
+{
+	struct uvc_device *uvc = video->uvc;
+	struct uvcg_format *ufmt;
+	struct uvcg_frame *ufrm;
+	u8 *fcc;
+	int i;
+
+	if (fmt->type != video->queue.queue.type)
+		return -EINVAL;
+
+	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
+	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
+		fmt->fmt.pix.pixelformat,
+		fcc[0], fcc[1], fcc[2], fcc[3],
+		fmt->fmt.pix.width, fmt->fmt.pix.height);
+
+	for (i = 0; i < uvc->nformats; i++)
+		if (uvc->fmt[i]->fcc == fmt->fmt.pix.pixelformat)
+			break;
+
+	if (i == uvc->nformats)
+		ufmt = video->def_format;
+
+	ufmt = uvc->fmt[i];
+
+	ufrm = find_frm_by_size(uvc, ufmt,
+				fmt->fmt.pix.width, fmt->fmt.pix.height);
+	if (!ufrm)
+		return -EINVAL;
+
+	fmt->fmt.pix.width = ufrm->frame.w_width;
+	fmt->fmt.pix.height = ufrm->frame.w_height;
 	fmt->fmt.pix.field = V4L2_FIELD_NONE;
-	fmt->fmt.pix.bytesperline = video->bpp * video->width / 8;
-	fmt->fmt.pix.sizeimage = video->imagesize;
+	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(ufmt, ufrm);
+	fmt->fmt.pix.sizeimage = ufrm->frame.dw_max_video_frame_buffer_size;
+	fmt->fmt.pix.pixelformat = ufmt->fcc;
 	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
 	fmt->fmt.pix.priv = 0;
 
+	if (!fmt->fmt.pix.sizeimage && fmt->fmt.pix.bytesperline)
+		fmt->fmt.pix.sizeimage = fmt->fmt.pix.bytesperline *
+				fmt->fmt.pix.height;
+
+	if (uvc_format != NULL)
+		*uvc_format = ufmt;
+	if (uvc_frame != NULL)
+		*uvc_frame = ufrm;
+
 	return 0;
 }
 
+static int
+uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvc_video *video = &uvc->video;
+
+	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
+}
+
 static int
 uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
-	struct uvc_format *format;
-	unsigned int imagesize;
-	unsigned int bpl;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(uvc_formats); ++i) {
-		format = &uvc_formats[i];
-		if (format->fcc == fmt->fmt.pix.pixelformat)
+	struct uvcg_format *ufmt;
+	struct uvcg_frame *ufrm;
+	int ret;
+
+	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
+	if (ret)
+		return ret;
+
+	video->cur_format = ufmt;
+	video->cur_frame = ufrm;
+
+	return ret;
+}
+
+static int
+uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
+		struct v4l2_frmivalenum *fival)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvcg_format *ufmt = NULL;
+	struct uvcg_frame *ufrm = NULL;
+	int i;
+
+	ufmt = find_format_by_pix(uvc, fival->pixel_format);
+	if (!ufmt)
+		return -EINVAL;
+
+	for (i = 0; i < uvc->nframes; ++i) {
+		if (uvc->frm[i]->fmt_type != ufmt->type)
+			continue;
+
+		if (uvc->frm[i]->frame.w_width == fival->width &&
+				uvc->frm[i]->frame.w_height == fival->height) {
+			ufrm = uvc->frm[i];
 			break;
+		}
 	}
+	if (!ufrm)
+		return -EINVAL;
 
-	if (i == ARRAY_SIZE(uvc_formats)) {
-		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
-			  fmt->fmt.pix.pixelformat);
+	if (fival->index >= ufrm->frame.b_frame_interval_type)
 		return -EINVAL;
-	}
 
-	bpl = format->bpp * fmt->fmt.pix.width / 8;
-	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
+	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
+	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
+	fival->discrete.numerator = ufrm->dw_frame_interval[fival->index];
+	fival->discrete.denominator = 10000000;
+	v4l2_simplify_fraction(&fival->discrete.numerator,
+		&fival->discrete.denominator, 8, 333);
 
-	video->fcc = format->fcc;
-	video->bpp = format->bpp;
-	video->width = fmt->fmt.pix.width;
-	video->height = fmt->fmt.pix.height;
-	video->imagesize = imagesize;
+	return 0;
+}
 
-	fmt->fmt.pix.field = V4L2_FIELD_NONE;
-	fmt->fmt.pix.bytesperline = bpl;
-	fmt->fmt.pix.sizeimage = imagesize;
-	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
-	fmt->fmt.pix.priv = 0;
+static int
+uvc_v4l2_enum_framesizes(struct file *file, void *fh,
+		struct v4l2_frmsizeenum *fsize)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvcg_format *ufmt = NULL;
+	struct uvcg_frame *ufrm = NULL;
+
+	ufmt = find_format_by_pix(uvc, fsize->pixel_format);
+	if (!ufmt)
+		return -EINVAL;
+
+	if (fsize->index >= ufmt->num_frames)
+		return -EINVAL;
+
+	ufrm = find_frame_by_index(uvc, ufmt, fsize->index + 1);
+	if (!ufrm)
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+	fsize->discrete.width = ufrm->frame.w_width;
+	fsize->discrete.height = ufrm->frame.w_height;
+
+	return 0;
+}
+
+static int
+uvc_v4l2_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvcg_format *ufmt;
+
+	if (f->index >= uvc->nformats)
+		return -EINVAL;
+
+	ufmt = uvc->fmt[f->index];
+	if (!ufmt)
+		return -EINVAL;
+
+	f->pixelformat = ufmt->fcc;
+	f->flags |= V4L2_FMT_FLAG_COMPRESSED;
+
+	strscpy(f->description, ufmt->name, sizeof(f->description));
+	f->description[sizeof(f->description) - 1] = 0;
 
 	return 0;
 }
@@ -258,8 +501,12 @@ uvc_v4l2_ioctl_default(struct file *file, void *fh, bool valid_prio,
 
 const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
 	.vidioc_querycap = uvc_v4l2_querycap,
+	.vidioc_try_fmt_vid_out = uvc_v4l2_try_fmt,
 	.vidioc_g_fmt_vid_out = uvc_v4l2_get_format,
 	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
+	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
+	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
+	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_fmt,
 	.vidioc_reqbufs = uvc_v4l2_reqbufs,
 	.vidioc_querybuf = uvc_v4l2_querybuf,
 	.vidioc_qbuf = uvc_v4l2_qbuf,
diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
index 1576005b61fd3..6e45103bbf793 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.h
+++ b/drivers/usb/gadget/function/uvc_v4l2.h
@@ -15,5 +15,6 @@
 
 extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
 extern const struct v4l2_file_operations uvc_v4l2_fops;
+extern u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm);
 
 #endif /* __UVC_V4L2_H__ */
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 633e23d58d868..b14780bddd838 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -17,7 +17,10 @@
 
 #include "uvc.h"
 #include "uvc_queue.h"
+#include "uvc_v4l2.h"
 #include "uvc_video.h"
+#include "u_uvc.h"
+#include "uvc_configfs.h"
 
 /* --------------------------------------------------------------------------
  * Video codecs
@@ -348,11 +351,8 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 	INIT_WORK(&video->pump, uvcg_video_pump);
 
 	video->uvc = uvc;
-	video->fcc = V4L2_PIX_FMT_YUYV;
-	video->bpp = 16;
-	video->width = 320;
-	video->height = 240;
-	video->imagesize = 320 * 240 * 2;
+	video->def_format = video->cur_format = uvc->fmt[0];
+	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];
 
 	/* Initialize the video buffers queue. */
 	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
-- 
2.29.2

[PATCH 3/3] usb: gadget: uvc: add format/frame handling code

[Michael Grzeschik]

The hostside format selection is currently only done in userspace, as
the events for SET_CUR and GET_CUR is allways moved to the application
layer. Since the v4l2 devices parses the configfs data, the format
negotiation can be done in the kernel. This patch adds the functions to
set the curent configuration and still forwards all unknown events to
the userspace level.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c     | 270 ++++++++++++++++++++++--
 drivers/usb/gadget/function/uvc.h       |  14 ++
 drivers/usb/gadget/function/uvc_v4l2.c  |  16 +-
 drivers/usb/gadget/function/uvc_video.c |  15 +-
 4 files changed, 297 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 7945ea93a775a..cd40a063751ae 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -201,21 +201,253 @@ static const struct usb_descriptor_header * const uvc_ss_streaming[] = {
  * Control requests
  */
 
+void uvc_fill_streaming_control(struct uvc_device *uvc,
+			   struct uvc_streaming_control *ctrl,
+			   int iframe, int iformat, unsigned int ival)
+{
+	struct uvcg_format *ufmt;
+	struct uvcg_frame *ufrm;
+	unsigned int i;
+
+	/* Restrict the iformat, iframe and ival to valid values. Negative
+	 * values for ifrmat and iframe will result in the maximum valid value
+	 * being selected
+	 */
+	iformat = clamp((unsigned int)iformat, 1U, (unsigned int)uvc->nformats);
+	ufmt = uvc->fmt[iformat-1];
+
+	iframe = clamp((unsigned int)iframe, 1U, (unsigned int)uvc->nframes);
+	ufrm = find_frame_by_index(uvc, ufmt, iframe);
+	if (!ufrm)
+		return;
+
+	for (i = 0; i < ufrm->frame.b_frame_interval_type; i++) {
+		if (ival <= ufrm->dw_frame_interval[i]) {
+			ival = ufrm->dw_frame_interval[i];
+			break;
+		}
+	}
+
+	if (i == ufrm->frame.b_frame_interval_type)
+		ival = ufrm->dw_frame_interval[ufrm->frame.b_frame_interval_type-1];
+
+	memset(ctrl, 0, sizeof(*ctrl));
+
+	ctrl->bmHint = 1;
+	ctrl->bFormatIndex = iformat;
+	ctrl->bFrameIndex = iframe;
+	ctrl->dwFrameInterval = ival;
+	ctrl->dwMaxVideoFrameSize = ufrm->frame.dw_max_video_frame_buffer_size;
+
+	switch (uvc->func.config->cdev->gadget->speed) {
+	case USB_SPEED_SUPER:
+		ctrl->dwMaxPayloadTransferSize = uvc_ss_streaming_ep.wMaxPacketSize;
+		break;
+	case USB_SPEED_HIGH:
+		ctrl->dwMaxPayloadTransferSize = uvc_hs_streaming_ep.wMaxPacketSize;
+		break;
+	case USB_SPEED_FULL:
+	default:
+		ctrl->dwMaxPayloadTransferSize = uvc_fs_streaming_ep.wMaxPacketSize;
+		break;
+	}
+	ctrl->bmFramingInfo = 3;
+	ctrl->bPreferedVersion = 1;
+	ctrl->bMaxVersion = 1;
+}
+
+static int uvc_events_process_data(struct uvc_device *uvc, struct usb_request *req)
+{
+	struct uvc_video *video = &uvc->video;
+	struct uvc_streaming_control *target;
+	struct uvc_streaming_control *ctrl;
+	struct uvcg_frame *ufrm;
+	struct uvcg_format *ufmt;
+
+	switch (video->control) {
+	case UVC_VS_PROBE_CONTROL:
+		pr_debug("setting probe control, length = %d\n", req->actual);
+		target = &video->probe;
+		break;
+
+	case UVC_VS_COMMIT_CONTROL:
+		pr_debug("setting commit control, length = %d\n", req->actual);
+		target = &video->commit;
+		break;
+
+	default:
+		pr_debug("setting unknown control, length = %d\n", req->actual);
+		return -EOPNOTSUPP;
+	}
+
+	ctrl = (struct uvc_streaming_control *)req->buf;
+
+	if (video->control == UVC_VS_PROBE_CONTROL)
+		uvc_fill_streaming_control(uvc, target, ctrl->bFormatIndex,
+					   ctrl->bFrameIndex, ctrl->dwFrameInterval);
+
+	ufmt = uvc->fmt[target->bFormatIndex-1];
+	if (!ufmt)
+		return -EINVAL;
+
+	ufrm = find_frame_by_index(uvc, ufmt, ctrl->bFrameIndex);
+	if (!ufrm)
+		return -EINVAL;
+
+	if (video->control == UVC_VS_COMMIT_CONTROL) {
+		spin_lock(&video->frame_lock);
+
+		uvc_fill_streaming_control(uvc, target, ctrl->bFormatIndex,
+				   ctrl->bFrameIndex, ctrl->dwFrameInterval);
+
+		video->cur_frame = ufrm;
+		video->cur_format = ufmt;
+
+		spin_unlock(&video->frame_lock);
+	}
+
+	return 0;
+}
+
+static void
+uvc_events_process_streaming(struct uvc_device *uvc, uint8_t req, uint8_t cs,
+			     struct uvc_request_data *resp)
+{
+	struct uvc_streaming_control *ctrl;
+
+	pr_debug("streaming request (req %02x cs %02x)\n", req, cs);
+
+	if (cs != UVC_VS_PROBE_CONTROL && cs != UVC_VS_COMMIT_CONTROL)
+		return;
+
+	ctrl = (struct uvc_streaming_control *)&resp->data;
+	resp->length = sizeof(*ctrl);
+
+	switch (req) {
+	case UVC_SET_CUR:
+		uvc->video.control = cs;
+		resp->length = 34;
+		break;
+
+	case UVC_GET_CUR:
+		if (cs == UVC_VS_PROBE_CONTROL)
+			memcpy(ctrl, &uvc->video.probe, sizeof(*ctrl));
+		else
+			memcpy(ctrl, &uvc->video.commit, sizeof(*ctrl));
+		break;
+
+	case UVC_GET_MIN:
+	case UVC_GET_MAX:
+	case UVC_GET_DEF:
+		if (req == UVC_GET_MAX)
+			uvc_fill_streaming_control(uvc, ctrl, -1, -1, UINT_MAX);
+		else
+			uvc_fill_streaming_control(uvc, ctrl, 1, 1, 0);
+		break;
+
+	case UVC_GET_RES:
+		memset(ctrl, 0, sizeof(*ctrl));
+		break;
+
+	case UVC_GET_LEN:
+		resp->data[0] = 0x00;
+		resp->data[1] = 0x22;
+		resp->length = 2;
+		break;
+
+	case UVC_GET_INFO:
+		resp->data[0] = 0x03;
+		resp->length = 1;
+		break;
+	}
+}
+
+static int
+uvc_events_process_class(struct uvc_device *uvc, const struct usb_ctrlrequest *ctrl,
+			 struct uvc_request_data *resp)
+{
+	if ((ctrl->bRequestType & USB_RECIP_MASK) != USB_RECIP_INTERFACE)
+		return -EINVAL;
+
+	switch (ctrl->wIndex & 0xff) {
+	case UVC_INTF_VIDEO_CONTROL:
+		return -EOPNOTSUPP;
+
+	case UVC_INTF_VIDEO_STREAMING:
+		uvc_events_process_streaming(uvc, ctrl->bRequest, ctrl->wValue >> 8, resp);
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int
+uvc_events_process_setup(struct uvc_device *uvc, const struct usb_ctrlrequest *ctrl,
+			 struct uvc_request_data *resp)
+{
+	uvc->video.control = 0;
+
+	pr_debug("bRequestType %02x bRequest %02x wValue %04x wIndex %04x wLength %04x\n",
+		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
+		ctrl->wIndex, ctrl->wLength);
+
+	switch (ctrl->bRequestType & USB_TYPE_MASK) {
+	case USB_TYPE_STANDARD:
+		return -EOPNOTSUPP;
+
+	case USB_TYPE_CLASS:
+		return uvc_events_process_class(uvc, ctrl, resp);
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+/* --------------------------------------------------------------------------
+ * Requests handling
+ */
+
+static
+int uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
+{
+	struct usb_composite_dev *cdev = uvc->func.config->cdev;
+	struct usb_request *req = uvc->control_req;
+
+	if (data->length < 0)
+		return usb_ep_set_halt(cdev->gadget->ep0);
+
+	req->length = min_t(unsigned int, uvc->event_length, data->length);
+	req->zero = data->length < uvc->event_length;
+
+	memcpy(req->buf, data->data, req->length);
+
+	return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
+}
+
 static void
 uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct uvc_device *uvc = req->context;
-	struct v4l2_event v4l2_event;
-	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+	int ret;
 
 	if (uvc->event_setup_out) {
 		uvc->event_setup_out = 0;
+		ret = uvc_events_process_data(uvc, req);
+		if (ret == -EOPNOTSUPP) {
+			struct v4l2_event v4l2_event;
+			struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
 
-		memset(&v4l2_event, 0, sizeof(v4l2_event));
-		v4l2_event.type = UVC_EVENT_DATA;
-		uvc_event->data.length = req->actual;
-		memcpy(&uvc_event->data.data, req->buf, req->actual);
-		v4l2_event_queue(&uvc->vdev, &v4l2_event);
+			memset(&v4l2_event, 0, sizeof(v4l2_event));
+			v4l2_event.type = UVC_EVENT_DATA;
+			uvc_event->data.length = req->actual;
+			memcpy(&uvc_event->data.data, req->buf, req->actual);
+			v4l2_event_queue(&uvc->vdev, &v4l2_event);
+		}
 	}
 }
 
@@ -223,8 +455,8 @@ static int
 uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 {
 	struct uvc_device *uvc = to_uvc(f);
-	struct v4l2_event v4l2_event;
-	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
+	struct uvc_request_data resp;
+	int ret = 0;
 
 	if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
 		uvcg_info(f, "invalid request type\n");
@@ -241,12 +473,22 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
 	uvc->event_length = le16_to_cpu(ctrl->wLength);
 
-	memset(&v4l2_event, 0, sizeof(v4l2_event));
-	v4l2_event.type = UVC_EVENT_SETUP;
-	memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
-	v4l2_event_queue(&uvc->vdev, &v4l2_event);
+	ret = uvc_events_process_setup(uvc, ctrl, &resp);
+	if (ret == -EOPNOTSUPP) {
+		struct v4l2_event v4l2_event;
+		struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
 
-	return 0;
+		memset(&v4l2_event, 0, sizeof(v4l2_event));
+		v4l2_event.type = UVC_EVENT_SETUP;
+		memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
+		v4l2_event_queue(&uvc->vdev, &v4l2_event);
+
+		return 0;
+	} else if (ret) {
+		return ret;
+	}
+
+	return uvc_send_response(uvc, &resp);
 }
 
 void uvc_function_setup_continue(struct uvc_device *uvc)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 62d7420a25666..d06b5282b4058 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/usb/composite.h>
+#include <linux/usb/video.h>
 #include <linux/videodev2.h>
 
 #include <media/v4l2-device.h>
@@ -85,6 +86,12 @@ struct uvc_video {
 	struct uvcg_frame *cur_frame;
 
 	struct mutex mutex;	/* protects frame parameters */
+	spinlock_t frame_lock;
+
+	struct uvc_streaming_control probe;
+	struct uvc_streaming_control commit;
+
+	int control;
 
 	/* Requests */
 	unsigned int req_size;
@@ -170,4 +177,11 @@ extern void uvc_function_connect(struct uvc_device *uvc);
 extern void uvc_function_disconnect(struct uvc_device *uvc);
 
 extern int uvc_frame_default(struct uvcg_format *ufmt);
+extern struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
+					      struct uvcg_format *ufmt,
+					      int index);
+extern void uvc_fill_streaming_control(struct uvc_device *uvc,
+			   struct uvc_streaming_control *ctrl,
+			   int iframe, int iformat, unsigned int ival);
+
 #endif /* _UVC_GADGET_H_ */
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 83830b8864a6e..a89d3d661d99f 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -205,7 +205,8 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
 }
 
 static int _uvc_v4l2_try_fmt(struct uvc_video *video,
-	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
+	struct v4l2_format *fmt, struct uvc_streaming_control *probe,
+	struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
 {
 	struct uvc_device *uvc = video->uvc;
 	struct uvcg_format *ufmt;
@@ -253,6 +254,9 @@ static int _uvc_v4l2_try_fmt(struct uvc_video *video,
 		*uvc_format = ufmt;
 	if (uvc_frame != NULL)
 		*uvc_frame = ufrm;
+	if (probe)
+		uvc_fill_streaming_control(uvc, probe,
+				i + 1, ufrm->frame.b_frame_index, 0);
 
 	return 0;
 }
@@ -264,7 +268,7 @@ uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
 
-	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
+	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL, NULL);
 }
 
 static int
@@ -273,17 +277,23 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
 	struct video_device *vdev = video_devdata(file);
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
+	struct uvc_streaming_control probe;
 	struct uvcg_format *ufmt;
 	struct uvcg_frame *ufrm;
 	int ret;
 
-	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
+	ret = _uvc_v4l2_try_fmt(video, fmt, &probe, &ufmt, &ufrm);
 	if (ret)
 		return ret;
 
+	spin_lock(&video->frame_lock);
+
+	video->commit = probe;
 	video->cur_format = ufmt;
 	video->cur_frame = ufrm;
 
+	spin_unlock(&video->frame_lock);
+
 	return ret;
 }
 
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b14780bddd838..8f549fe608362 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -346,13 +346,26 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
  */
 int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 {
+	int framedef;
+
 	INIT_LIST_HEAD(&video->req_free);
 	spin_lock_init(&video->req_lock);
+	spin_lock_init(&video->frame_lock);
+
+	/* Allocate a stream specific work queue for asynchronous tasks. */
+	video->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE, 0);
+	if (!video->async_wq)
+		return -EINVAL;
+
 	INIT_WORK(&video->pump, uvcg_video_pump);
 
 	video->uvc = uvc;
 	video->def_format = video->cur_format = uvc->fmt[0];
-	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];
+	framedef = uvc_frame_default(video->def_format);
+	video->cur_frame = uvc->frm[framedef - 1];
+
+	uvc_fill_streaming_control(uvc, &video->probe, 1, framedef, 0);
+	uvc_fill_streaming_control(uvc, &video->commit, 1, framedef, 0);
 
 	/* Initialize the video buffers queue. */
 	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
-- 
2.29.2

Re: [PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[Greg KH]

<resending as your email headers were b0rked causing replies to only go
to you...>

On Mon, May 31, 2021 at 12:22:38AM +0200, Michael Grzeschik wrote:
> This patch adds support to the v4l2 VIDIOC for enum_format,
> enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
> setup gets parsed and this configfs data is used in the v4l2 interface
> functions.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c        |  54 ++++
>  drivers/usb/gadget/function/uvc.h          |  18 +-
>  drivers/usb/gadget/function/uvc_configfs.c |   5 +
>  drivers/usb/gadget/function/uvc_configfs.h |   2 +
>  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
>  drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
>  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
>  drivers/usb/gadget/function/uvc_video.c    |  10 +-
>  8 files changed, 369 insertions(+), 50 deletions(-)

It would be great to have some v4l developers review/ack this so we know
if it's ok to take or not.

thanks,

greg k-h

Re: [PATCH 0/3] usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS

[paul.elder]

Hi Michael,

Thank you for the patch series.

On Mon, May 31, 2021 at 12:22:36AM +0200, Michael Grzeschik wrote:
> This series improves the uvc video gadget by parsing the configfs
> entries. With the configfs data, the driver now is able to negotiate the
> format with the usb host in the kernel and also exports the supported
> frames/formats/intervals via the v4l2 VIDIOC interface.

Sorry for the delay, I'll start looking at this (and your other series)
now.

Paul

> 
> Michael Grzeschik (3):
>   usb: gadget: uvc: move structs to common header
>   usb: gadget: uvc: add VIDIOC function
>   usb: gadget: uvc: add format/frame handling code
> 
>  drivers/usb/gadget/function/f_uvc.c        | 324 +++++++++++++++++++-
>  drivers/usb/gadget/function/uvc.h          |  32 +-
>  drivers/usb/gadget/function/uvc_configfs.c | 116 +------
>  drivers/usb/gadget/function/uvc_configfs.h | 121 ++++++++
>  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
>  drivers/usb/gadget/function/uvc_v4l2.c     | 335 ++++++++++++++++++---
>  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
>  drivers/usb/gadget/function/uvc_video.c    |  23 +-
>  8 files changed, 781 insertions(+), 175 deletions(-)
> 
> -- 
> 2.29.2
> 

Re: [PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[paul.elder]

Hi Michael,

Thank you for the patch.

On Mon, May 31, 2021 at 12:22:38AM +0200, Michael Grzeschik wrote:
> This patch adds support to the v4l2 VIDIOC for enum_format,
> enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
> setup gets parsed and this configfs data is used in the v4l2 interface
> functions.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c        |  54 ++++
>  drivers/usb/gadget/function/uvc.h          |  18 +-
>  drivers/usb/gadget/function/uvc_configfs.c |   5 +
>  drivers/usb/gadget/function/uvc_configfs.h |   2 +
>  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
>  drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
>  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
>  drivers/usb/gadget/function/uvc_video.c    |  10 +-
>  8 files changed, 369 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index f48a00e497945..7945ea93a775a 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -410,6 +410,44 @@ static ssize_t function_name_show(struct device *dev,
>  
>  static DEVICE_ATTR_RO(function_name);
>  
> +static int
> +uvc_analyze_configfs(struct uvc_device *uvc)
> +{
> +	struct uvcg_streaming_header *src_hdr = uvc->h;
> +	struct config_item *item;
> +	struct config_group *grp;
> +	struct uvcg_format_ptr *f;
> +	int i = 0, j = 0;
> +
> +	if (!src_hdr->linked)
> +		return -EBUSY;
> +
> +	list_for_each_entry(f, &src_hdr->formats, entry)
> +		uvc->nframes += f->fmt->num_frames;
> +
> +	uvc->nformats = src_hdr->num_fmt;
> +
> +	uvc->frm = kcalloc(uvc->nframes, sizeof(struct uvcg_frame *), GFP_KERNEL);
> +	if (!uvc->frm)
> +		return -ENOMEM;
> +
> +	uvc->fmt = kcalloc(uvc->nformats, sizeof(struct uvcg_format *), GFP_KERNEL);
> +	if (!uvc->fmt) {
> +		kfree(uvc->frm);
> +		return -ENOMEM;
> +	}

nformats/nframes should be set to zero if the kcalloc(s) fail.

> +
> +	list_for_each_entry(f, &src_hdr->formats, entry) {
> +		uvc->fmt[i++] = f->fmt;
> +		grp = &f->fmt->group;
> +		list_for_each_entry(item, &grp->cg_children, ci_entry) {
> +			uvc->frm[j++] = to_uvcg_frame(item);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  uvc_register_video(struct uvc_device *uvc)
>  {
> @@ -742,6 +780,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  		goto error;
>  	}
>  
> +	/* Register configfs formats and frames. */
> +	ret = uvc_analyze_configfs(uvc);
> +	if (ret < 0) {
> +		uvcg_err(f, "failed to read configfs\n");
> +		goto v4l2_error;

s/v4l2_error/error/ ?

> +	}
> +
>  	/* Initialise video. */
>  	ret = uvcg_video_init(&uvc->video, uvc);
>  	if (ret < 0)
> @@ -905,6 +950,8 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>  	struct uvc_device *uvc;
>  	struct f_uvc_opts *opts;
>  	struct uvc_descriptor_header **strm_cls;
> +	struct config_item *item;
> +	struct config_group *grp;
>  
>  	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
>  	if (uvc == NULL)
> @@ -936,6 +983,13 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>  	uvc->desc.fs_streaming = opts->fs_streaming;
>  	uvc->desc.hs_streaming = opts->hs_streaming;
>  	uvc->desc.ss_streaming = opts->ss_streaming;
> +
> +	grp = &opts->func_inst.group;
> +	item = config_group_find_item(grp, "streaming");
> +	item = config_group_find_item(to_config_group(item), "header");
> +	item = config_group_find_item(to_config_group(item), "h");

These return values should be checked. It's conceivable that userspace
neglects to create these directories (either by accident or on purpose).

> +	uvc->h = to_uvcg_streaming_header(item);
> +
>  	++opts->refcnt;
>  	mutex_unlock(&opts->lock);
>  
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 23ee25383c1f7..62d7420a25666 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -80,11 +80,10 @@ struct uvc_video {
>  	struct work_struct pump;
>  
>  	/* Frame parameters */
> -	u8 bpp;
> -	u32 fcc;
> -	unsigned int width;
> -	unsigned int height;
> -	unsigned int imagesize;
> +	struct uvcg_format *def_format;
> +	struct uvcg_format *cur_format;
> +	struct uvcg_frame *cur_frame;
> +
>  	struct mutex mutex;	/* protects frame parameters */
>  
>  	/* Requests */
> @@ -118,6 +117,14 @@ struct uvc_device {
>  	struct usb_function func;
>  	struct uvc_video video;
>  
> +	struct uvcg_streaming_header *h;
> +
> +	struct uvcg_frame **frm;
> +	int nframes;
> +
> +	struct uvcg_format **fmt;
> +	int nformats;
> +
>  	/* Descriptors */
>  	struct {
>  		const struct uvc_descriptor_header * const *fs_control;
> @@ -162,4 +169,5 @@ extern void uvc_endpoint_stream(struct uvc_device *dev);
>  extern void uvc_function_connect(struct uvc_device *uvc);
>  extern void uvc_function_disconnect(struct uvc_device *uvc);
>  
> +extern int uvc_frame_default(struct uvcg_format *ufmt);
>  #endif /* _UVC_GADGET_H_ */
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 86463bb2639ed..009c80d0e1780 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/sort.h>
> +#include <linux/videodev2.h>
>  
>  #include "u_uvc.h"
>  #include "uvc_configfs.h"
> @@ -1547,6 +1548,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>  	h->desc.bCopyProtect		= 0;
>  
>  	h->fmt.type = UVCG_UNCOMPRESSED;
> +	h->fmt.fcc = V4L2_PIX_FMT_YUYV;
> +	h->fmt.name = "YUV 4:2:2 (YUYV)";
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_uncompressed_type);
>  
> @@ -1721,6 +1724,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  	h->desc.bCopyProtect		= 0;
>  
>  	h->fmt.type = UVCG_MJPEG;
> +	h->fmt.fcc = V4L2_PIX_FMT_MJPEG;
> +	h->fmt.name = "MJPEG";
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_mjpeg_type);
>  
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index f905d29570eb4..8ed966275f838 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -52,6 +52,8 @@ struct uvcg_format {
>  	enum uvcg_format_type	type;
>  	unsigned		linked;
>  	unsigned		num_frames;
> +	char			*name;
> +	u32			fcc;
>  	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>  };
>  
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index 61e2c94cc0b0c..6afc4b79adfe9 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -20,6 +20,8 @@
>  #include <media/videobuf2-vmalloc.h>
>  
>  #include "uvc.h"
> +#include "u_uvc.h"
> +#include "uvc_configfs.h"
>  
>  /* ------------------------------------------------------------------------
>   * Video buffers queue management.
> @@ -49,7 +51,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  
>  	*nplanes = 1;
>  
> -	sizes[0] = video->imagesize;
> +	sizes[0] = video->cur_frame->frame.dw_max_video_frame_buffer_size;
>  
>  	return 0;
>  }
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 4ca89eab61590..83830b8864a6e 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -24,6 +24,127 @@
>  #include "uvc_queue.h"
>  #include "uvc_video.h"
>  #include "uvc_v4l2.h"
> +#include "u_uvc.h"
> +#include "uvc_configfs.h"
> +
> +u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm)
> +{
> +	struct uvcg_uncompressed *u;
> +
> +	switch (fmt->type) {
> +	case UVCG_UNCOMPRESSED:
> +		u = to_uvcg_uncompressed(&fmt->group.cg_item);
> +		if (!u)
> +			return 0;
> +
> +		return u->desc.bBitsPerPixel * frm->frame.w_width / 8;
> +	case UVCG_MJPEG:
> +		return frm->frame.w_width;

From the struct v4l2_pix_format documentation:
"For compressed formats the bytesperline value makes no sense.
Applications and drivers must set this to 0 in that case."

Which means that some sizeimage calculations in uvc_v4l2.c would have to
be adjusted accordingly.

> +	}
> +
> +	return 0;
> +}
> +
> +struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
> +					      struct uvcg_format *ufmt,
> +					      int index)

Indentation.

> +{
> +	int i;
> +
> +	for (i = 0; i < uvc->nframes; i++) {
> +		if (uvc->frm[i]->fmt_type != ufmt->type)
> +			continue;
> +
> +		if (index == uvc->frm[i]->frame.b_frame_index)
> +			break;
> +	}
> +
> +	if (i == uvc->nframes)
> +		return NULL;
> +
> +	return uvc->frm[i];
> +}
> +
> +static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
> +						    u32 pixelformat)

Indentation.

> +{
> +	int i;
> +
> +	for (i = 0; i < uvc->nformats; i++)
> +		if (uvc->fmt[i]->fcc == pixelformat)
> +			break;
> +
> +	if (i == uvc->nformats)
> +		return NULL;
> +
> +	return uvc->fmt[i];
> +}
> +
> +int uvc_frame_default(struct uvcg_format *ufmt)
> +{
> +	struct uvcg_uncompressed *m;
> +	struct uvcg_uncompressed *u;
> +	int ret = 1;
> +
> +	switch (ufmt->type) {
> +	case UVCG_UNCOMPRESSED:
> +		u = to_uvcg_uncompressed(&ufmt->group.cg_item);
> +		if (u)
> +			ret = u->desc.bDefaultFrameIndex;
> +		break;
> +	case UVCG_MJPEG:
> +		m = to_uvcg_uncompressed(&ufmt->group.cg_item);
> +		if (m)
> +			ret = m->desc.bDefaultFrameIndex;
> +		break;
> +	}
> +
> +	if (!ret)
> +		ret = 1;

Isn't 1 a valid frame index? Should this (and the initialization above)
be -1 instead?

> +
> +	return ret;
> +}
> +
> +static struct uvcg_frame *find_frm_by_size(struct uvc_device *uvc,
> +					   struct uvcg_format *ufmt,
> +					   u16 rw, u16 rh)

Also, since you have find_frame_by_index, maybe this should be
find_frame_by_size for consistency. Or maybe find_closest_frame_by_size?
Up to you.

> +{
> +	struct uvc_video *video = &uvc->video;
> +	struct uvcg_frame *ufrm = NULL;
> +	unsigned int d, maxd;
> +	int i;
> +
> +	/* Find the closest image size. The distance between image sizes is
> +	 * the size in pixels of the non-overlapping regions between the
> +	 * requested size and the frame-specified size.
> +	 */
> +	maxd = (unsigned int)-1;
> +
> +	for (i = 0; i < uvc->nframes; i++) {
> +		u16 w, h;
> +
> +		if (uvc->frm[i]->fmt_type != ufmt->type)
> +			continue;
> +
> +		w = uvc->frm[i]->frame.w_width;
> +		h = uvc->frm[i]->frame.w_height;
> +
> +		d = min(w, rw) * min(h, rh);
> +		d = w*h + rw*rh - 2*d;
> +		if (d < maxd) {
> +			maxd = d;
> +			ufrm = uvc->frm[i];
> +		}
> +
> +		if (maxd == 0)
> +			break;
> +	}
> +
> +	if (ufrm == NULL)
> +		uvcg_dbg(&video->uvc->func, "Unsupported size %ux%u\n", rw, rh);
> +
> +	return ufrm;
> +}
>  
>  /* --------------------------------------------------------------------------
>   * Requests handling
> @@ -50,16 +171,6 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
>   * V4L2 ioctls
>   */
>  
> -struct uvc_format {
> -	u8 bpp;
> -	u32 fcc;
> -};
> -
> -static struct uvc_format uvc_formats[] = {
> -	{ 16, V4L2_PIX_FMT_YUYV  },
> -	{ 0,  V4L2_PIX_FMT_MJPEG },
> -};
> -
>  static int
>  uvc_v4l2_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>  {
> @@ -81,55 +192,187 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>  	struct uvc_video *video = &uvc->video;
>  
> -	fmt->fmt.pix.pixelformat = video->fcc;
> -	fmt->fmt.pix.width = video->width;
> -	fmt->fmt.pix.height = video->height;
> +	fmt->fmt.pix.pixelformat = video->cur_format->fcc;
> +	fmt->fmt.pix.width = video->cur_frame->frame.w_width;
> +	fmt->fmt.pix.height = video->cur_frame->frame.w_height;
> +	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format, video->cur_frame);
> +	fmt->fmt.pix.sizeimage = video->cur_frame->frame.dw_max_video_frame_buffer_size;
> +	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->fmt.pix.priv = 0;
> +
> +	return 0;
> +}
> +
> +static int _uvc_v4l2_try_fmt(struct uvc_video *video,
> +	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)

Indentation.

> +{
> +	struct uvc_device *uvc = video->uvc;
> +	struct uvcg_format *ufmt;
> +	struct uvcg_frame *ufrm;
> +	u8 *fcc;
> +	int i;
> +
> +	if (fmt->type != video->queue.queue.type)
> +		return -EINVAL;
> +
> +	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
> +	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
> +		fmt->fmt.pix.pixelformat,
> +		fcc[0], fcc[1], fcc[2], fcc[3],
> +		fmt->fmt.pix.width, fmt->fmt.pix.height);
> +
> +	for (i = 0; i < uvc->nformats; i++)
> +		if (uvc->fmt[i]->fcc == fmt->fmt.pix.pixelformat)
> +			break;
> +
> +	if (i == uvc->nformats)
> +		ufmt = video->def_format;

This means we don't support the requested format. Shouldn't we return
-EINVAL at this point?

> +
> +	ufmt = uvc->fmt[i];
> +
> +	ufrm = find_frm_by_size(uvc, ufmt,
> +				fmt->fmt.pix.width, fmt->fmt.pix.height);
> +	if (!ufrm)
> +		return -EINVAL;
> +
> +	fmt->fmt.pix.width = ufrm->frame.w_width;
> +	fmt->fmt.pix.height = ufrm->frame.w_height;
>  	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> -	fmt->fmt.pix.bytesperline = video->bpp * video->width / 8;
> -	fmt->fmt.pix.sizeimage = video->imagesize;
> +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(ufmt, ufrm);

As mentioned earlier, this should be 0 for compressed formats.

> +	fmt->fmt.pix.sizeimage = ufrm->frame.dw_max_video_frame_buffer_size;
> +	fmt->fmt.pix.pixelformat = ufmt->fcc;
>  	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>  	fmt->fmt.pix.priv = 0;
>  
> +	if (!fmt->fmt.pix.sizeimage && fmt->fmt.pix.bytesperline)
> +		fmt->fmt.pix.sizeimage = fmt->fmt.pix.bytesperline *
> +				fmt->fmt.pix.height;

Since the configfs configuration is required to configure the driver, I
think it should be fine to just take the value from there, especially
since bytesperline will be zero for compressed formats anyway.

Maybe for uncompressed we could just use what's requested?

> +
> +	if (uvc_format != NULL)
> +		*uvc_format = ufmt;
> +	if (uvc_frame != NULL)
> +		*uvc_frame = ufrm;
> +
>  	return 0;
>  }
>  
> +static int
> +uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
> +
> +	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
> +}
> +
>  static int
>  uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  {
>  	struct video_device *vdev = video_devdata(file);
>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>  	struct uvc_video *video = &uvc->video;
> -	struct uvc_format *format;
> -	unsigned int imagesize;
> -	unsigned int bpl;
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(uvc_formats); ++i) {
> -		format = &uvc_formats[i];
> -		if (format->fcc == fmt->fmt.pix.pixelformat)
> +	struct uvcg_format *ufmt;
> +	struct uvcg_frame *ufrm;
> +	int ret;
> +
> +	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
> +	if (ret)
> +		return ret;
> +
> +	video->cur_format = ufmt;
> +	video->cur_frame = ufrm;
> +
> +	return ret;
> +}
> +
> +static int
> +uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
> +		struct v4l2_frmivalenum *fival)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvcg_format *ufmt = NULL;
> +	struct uvcg_frame *ufrm = NULL;
> +	int i;
> +
> +	ufmt = find_format_by_pix(uvc, fival->pixel_format);
> +	if (!ufmt)
> +		return -EINVAL;
> +
> +	for (i = 0; i < uvc->nframes; ++i) {
> +		if (uvc->frm[i]->fmt_type != ufmt->type)
> +			continue;
> +
> +		if (uvc->frm[i]->frame.w_width == fival->width &&
> +				uvc->frm[i]->frame.w_height == fival->height) {

Indentation.

> +			ufrm = uvc->frm[i];
>  			break;
> +		}
>  	}
> +	if (!ufrm)
> +		return -EINVAL;
>  
> -	if (i == ARRAY_SIZE(uvc_formats)) {
> -		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
> -			  fmt->fmt.pix.pixelformat);
> +	if (fival->index >= ufrm->frame.b_frame_interval_type)
>  		return -EINVAL;
> -	}
>  
> -	bpl = format->bpp * fmt->fmt.pix.width / 8;
> -	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
> +	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +	fival->discrete.numerator = ufrm->dw_frame_interval[fival->index];
> +	fival->discrete.denominator = 10000000;
> +	v4l2_simplify_fraction(&fival->discrete.numerator,

Where does this function come from?

> +		&fival->discrete.denominator, 8, 333);
>  
> -	video->fcc = format->fcc;
> -	video->bpp = format->bpp;
> -	video->width = fmt->fmt.pix.width;
> -	video->height = fmt->fmt.pix.height;
> -	video->imagesize = imagesize;
> +	return 0;
> +}
>  
> -	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> -	fmt->fmt.pix.bytesperline = bpl;
> -	fmt->fmt.pix.sizeimage = imagesize;
> -	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->fmt.pix.priv = 0;
> +static int
> +uvc_v4l2_enum_framesizes(struct file *file, void *fh,
> +		struct v4l2_frmsizeenum *fsize)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvcg_format *ufmt = NULL;
> +	struct uvcg_frame *ufrm = NULL;
> +
> +	ufmt = find_format_by_pix(uvc, fsize->pixel_format);
> +	if (!ufmt)
> +		return -EINVAL;
> +
> +	if (fsize->index >= ufmt->num_frames)
> +		return -EINVAL;
> +
> +	ufrm = find_frame_by_index(uvc, ufmt, fsize->index + 1);
> +	if (!ufrm)
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> +	fsize->discrete.width = ufrm->frame.w_width;
> +	fsize->discrete.height = ufrm->frame.w_height;
> +
> +	return 0;
> +}
> +
> +static int
> +uvc_v4l2_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvcg_format *ufmt;
> +
> +	if (f->index >= uvc->nformats)
> +		return -EINVAL;
> +
> +	ufmt = uvc->fmt[f->index];
> +	if (!ufmt)
> +		return -EINVAL;
> +
> +	f->pixelformat = ufmt->fcc;
> +	f->flags |= V4L2_FMT_FLAG_COMPRESSED;

This shouldn't be set if the format isn't compressed.

> +
> +	strscpy(f->description, ufmt->name, sizeof(f->description));
> +	f->description[sizeof(f->description) - 1] = 0;

If sizeof(ufmt->name) < sizeof(f->description), then the string won't be
properly null-terminated.

>  
>  	return 0;
>  }
> @@ -258,8 +501,12 @@ uvc_v4l2_ioctl_default(struct file *file, void *fh, bool valid_prio,
>  
>  const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_querycap = uvc_v4l2_querycap,
> +	.vidioc_try_fmt_vid_out = uvc_v4l2_try_fmt,
>  	.vidioc_g_fmt_vid_out = uvc_v4l2_get_format,
>  	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
> +	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
> +	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
> +	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_fmt,
>  	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>  	.vidioc_querybuf = uvc_v4l2_querybuf,
>  	.vidioc_qbuf = uvc_v4l2_qbuf,
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
> index 1576005b61fd3..6e45103bbf793 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.h
> +++ b/drivers/usb/gadget/function/uvc_v4l2.h
> @@ -15,5 +15,6 @@
>  
>  extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
>  extern const struct v4l2_file_operations uvc_v4l2_fops;
> +extern u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm);

Do you need this here? Nobody outside of uvc_v4l2.c uses it.

>  
>  #endif /* __UVC_V4L2_H__ */
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 633e23d58d868..b14780bddd838 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -17,7 +17,10 @@
>  
>  #include "uvc.h"
>  #include "uvc_queue.h"
> +#include "uvc_v4l2.h"
>  #include "uvc_video.h"
> +#include "u_uvc.h"
> +#include "uvc_configfs.h"
>  
>  /* --------------------------------------------------------------------------
>   * Video codecs
> @@ -348,11 +351,8 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>  	INIT_WORK(&video->pump, uvcg_video_pump);
>  
>  	video->uvc = uvc;
> -	video->fcc = V4L2_PIX_FMT_YUYV;
> -	video->bpp = 16;
> -	video->width = 320;
> -	video->height = 240;
> -	video->imagesize = 320 * 240 * 2;
> +	video->def_format = video->cur_format = uvc->fmt[0];
> +	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];

I think we'll need something here to validate the configfs, to make sure
it's filled corretly. I anticipate that uvc->fmt[0] might cause problems
if no format is set in configfs.

Or maybe such protection is already around and I'm not aware of it.


Thanks,

Paul

>  
>  	/* Initialize the video buffers queue. */
>  	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
> -- 
> 2.29.2
> 

Re: [PATCH 1/3] usb: gadget: uvc: move structs to common header

[paul.elder]

On Mon, May 31, 2021 at 12:22:37AM +0200, Michael Grzeschik wrote:
> The functions and structs of the configfs interface should also be used
> by the uvc gadget driver. This patch prepares the stack by moving the
> common structs and functions to the common header file.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 111 -------------------
>  drivers/usb/gadget/function/uvc_configfs.h | 119 +++++++++++++++++++++
>  2 files changed, 119 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 77d64031aa9c2..86463bb2639ed 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -19,8 +19,6 @@
>   * Global Utility Structures and Macros
>   */
>  
> -#define UVCG_STREAMING_CONTROL_SIZE	1
> -
>  #define UVC_ATTR(prefix, cname, aname) \
>  static struct configfs_attribute prefix##attr_##cname = { \
>  	.ca_name	= __stringify(aname),				\
> @@ -49,12 +47,6 @@ static int uvcg_config_compare_u32(const void *l, const void *r)
>  	return li < ri ? -1 : li == ri ? 0 : 1;
>  }
>  
> -static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
> -{
> -	return container_of(to_config_group(item), struct f_uvc_opts,
> -			    func_inst.group);
> -}
> -
>  struct uvcg_config_group_type {
>  	struct config_item_type type;
>  	const char *name;
> @@ -125,19 +117,6 @@ static void uvcg_config_remove_children(struct config_group *group)
>   * control/header
>   */
>  
> -DECLARE_UVC_HEADER_DESCRIPTOR(1);
> -
> -struct uvcg_control_header {
> -	struct config_item		item;
> -	struct UVC_HEADER_DESCRIPTOR(1)	desc;
> -	unsigned			linked;
> -};
> -
> -static struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_control_header, item);
> -}
> -
>  #define UVCG_CTRL_HDR_ATTR(cname, aname, bits, limit)			\
>  static ssize_t uvcg_control_header_##cname##_show(			\
>  	struct config_item *item, char *page)				\
> @@ -764,29 +743,6 @@ static const struct uvcg_config_group_type uvcg_control_grp_type = {
>   * streaming/mjpeg
>   */
>  
> -static const char * const uvcg_format_names[] = {
> -	"uncompressed",
> -	"mjpeg",
> -};
> -
> -enum uvcg_format_type {
> -	UVCG_UNCOMPRESSED = 0,
> -	UVCG_MJPEG,
> -};
> -
> -struct uvcg_format {
> -	struct config_group	group;
> -	enum uvcg_format_type	type;
> -	unsigned		linked;
> -	unsigned		num_frames;
> -	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> -};
> -
> -static struct uvcg_format *to_uvcg_format(struct config_item *item)
> -{
> -	return container_of(to_config_group(item), struct uvcg_format, group);
> -}
> -
>  static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>  {
>  	struct f_uvc_opts *opts;
> @@ -845,29 +801,11 @@ static ssize_t uvcg_format_bma_controls_store(struct uvcg_format *ch,
>  	return ret;
>  }
>  
> -struct uvcg_format_ptr {
> -	struct uvcg_format	*fmt;
> -	struct list_head	entry;
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * streaming/header/<NAME>
>   * streaming/header
>   */
>  
> -struct uvcg_streaming_header {
> -	struct config_item				item;
> -	struct uvc_input_header_descriptor		desc;
> -	unsigned					linked;
> -	struct list_head				formats;
> -	unsigned					num_fmt;
> -};
> -
> -static struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_streaming_header, item);
> -}
> -
>  static void uvcg_format_set_indices(struct config_group *fmt);
>  
>  static int uvcg_streaming_header_allow_link(struct config_item *src,
> @@ -1059,31 +997,6 @@ static const struct uvcg_config_group_type uvcg_streaming_header_grp_type = {
>   * streaming/<mode>/<format>/<NAME>
>   */
>  
> -struct uvcg_frame {
> -	struct config_item	item;
> -	enum uvcg_format_type	fmt_type;
> -	struct {
> -		u8	b_length;
> -		u8	b_descriptor_type;
> -		u8	b_descriptor_subtype;
> -		u8	b_frame_index;
> -		u8	bm_capabilities;
> -		u16	w_width;
> -		u16	w_height;
> -		u32	dw_min_bit_rate;
> -		u32	dw_max_bit_rate;
> -		u32	dw_max_video_frame_buffer_size;
> -		u32	dw_default_frame_interval;
> -		u8	b_frame_interval_type;
> -	} __attribute__((packed)) frame;
> -	u32 *dw_frame_interval;
> -};
> -
> -static struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_frame, item);
> -}
> -
>  #define UVCG_FRAME_ATTR(cname, aname, bits) \
>  static ssize_t uvcg_frame_##cname##_show(struct config_item *item, char *page)\
>  {									\
> @@ -1420,18 +1333,6 @@ static void uvcg_format_set_indices(struct config_group *fmt)
>   * streaming/uncompressed/<NAME>
>   */
>  
> -struct uvcg_uncompressed {
> -	struct uvcg_format		fmt;
> -	struct uvc_format_uncompressed	desc;
> -};
> -
> -static struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> -{
> -	return container_of(
> -		container_of(to_config_group(item), struct uvcg_format, group),
> -		struct uvcg_uncompressed, fmt);
> -}
> -
>  static struct configfs_group_operations uvcg_uncompressed_group_ops = {
>  	.make_item		= uvcg_frame_make,
>  	.drop_item		= uvcg_frame_drop,
> @@ -1669,18 +1570,6 @@ static const struct uvcg_config_group_type uvcg_uncompressed_grp_type = {
>   * streaming/mjpeg/<NAME>
>   */
>  
> -struct uvcg_mjpeg {
> -	struct uvcg_format		fmt;
> -	struct uvc_format_mjpeg		desc;
> -};
> -
> -static struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> -{
> -	return container_of(
> -		container_of(to_config_group(item), struct uvcg_format, group),
> -		struct uvcg_mjpeg, fmt);
> -}
> -
>  static struct configfs_group_operations uvcg_mjpeg_group_ops = {
>  	.make_item		= uvcg_frame_make,
>  	.drop_item		= uvcg_frame_drop,
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index 7e1d7ca29bf21..f905d29570eb4 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -16,4 +16,123 @@ struct f_uvc_opts;
>  
>  int uvcg_attach_configfs(struct f_uvc_opts *opts);
>  
> +static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
> +{
> +	return container_of(to_config_group(item), struct f_uvc_opts,
> +			    func_inst.group);
> +}
> +
> +#define UVCG_STREAMING_CONTROL_SIZE	1
> +
> +DECLARE_UVC_HEADER_DESCRIPTOR(1);
> +
> +struct uvcg_control_header {
> +	struct config_item		item;
> +	struct UVC_HEADER_DESCRIPTOR(1)	desc;
> +	unsigned			linked;
> +};
> +
> +static inline struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_control_header, item);
> +}
> +
> +static const char * const uvcg_format_names[] = {
> +	"uncompressed",
> +	"mjpeg",
> +};
> +
> +enum uvcg_format_type {
> +	UVCG_UNCOMPRESSED = 0,
> +	UVCG_MJPEG,
> +};
> +
> +struct uvcg_format {
> +	struct config_group	group;
> +	enum uvcg_format_type	type;
> +	unsigned		linked;
> +	unsigned		num_frames;
> +	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +};
> +
> +struct uvcg_format_ptr {
> +	struct uvcg_format	*fmt;
> +	struct list_head	entry;
> +};
> +
> +static inline struct uvcg_format *to_uvcg_format(struct config_item *item)
> +{
> +	return container_of(to_config_group(item), struct uvcg_format, group);
> +}
> +
> +struct uvcg_streaming_header {
> +	struct config_item				item;
> +	struct uvc_input_header_descriptor		desc;
> +	unsigned					linked;
> +	struct list_head				formats;
> +	unsigned					num_fmt;
> +};
> +
> +static inline struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_streaming_header, item);
> +}
> +
> +struct uvcg_frame {
> +	struct config_item	item;
> +	enum uvcg_format_type	fmt_type;
> +	struct {
> +		u8	b_length;
> +		u8	b_descriptor_type;
> +		u8	b_descriptor_subtype;
> +		u8	b_frame_index;
> +		u8	bm_capabilities;
> +		u16	w_width;
> +		u16	w_height;
> +		u32	dw_min_bit_rate;
> +		u32	dw_max_bit_rate;
> +		u32	dw_max_video_frame_buffer_size;
> +		u32	dw_default_frame_interval;
> +		u8	b_frame_interval_type;
> +	} __attribute__((packed)) frame;
> +	u32 *dw_frame_interval;
> +};
> +
> +static inline struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_frame, item);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/uncompressed/<NAME>
> + */
> +
> +struct uvcg_uncompressed {
> +	struct uvcg_format		fmt;
> +	struct uvc_format_uncompressed	desc;
> +};
> +
> +static inline struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> +{
> +	return container_of(
> +		container_of(to_config_group(item), struct uvcg_format, group),
> +		struct uvcg_uncompressed, fmt);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/mjpeg/<NAME>
> + */
> +
> +struct uvcg_mjpeg {
> +	struct uvcg_format		fmt;
> +	struct uvc_format_mjpeg		desc;
> +};
> +
> +static inline struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> +{
> +	return container_of(
> +		container_of(to_config_group(item), struct uvcg_format, group),
> +		struct uvcg_mjpeg, fmt);
> +}
> +
>  #endif /* UVC_CONFIGFS_H */
> -- 
> 2.29.2
> 

Re: [PATCH 3/3] usb: gadget: uvc: add format/frame handling code

[paul.elder]

Hi Michael,

Thank you for the patch.

On Mon, May 31, 2021 at 12:22:39AM +0200, Michael Grzeschik wrote:
> The hostside format selection is currently only done in userspace, as
> the events for SET_CUR and GET_CUR is allways moved to the application

s/is allways/are always/

> layer. Since the v4l2 devices parses the configfs data, the format

s/devices/device/

> negotiation can be done in the kernel. This patch adds the functions to
> set the curent configuration and still forwards all unknown events to

s/curent/current/

Reading just from this sentence, it sounds like only unknown events are
forwarded to userspace, and all known events are not. I see from the
code that this is indeed the case.

s/and still fowards/while continuing to forward/

> the userspace level.

Ah, I see a lot of this is lifted from our uvc-gadget :)

Which is fine. It's nice to see this processing moved to the kernel,
especially since configfs already has the necessary information.

> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c     | 270 ++++++++++++++++++++++--
>  drivers/usb/gadget/function/uvc.h       |  14 ++
>  drivers/usb/gadget/function/uvc_v4l2.c  |  16 +-
>  drivers/usb/gadget/function/uvc_video.c |  15 +-
>  4 files changed, 297 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 7945ea93a775a..cd40a063751ae 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -201,21 +201,253 @@ static const struct usb_descriptor_header * const uvc_ss_streaming[] = {
>   * Control requests
>   */
>  
> +void uvc_fill_streaming_control(struct uvc_device *uvc,
> +			   struct uvc_streaming_control *ctrl,
> +			   int iframe, int iformat, unsigned int ival)

Indentation.

> +{
> +	struct uvcg_format *ufmt;
> +	struct uvcg_frame *ufrm;
> +	unsigned int i;
> +
> +	/* Restrict the iformat, iframe and ival to valid values. Negative
> +	 * values for ifrmat and iframe will result in the maximum valid value
> +	 * being selected
> +	 */
> +	iformat = clamp((unsigned int)iformat, 1U, (unsigned int)uvc->nformats);
> +	ufmt = uvc->fmt[iformat-1];
> +
> +	iframe = clamp((unsigned int)iframe, 1U, (unsigned int)uvc->nframes);
> +	ufrm = find_frame_by_index(uvc, ufmt, iframe);
> +	if (!ufrm)
> +		return;
> +
> +	for (i = 0; i < ufrm->frame.b_frame_interval_type; i++) {
> +		if (ival <= ufrm->dw_frame_interval[i]) {
> +			ival = ufrm->dw_frame_interval[i];
> +			break;
> +		}
> +	}
> +
> +	if (i == ufrm->frame.b_frame_interval_type)
> +		ival = ufrm->dw_frame_interval[ufrm->frame.b_frame_interval_type-1];
> +
> +	memset(ctrl, 0, sizeof(*ctrl));
> +
> +	ctrl->bmHint = 1;
> +	ctrl->bFormatIndex = iformat;
> +	ctrl->bFrameIndex = iframe;
> +	ctrl->dwFrameInterval = ival;
> +	ctrl->dwMaxVideoFrameSize = ufrm->frame.dw_max_video_frame_buffer_size;
> +
> +	switch (uvc->func.config->cdev->gadget->speed) {
> +	case USB_SPEED_SUPER:
> +		ctrl->dwMaxPayloadTransferSize = uvc_ss_streaming_ep.wMaxPacketSize;
> +		break;
> +	case USB_SPEED_HIGH:
> +		ctrl->dwMaxPayloadTransferSize = uvc_hs_streaming_ep.wMaxPacketSize;
> +		break;
> +	case USB_SPEED_FULL:
> +	default:
> +		ctrl->dwMaxPayloadTransferSize = uvc_fs_streaming_ep.wMaxPacketSize;
> +		break;
> +	}
> +	ctrl->bmFramingInfo = 3;
> +	ctrl->bPreferedVersion = 1;
> +	ctrl->bMaxVersion = 1;
> +}
> +
> +static int uvc_events_process_data(struct uvc_device *uvc, struct usb_request *req)
> +{
> +	struct uvc_video *video = &uvc->video;
> +	struct uvc_streaming_control *target;
> +	struct uvc_streaming_control *ctrl;
> +	struct uvcg_frame *ufrm;
> +	struct uvcg_format *ufmt;
> +
> +	switch (video->control) {
> +	case UVC_VS_PROBE_CONTROL:
> +		pr_debug("setting probe control, length = %d\n", req->actual);
> +		target = &video->probe;
> +		break;
> +
> +	case UVC_VS_COMMIT_CONTROL:
> +		pr_debug("setting commit control, length = %d\n", req->actual);
> +		target = &video->commit;
> +		break;
> +
> +	default:
> +		pr_debug("setting unknown control, length = %d\n", req->actual);

Should this be error instead of debug?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	ctrl = (struct uvc_streaming_control *)req->buf;
> +
> +	if (video->control == UVC_VS_PROBE_CONTROL)
> +		uvc_fill_streaming_control(uvc, target, ctrl->bFormatIndex,
> +					   ctrl->bFrameIndex, ctrl->dwFrameInterval);
> +
> +	ufmt = uvc->fmt[target->bFormatIndex-1];
> +	if (!ufmt)
> +		return -EINVAL;
> +
> +	ufrm = find_frame_by_index(uvc, ufmt, ctrl->bFrameIndex);
> +	if (!ufrm)
> +		return -EINVAL;
> +
> +	if (video->control == UVC_VS_COMMIT_CONTROL) {
> +		spin_lock(&video->frame_lock);
> +
> +		uvc_fill_streaming_control(uvc, target, ctrl->bFormatIndex,
> +				   ctrl->bFrameIndex, ctrl->dwFrameInterval);
> +
> +		video->cur_frame = ufrm;
> +		video->cur_format = ufmt;
> +
> +		spin_unlock(&video->frame_lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +uvc_events_process_streaming(struct uvc_device *uvc, uint8_t req, uint8_t cs,
> +			     struct uvc_request_data *resp)
> +{
> +	struct uvc_streaming_control *ctrl;
> +
> +	pr_debug("streaming request (req %02x cs %02x)\n", req, cs);
> +
> +	if (cs != UVC_VS_PROBE_CONTROL && cs != UVC_VS_COMMIT_CONTROL)
> +		return;
> +
> +	ctrl = (struct uvc_streaming_control *)&resp->data;
> +	resp->length = sizeof(*ctrl);
> +
> +	switch (req) {
> +	case UVC_SET_CUR:
> +		uvc->video.control = cs;
> +		resp->length = 34;
> +		break;
> +
> +	case UVC_GET_CUR:
> +		if (cs == UVC_VS_PROBE_CONTROL)
> +			memcpy(ctrl, &uvc->video.probe, sizeof(*ctrl));
> +		else
> +			memcpy(ctrl, &uvc->video.commit, sizeof(*ctrl));
> +		break;
> +
> +	case UVC_GET_MIN:
> +	case UVC_GET_MAX:
> +	case UVC_GET_DEF:
> +		if (req == UVC_GET_MAX)
> +			uvc_fill_streaming_control(uvc, ctrl, -1, -1, UINT_MAX);
> +		else
> +			uvc_fill_streaming_control(uvc, ctrl, 1, 1, 0);
> +		break;
> +
> +	case UVC_GET_RES:
> +		memset(ctrl, 0, sizeof(*ctrl));
> +		break;
> +
> +	case UVC_GET_LEN:
> +		resp->data[0] = 0x00;
> +		resp->data[1] = 0x22;
> +		resp->length = 2;
> +		break;
> +
> +	case UVC_GET_INFO:
> +		resp->data[0] = 0x03;
> +		resp->length = 1;
> +		break;
> +	}
> +}
> +
> +static int
> +uvc_events_process_class(struct uvc_device *uvc, const struct usb_ctrlrequest *ctrl,
> +			 struct uvc_request_data *resp)
> +{
> +	if ((ctrl->bRequestType & USB_RECIP_MASK) != USB_RECIP_INTERFACE)
> +		return -EINVAL;
> +
> +	switch (ctrl->wIndex & 0xff) {
> +	case UVC_INTF_VIDEO_CONTROL:
> +		return -EOPNOTSUPP;
> +
> +	case UVC_INTF_VIDEO_STREAMING:
> +		uvc_events_process_streaming(uvc, ctrl->bRequest, ctrl->wValue >> 8, resp);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +uvc_events_process_setup(struct uvc_device *uvc, const struct usb_ctrlrequest *ctrl,
> +			 struct uvc_request_data *resp)
> +{
> +	uvc->video.control = 0;
> +
> +	pr_debug("bRequestType %02x bRequest %02x wValue %04x wIndex %04x wLength %04x\n",
> +		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
> +		ctrl->wIndex, ctrl->wLength);
> +
> +	switch (ctrl->bRequestType & USB_TYPE_MASK) {
> +	case USB_TYPE_STANDARD:
> +		return -EOPNOTSUPP;
> +
> +	case USB_TYPE_CLASS:
> +		return uvc_events_process_class(uvc, ctrl, resp);
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +/* --------------------------------------------------------------------------
> + * Requests handling
> + */
> +
> +static
> +int uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
> +{
> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> +	struct usb_request *req = uvc->control_req;
> +
> +	if (data->length < 0)
> +		return usb_ep_set_halt(cdev->gadget->ep0);
> +
> +	req->length = min_t(unsigned int, uvc->event_length, data->length);
> +	req->zero = data->length < uvc->event_length;
> +
> +	memcpy(req->buf, data->data, req->length);
> +
> +	return usb_ep_queue(cdev->gadget->ep0, req, GFP_KERNEL);
> +}

An exact copy of this function already exists in uvc_v4l2.c. Over there
it's used to handle UVCIOC_SEND_RESPONSE, but the content and purpose of
this and the other copy are the same, so I think they can be unified.

> +
>  static void
>  uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct uvc_device *uvc = req->context;
> -	struct v4l2_event v4l2_event;
> -	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
> +	int ret;
>  
>  	if (uvc->event_setup_out) {
>  		uvc->event_setup_out = 0;
> +		ret = uvc_events_process_data(uvc, req);

If !ret, don't you still need to uvc_send_response()? That was the
previous behavior, just that userspace would receive the v4l2 event, and
then respond with UVCIOC_SEND_RESPONSE.

> +		if (ret == -EOPNOTSUPP) {
> +			struct v4l2_event v4l2_event;
> +			struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
>  
> -		memset(&v4l2_event, 0, sizeof(v4l2_event));
> -		v4l2_event.type = UVC_EVENT_DATA;
> -		uvc_event->data.length = req->actual;
> -		memcpy(&uvc_event->data.data, req->buf, req->actual);
> -		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> +			memset(&v4l2_event, 0, sizeof(v4l2_event));
> +			v4l2_event.type = UVC_EVENT_DATA;
> +			uvc_event->data.length = req->actual;
> +			memcpy(&uvc_event->data.data, req->buf, req->actual);
> +			v4l2_event_queue(&uvc->vdev, &v4l2_event);
> +		}
>  	}
>  }
>  
> @@ -223,8 +455,8 @@ static int
>  uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  {
>  	struct uvc_device *uvc = to_uvc(f);
> -	struct v4l2_event v4l2_event;
> -	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
> +	struct uvc_request_data resp;
> +	int ret = 0;
>  
>  	if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
>  		uvcg_info(f, "invalid request type\n");
> @@ -241,12 +473,22 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  	uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
>  	uvc->event_length = le16_to_cpu(ctrl->wLength);
>  
> -	memset(&v4l2_event, 0, sizeof(v4l2_event));
> -	v4l2_event.type = UVC_EVENT_SETUP;
> -	memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
> -	v4l2_event_queue(&uvc->vdev, &v4l2_event);
> +	ret = uvc_events_process_setup(uvc, ctrl, &resp);
> +	if (ret == -EOPNOTSUPP) {
> +		struct v4l2_event v4l2_event;
> +		struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
>  
> -	return 0;
> +		memset(&v4l2_event, 0, sizeof(v4l2_event));
> +		v4l2_event.type = UVC_EVENT_SETUP;
> +		memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
> +		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> +
> +		return 0;
> +	} else if (ret) {
> +		return ret;
> +	}
> +
> +	return uvc_send_response(uvc, &resp);
>  }
>  
>  void uvc_function_setup_continue(struct uvc_device *uvc)
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 62d7420a25666..d06b5282b4058 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -13,6 +13,7 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/usb/composite.h>
> +#include <linux/usb/video.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/v4l2-device.h>
> @@ -85,6 +86,12 @@ struct uvc_video {
>  	struct uvcg_frame *cur_frame;
>  
>  	struct mutex mutex;	/* protects frame parameters */
> +	spinlock_t frame_lock;
> +
> +	struct uvc_streaming_control probe;
> +	struct uvc_streaming_control commit;
> +
> +	int control;
>  
>  	/* Requests */
>  	unsigned int req_size;
> @@ -170,4 +177,11 @@ extern void uvc_function_connect(struct uvc_device *uvc);
>  extern void uvc_function_disconnect(struct uvc_device *uvc);
>  
>  extern int uvc_frame_default(struct uvcg_format *ufmt);
> +extern struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
> +					      struct uvcg_format *ufmt,
> +					      int index);
> +extern void uvc_fill_streaming_control(struct uvc_device *uvc,
> +			   struct uvc_streaming_control *ctrl,
> +			   int iframe, int iformat, unsigned int ival);

Indentation.

> +
>  #endif /* _UVC_GADGET_H_ */
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 83830b8864a6e..a89d3d661d99f 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -205,7 +205,8 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  }
>  
>  static int _uvc_v4l2_try_fmt(struct uvc_video *video,
> -	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
> +	struct v4l2_format *fmt, struct uvc_streaming_control *probe,
> +	struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
>  {
>  	struct uvc_device *uvc = video->uvc;
>  	struct uvcg_format *ufmt;
> @@ -253,6 +254,9 @@ static int _uvc_v4l2_try_fmt(struct uvc_video *video,
>  		*uvc_format = ufmt;
>  	if (uvc_frame != NULL)
>  		*uvc_frame = ufrm;
> +	if (probe)
> +		uvc_fill_streaming_control(uvc, probe,
> +				i + 1, ufrm->frame.b_frame_index, 0);
>  
>  	return 0;
>  }
> @@ -264,7 +268,7 @@ uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>  	struct uvc_video *video = &uvc->video;
>  
> -	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
> +	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL, NULL);
>  }
>  
>  static int
> @@ -273,17 +277,23 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	struct video_device *vdev = video_devdata(file);
>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>  	struct uvc_video *video = &uvc->video;
> +	struct uvc_streaming_control probe;
>  	struct uvcg_format *ufmt;
>  	struct uvcg_frame *ufrm;
>  	int ret;
>  
> -	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
> +	ret = _uvc_v4l2_try_fmt(video, fmt, &probe, &ufmt, &ufrm);
>  	if (ret)
>  		return ret;
>  
> +	spin_lock(&video->frame_lock);
> +
> +	video->commit = probe;
>  	video->cur_format = ufmt;
>  	video->cur_frame = ufrm;
>  
> +	spin_unlock(&video->frame_lock);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index b14780bddd838..8f549fe608362 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -346,13 +346,26 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>   */
>  int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>  {
> +	int framedef;
> +
>  	INIT_LIST_HEAD(&video->req_free);
>  	spin_lock_init(&video->req_lock);
> +	spin_lock_init(&video->frame_lock);
> +
> +	/* Allocate a stream specific work queue for asynchronous tasks. */
> +	video->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE, 0);

Where does async_wq come from?


Thanks,

Paul

> +	if (!video->async_wq)
> +		return -EINVAL;
> +
>  	INIT_WORK(&video->pump, uvcg_video_pump);
>  
>  	video->uvc = uvc;
>  	video->def_format = video->cur_format = uvc->fmt[0];
> -	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];
> +	framedef = uvc_frame_default(video->def_format);
> +	video->cur_frame = uvc->frm[framedef - 1];
> +
> +	uvc_fill_streaming_control(uvc, &video->probe, 1, framedef, 0);
> +	uvc_fill_streaming_control(uvc, &video->commit, 1, framedef, 0);
>  
>  	/* Initialize the video buffers queue. */
>  	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
> -- 
> 2.29.2
> 

Re: [PATCH 1/3] usb: gadget: uvc: move structs to common header

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Mon, May 31, 2021 at 12:22:37AM +0200, Michael Grzeschik wrote:
> The functions and structs of the configfs interface should also be used
> by the uvc gadget driver. This patch prepares the stack by moving the
> common structs and functions to the common header file.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 111 -------------------
>  drivers/usb/gadget/function/uvc_configfs.h | 119 +++++++++++++++++++++
>  2 files changed, 119 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 77d64031aa9c2..86463bb2639ed 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -19,8 +19,6 @@
>   * Global Utility Structures and Macros
>   */
>  
> -#define UVCG_STREAMING_CONTROL_SIZE	1
> -
>  #define UVC_ATTR(prefix, cname, aname) \
>  static struct configfs_attribute prefix##attr_##cname = { \
>  	.ca_name	= __stringify(aname),				\
> @@ -49,12 +47,6 @@ static int uvcg_config_compare_u32(const void *l, const void *r)
>  	return li < ri ? -1 : li == ri ? 0 : 1;
>  }
>  
> -static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
> -{
> -	return container_of(to_config_group(item), struct f_uvc_opts,
> -			    func_inst.group);
> -}
> -
>  struct uvcg_config_group_type {
>  	struct config_item_type type;
>  	const char *name;
> @@ -125,19 +117,6 @@ static void uvcg_config_remove_children(struct config_group *group)
>   * control/header
>   */
>  
> -DECLARE_UVC_HEADER_DESCRIPTOR(1);
> -
> -struct uvcg_control_header {
> -	struct config_item		item;
> -	struct UVC_HEADER_DESCRIPTOR(1)	desc;
> -	unsigned			linked;
> -};
> -
> -static struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_control_header, item);
> -}
> -
>  #define UVCG_CTRL_HDR_ATTR(cname, aname, bits, limit)			\
>  static ssize_t uvcg_control_header_##cname##_show(			\
>  	struct config_item *item, char *page)				\
> @@ -764,29 +743,6 @@ static const struct uvcg_config_group_type uvcg_control_grp_type = {
>   * streaming/mjpeg
>   */
>  
> -static const char * const uvcg_format_names[] = {
> -	"uncompressed",
> -	"mjpeg",
> -};
> -
> -enum uvcg_format_type {
> -	UVCG_UNCOMPRESSED = 0,
> -	UVCG_MJPEG,
> -};
> -
> -struct uvcg_format {
> -	struct config_group	group;
> -	enum uvcg_format_type	type;
> -	unsigned		linked;
> -	unsigned		num_frames;
> -	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> -};
> -
> -static struct uvcg_format *to_uvcg_format(struct config_item *item)
> -{
> -	return container_of(to_config_group(item), struct uvcg_format, group);
> -}
> -
>  static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
>  {
>  	struct f_uvc_opts *opts;
> @@ -845,29 +801,11 @@ static ssize_t uvcg_format_bma_controls_store(struct uvcg_format *ch,
>  	return ret;
>  }
>  
> -struct uvcg_format_ptr {
> -	struct uvcg_format	*fmt;
> -	struct list_head	entry;
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * streaming/header/<NAME>
>   * streaming/header
>   */
>  
> -struct uvcg_streaming_header {
> -	struct config_item				item;
> -	struct uvc_input_header_descriptor		desc;
> -	unsigned					linked;
> -	struct list_head				formats;
> -	unsigned					num_fmt;
> -};
> -
> -static struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_streaming_header, item);
> -}
> -
>  static void uvcg_format_set_indices(struct config_group *fmt);
>  
>  static int uvcg_streaming_header_allow_link(struct config_item *src,
> @@ -1059,31 +997,6 @@ static const struct uvcg_config_group_type uvcg_streaming_header_grp_type = {
>   * streaming/<mode>/<format>/<NAME>
>   */
>  
> -struct uvcg_frame {
> -	struct config_item	item;
> -	enum uvcg_format_type	fmt_type;
> -	struct {
> -		u8	b_length;
> -		u8	b_descriptor_type;
> -		u8	b_descriptor_subtype;
> -		u8	b_frame_index;
> -		u8	bm_capabilities;
> -		u16	w_width;
> -		u16	w_height;
> -		u32	dw_min_bit_rate;
> -		u32	dw_max_bit_rate;
> -		u32	dw_max_video_frame_buffer_size;
> -		u32	dw_default_frame_interval;
> -		u8	b_frame_interval_type;
> -	} __attribute__((packed)) frame;
> -	u32 *dw_frame_interval;
> -};
> -
> -static struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> -{
> -	return container_of(item, struct uvcg_frame, item);
> -}
> -
>  #define UVCG_FRAME_ATTR(cname, aname, bits) \
>  static ssize_t uvcg_frame_##cname##_show(struct config_item *item, char *page)\
>  {									\
> @@ -1420,18 +1333,6 @@ static void uvcg_format_set_indices(struct config_group *fmt)
>   * streaming/uncompressed/<NAME>
>   */
>  
> -struct uvcg_uncompressed {
> -	struct uvcg_format		fmt;
> -	struct uvc_format_uncompressed	desc;
> -};
> -
> -static struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> -{
> -	return container_of(
> -		container_of(to_config_group(item), struct uvcg_format, group),
> -		struct uvcg_uncompressed, fmt);
> -}
> -
>  static struct configfs_group_operations uvcg_uncompressed_group_ops = {
>  	.make_item		= uvcg_frame_make,
>  	.drop_item		= uvcg_frame_drop,
> @@ -1669,18 +1570,6 @@ static const struct uvcg_config_group_type uvcg_uncompressed_grp_type = {
>   * streaming/mjpeg/<NAME>
>   */
>  
> -struct uvcg_mjpeg {
> -	struct uvcg_format		fmt;
> -	struct uvc_format_mjpeg		desc;
> -};
> -
> -static struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> -{
> -	return container_of(
> -		container_of(to_config_group(item), struct uvcg_format, group),
> -		struct uvcg_mjpeg, fmt);
> -}
> -
>  static struct configfs_group_operations uvcg_mjpeg_group_ops = {
>  	.make_item		= uvcg_frame_make,
>  	.drop_item		= uvcg_frame_drop,
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index 7e1d7ca29bf21..f905d29570eb4 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -16,4 +16,123 @@ struct f_uvc_opts;
>  
>  int uvcg_attach_configfs(struct f_uvc_opts *opts);

Function declarations are traditionally placed after structure
definitions, I would thus move this line to the end of the file.

>  
> +static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)

Headers should be self-contained as much as possible. This means that
you need forward declarations and/or #include statements corresponding
to the structure and functions used herein. A very simple way to test
this is to apply the following change:

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index cd28dec837dd..5303b790911b 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -10,10 +10,11 @@
  * Author: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
  */

+#include "uvc_configfs.h"
+
 #include <linux/sort.h>

 #include "u_uvc.h"
-#include "uvc_configfs.h"

 /* -----------------------------------------------------------------------------
  * Global Utility Structures and Macros

Compilation will fail if uvc_configfs.h isn't self-contained. Feel free
to squash the above diff in this patch.

> +{
> +	return container_of(to_config_group(item), struct f_uvc_opts,
> +			    func_inst.group);
> +}
> +
> +#define UVCG_STREAMING_CONTROL_SIZE	1
> +
> +DECLARE_UVC_HEADER_DESCRIPTOR(1);
> +
> +struct uvcg_control_header {
> +	struct config_item		item;
> +	struct UVC_HEADER_DESCRIPTOR(1)	desc;
> +	unsigned			linked;
> +};
> +
> +static inline struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_control_header, item);
> +}
> +
> +static const char * const uvcg_format_names[] = {
> +	"uncompressed",
> +	"mjpeg",
> +};
> +
> +enum uvcg_format_type {
> +	UVCG_UNCOMPRESSED = 0,
> +	UVCG_MJPEG,
> +};
> +
> +struct uvcg_format {
> +	struct config_group	group;
> +	enum uvcg_format_type	type;
> +	unsigned		linked;
> +	unsigned		num_frames;
> +	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +};
> +
> +struct uvcg_format_ptr {
> +	struct uvcg_format	*fmt;
> +	struct list_head	entry;
> +};
> +
> +static inline struct uvcg_format *to_uvcg_format(struct config_item *item)
> +{
> +	return container_of(to_config_group(item), struct uvcg_format, group);
> +}
> +
> +struct uvcg_streaming_header {
> +	struct config_item				item;
> +	struct uvc_input_header_descriptor		desc;
> +	unsigned					linked;
> +	struct list_head				formats;
> +	unsigned					num_fmt;
> +};
> +
> +static inline struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_streaming_header, item);
> +}
> +
> +struct uvcg_frame {
> +	struct config_item	item;
> +	enum uvcg_format_type	fmt_type;
> +	struct {
> +		u8	b_length;
> +		u8	b_descriptor_type;
> +		u8	b_descriptor_subtype;
> +		u8	b_frame_index;
> +		u8	bm_capabilities;
> +		u16	w_width;
> +		u16	w_height;
> +		u32	dw_min_bit_rate;
> +		u32	dw_max_bit_rate;
> +		u32	dw_max_video_frame_buffer_size;
> +		u32	dw_default_frame_interval;
> +		u8	b_frame_interval_type;
> +	} __attribute__((packed)) frame;
> +	u32 *dw_frame_interval;
> +};
> +
> +static inline struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> +{
> +	return container_of(item, struct uvcg_frame, item);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/uncompressed/<NAME>
> + */
> +
> +struct uvcg_uncompressed {
> +	struct uvcg_format		fmt;
> +	struct uvc_format_uncompressed	desc;
> +};
> +
> +static inline struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> +{
> +	return container_of(
> +		container_of(to_config_group(item), struct uvcg_format, group),
> +		struct uvcg_uncompressed, fmt);

While at it, how about writing this

	return container_of(to_uvcg_format(item), struct uvcg_uncompressed, fmt);

?

> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/mjpeg/<NAME>
> + */
> +
> +struct uvcg_mjpeg {
> +	struct uvcg_format		fmt;
> +	struct uvc_format_mjpeg		desc;
> +};
> +
> +static inline struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> +{
> +	return container_of(
> +		container_of(to_config_group(item), struct uvcg_format, group),
> +		struct uvcg_mjpeg, fmt);

Same here with

	return container_of(to_uvcg_format(item), struct uvcg_mjpeg, fmt);

With these issues addressed,

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

> +}
> +
>  #endif /* UVC_CONFIGFS_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Fri, Jun 11, 2021 at 03:40:35PM +0900, paul.elder@ideasonboard.com wrote:
> On Mon, May 31, 2021 at 12:22:38AM +0200, Michael Grzeschik wrote:
> > This patch adds support to the v4l2 VIDIOC for enum_format,
> > enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
> > setup gets parsed and this configfs data is used in the v4l2 interface
> > functions.

Could you please expand the commit message with an explanation of why
this is a good idea ?

> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/usb/gadget/function/f_uvc.c        |  54 ++++
> >  drivers/usb/gadget/function/uvc.h          |  18 +-
> >  drivers/usb/gadget/function/uvc_configfs.c |   5 +
> >  drivers/usb/gadget/function/uvc_configfs.h |   2 +
> >  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
> >  drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
> >  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
> >  drivers/usb/gadget/function/uvc_video.c    |  10 +-
> >  8 files changed, 369 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > index f48a00e497945..7945ea93a775a 100644
> > --- a/drivers/usb/gadget/function/f_uvc.c
> > +++ b/drivers/usb/gadget/function/f_uvc.c
> > @@ -410,6 +410,44 @@ static ssize_t function_name_show(struct device *dev,
> >  
> >  static DEVICE_ATTR_RO(function_name);
> >  
> > +static int
> > +uvc_analyze_configfs(struct uvc_device *uvc)
> > +{
> > +	struct uvcg_streaming_header *src_hdr = uvc->h;

Why 'src' ? There's no source here. You can name the variable header.

> > +	struct config_item *item;
> > +	struct config_group *grp;

And I'd group in full.

> > +	struct uvcg_format_ptr *f;

I'd make all those pointers const if possible.

> > +	int i = 0, j = 0;

i and j are never negative, you can make them unsigned int.

> > +
> > +	if (!src_hdr->linked)
> > +		return -EBUSY;
> > +
> > +	list_for_each_entry(f, &src_hdr->formats, entry)
> > +		uvc->nframes += f->fmt->num_frames;

Can this overflow uvc->nframes if userspace creates a malicious config
with a large number of formats and frames ? Or are there limits set
elsewhere that would prevent this from happening ?

> > +
> > +	uvc->nformats = src_hdr->num_fmt;
> > +
> > +	uvc->frm = kcalloc(uvc->nframes, sizeof(struct uvcg_frame *), GFP_KERNEL);

sizeof(*uvc->frm)

> > +	if (!uvc->frm)
> > +		return -ENOMEM;
> > +
> > +	uvc->fmt = kcalloc(uvc->nformats, sizeof(struct uvcg_format *), GFP_KERNEL);

sizeof(*uvc->fmt)

> > +	if (!uvc->fmt) {
> > +		kfree(uvc->frm);
> > +		return -ENOMEM;
> > +	}
> 
> nformats/nframes should be set to zero if the kcalloc(s) fail.
> 
> > +
> > +	list_for_each_entry(f, &src_hdr->formats, entry) {
> > +		uvc->fmt[i++] = f->fmt;
> > +		grp = &f->fmt->group;
> > +		list_for_each_entry(item, &grp->cg_children, ci_entry) {
> > +			uvc->frm[j++] = to_uvcg_frame(item);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  uvc_register_video(struct uvc_device *uvc)
> >  {
> > @@ -742,6 +780,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> >  		goto error;
> >  	}
> >  
> > +	/* Register configfs formats and frames. */
> > +	ret = uvc_analyze_configfs(uvc);
> > +	if (ret < 0) {
> > +		uvcg_err(f, "failed to read configfs\n");
> > +		goto v4l2_error;
> 
> s/v4l2_error/error/ ?

I think v4l2_error is correct as this code is just after
v4l2_device_register().

> > +	}
> > +
> >  	/* Initialise video. */
> >  	ret = uvcg_video_init(&uvc->video, uvc);
> >  	if (ret < 0)
> > @@ -905,6 +950,8 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
> >  	struct uvc_device *uvc;
> >  	struct f_uvc_opts *opts;
> >  	struct uvc_descriptor_header **strm_cls;
> > +	struct config_item *item;
> > +	struct config_group *grp;

s/grp/group/

> >  
> >  	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
> >  	if (uvc == NULL)
> > @@ -936,6 +983,13 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
> >  	uvc->desc.fs_streaming = opts->fs_streaming;
> >  	uvc->desc.hs_streaming = opts->hs_streaming;
> >  	uvc->desc.ss_streaming = opts->ss_streaming;
> > +
> > +	grp = &opts->func_inst.group;
> > +	item = config_group_find_item(grp, "streaming");

As grp is used once only, you could also write

	item = config_group_find_item(&opts->func_inst.group, "streaming");

> > +	item = config_group_find_item(to_config_group(item), "header");
> > +	item = config_group_find_item(to_config_group(item), "h");
> 
> These return values should be checked. It's conceivable that userspace
> neglects to create these directories (either by accident or on purpose).

Furthermore, config_group_find_item() increases the refcout on the
returned item, which needs to be released.

> > +	uvc->h = to_uvcg_streaming_header(item);

As a pointer to the last item is indirectly stored here, that refcount
must be released only when uvc->h isn't needed anymore.

> > +
> >  	++opts->refcnt;
> >  	mutex_unlock(&opts->lock);
> >  
> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> > index 23ee25383c1f7..62d7420a25666 100644
> > --- a/drivers/usb/gadget/function/uvc.h
> > +++ b/drivers/usb/gadget/function/uvc.h
> > @@ -80,11 +80,10 @@ struct uvc_video {
> >  	struct work_struct pump;
> >  
> >  	/* Frame parameters */
> > -	u8 bpp;
> > -	u32 fcc;
> > -	unsigned int width;
> > -	unsigned int height;
> > -	unsigned int imagesize;
> > +	struct uvcg_format *def_format;
> > +	struct uvcg_format *cur_format;
> > +	struct uvcg_frame *cur_frame;
> > +
> >  	struct mutex mutex;	/* protects frame parameters */
> >  
> >  	/* Requests */
> > @@ -118,6 +117,14 @@ struct uvc_device {
> >  	struct usb_function func;
> >  	struct uvc_video video;
> >  
> > +	struct uvcg_streaming_header *h;
> > +
> > +	struct uvcg_frame **frm;
> > +	int nframes;
> > +
> > +	struct uvcg_format **fmt;

Please spell frames and formats in full.

> > +	int nformats;

nframes and nformats should be unsigned int.

> > +
> >  	/* Descriptors */
> >  	struct {
> >  		const struct uvc_descriptor_header * const *fs_control;
> > @@ -162,4 +169,5 @@ extern void uvc_endpoint_stream(struct uvc_device *dev);
> >  extern void uvc_function_connect(struct uvc_device *uvc);
> >  extern void uvc_function_disconnect(struct uvc_device *uvc);
> >  
> > +extern int uvc_frame_default(struct uvcg_format *ufmt);
> >  #endif /* _UVC_GADGET_H_ */
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > index 86463bb2639ed..009c80d0e1780 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -11,6 +11,7 @@
> >   */
> >  
> >  #include <linux/sort.h>
> > +#include <linux/videodev2.h>
> >  
> >  #include "u_uvc.h"
> >  #include "uvc_configfs.h"
> > @@ -1547,6 +1548,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
> >  	h->desc.bCopyProtect		= 0;
> >  
> >  	h->fmt.type = UVCG_UNCOMPRESSED;
> > +	h->fmt.fcc = V4L2_PIX_FMT_YUYV;
> > +	h->fmt.name = "YUV 4:2:2 (YUYV)";

Uncompressed formats are not limited to YUYV, you need to support any
uncompressed format, or this will be a regression.

> >  	config_group_init_type_name(&h->fmt.group, name,
> >  				    &uvcg_uncompressed_type);
> >  
> > @@ -1721,6 +1724,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
> >  	h->desc.bCopyProtect		= 0;
> >  
> >  	h->fmt.type = UVCG_MJPEG;
> > +	h->fmt.fcc = V4L2_PIX_FMT_MJPEG;
> > +	h->fmt.name = "MJPEG";
> >  	config_group_init_type_name(&h->fmt.group, name,
> >  				    &uvcg_mjpeg_type);
> >  
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> > index f905d29570eb4..8ed966275f838 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.h
> > +++ b/drivers/usb/gadget/function/uvc_configfs.h
> > @@ -52,6 +52,8 @@ struct uvcg_format {
> >  	enum uvcg_format_type	type;
> >  	unsigned		linked;
> >  	unsigned		num_frames;
> > +	char			*name;
> > +	u32			fcc;
> >  	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> >  };
> >  
> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> > index 61e2c94cc0b0c..6afc4b79adfe9 100644
> > --- a/drivers/usb/gadget/function/uvc_queue.c
> > +++ b/drivers/usb/gadget/function/uvc_queue.c
> > @@ -20,6 +20,8 @@
> >  #include <media/videobuf2-vmalloc.h>
> >  
> >  #include "uvc.h"
> > +#include "u_uvc.h"
> > +#include "uvc_configfs.h"
> >  
> >  /* ------------------------------------------------------------------------
> >   * Video buffers queue management.
> > @@ -49,7 +51,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
> >  
> >  	*nplanes = 1;
> >  
> > -	sizes[0] = video->imagesize;
> > +	sizes[0] = video->cur_frame->frame.dw_max_video_frame_buffer_size;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> > index 4ca89eab61590..83830b8864a6e 100644
> > --- a/drivers/usb/gadget/function/uvc_v4l2.c
> > +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> > @@ -24,6 +24,127 @@
> >  #include "uvc_queue.h"
> >  #include "uvc_video.h"
> >  #include "uvc_v4l2.h"
> > +#include "u_uvc.h"
> > +#include "uvc_configfs.h"
> > +
> > +u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm)
> > +{
> > +	struct uvcg_uncompressed *u;
> > +
> > +	switch (fmt->type) {
> > +	case UVCG_UNCOMPRESSED:
> > +		u = to_uvcg_uncompressed(&fmt->group.cg_item);
> > +		if (!u)
> > +			return 0;
> > +
> > +		return u->desc.bBitsPerPixel * frm->frame.w_width / 8;
> > +	case UVCG_MJPEG:
> > +		return frm->frame.w_width;
> 
> From the struct v4l2_pix_format documentation:
> "For compressed formats the bytesperline value makes no sense.
> Applications and drivers must set this to 0 in that case."
> 
> Which means that some sizeimage calculations in uvc_v4l2.c would have to
> be adjusted accordingly.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
> > +					      struct uvcg_format *ufmt,
> > +					      int index)
> 
> Indentation.
> 
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < uvc->nframes; i++) {
> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
> > +			continue;
> > +
> > +		if (index == uvc->frm[i]->frame.b_frame_index)
> > +			break;
> > +	}
> > +
> > +	if (i == uvc->nframes)
> > +		return NULL;
> > +
> > +	return uvc->frm[i];
> > +}
> > +
> > +static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
> > +						    u32 pixelformat)
> 
> Indentation.
> 
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < uvc->nformats; i++)
> > +		if (uvc->fmt[i]->fcc == pixelformat)
> > +			break;
> > +
> > +	if (i == uvc->nformats)
> > +		return NULL;
> > +
> > +	return uvc->fmt[i];
> > +}
> > +
> > +int uvc_frame_default(struct uvcg_format *ufmt)
> > +{
> > +	struct uvcg_uncompressed *m;
> > +	struct uvcg_uncompressed *u;
> > +	int ret = 1;
> > +
> > +	switch (ufmt->type) {
> > +	case UVCG_UNCOMPRESSED:
> > +		u = to_uvcg_uncompressed(&ufmt->group.cg_item);
> > +		if (u)
> > +			ret = u->desc.bDefaultFrameIndex;
> > +		break;
> > +	case UVCG_MJPEG:
> > +		m = to_uvcg_uncompressed(&ufmt->group.cg_item);
> > +		if (m)
> > +			ret = m->desc.bDefaultFrameIndex;
> > +		break;
> > +	}
> > +
> > +	if (!ret)
> > +		ret = 1;
> 
> Isn't 1 a valid frame index? Should this (and the initialization above)
> be -1 instead?
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static struct uvcg_frame *find_frm_by_size(struct uvc_device *uvc,
> > +					   struct uvcg_format *ufmt,
> > +					   u16 rw, u16 rh)
> 
> Also, since you have find_frame_by_index, maybe this should be
> find_frame_by_size for consistency. Or maybe find_closest_frame_by_size?
> Up to you.
> 
> > +{
> > +	struct uvc_video *video = &uvc->video;
> > +	struct uvcg_frame *ufrm = NULL;
> > +	unsigned int d, maxd;
> > +	int i;
> > +
> > +	/* Find the closest image size. The distance between image sizes is
> > +	 * the size in pixels of the non-overlapping regions between the
> > +	 * requested size and the frame-specified size.
> > +	 */
> > +	maxd = (unsigned int)-1;
> > +
> > +	for (i = 0; i < uvc->nframes; i++) {
> > +		u16 w, h;
> > +
> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
> > +			continue;
> > +
> > +		w = uvc->frm[i]->frame.w_width;
> > +		h = uvc->frm[i]->frame.w_height;
> > +
> > +		d = min(w, rw) * min(h, rh);
> > +		d = w*h + rw*rh - 2*d;
> > +		if (d < maxd) {
> > +			maxd = d;
> > +			ufrm = uvc->frm[i];
> > +		}
> > +
> > +		if (maxd == 0)
> > +			break;
> > +	}
> > +
> > +	if (ufrm == NULL)
> > +		uvcg_dbg(&video->uvc->func, "Unsupported size %ux%u\n", rw, rh);
> > +
> > +	return ufrm;
> > +}
> >  
> >  /* --------------------------------------------------------------------------
> >   * Requests handling
> > @@ -50,16 +171,6 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
> >   * V4L2 ioctls
> >   */
> >  
> > -struct uvc_format {
> > -	u8 bpp;
> > -	u32 fcc;
> > -};
> > -
> > -static struct uvc_format uvc_formats[] = {
> > -	{ 16, V4L2_PIX_FMT_YUYV  },
> > -	{ 0,  V4L2_PIX_FMT_MJPEG },
> > -};
> > -
> >  static int
> >  uvc_v4l2_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
> >  {
> > @@ -81,55 +192,187 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
> >  	struct uvc_device *uvc = video_get_drvdata(vdev);
> >  	struct uvc_video *video = &uvc->video;
> >  
> > -	fmt->fmt.pix.pixelformat = video->fcc;
> > -	fmt->fmt.pix.width = video->width;
> > -	fmt->fmt.pix.height = video->height;
> > +	fmt->fmt.pix.pixelformat = video->cur_format->fcc;
> > +	fmt->fmt.pix.width = video->cur_frame->frame.w_width;
> > +	fmt->fmt.pix.height = video->cur_frame->frame.w_height;
> > +	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> > +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format, video->cur_frame);
> > +	fmt->fmt.pix.sizeimage = video->cur_frame->frame.dw_max_video_frame_buffer_size;
> > +	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > +	fmt->fmt.pix.priv = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static int _uvc_v4l2_try_fmt(struct uvc_video *video,
> > +	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
> 
> Indentation.
> 
> > +{
> > +	struct uvc_device *uvc = video->uvc;
> > +	struct uvcg_format *ufmt;
> > +	struct uvcg_frame *ufrm;
> > +	u8 *fcc;
> > +	int i;
> > +
> > +	if (fmt->type != video->queue.queue.type)
> > +		return -EINVAL;
> > +
> > +	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
> > +	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
> > +		fmt->fmt.pix.pixelformat,
> > +		fcc[0], fcc[1], fcc[2], fcc[3],
> > +		fmt->fmt.pix.width, fmt->fmt.pix.height);
> > +
> > +	for (i = 0; i < uvc->nformats; i++)
> > +		if (uvc->fmt[i]->fcc == fmt->fmt.pix.pixelformat)
> > +			break;
> > +
> > +	if (i == uvc->nformats)
> > +		ufmt = video->def_format;
> 
> This means we don't support the requested format. Shouldn't we return
> -EINVAL at this point?
> 
> > +
> > +	ufmt = uvc->fmt[i];
> > +
> > +	ufrm = find_frm_by_size(uvc, ufmt,
> > +				fmt->fmt.pix.width, fmt->fmt.pix.height);
> > +	if (!ufrm)
> > +		return -EINVAL;
> > +
> > +	fmt->fmt.pix.width = ufrm->frame.w_width;
> > +	fmt->fmt.pix.height = ufrm->frame.w_height;
> >  	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> > -	fmt->fmt.pix.bytesperline = video->bpp * video->width / 8;
> > -	fmt->fmt.pix.sizeimage = video->imagesize;
> > +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(ufmt, ufrm);
> 
> As mentioned earlier, this should be 0 for compressed formats.
> 
> > +	fmt->fmt.pix.sizeimage = ufrm->frame.dw_max_video_frame_buffer_size;
> > +	fmt->fmt.pix.pixelformat = ufmt->fcc;
> >  	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> >  	fmt->fmt.pix.priv = 0;
> >  
> > +	if (!fmt->fmt.pix.sizeimage && fmt->fmt.pix.bytesperline)
> > +		fmt->fmt.pix.sizeimage = fmt->fmt.pix.bytesperline *
> > +				fmt->fmt.pix.height;
> 
> Since the configfs configuration is required to configure the driver, I
> think it should be fine to just take the value from there, especially
> since bytesperline will be zero for compressed formats anyway.
> 
> Maybe for uncompressed we could just use what's requested?
> 
> > +
> > +	if (uvc_format != NULL)
> > +		*uvc_format = ufmt;
> > +	if (uvc_frame != NULL)
> > +		*uvc_frame = ufrm;
> > +
> >  	return 0;
> >  }
> >  
> > +static int
> > +uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
> > +	struct uvc_video *video = &uvc->video;
> > +
> > +	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
> > +}
> > +
> >  static int
> >  uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
> >  {
> >  	struct video_device *vdev = video_devdata(file);
> >  	struct uvc_device *uvc = video_get_drvdata(vdev);
> >  	struct uvc_video *video = &uvc->video;
> > -	struct uvc_format *format;
> > -	unsigned int imagesize;
> > -	unsigned int bpl;
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < ARRAY_SIZE(uvc_formats); ++i) {
> > -		format = &uvc_formats[i];
> > -		if (format->fcc == fmt->fmt.pix.pixelformat)
> > +	struct uvcg_format *ufmt;
> > +	struct uvcg_frame *ufrm;
> > +	int ret;
> > +
> > +	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	video->cur_format = ufmt;
> > +	video->cur_frame = ufrm;
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
> > +		struct v4l2_frmivalenum *fival)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
> > +	struct uvcg_format *ufmt = NULL;
> > +	struct uvcg_frame *ufrm = NULL;
> > +	int i;
> > +
> > +	ufmt = find_format_by_pix(uvc, fival->pixel_format);
> > +	if (!ufmt)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < uvc->nframes; ++i) {
> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
> > +			continue;
> > +
> > +		if (uvc->frm[i]->frame.w_width == fival->width &&
> > +				uvc->frm[i]->frame.w_height == fival->height) {
> 
> Indentation.
> 
> > +			ufrm = uvc->frm[i];
> >  			break;
> > +		}
> >  	}
> > +	if (!ufrm)
> > +		return -EINVAL;
> >  
> > -	if (i == ARRAY_SIZE(uvc_formats)) {
> > -		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
> > -			  fmt->fmt.pix.pixelformat);
> > +	if (fival->index >= ufrm->frame.b_frame_interval_type)
> >  		return -EINVAL;
> > -	}
> >  
> > -	bpl = format->bpp * fmt->fmt.pix.width / 8;
> > -	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
> > +	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
> > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > +	fival->discrete.numerator = ufrm->dw_frame_interval[fival->index];
> > +	fival->discrete.denominator = 10000000;
> > +	v4l2_simplify_fraction(&fival->discrete.numerator,
> 
> Where does this function come from?
> 
> > +		&fival->discrete.denominator, 8, 333);
> >  
> > -	video->fcc = format->fcc;
> > -	video->bpp = format->bpp;
> > -	video->width = fmt->fmt.pix.width;
> > -	video->height = fmt->fmt.pix.height;
> > -	video->imagesize = imagesize;
> > +	return 0;
> > +}
> >  
> > -	fmt->fmt.pix.field = V4L2_FIELD_NONE;
> > -	fmt->fmt.pix.bytesperline = bpl;
> > -	fmt->fmt.pix.sizeimage = imagesize;
> > -	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > -	fmt->fmt.pix.priv = 0;
> > +static int
> > +uvc_v4l2_enum_framesizes(struct file *file, void *fh,
> > +		struct v4l2_frmsizeenum *fsize)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
> > +	struct uvcg_format *ufmt = NULL;
> > +	struct uvcg_frame *ufrm = NULL;
> > +
> > +	ufmt = find_format_by_pix(uvc, fsize->pixel_format);
> > +	if (!ufmt)
> > +		return -EINVAL;
> > +
> > +	if (fsize->index >= ufmt->num_frames)
> > +		return -EINVAL;
> > +
> > +	ufrm = find_frame_by_index(uvc, ufmt, fsize->index + 1);
> > +	if (!ufrm)
> > +		return -EINVAL;
> > +
> > +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> > +	fsize->discrete.width = ufrm->frame.w_width;
> > +	fsize->discrete.height = ufrm->frame.w_height;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +uvc_v4l2_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
> > +	struct uvcg_format *ufmt;
> > +
> > +	if (f->index >= uvc->nformats)
> > +		return -EINVAL;
> > +
> > +	ufmt = uvc->fmt[f->index];
> > +	if (!ufmt)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = ufmt->fcc;
> > +	f->flags |= V4L2_FMT_FLAG_COMPRESSED;
> 
> This shouldn't be set if the format isn't compressed.
> 
> > +
> > +	strscpy(f->description, ufmt->name, sizeof(f->description));
> > +	f->description[sizeof(f->description) - 1] = 0;
> 
> If sizeof(ufmt->name) < sizeof(f->description), then the string won't be
> properly null-terminated.
> 
> >  
> >  	return 0;
> >  }
> > @@ -258,8 +501,12 @@ uvc_v4l2_ioctl_default(struct file *file, void *fh, bool valid_prio,
> >  
> >  const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
> >  	.vidioc_querycap = uvc_v4l2_querycap,
> > +	.vidioc_try_fmt_vid_out = uvc_v4l2_try_fmt,
> >  	.vidioc_g_fmt_vid_out = uvc_v4l2_get_format,
> >  	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
> > +	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
> > +	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
> > +	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_fmt,

What's the point of implementing these functions, when userspace will
have created the descriptors in configfs, and thus know what the
supported formats, sizes and frame intervals are ?

To be honest, I'm not thrilled by this series. Unless there's a very
good reason to handle all this on the kernel side, I think it just
increases the complexity of the kernel code without much benefit.

> >  	.vidioc_reqbufs = uvc_v4l2_reqbufs,
> >  	.vidioc_querybuf = uvc_v4l2_querybuf,
> >  	.vidioc_qbuf = uvc_v4l2_qbuf,
> > diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
> > index 1576005b61fd3..6e45103bbf793 100644
> > --- a/drivers/usb/gadget/function/uvc_v4l2.h
> > +++ b/drivers/usb/gadget/function/uvc_v4l2.h
> > @@ -15,5 +15,6 @@
> >  
> >  extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
> >  extern const struct v4l2_file_operations uvc_v4l2_fops;
> > +extern u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm);
> 
> Do you need this here? Nobody outside of uvc_v4l2.c uses it.
> 
> >  
> >  #endif /* __UVC_V4L2_H__ */
> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> > index 633e23d58d868..b14780bddd838 100644
> > --- a/drivers/usb/gadget/function/uvc_video.c
> > +++ b/drivers/usb/gadget/function/uvc_video.c
> > @@ -17,7 +17,10 @@
> >  
> >  #include "uvc.h"
> >  #include "uvc_queue.h"
> > +#include "uvc_v4l2.h"
> >  #include "uvc_video.h"
> > +#include "u_uvc.h"
> > +#include "uvc_configfs.h"
> >  
> >  /* --------------------------------------------------------------------------
> >   * Video codecs
> > @@ -348,11 +351,8 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
> >  	INIT_WORK(&video->pump, uvcg_video_pump);
> >  
> >  	video->uvc = uvc;
> > -	video->fcc = V4L2_PIX_FMT_YUYV;
> > -	video->bpp = 16;
> > -	video->width = 320;
> > -	video->height = 240;
> > -	video->imagesize = 320 * 240 * 2;
> > +	video->def_format = video->cur_format = uvc->fmt[0];

There's nothing in the UVC specification that requires the first format
to be the default. It's entirely conceivable that a device could select
the default dynamically.

> > +	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];
> 
> I think we'll need something here to validate the configfs, to make sure
> it's filled corretly. I anticipate that uvc->fmt[0] might cause problems
> if no format is set in configfs.
> 
> Or maybe such protection is already around and I'm not aware of it.
> 
> >  	/* Initialize the video buffers queue. */
> >  	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[Laurent Pinchart]

Hi Greg,

On Wed, Jun 09, 2021 at 10:16:53AM +0200, Greg KH wrote:
> <resending as your email headers were b0rked causing replies to only go
> to you...>
> 
> On Mon, May 31, 2021 at 12:22:38AM +0200, Michael Grzeschik wrote:
> > This patch adds support to the v4l2 VIDIOC for enum_format,
> > enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
> > setup gets parsed and this configfs data is used in the v4l2 interface
> > functions.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/usb/gadget/function/f_uvc.c        |  54 ++++
> >  drivers/usb/gadget/function/uvc.h          |  18 +-
> >  drivers/usb/gadget/function/uvc_configfs.c |   5 +
> >  drivers/usb/gadget/function/uvc_configfs.h |   2 +
> >  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
> >  drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
> >  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
> >  drivers/usb/gadget/function/uvc_video.c    |  10 +-
> >  8 files changed, 369 insertions(+), 50 deletions(-)
> 
> It would be great to have some v4l developers review/ack this so we know
> if it's ok to take or not.

Done. Sorry for the delay.

tl;dr: I miss the rationale for this patch series, I don't really see
the benefit of moving code from the userspace helper application to the
kernel.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] usb: gadget: uvc: add VIDIOC function

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 28625 bytes --]

Hi Laurent!

On Tue, Jun 15, 2021 at 04:17:36AM +0300, Laurent Pinchart wrote:
>Hi Michael,
>
>Thank you for the patch.
>
>On Fri, Jun 11, 2021 at 03:40:35PM +0900, paul.elder@ideasonboard.com wrote:
>> On Mon, May 31, 2021 at 12:22:38AM +0200, Michael Grzeschik wrote:
>> > This patch adds support to the v4l2 VIDIOC for enum_format,
>> > enum_framesizes, enum_frameintervals and try_fmt. The configfs userspace
>> > setup gets parsed and this configfs data is used in the v4l2 interface
>> > functions.
>
>Could you please expand the commit message with an explanation of why
>this is a good idea ?

The Idea behind this, is to use the v4l2 api to get the configurable
formats. So the userspace has not to bring its own configfs parser
like in the uvc-gadget tool.

Why not use the interface that is already there?

I will add those details to the commit message.

>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> > ---
>> >  drivers/usb/gadget/function/f_uvc.c        |  54 ++++
>> >  drivers/usb/gadget/function/uvc.h          |  18 +-
>> >  drivers/usb/gadget/function/uvc_configfs.c |   5 +
>> >  drivers/usb/gadget/function/uvc_configfs.h |   2 +
>> >  drivers/usb/gadget/function/uvc_queue.c    |   4 +-
>> >  drivers/usb/gadget/function/uvc_v4l2.c     | 325 ++++++++++++++++++---
>> >  drivers/usb/gadget/function/uvc_v4l2.h     |   1 +
>> >  drivers/usb/gadget/function/uvc_video.c    |  10 +-
>> >  8 files changed, 369 insertions(+), 50 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> > index f48a00e497945..7945ea93a775a 100644
>> > --- a/drivers/usb/gadget/function/f_uvc.c
>> > +++ b/drivers/usb/gadget/function/f_uvc.c
>> > @@ -410,6 +410,44 @@ static ssize_t function_name_show(struct device *dev,
>> >
>> >  static DEVICE_ATTR_RO(function_name);
>> >
>> > +static int
>> > +uvc_analyze_configfs(struct uvc_device *uvc)
>> > +{
>> > +	struct uvcg_streaming_header *src_hdr = uvc->h;
>
>Why 'src' ? There's no source here. You can name the variable header.

I will rename it to header.

>
>> > +	struct config_item *item;
>> > +	struct config_group *grp;
>
>And I'd group in full.

Right.

>
>> > +	struct uvcg_format_ptr *f;
>
>I'd make all those pointers const if possible.

const sounds good. I will fix it.

>
>> > +	int i = 0, j = 0;
>
>i and j are never negative, you can make them unsigned int.

OK.

>
>> > +
>> > +	if (!src_hdr->linked)
>> > +		return -EBUSY;
>> > +
>> > +	list_for_each_entry(f, &src_hdr->formats, entry)
>> > +		uvc->nframes += f->fmt->num_frames;
>
>Can this overflow uvc->nframes if userspace creates a malicious config
>with a large number of formats and frames ? Or are there limits set
>elsewhere that would prevent this from happening ?

I will check this and handle it in v2. Thanks

>
>> > +
>> > +	uvc->nformats = src_hdr->num_fmt;
>> > +
>> > +	uvc->frm = kcalloc(uvc->nframes, sizeof(struct uvcg_frame *), GFP_KERNEL);
>
>sizeof(*uvc->frm)

Right.

>
>> > +	if (!uvc->frm)
>> > +		return -ENOMEM;
>> > +
>> > +	uvc->fmt = kcalloc(uvc->nformats, sizeof(struct uvcg_format *), GFP_KERNEL);
>
>sizeof(*uvc->fmt)

Right.

>
>> > +	if (!uvc->fmt) {
>> > +		kfree(uvc->frm);
>> > +		return -ENOMEM;
>> > +	}
>>
>> nformats/nframes should be set to zero if the kcalloc(s) fail.
>>
>> > +
>> > +	list_for_each_entry(f, &src_hdr->formats, entry) {
>> > +		uvc->fmt[i++] = f->fmt;
>> > +		grp = &f->fmt->group;
>> > +		list_for_each_entry(item, &grp->cg_children, ci_entry) {
>> > +			uvc->frm[j++] = to_uvcg_frame(item);
>> > +		}
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> >  static int
>> >  uvc_register_video(struct uvc_device *uvc)
>> >  {
>> > @@ -742,6 +780,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>> >  		goto error;
>> >  	}
>> >
>> > +	/* Register configfs formats and frames. */
>> > +	ret = uvc_analyze_configfs(uvc);
>> > +	if (ret < 0) {
>> > +		uvcg_err(f, "failed to read configfs\n");
>> > +		goto v4l2_error;
>>
>> s/v4l2_error/error/ ?
>
>I think v4l2_error is correct as this code is just after
>v4l2_device_register().


Yes. I will keep it.

>
>> > +	}
>> > +
>> >  	/* Initialise video. */
>> >  	ret = uvcg_video_init(&uvc->video, uvc);
>> >  	if (ret < 0)
>> > @@ -905,6 +950,8 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>> >  	struct uvc_device *uvc;
>> >  	struct f_uvc_opts *opts;
>> >  	struct uvc_descriptor_header **strm_cls;
>> > +	struct config_item *item;
>> > +	struct config_group *grp;
>
>s/grp/group/

Right.

>
>> >
>> >  	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
>> >  	if (uvc == NULL)
>> > @@ -936,6 +983,13 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>> >  	uvc->desc.fs_streaming = opts->fs_streaming;
>> >  	uvc->desc.hs_streaming = opts->hs_streaming;
>> >  	uvc->desc.ss_streaming = opts->ss_streaming;
>> > +
>> > +	grp = &opts->func_inst.group;
>> > +	item = config_group_find_item(grp, "streaming");
>
>As grp is used once only, you could also write
>
>	item = config_group_find_item(&opts->func_inst.group, "streaming");
>
>> > +	item = config_group_find_item(to_config_group(item), "header");
>> > +	item = config_group_find_item(to_config_group(item), "h");
>>
>> These return values should be checked. It's conceivable that userspace
>> neglects to create these directories (either by accident or on purpose).
>
>Furthermore, config_group_find_item() increases the refcout on the
>returned item, which needs to be released.


I will handle the release in v2.

>
>> > +	uvc->h = to_uvcg_streaming_header(item);
>
>As a pointer to the last item is indirectly stored here, that refcount
>must be released only when uvc->h isn't needed anymore.


Right. Will fix it.

>
>> > +
>> >  	++opts->refcnt;
>> >  	mutex_unlock(&opts->lock);
>> >
>> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> > index 23ee25383c1f7..62d7420a25666 100644
>> > --- a/drivers/usb/gadget/function/uvc.h
>> > +++ b/drivers/usb/gadget/function/uvc.h
>> > @@ -80,11 +80,10 @@ struct uvc_video {
>> >  	struct work_struct pump;
>> >
>> >  	/* Frame parameters */
>> > -	u8 bpp;
>> > -	u32 fcc;
>> > -	unsigned int width;
>> > -	unsigned int height;
>> > -	unsigned int imagesize;
>> > +	struct uvcg_format *def_format;
>> > +	struct uvcg_format *cur_format;
>> > +	struct uvcg_frame *cur_frame;
>> > +
>> >  	struct mutex mutex;	/* protects frame parameters */
>> >
>> >  	/* Requests */
>> > @@ -118,6 +117,14 @@ struct uvc_device {
>> >  	struct usb_function func;
>> >  	struct uvc_video video;
>> >
>> > +	struct uvcg_streaming_header *h;
>> > +
>> > +	struct uvcg_frame **frm;
>> > +	int nframes;
>> > +
>> > +	struct uvcg_format **fmt;
>
>Please spell frames and formats in full.

Right.

>
>> > +	int nformats;
>
>nframes and nformats should be unsigned int.

Right.

>
>> > +
>> >  	/* Descriptors */
>> >  	struct {
>> >  		const struct uvc_descriptor_header * const *fs_control;
>> > @@ -162,4 +169,5 @@ extern void uvc_endpoint_stream(struct uvc_device *dev);
>> >  extern void uvc_function_connect(struct uvc_device *uvc);
>> >  extern void uvc_function_disconnect(struct uvc_device *uvc);
>> >
>> > +extern int uvc_frame_default(struct uvcg_format *ufmt);
>> >  #endif /* _UVC_GADGET_H_ */
>> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> > index 86463bb2639ed..009c80d0e1780 100644
>> > --- a/drivers/usb/gadget/function/uvc_configfs.c
>> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> > @@ -11,6 +11,7 @@
>> >   */
>> >
>> >  #include <linux/sort.h>
>> > +#include <linux/videodev2.h>
>> >
>> >  #include "u_uvc.h"
>> >  #include "uvc_configfs.h"
>> > @@ -1547,6 +1548,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>> >  	h->desc.bCopyProtect		= 0;
>> >
>> >  	h->fmt.type = UVCG_UNCOMPRESSED;
>> > +	h->fmt.fcc = V4L2_PIX_FMT_YUYV;
>> > +	h->fmt.name = "YUV 4:2:2 (YUYV)";
>
>Uncompressed formats are not limited to YUYV, you need to support any
>uncompressed format, or this will be a regression.

Right. These are currently the defaults that will be set when a new
uncompressed entry in the configfs will be created. We will have to make
sure that these can be overwritten by the userspace.

>> >  	config_group_init_type_name(&h->fmt.group, name,
>> >  				    &uvcg_uncompressed_type);
>> >
>> > @@ -1721,6 +1724,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>> >  	h->desc.bCopyProtect		= 0;
>> >
>> >  	h->fmt.type = UVCG_MJPEG;
>> > +	h->fmt.fcc = V4L2_PIX_FMT_MJPEG;
>> > +	h->fmt.name = "MJPEG";
>> >  	config_group_init_type_name(&h->fmt.group, name,
>> >  				    &uvcg_mjpeg_type);
>> >
>> > diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
>> > index f905d29570eb4..8ed966275f838 100644
>> > --- a/drivers/usb/gadget/function/uvc_configfs.h
>> > +++ b/drivers/usb/gadget/function/uvc_configfs.h
>> > @@ -52,6 +52,8 @@ struct uvcg_format {
>> >  	enum uvcg_format_type	type;
>> >  	unsigned		linked;
>> >  	unsigned		num_frames;
>> > +	char			*name;
>> > +	u32			fcc;
>> >  	__u8			bmaControls[UVCG_STREAMING_CONTROL_SIZE];
>> >  };
>> >
>> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> > index 61e2c94cc0b0c..6afc4b79adfe9 100644
>> > --- a/drivers/usb/gadget/function/uvc_queue.c
>> > +++ b/drivers/usb/gadget/function/uvc_queue.c
>> > @@ -20,6 +20,8 @@
>> >  #include <media/videobuf2-vmalloc.h>
>> >
>> >  #include "uvc.h"
>> > +#include "u_uvc.h"
>> > +#include "uvc_configfs.h"
>> >
>> >  /* ------------------------------------------------------------------------
>> >   * Video buffers queue management.
>> > @@ -49,7 +51,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>> >
>> >  	*nplanes = 1;
>> >
>> > -	sizes[0] = video->imagesize;
>> > +	sizes[0] = video->cur_frame->frame.dw_max_video_frame_buffer_size;
>> >
>> >  	return 0;
>> >  }
>> > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>> > index 4ca89eab61590..83830b8864a6e 100644
>> > --- a/drivers/usb/gadget/function/uvc_v4l2.c
>> > +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>> > @@ -24,6 +24,127 @@
>> >  #include "uvc_queue.h"
>> >  #include "uvc_video.h"
>> >  #include "uvc_v4l2.h"
>> > +#include "u_uvc.h"
>> > +#include "uvc_configfs.h"
>> > +
>> > +u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm)
>> > +{
>> > +	struct uvcg_uncompressed *u;
>> > +
>> > +	switch (fmt->type) {
>> > +	case UVCG_UNCOMPRESSED:
>> > +		u = to_uvcg_uncompressed(&fmt->group.cg_item);
>> > +		if (!u)
>> > +			return 0;
>> > +
>> > +		return u->desc.bBitsPerPixel * frm->frame.w_width / 8;
>> > +	case UVCG_MJPEG:
>> > +		return frm->frame.w_width;
>>
>> From the struct v4l2_pix_format documentation:
>> "For compressed formats the bytesperline value makes no sense.
>> Applications and drivers must set this to 0 in that case."
>>
>> Which means that some sizeimage calculations in uvc_v4l2.c would have to
>> be adjusted accordingly.
>>
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
>> > +					      struct uvcg_format *ufmt,
>> > +					      int index)
>>
>> Indentation.
>>
>> > +{
>> > +	int i;
>> > +
>> > +	for (i = 0; i < uvc->nframes; i++) {
>> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
>> > +			continue;
>> > +
>> > +		if (index == uvc->frm[i]->frame.b_frame_index)
>> > +			break;
>> > +	}
>> > +
>> > +	if (i == uvc->nframes)
>> > +		return NULL;
>> > +
>> > +	return uvc->frm[i];
>> > +}
>> > +
>> > +static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
>> > +						    u32 pixelformat)
>>
>> Indentation.
>>
>> > +{
>> > +	int i;
>> > +
>> > +	for (i = 0; i < uvc->nformats; i++)
>> > +		if (uvc->fmt[i]->fcc == pixelformat)
>> > +			break;
>> > +
>> > +	if (i == uvc->nformats)
>> > +		return NULL;
>> > +
>> > +	return uvc->fmt[i];
>> > +}
>> > +
>> > +int uvc_frame_default(struct uvcg_format *ufmt)
>> > +{
>> > +	struct uvcg_uncompressed *m;
>> > +	struct uvcg_uncompressed *u;
>> > +	int ret = 1;
>> > +
>> > +	switch (ufmt->type) {
>> > +	case UVCG_UNCOMPRESSED:
>> > +		u = to_uvcg_uncompressed(&ufmt->group.cg_item);
>> > +		if (u)
>> > +			ret = u->desc.bDefaultFrameIndex;
>> > +		break;
>> > +	case UVCG_MJPEG:
>> > +		m = to_uvcg_uncompressed(&ufmt->group.cg_item);
>> > +		if (m)
>> > +			ret = m->desc.bDefaultFrameIndex;
>> > +		break;
>> > +	}
>> > +
>> > +	if (!ret)
>> > +		ret = 1;
>>
>> Isn't 1 a valid frame index? Should this (and the initialization above)
>> be -1 instead?
>>
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +static struct uvcg_frame *find_frm_by_size(struct uvc_device *uvc,
>> > +					   struct uvcg_format *ufmt,
>> > +					   u16 rw, u16 rh)
>>
>> Also, since you have find_frame_by_index, maybe this should be
>> find_frame_by_size for consistency. Or maybe find_closest_frame_by_size?
>> Up to you.
>>
>> > +{
>> > +	struct uvc_video *video = &uvc->video;
>> > +	struct uvcg_frame *ufrm = NULL;
>> > +	unsigned int d, maxd;
>> > +	int i;
>> > +
>> > +	/* Find the closest image size. The distance between image sizes is
>> > +	 * the size in pixels of the non-overlapping regions between the
>> > +	 * requested size and the frame-specified size.
>> > +	 */
>> > +	maxd = (unsigned int)-1;
>> > +
>> > +	for (i = 0; i < uvc->nframes; i++) {
>> > +		u16 w, h;
>> > +
>> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
>> > +			continue;
>> > +
>> > +		w = uvc->frm[i]->frame.w_width;
>> > +		h = uvc->frm[i]->frame.w_height;
>> > +
>> > +		d = min(w, rw) * min(h, rh);
>> > +		d = w*h + rw*rh - 2*d;
>> > +		if (d < maxd) {
>> > +			maxd = d;
>> > +			ufrm = uvc->frm[i];
>> > +		}
>> > +
>> > +		if (maxd == 0)
>> > +			break;
>> > +	}
>> > +
>> > +	if (ufrm == NULL)
>> > +		uvcg_dbg(&video->uvc->func, "Unsupported size %ux%u\n", rw, rh);
>> > +
>> > +	return ufrm;
>> > +}
>> >
>> >  /* --------------------------------------------------------------------------
>> >   * Requests handling
>> > @@ -50,16 +171,6 @@ uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data)
>> >   * V4L2 ioctls
>> >   */
>> >
>> > -struct uvc_format {
>> > -	u8 bpp;
>> > -	u32 fcc;
>> > -};
>> > -
>> > -static struct uvc_format uvc_formats[] = {
>> > -	{ 16, V4L2_PIX_FMT_YUYV  },
>> > -	{ 0,  V4L2_PIX_FMT_MJPEG },
>> > -};
>> > -
>> >  static int
>> >  uvc_v4l2_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>> >  {
>> > @@ -81,55 +192,187 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt)
>> >  	struct uvc_device *uvc = video_get_drvdata(vdev);
>> >  	struct uvc_video *video = &uvc->video;
>> >
>> > -	fmt->fmt.pix.pixelformat = video->fcc;
>> > -	fmt->fmt.pix.width = video->width;
>> > -	fmt->fmt.pix.height = video->height;
>> > +	fmt->fmt.pix.pixelformat = video->cur_format->fcc;
>> > +	fmt->fmt.pix.width = video->cur_frame->frame.w_width;
>> > +	fmt->fmt.pix.height = video->cur_frame->frame.w_height;
>> > +	fmt->fmt.pix.field = V4L2_FIELD_NONE;
>> > +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format, video->cur_frame);
>> > +	fmt->fmt.pix.sizeimage = video->cur_frame->frame.dw_max_video_frame_buffer_size;
>> > +	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>> > +	fmt->fmt.pix.priv = 0;
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int _uvc_v4l2_try_fmt(struct uvc_video *video,
>> > +	struct v4l2_format *fmt, struct uvcg_format **uvc_format, struct uvcg_frame **uvc_frame)
>>
>> Indentation.
>>
>> > +{
>> > +	struct uvc_device *uvc = video->uvc;
>> > +	struct uvcg_format *ufmt;
>> > +	struct uvcg_frame *ufrm;
>> > +	u8 *fcc;
>> > +	int i;
>> > +
>> > +	if (fmt->type != video->queue.queue.type)
>> > +		return -EINVAL;
>> > +
>> > +	fcc = (u8 *)&fmt->fmt.pix.pixelformat;
>> > +	uvcg_dbg(&uvc->func, "Trying format 0x%08x (%c%c%c%c): %ux%u\n",
>> > +		fmt->fmt.pix.pixelformat,
>> > +		fcc[0], fcc[1], fcc[2], fcc[3],
>> > +		fmt->fmt.pix.width, fmt->fmt.pix.height);
>> > +
>> > +	for (i = 0; i < uvc->nformats; i++)
>> > +		if (uvc->fmt[i]->fcc == fmt->fmt.pix.pixelformat)
>> > +			break;
>> > +
>> > +	if (i == uvc->nformats)
>> > +		ufmt = video->def_format;
>>
>> This means we don't support the requested format. Shouldn't we return
>> -EINVAL at this point?
>>
>> > +
>> > +	ufmt = uvc->fmt[i];
>> > +
>> > +	ufrm = find_frm_by_size(uvc, ufmt,
>> > +				fmt->fmt.pix.width, fmt->fmt.pix.height);
>> > +	if (!ufrm)
>> > +		return -EINVAL;
>> > +
>> > +	fmt->fmt.pix.width = ufrm->frame.w_width;
>> > +	fmt->fmt.pix.height = ufrm->frame.w_height;
>> >  	fmt->fmt.pix.field = V4L2_FIELD_NONE;
>> > -	fmt->fmt.pix.bytesperline = video->bpp * video->width / 8;
>> > -	fmt->fmt.pix.sizeimage = video->imagesize;
>> > +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(ufmt, ufrm);
>>
>> As mentioned earlier, this should be 0 for compressed formats.
>>
>> > +	fmt->fmt.pix.sizeimage = ufrm->frame.dw_max_video_frame_buffer_size;
>> > +	fmt->fmt.pix.pixelformat = ufmt->fcc;
>> >  	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>> >  	fmt->fmt.pix.priv = 0;
>> >
>> > +	if (!fmt->fmt.pix.sizeimage && fmt->fmt.pix.bytesperline)
>> > +		fmt->fmt.pix.sizeimage = fmt->fmt.pix.bytesperline *
>> > +				fmt->fmt.pix.height;
>>
>> Since the configfs configuration is required to configure the driver, I
>> think it should be fine to just take the value from there, especially
>> since bytesperline will be zero for compressed formats anyway.
>>
>> Maybe for uncompressed we could just use what's requested?
>>
>> > +
>> > +	if (uvc_format != NULL)
>> > +		*uvc_format = ufmt;
>> > +	if (uvc_frame != NULL)
>> > +		*uvc_frame = ufrm;
>> > +
>> >  	return 0;
>> >  }
>> >
>> > +static int
>> > +uvc_v4l2_try_fmt(struct file *file, void *fh, struct v4l2_format *fmt)
>> > +{
>> > +	struct video_device *vdev = video_devdata(file);
>> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> > +	struct uvc_video *video = &uvc->video;
>> > +
>> > +	return _uvc_v4l2_try_fmt(video, fmt, NULL, NULL);
>> > +}
>> > +
>> >  static int
>> >  uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>> >  {
>> >  	struct video_device *vdev = video_devdata(file);
>> >  	struct uvc_device *uvc = video_get_drvdata(vdev);
>> >  	struct uvc_video *video = &uvc->video;
>> > -	struct uvc_format *format;
>> > -	unsigned int imagesize;
>> > -	unsigned int bpl;
>> > -	unsigned int i;
>> > -
>> > -	for (i = 0; i < ARRAY_SIZE(uvc_formats); ++i) {
>> > -		format = &uvc_formats[i];
>> > -		if (format->fcc == fmt->fmt.pix.pixelformat)
>> > +	struct uvcg_format *ufmt;
>> > +	struct uvcg_frame *ufrm;
>> > +	int ret;
>> > +
>> > +	ret = _uvc_v4l2_try_fmt(video, fmt, &ufmt, &ufrm);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	video->cur_format = ufmt;
>> > +	video->cur_frame = ufrm;
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +static int
>> > +uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>> > +		struct v4l2_frmivalenum *fival)
>> > +{
>> > +	struct video_device *vdev = video_devdata(file);
>> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> > +	struct uvcg_format *ufmt = NULL;
>> > +	struct uvcg_frame *ufrm = NULL;
>> > +	int i;
>> > +
>> > +	ufmt = find_format_by_pix(uvc, fival->pixel_format);
>> > +	if (!ufmt)
>> > +		return -EINVAL;
>> > +
>> > +	for (i = 0; i < uvc->nframes; ++i) {
>> > +		if (uvc->frm[i]->fmt_type != ufmt->type)
>> > +			continue;
>> > +
>> > +		if (uvc->frm[i]->frame.w_width == fival->width &&
>> > +				uvc->frm[i]->frame.w_height == fival->height) {
>>
>> Indentation.
>>
>> > +			ufrm = uvc->frm[i];
>> >  			break;
>> > +		}
>> >  	}
>> > +	if (!ufrm)
>> > +		return -EINVAL;
>> >
>> > -	if (i == ARRAY_SIZE(uvc_formats)) {
>> > -		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
>> > -			  fmt->fmt.pix.pixelformat);
>> > +	if (fival->index >= ufrm->frame.b_frame_interval_type)
>> >  		return -EINVAL;
>> > -	}
>> >
>> > -	bpl = format->bpp * fmt->fmt.pix.width / 8;
>> > -	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
>> > +	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
>> > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>> > +	fival->discrete.numerator = ufrm->dw_frame_interval[fival->index];
>> > +	fival->discrete.denominator = 10000000;
>> > +	v4l2_simplify_fraction(&fival->discrete.numerator,
>>
>> Where does this function come from?
>>
>> > +		&fival->discrete.denominator, 8, 333);
>> >
>> > -	video->fcc = format->fcc;
>> > -	video->bpp = format->bpp;
>> > -	video->width = fmt->fmt.pix.width;
>> > -	video->height = fmt->fmt.pix.height;
>> > -	video->imagesize = imagesize;
>> > +	return 0;
>> > +}
>> >
>> > -	fmt->fmt.pix.field = V4L2_FIELD_NONE;
>> > -	fmt->fmt.pix.bytesperline = bpl;
>> > -	fmt->fmt.pix.sizeimage = imagesize;
>> > -	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>> > -	fmt->fmt.pix.priv = 0;
>> > +static int
>> > +uvc_v4l2_enum_framesizes(struct file *file, void *fh,
>> > +		struct v4l2_frmsizeenum *fsize)
>> > +{
>> > +	struct video_device *vdev = video_devdata(file);
>> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> > +	struct uvcg_format *ufmt = NULL;
>> > +	struct uvcg_frame *ufrm = NULL;
>> > +
>> > +	ufmt = find_format_by_pix(uvc, fsize->pixel_format);
>> > +	if (!ufmt)
>> > +		return -EINVAL;
>> > +
>> > +	if (fsize->index >= ufmt->num_frames)
>> > +		return -EINVAL;
>> > +
>> > +	ufrm = find_frame_by_index(uvc, ufmt, fsize->index + 1);
>> > +	if (!ufrm)
>> > +		return -EINVAL;
>> > +
>> > +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>> > +	fsize->discrete.width = ufrm->frame.w_width;
>> > +	fsize->discrete.height = ufrm->frame.w_height;
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int
>> > +uvc_v4l2_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>> > +{
>> > +	struct video_device *vdev = video_devdata(file);
>> > +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> > +	struct uvcg_format *ufmt;
>> > +
>> > +	if (f->index >= uvc->nformats)
>> > +		return -EINVAL;
>> > +
>> > +	ufmt = uvc->fmt[f->index];
>> > +	if (!ufmt)
>> > +		return -EINVAL;
>> > +
>> > +	f->pixelformat = ufmt->fcc;
>> > +	f->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>
>> This shouldn't be set if the format isn't compressed.
>>
>> > +
>> > +	strscpy(f->description, ufmt->name, sizeof(f->description));
>> > +	f->description[sizeof(f->description) - 1] = 0;
>>
>> If sizeof(ufmt->name) < sizeof(f->description), then the string won't be
>> properly null-terminated.
>>
>> >
>> >  	return 0;
>> >  }
>> > @@ -258,8 +501,12 @@ uvc_v4l2_ioctl_default(struct file *file, void *fh, bool valid_prio,
>> >
>> >  const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>> >  	.vidioc_querycap = uvc_v4l2_querycap,
>> > +	.vidioc_try_fmt_vid_out = uvc_v4l2_try_fmt,
>> >  	.vidioc_g_fmt_vid_out = uvc_v4l2_get_format,
>> >  	.vidioc_s_fmt_vid_out = uvc_v4l2_set_format,
>> > +	.vidioc_enum_frameintervals = uvc_v4l2_enum_frameintervals,
>> > +	.vidioc_enum_framesizes = uvc_v4l2_enum_framesizes,
>> > +	.vidioc_enum_fmt_vid_out = uvc_v4l2_enum_fmt,
>
>What's the point of implementing these functions, when userspace will
>have created the descriptors in configfs, and thus know what the
>supported formats, sizes and frame intervals are ?

The application that is creating the configfs entries does not necessary
have to be the application that serves the uvc gadget. Look at different
composite gadgets that serve different functions over the interface.

To separate the tasks of creating the gadget and serving the gadget the
latter can use the already available v4l2 api that many applications
like gstreamer already understand.

>To be honest, I'm not thrilled by this series. Unless there's a very
>good reason to handle all this on the kernel side, I think it just
>increases the complexity of the kernel code without much benefit.

It does not really increase complexity.

Just look into

https://git.ideasonboard.org/uvc-gadget.git/blob/HEAD:/lib/configfs.c

which implements its own configfs parser for the userspace, just to be
flexible enough to handle the different formats that some other
userspace tool configured. The idea is to get rid of thus extras and
just use the v4l2 api on the v4l2 dev.

While being here; I see that the s_fmt_vid_out callback can actually be
removed. This userspace callback is not very usefull for an uvc v4l2
device thats format should only be set by the host that it is connected to.
When you look into my implementation, this callback is even competing with
the host api and currently needs to be locked. This obviously makes no
sense.

>> >  	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>> >  	.vidioc_querybuf = uvc_v4l2_querybuf,
>> >  	.vidioc_qbuf = uvc_v4l2_qbuf,
>> > diff --git a/drivers/usb/gadget/function/uvc_v4l2.h b/drivers/usb/gadget/function/uvc_v4l2.h
>> > index 1576005b61fd3..6e45103bbf793 100644
>> > --- a/drivers/usb/gadget/function/uvc_v4l2.h
>> > +++ b/drivers/usb/gadget/function/uvc_v4l2.h
>> > @@ -15,5 +15,6 @@
>> >
>> >  extern const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops;
>> >  extern const struct v4l2_file_operations uvc_v4l2_fops;
>> > +extern u32 uvc_v4l2_get_bytesperline(struct uvcg_format *fmt, struct uvcg_frame *frm);
>>
>> Do you need this here? Nobody outside of uvc_v4l2.c uses it.
>>
>> >
>> >  #endif /* __UVC_V4L2_H__ */
>> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> > index 633e23d58d868..b14780bddd838 100644
>> > --- a/drivers/usb/gadget/function/uvc_video.c
>> > +++ b/drivers/usb/gadget/function/uvc_video.c
>> > @@ -17,7 +17,10 @@
>> >
>> >  #include "uvc.h"
>> >  #include "uvc_queue.h"
>> > +#include "uvc_v4l2.h"
>> >  #include "uvc_video.h"
>> > +#include "u_uvc.h"
>> > +#include "uvc_configfs.h"
>> >
>> >  /* --------------------------------------------------------------------------
>> >   * Video codecs
>> > @@ -348,11 +351,8 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>> >  	INIT_WORK(&video->pump, uvcg_video_pump);
>> >
>> >  	video->uvc = uvc;
>> > -	video->fcc = V4L2_PIX_FMT_YUYV;
>> > -	video->bpp = 16;
>> > -	video->width = 320;
>> > -	video->height = 240;
>> > -	video->imagesize = 320 * 240 * 2;
>> > +	video->def_format = video->cur_format = uvc->fmt[0];
>
>There's nothing in the UVC specification that requires the first format
>to be the default. It's entirely conceivable that a device could select
>the default dynamically.

In the UVC spec there is bDefaultFrameIndex.

But I did not find any bDefaultFormatIndex. So I thought just
take the first in the configfs pool.

The host will call GET_CUR to get the current format before deciding
which format and frame to choose. So I think in the beginning we can
take over the decision. As long as we also offer the formats that is
selected by default there should be no problem.

>
>> > +	video->cur_frame = uvc->frm[uvc_frame_default(video->def_format) - 1];
>>
>> I think we'll need something here to validate the configfs, to make sure
>> it's filled corretly. I anticipate that uvc->fmt[0] might cause problems
>> if no format is set in configfs.
>>
>> Or maybe such protection is already around and I'm not aware of it.
>>
>> >  	/* Initialize the video buffers queue. */
>> >  	uvcg_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT,
>


Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]