mbox series

[v13,0/6] usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS

Message ID 20220908194750.3750310-1-m.grzeschik@pengutronix.de
Headers show
Series usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS | expand

Message

Michael Grzeschik Sept. 8, 2022, 7:47 p.m. UTC
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.

The uvc userspace stack is also under development. One example is an generic
v4l2uvcsink gstreamer elemnt, which is currently under discussion. [1]

[1] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1304

With the libusbgx library [1] used by the gadget-tool [2] it is now also
possible to fully describe the configfs layout of the uvc gadget with scheme
files.

[2] https://github.com/linux-usb-gadgets/libusbgx/pull/61/commits/53231c76f9d512f59fdc23b65cd5c46b7fb09eb4

[3] https://github.com/linux-usb-gadgets/gt/tree/master/examples/systemd

The bigger picture of these patches is to provide a more versatile interface to
the uvc gadget. The goal is to simply start a uvc-gadget with the following
commands:

$ gt load uvc.scheme
$ gst-launch v4l2src ! v4l2uvcsink

--

v1: https://lore.kernel.org/linux-usb/20210530222239.8793-1-m.grzeschik@pengutronix.de/
v2: https://lore.kernel.org/linux-usb/20211117004432.3763306-1-m.grzeschik@pengutronix.de/
v3: https://lore.kernel.org/linux-usb/20211117122435.2409362-1-m.grzeschik@pengutronix.de/
v4: https://lore.kernel.org/linux-usb/20211205225803.268492-1-m.grzeschik@pengutronix.de/
v5: https://lore.kernel.org/linux-usb/20211209084322.2662616-1-m.grzeschik@pengutronix.de/
v6: https://lore.kernel.org/linux-usb/20220105115527.3592860-1-m.grzeschik@pengutronix.de/
v7: https://lore.kernel.org/linux-usb/20220608105748.139922-1-m.grzeschik@pengutronix.de/
v8: https://lore.kernel.org/linux-usb/20220907140254.2378109-1-m.grzeschik@pengutronix.de/
v9: https://lore.kernel.org/linux-usb/20220907150457.2572474-1-m.grzeschik@pengutronix.de/
v10: https://lore.kernel.org/linux-usb/20220907152101.2582112-1-m.grzeschik@pengutronix.de/
v11: https://lore.kernel.org/linux-usb/20220907152354.2583258-1-m.grzeschik@pengutronix.de/
v12: https://lore.kernel.org/linux-usb/20220908160245.3601176-1-m.grzeschik@pengutronix.de/

Regards,
Michael

Michael Grzeschik (6):
  media: v4l: move helper functions for fractions from uvc to
    v4l2-common
  media: uvcvideo: move uvc_format_desc to common header
  usb: gadget: uvc: add v4l2 enumeration api calls
  usb: gadget: uvc: add v4l2 try_format api call
  usb: gadget: uvc: add VIDIOC hostside config feedback
  usb: gadget: uvc: add format/frame handling code

 drivers/media/usb/uvc/uvc_ctrl.c        |   1 +
 drivers/media/usb/uvc/uvc_driver.c      | 290 +---------------
 drivers/media/usb/uvc/uvc_v4l2.c        |  14 +-
 drivers/media/usb/uvc/uvcvideo.h        | 147 --------
 drivers/media/v4l2-core/v4l2-common.c   |  86 +++++
 drivers/usb/gadget/function/f_uvc.c     | 273 ++++++++++++++-
 drivers/usb/gadget/function/uvc.h       |  41 ++-
 drivers/usb/gadget/function/uvc_queue.c |   2 +-
 drivers/usb/gadget/function/uvc_v4l2.c  | 423 +++++++++++++++++++++---
 drivers/usb/gadget/function/uvc_video.c |  71 +++-
 include/media/v4l2-common.h             |   4 +
 include/media/v4l2-uvc.h                | 359 ++++++++++++++++++++
 12 files changed, 1208 insertions(+), 503 deletions(-)
 create mode 100644 include/media/v4l2-uvc.h

Comments

Daniel Scally Sept. 9, 2022, 11 a.m. UTC | #1
Hello Michael - thanks for the set

On 08/09/2022 20:47, Michael Grzeschik wrote:
> This patch extends the v4l2 VIDIOCs for enum_format, enum_framesizes,
> enum_frameintervals and set_fmt. The active host side configuration is
> reported in these v4l2 interface functions. So the enum functions will
> on set configuration return the currently set format/frmae/interval.
> The set_format function has changed to be an noop since there is
> no point for the kernel to set the currently decided format.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>


I actually get problems in uvc-gadget with this patch applied, when I 
call v4l2-ctl --stream-mmap on the host side then on the gadget I get:


/dev/video8: unable to queue buffer index 1/4 (22)
[  336.655157] vb2-v4l2: [00000000ba5171ac] vb2_fill_vb2_v4l2_buffer: 
plane parameters verification failed: -22


And the reason for that verification failure is a mismatch in 
__verify_length() between length which is 460800 and b->bytesused which 
is 1843200. I set the format with v4l2-ctl --set-fmt-video 
width=1280,height=720,pixelformat=YUYV so I'm expecting a size of 
1843200. I'm setting the dwMaxVideoFrameBufferSize for that format to 
1843200, so that setting doesn't seem to be reflected everywhere it 
should be.

>
> ---
> v1 -> v2:
>     - fixed indentation of find_frame/format_by_index
>     - fixed function name find_frm_by_size to find_frame_by_size
>     - fixed indentation of _uvc_v4l2_try_fmt
>     - fixed indentation in uvc_v4l2_enum_frameintervals
>     - removed unneeded declaration of uvc_v4l2_get_bytesperline in uvc_v4l2.h
>     - checked return values on config_group_find_item, handling refcount
>     - fixed sizeof using variables instead of types
>     - removed unsused def_format variable
>     - wrting grp, hdr, fmt and frm in full
>     - added proper ival handling
>     - removed analyze_configfs function
>     - added linked list of frames to uvcg_format
>     - added functon find_frame_by_index
> v2 -> v3:
>     - fixed usage of u_uvc.h
>     - removed unused variable i in _try_fmt
>     - made uvc_v4l2_get_bytesperline static
> v3 -> v4:
>     - conditionally return current or all frames/formats/frameintervals on enum
>     - dropped setting format and frame with set_format
>     - combined try and set format function to one call
> v4 -> v5:
>     - fixed uninitialized return values reported by kernel test robot
>     - added local video variable to uvc_v4l2_enum_frameintervals
> v5 -> v6:
>     -
> v6 -> v7:
>     - fixed unlocking in f_uvc uvc_alloc function
>     - add uvc_get_frame_size function for sizeimage calculation
>     - add fallback to frame.dw_max_video_frame_buffer_size
> v7 -> v12:
>     - moved the enum callbacks to a separate patch
>     - rephrased the commit message
> v7 -> v13:
>     - moved the try_format callback to a separate patch
>
>   drivers/usb/gadget/function/f_uvc.c     |   4 +
>   drivers/usb/gadget/function/uvc.h       |  20 +++-
>   drivers/usb/gadget/function/uvc_queue.c |   2 +-
>   drivers/usb/gadget/function/uvc_v4l2.c  | 117 ++++++++++++------------
>   drivers/usb/gadget/function/uvc_video.c |  61 +++++++++++-
>   5 files changed, 132 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 7c416170b499e0..6f1138e819bdbf 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -327,6 +327,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>   		if (uvc->video.ep)
>   			usb_ep_disable(uvc->video.ep);
>   
> +		uvc->streamon = 0;
> +
>   		memset(&v4l2_event, 0, sizeof(v4l2_event));
>   		v4l2_event.type = UVC_EVENT_STREAMOFF;
>   		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> @@ -350,6 +352,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>   			return ret;
>   		usb_ep_enable(uvc->video.ep);
>   
> +		uvc->streamon = 1;
> +
>   		memset(&v4l2_event, 0, sizeof(v4l2_event));
>   		v4l2_event.type = UVC_EVENT_STREAMON;
>   		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 641cf2e7afaf6e..d32f34a7dbc423 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -90,11 +90,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 *cur_format;
> +	struct uvcg_frame *cur_frame;
> +	unsigned int cur_ival;
> +
>   	struct mutex mutex;	/* protects frame parameters */
>   
>   	unsigned int uvc_num_requests;
> @@ -144,6 +143,8 @@ struct uvc_device {
>   		const struct uvc_descriptor_header * const *ss_streaming;
>   	} desc;
>   
> +	bool streamon;
> +
>   	unsigned int control_intf;
>   	struct usb_ep *control_ep;
>   	struct usb_request *control_req;
> @@ -180,4 +181,13 @@ 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_get_frame_size(struct uvcg_format *uformat,
> +		       struct uvcg_frame *uframe);
> +
> +extern struct uvcg_format *find_format_by_index(struct uvc_device *uvc,
> +						int index);
> +extern struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
> +					      struct uvcg_format *uformat,
> +					      int index);
> +
>   #endif /* _UVC_GADGET_H_ */
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ec500ee499eed1..3353d0566a59d5 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -52,7 +52,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>   
>   	*nplanes = 1;
>   
> -	sizes[0] = video->imagesize;
> +	sizes[0] = uvc_get_frame_size(video->cur_format, video->cur_frame);
>   
>   	req_size = video->ep->maxpacket
>   		 * max_t(unsigned int, video->ep->maxburst, 1)
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 761bc833d1e54f..bc5e2076c6b7e4 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -199,16 +199,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)
>   {
> @@ -229,56 +219,34 @@ uvc_v4l2_get_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_desc *fmtdesc;
>   
> -	fmt->fmt.pix.pixelformat = video->fcc;
> -	fmt->fmt.pix.width = video->width;
> -	fmt->fmt.pix.height = video->height;
> +	fmtdesc = to_uvc_format(video->cur_format);
> +
> +	fmt->fmt.pix.pixelformat = fmtdesc->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 = video->bpp * video->width / 8;
> -	fmt->fmt.pix.sizeimage = video->imagesize;
> +	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format,
> +							      video->cur_frame);
> +	fmt->fmt.pix.sizeimage = uvc_get_frame_size(video->cur_format,
> +						    video->cur_frame);
>   	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>   	fmt->fmt.pix.priv = 0;
>   
>   	return 0;
>   }
>   
> +/* set_format is only allowed by the host side, so this is a noop */
>   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)
> -			break;
> -	}
>   
> -	if (i == ARRAY_SIZE(uvc_formats)) {
> -		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
> -			  fmt->fmt.pix.pixelformat);
> +	if (fmt->type != video->queue.queue.type)
>   		return -EINVAL;
> -	}
> -
> -	bpl = format->bpp * fmt->fmt.pix.width / 8;
> -	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
> -
> -	video->fcc = format->fcc;
> -	video->bpp = format->bpp;
> -	video->width = fmt->fmt.pix.width;
> -	video->height = fmt->fmt.pix.height;
> -	video->imagesize = imagesize;
> -
> -	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;
>   
>   	return 0;
>   }
> @@ -329,6 +297,7 @@ uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>   {
>   	struct video_device *vdev = video_devdata(file);
>   	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
>   	struct uvcg_format *uformat = NULL;
>   	struct uvcg_frame *uframe = NULL;
>   	struct uvcg_frame_ptr *frame;
> @@ -347,11 +316,19 @@ uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>   	if (!uframe)
>   		return -EINVAL;
>   
> -	if (fival->index >= uframe->frame.b_frame_interval_type)
> -		return -EINVAL;
> +	if (uvc->streamon) {
> +		if (fival->index >= 1)
> +			return -EINVAL;
>   
> -	fival->discrete.numerator =
> -		uframe->dw_frame_interval[fival->index];
> +		fival->discrete.numerator =
> +			uframe->dw_frame_interval[video->cur_ival - 1];
> +	} else {
> +		if (fival->index >= uframe->frame.b_frame_interval_type)
> +			return -EINVAL;
> +
> +		fival->discrete.numerator =
> +			uframe->dw_frame_interval[fival->index];
> +	}
>   
>   	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
>   	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> @@ -368,19 +345,28 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh,
>   {
>   	struct video_device *vdev = video_devdata(file);
>   	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
>   	struct uvcg_format *uformat = NULL;
>   	struct uvcg_frame *uframe = NULL;
>   
> -	uformat = find_format_by_pix(uvc, fsize->pixel_format);
> -	if (!uformat)
> -		return -EINVAL;
> +	if (uvc->streamon) {
> +		if (fsize->index >= 1)
> +			return -EINVAL;
>   
> -	if (fsize->index >= uformat->num_frames)
> -		return -EINVAL;
> +		uformat = video->cur_format;
> +		uframe = video->cur_frame;
> +	} else {
> +		uformat = find_format_by_pix(uvc, fsize->pixel_format);
> +		if (!uformat)
> +			return -EINVAL;
>   
> -	uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
> -	if (!uframe)
> -		return -EINVAL;
> +		if (fsize->index >= uformat->num_frames)
> +			return -EINVAL;
> +
> +		uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
> +		if (!uframe)
> +			return -EINVAL;
> +	}
>   
>   	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>   	fsize->discrete.width = uframe->frame.w_width;
> @@ -394,15 +380,24 @@ uvc_v4l2_enum_format(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 uvc_video *video = &uvc->video;
>   	struct uvc_format_desc *fmtdesc;
>   	struct uvcg_format *uformat;
>   
> -	if (f->index >= uvc->header->num_fmt)
> -		return -EINVAL;
> +	if (uvc->streamon) {
> +		if (f->index >= 1)
> +			return -EINVAL;
>   
> -	uformat = find_format_by_index(uvc, f->index + 1);
> -	if (!uformat)
> -		return -EINVAL;
> +		uformat = video->cur_format;
> +	} else {
> +		if (f->index >= uvc->header->num_fmt)
> +			return -EINVAL;
> +
> +		uformat = find_format_by_index(uvc, f->index + 1);
> +		if (!uformat)
> +			return -EINVAL;
> +
> +	}
>   
>   	if (uformat->type != UVCG_UNCOMPRESSED)
>   		f->flags |= V4L2_FMT_FLAG_COMPRESSED;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index c00ce0e91f5d5c..37867c93073418 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -19,6 +19,7 @@
>   #include "uvc.h"
>   #include "uvc_queue.h"
>   #include "uvc_video.h"
> +#include "uvc_configfs.h"
>   
>   /* --------------------------------------------------------------------------
>    * Video codecs
> @@ -490,21 +491,71 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>   	return ret;
>   }
>   
> +static int uvc_frame_default(struct uvcg_format *uformat)
> +{
> +	struct uvcg_uncompressed *u;
> +	struct uvcg_mjpeg *m;
> +
> +	switch (uformat->type) {
> +	case UVCG_UNCOMPRESSED:
> +		u = to_uvcg_uncompressed(&uformat->group.cg_item);
> +		if (u)
> +			return u->desc.bDefaultFrameIndex;
> +		break;
> +	case UVCG_MJPEG:
> +		m = to_uvcg_mjpeg(&uformat->group.cg_item);
> +		if (m)
> +			return m->desc.bDefaultFrameIndex;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int uvc_default_frame_interval(struct uvc_video *video)
> +{
> +	int i;
> +
> +	for (i = 0; i < video->cur_frame->frame.b_frame_interval_type; i++) {
> +		if (video->cur_frame->frame.dw_default_frame_interval ==
> +			video->cur_frame->dw_frame_interval[i]) {
> +			video->cur_ival = i + 1;
> +			return i + 1;
> +		}
> +	}
> +
> +	/* fallback */
> +	return 1;
> +}
> +
>   /*
>    * Initialize the UVC video stream.
>    */
>   int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>   {
> +	int iframe;
> +
>   	INIT_LIST_HEAD(&video->req_free);
>   	spin_lock_init(&video->req_lock);
>   	INIT_WORK(&video->pump, uvcg_video_pump);
>   
> +	if (list_empty(&uvc->header->formats))
> +		return -EINVAL;
> +
>   	video->uvc = uvc;
> -	video->fcc = V4L2_PIX_FMT_YUYV;
> -	video->bpp = 16;
> -	video->width = 320;
> -	video->height = 240;
> -	video->imagesize = 320 * 240 * 2;
> +	video->cur_format = find_format_by_index(uvc, 1);
> +	if (!video->cur_format)
> +		return -EINVAL;
> +
> +	iframe = uvc_frame_default(video->cur_format);
> +	if (!iframe)
> +		return -EINVAL;
> +
> +	video->cur_frame = find_frame_by_index(uvc, video->cur_format, iframe);
> +	if (!video->cur_frame)
> +		return -EINVAL;
> +
> +	video->cur_ival = uvc_default_frame_interval(video);
>   
>   	/* Initialize the video buffers queue. */
>   	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,
Daniel Scally Sept. 9, 2022, 12:56 p.m. UTC | #2
Hi Michael - thanks for the patch

On 08/09/2022 20:47, Michael Grzeschik wrote:
> This patch adds support to the v4l2 VIDIOCs for enum_format,
> enum_framesizes and enum_frameintervals. This way, the userspace
> application can use these VIDIOCS to query the via configfs exported
> frame capabilities. With thes callbacks the userspace doesn't have to
> bring its own configfs parser.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
> ---
> v1 -> v13:
>     - refactored the enum_ callbacks to this separate new patch
>     - renamed +uvc_v4l2_enum_fmt to uvc_v4l2_enum_format
>     - improved coding style
>     - removed unused leftover variable uvc_video in enum functions
>
>   drivers/usb/gadget/function/f_uvc.c    |  32 +++++
>   drivers/usb/gadget/function/uvc.h      |   2 +
>   drivers/usb/gadget/function/uvc_v4l2.c | 176 +++++++++++++++++++++++++
>   3 files changed, 210 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index f4f6cf75930beb..7c416170b499e0 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -888,6 +888,7 @@ static void uvc_free(struct usb_function *f)
>   	struct uvc_device *uvc = to_uvc(f);
>   	struct f_uvc_opts *opts = container_of(f->fi, struct f_uvc_opts,
>   					       func_inst);
> +	config_item_put(&uvc->header->item);
>   	--opts->refcnt;
>   	kfree(uvc);
>   }
> @@ -941,6 +942,7 @@ 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 *streaming, *header, *h;
>   
>   	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
>   	if (uvc == NULL)
> @@ -973,6 +975,36 @@ 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;
> +
> +	streaming = config_group_find_item(&opts->func_inst.group, "streaming");
> +	if (!streaming) {
> +		config_item_put(streaming);


This shouldn't be necessary as it's a no-op if streaming is null

> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(-ENOMEM);


Why ENOMEM? I wouldn't expect that error here...I think I'd expect 
ENOENT. You also aren't freeing uvc here so that memory would be lost.


> +	}
> +
> +	header = config_group_find_item(to_config_group(streaming), "header");
> +	config_item_put(streaming);
> +	if (!header) {
> +		config_item_put(header);
> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	h = config_group_find_item(to_config_group(header), "h");
> +	config_item_put(header);
> +	if (!h) {
> +		config_item_put(h);
> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(-ENOMEM);
> +	}


Similar comments for these two error paths. Given the similarity of 
those sections you could have something like;


     streaming = config_group_find_item(&opts->func_inst.group, 
"streaming");
     if (!streaming)
         goto err_config;

     ... rest of the function ...

     return &uvc->func;

err_config:
     mutex_unlock(&opts->lock);
     kfree(uvc);
     return -ENOENT; // or whatever is appropriate
}

> +
> +	uvc->header = to_uvcg_streaming_header(h);
> +	if (!uvc->header->linked) {
> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(-EBUSY);


This path on the other hand should have config_item_put(h) I think, and 
would also need to kfree(uvc).


> +	}
> +
>   	++opts->refcnt;
>   	mutex_unlock(&opts->lock);
>   
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 58e383afdd4406..641cf2e7afaf6e 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -133,6 +133,8 @@ struct uvc_device {
>   	bool func_connected;
>   	wait_queue_head_t func_connected_queue;
>   
> +	struct uvcg_streaming_header *header;
> +
>   	/* Descriptors */
>   	struct {
>   		const struct uvc_descriptor_header * const *fs_control;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 511f106f984375..63cb5a40306c75 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -18,12 +18,92 @@
>   #include <media/v4l2-dev.h>
>   #include <media/v4l2-event.h>
>   #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-uvc.h>
>   
>   #include "f_uvc.h"
>   #include "uvc.h"
>   #include "uvc_queue.h"
>   #include "uvc_video.h"
>   #include "uvc_v4l2.h"
> +#include "uvc_configfs.h"
> +
> +static struct uvc_format_desc *to_uvc_format(struct uvcg_format *uformat)
> +{
> +	char guid[16] = UVC_GUID_FORMAT_MJPEG;
> +	struct uvc_format_desc *format;
> +	struct uvcg_uncompressed *unc;
> +
> +	if (uformat->type == UVCG_UNCOMPRESSED) {
> +		unc = to_uvcg_uncompressed(&uformat->group.cg_item);
> +		if (!unc)
> +			return ERR_PTR(-EINVAL);
> +
> +		memcpy(guid, unc->desc.guidFormat, sizeof(guid));
> +	}
> +
> +	format = uvc_format_by_guid(guid);
> +	if (!format)
> +		return ERR_PTR(-EINVAL);
> +
> +	return format;
> +}
> +
> +struct uvcg_format *find_format_by_index(struct uvc_device *uvc, int index)
> +{
> +	struct uvcg_format_ptr *format;
> +	struct uvcg_format *uformat = NULL;
> +	int i = 1;
> +
> +	list_for_each_entry(format, &uvc->header->formats, entry) {
> +		if (index == i) {
> +			uformat = format->fmt;
> +			break;
> +		}
> +		i++;
> +	}
> +
> +	return uformat;
> +}
> +
> +struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
> +				       struct uvcg_format *uformat,
> +				       int index)
> +{
> +	struct uvcg_format_ptr *format;
> +	struct uvcg_frame_ptr *frame;
> +	struct uvcg_frame *uframe = NULL;
> +
> +	list_for_each_entry(format, &uvc->header->formats, entry) {
> +		if (format->fmt->type != uformat->type)
> +			continue;
> +		list_for_each_entry(frame, &format->fmt->frames, entry) {
> +			if (index == frame->frm->frame.b_frame_index) {
> +				uframe = frame->frm;
> +				break;
> +			}
> +		}
> +	}
> +
> +	return uframe;
> +}
> +
> +static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
> +					      u32 pixelformat)
> +{
> +	struct uvcg_format_ptr *format;
> +	struct uvcg_format *uformat = NULL;
> +
> +	list_for_each_entry(format, &uvc->header->formats, entry) {
> +		struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt);
> +
> +		if (fmtdesc->fcc == pixelformat) {
> +			uformat = format->fmt;
> +			break;
> +		}
> +	}
> +
> +	return uformat;
> +}
>   
>   /* --------------------------------------------------------------------------
>    * Requests handling
> @@ -134,6 +214,99 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>   	return 0;
>   }
>   
> +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 *uformat = NULL;
> +	struct uvcg_frame *uframe = NULL;
> +	struct uvcg_frame_ptr *frame;
> +
> +	uformat = find_format_by_pix(uvc, fival->pixel_format);
> +	if (!uformat)
> +		return -EINVAL;
> +
> +	list_for_each_entry(frame, &uformat->frames, entry) {
> +		if (frame->frm->frame.w_width == fival->width &&
> +		    frame->frm->frame.w_height == fival->height) {
> +			uframe = frame->frm;
> +			break;
> +		}
> +	}
> +	if (!uframe)
> +		return -EINVAL;
> +
> +	if (fival->index >= uframe->frame.b_frame_interval_type)
> +		return -EINVAL;
> +
> +	fival->discrete.numerator =
> +		uframe->dw_frame_interval[fival->index];
> +
> +	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +	fival->discrete.denominator = 10000000;
> +	v4l2_simplify_fraction(&fival->discrete.numerator,
> +		&fival->discrete.denominator, 8, 333);
> +
> +	return 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 *uformat = NULL;
> +	struct uvcg_frame *uframe = NULL;
> +
> +	uformat = find_format_by_pix(uvc, fsize->pixel_format);
> +	if (!uformat)
> +		return -EINVAL;
> +
> +	if (fsize->index >= uformat->num_frames)
> +		return -EINVAL;
> +
> +	uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
> +	if (!uframe)
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> +	fsize->discrete.width = uframe->frame.w_width;
> +	fsize->discrete.height = uframe->frame.w_height;
> +
> +	return 0;
> +}
> +
> +static int
> +uvc_v4l2_enum_format(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 uvc_format_desc *fmtdesc;
> +	struct uvcg_format *uformat;
> +
> +	if (f->index >= uvc->header->num_fmt)
> +		return -EINVAL;
> +
> +	uformat = find_format_by_index(uvc, f->index + 1);
> +	if (!uformat)
> +		return -EINVAL;
> +
> +	if (uformat->type != UVCG_UNCOMPRESSED)
> +		f->flags |= V4L2_FMT_FLAG_COMPRESSED;
> +
> +	fmtdesc = to_uvc_format(uformat);
> +	f->pixelformat = fmtdesc->fcc;
> +
> +	strscpy(f->description, fmtdesc->name, sizeof(f->description));
> +	f->description[strlen(fmtdesc->name) - 1] = 0;
> +
> +	return 0;
> +}
> +
>   static int
>   uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>   {
> @@ -300,6 +473,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>   	.vidioc_querycap = uvc_v4l2_querycap,
>   	.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_format,
>   	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>   	.vidioc_querybuf = uvc_v4l2_querybuf,
>   	.vidioc_qbuf = uvc_v4l2_qbuf,
Daniel Scally Sept. 9, 2022, 12:57 p.m. UTC | #3
Hi Michael - thanks for the patch

On 08/09/2022 20:47, Michael Grzeschik wrote:
> The functions uvc_simplify_fraction and uvc_fraction_to_interval are
> generic helpers which are also useful for other v4l2 drivers. This patch
> moves them to v4l2-common.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>


This and #2 make sense to me, so for these two:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

>
> ---
> v1 -> v7: -
> v7 -> v8: - ported all style fixes and broken links from latest version on rebase
> v8 -> v13: -
>
>   drivers/media/usb/uvc/uvc_driver.c    | 84 --------------------------
>   drivers/media/usb/uvc/uvc_v4l2.c      | 14 ++---
>   drivers/media/usb/uvc/uvcvideo.h      |  3 -
>   drivers/media/v4l2-core/v4l2-common.c | 86 +++++++++++++++++++++++++++
>   include/media/v4l2-common.h           |  4 ++
>   5 files changed, 97 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9c05776f11d1f0..0f14dee4b6d794 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -329,90 +329,6 @@ static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
>   	return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
>   }
>   
> -/*
> - * Simplify a fraction using a simple continued fraction decomposition. The
> - * idea here is to convert fractions such as 333333/10000000 to 1/30 using
> - * 32 bit arithmetic only. The algorithm is not perfect and relies upon two
> - * arbitrary parameters to remove non-significative terms from the simple
> - * continued fraction decomposition. Using 8 and 333 for n_terms and threshold
> - * respectively seems to give nice results.
> - */
> -void uvc_simplify_fraction(u32 *numerator, u32 *denominator,
> -		unsigned int n_terms, unsigned int threshold)
> -{
> -	u32 *an;
> -	u32 x, y, r;
> -	unsigned int i, n;
> -
> -	an = kmalloc_array(n_terms, sizeof(*an), GFP_KERNEL);
> -	if (an == NULL)
> -		return;
> -
> -	/*
> -	 * Convert the fraction to a simple continued fraction. See
> -	 * https://en.wikipedia.org/wiki/Continued_fraction
> -	 * Stop if the current term is bigger than or equal to the given
> -	 * threshold.
> -	 */
> -	x = *numerator;
> -	y = *denominator;
> -
> -	for (n = 0; n < n_terms && y != 0; ++n) {
> -		an[n] = x / y;
> -		if (an[n] >= threshold) {
> -			if (n < 2)
> -				n++;
> -			break;
> -		}
> -
> -		r = x - an[n] * y;
> -		x = y;
> -		y = r;
> -	}
> -
> -	/* Expand the simple continued fraction back to an integer fraction. */
> -	x = 0;
> -	y = 1;
> -
> -	for (i = n; i > 0; --i) {
> -		r = y;
> -		y = an[i-1] * y + x;
> -		x = r;
> -	}
> -
> -	*numerator = y;
> -	*denominator = x;
> -	kfree(an);
> -}
> -
> -/*
> - * Convert a fraction to a frame interval in 100ns multiples. The idea here is
> - * to compute numerator / denominator * 10000000 using 32 bit fixed point
> - * arithmetic only.
> - */
> -u32 uvc_fraction_to_interval(u32 numerator, u32 denominator)
> -{
> -	u32 multiplier;
> -
> -	/* Saturate the result if the operation would overflow. */
> -	if (denominator == 0 ||
> -	    numerator/denominator >= ((u32)-1)/10000000)
> -		return (u32)-1;
> -
> -	/*
> -	 * Divide both the denominator and the multiplier by two until
> -	 * numerator * multiplier doesn't overflow. If anyone knows a better
> -	 * algorithm please let me know.
> -	 */
> -	multiplier = 10000000;
> -	while (numerator > ((u32)-1)/multiplier) {
> -		multiplier /= 2;
> -		denominator /= 2;
> -	}
> -
> -	return denominator ? numerator * multiplier / denominator : 0;
> -}
> -
>   /* ------------------------------------------------------------------------
>    * Terminal and unit management
>    */
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 4cc3fa6b8c9812..f4d4c33b6dfbd7 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -386,7 +386,7 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
>   	mutex_unlock(&stream->mutex);
>   
>   	denominator = 10000000;
> -	uvc_simplify_fraction(&numerator, &denominator, 8, 333);
> +	v4l2_simplify_fraction(&numerator, &denominator, 8, 333);
>   
>   	memset(parm, 0, sizeof(*parm));
>   	parm->type = stream->type;
> @@ -427,7 +427,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
>   	else
>   		timeperframe = parm->parm.output.timeperframe;
>   
> -	interval = uvc_fraction_to_interval(timeperframe.numerator,
> +	interval = v4l2_fraction_to_interval(timeperframe.numerator,
>   		timeperframe.denominator);
>   	uvc_dbg(stream->dev, FORMAT, "Setting frame interval to %u/%u (%u)\n",
>   		timeperframe.numerator, timeperframe.denominator, interval);
> @@ -481,7 +481,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
>   	/* Return the actual frame period. */
>   	timeperframe.numerator = probe.dwFrameInterval;
>   	timeperframe.denominator = 10000000;
> -	uvc_simplify_fraction(&timeperframe.numerator,
> +	v4l2_simplify_fraction(&timeperframe.numerator,
>   		&timeperframe.denominator, 8, 333);
>   
>   	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> @@ -1275,7 +1275,7 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
>   		fival->discrete.numerator =
>   			frame->dwFrameInterval[index];
>   		fival->discrete.denominator = 10000000;
> -		uvc_simplify_fraction(&fival->discrete.numerator,
> +		v4l2_simplify_fraction(&fival->discrete.numerator,
>   			&fival->discrete.denominator, 8, 333);
>   	} else {
>   		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> @@ -1285,11 +1285,11 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
>   		fival->stepwise.max.denominator = 10000000;
>   		fival->stepwise.step.numerator = frame->dwFrameInterval[2];
>   		fival->stepwise.step.denominator = 10000000;
> -		uvc_simplify_fraction(&fival->stepwise.min.numerator,
> +		v4l2_simplify_fraction(&fival->stepwise.min.numerator,
>   			&fival->stepwise.min.denominator, 8, 333);
> -		uvc_simplify_fraction(&fival->stepwise.max.numerator,
> +		v4l2_simplify_fraction(&fival->stepwise.max.numerator,
>   			&fival->stepwise.max.denominator, 8, 333);
> -		uvc_simplify_fraction(&fival->stepwise.step.numerator,
> +		v4l2_simplify_fraction(&fival->stepwise.step.numerator,
>   			&fival->stepwise.step.denominator, 8, 333);
>   	}
>   
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 24c911aeebce56..ff710bdd38b3fd 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -911,9 +911,6 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>   		      struct uvc_xu_control_query *xqry);
>   
>   /* Utility functions */
> -void uvc_simplify_fraction(u32 *numerator, u32 *denominator,
> -			   unsigned int n_terms, unsigned int threshold);
> -u32 uvc_fraction_to_interval(u32 numerator, u32 denominator);
>   struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts,
>   					    u8 epaddr);
>   u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index e0fbe6ba4b6c49..40f56e044640d7 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -484,3 +484,89 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
>   	return freq > 0 ? freq : -EINVAL;
>   }
>   EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
> +
> +/*
> + * Simplify a fraction using a simple continued fraction decomposition. The
> + * idea here is to convert fractions such as 333333/10000000 to 1/30 using
> + * 32 bit arithmetic only. The algorithm is not perfect and relies upon two
> + * arbitrary parameters to remove non-significative terms from the simple
> + * continued fraction decomposition. Using 8 and 333 for n_terms and threshold
> + * respectively seems to give nice results.
> + */
> +void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
> +		unsigned int n_terms, unsigned int threshold)
> +{
> +	u32 *an;
> +	u32 x, y, r;
> +	unsigned int i, n;
> +
> +	an = kmalloc_array(n_terms, sizeof(*an), GFP_KERNEL);
> +	if (an == NULL)
> +		return;
> +
> +	/*
> +	 * Convert the fraction to a simple continued fraction. See
> +	 * https://en.wikipedia.org/wiki/Continued_fraction
> +	 * Stop if the current term is bigger than or equal to the given
> +	 * threshold.
> +	 */
> +	x = *numerator;
> +	y = *denominator;
> +
> +	for (n = 0; n < n_terms && y != 0; ++n) {
> +		an[n] = x / y;
> +		if (an[n] >= threshold) {
> +			if (n < 2)
> +				n++;
> +			break;
> +		}
> +
> +		r = x - an[n] * y;
> +		x = y;
> +		y = r;
> +	}
> +
> +	/* Expand the simple continued fraction back to an integer fraction. */
> +	x = 0;
> +	y = 1;
> +
> +	for (i = n; i > 0; --i) {
> +		r = y;
> +		y = an[i-1] * y + x;
> +		x = r;
> +	}
> +
> +	*numerator = y;
> +	*denominator = x;
> +	kfree(an);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_simplify_fraction);
> +
> +/*
> + * Convert a fraction to a frame interval in 100ns multiples. The idea here is
> + * to compute numerator / denominator * 10000000 using 32 bit fixed point
> + * arithmetic only.
> + */
> +u32 v4l2_fraction_to_interval(u32 numerator, u32 denominator)
> +{
> +	u32 multiplier;
> +
> +	/* Saturate the result if the operation would overflow. */
> +	if (denominator == 0 ||
> +	    numerator/denominator >= ((u32)-1)/10000000)
> +		return (u32)-1;
> +
> +	/*
> +	 * Divide both the denominator and the multiplier by two until
> +	 * numerator * multiplier doesn't overflow. If anyone knows a better
> +	 * algorithm please let me know.
> +	 */
> +	multiplier = 10000000;
> +	while (numerator > ((u32)-1)/multiplier) {
> +		multiplier /= 2;
> +		denominator /= 2;
> +	}
> +
> +	return denominator ? numerator * multiplier / denominator : 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fraction_to_interval);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index b708d63995f458..725ff91b26e063 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -540,6 +540,10 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>   s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
>   		       unsigned int div);
>   
> +void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
> +		unsigned int n_terms, unsigned int threshold);
> +u32 v4l2_fraction_to_interval(u32 numerator, u32 denominator);
> +
>   static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
>   {
>   	/*
Michael Grzeschik Sept. 9, 2022, 3:11 p.m. UTC | #4
Hi Daniel,

On Fri, Sep 09, 2022 at 01:56:50PM +0100, Dan Scally wrote:
>On 08/09/2022 20:47, Michael Grzeschik wrote:
>>This patch adds support to the v4l2 VIDIOCs for enum_format,
>>enum_framesizes and enum_frameintervals. This way, the userspace
>>application can use these VIDIOCS to query the via configfs exported
>>frame capabilities. With thes callbacks the userspace doesn't have to
>>bring its own configfs parser.
>>
>>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>>---
>>v1 -> v13:
>>    - refactored the enum_ callbacks to this separate new patch
>>    - renamed +uvc_v4l2_enum_fmt to uvc_v4l2_enum_format
>>    - improved coding style
>>    - removed unused leftover variable uvc_video in enum functions
>>
>>  drivers/usb/gadget/function/f_uvc.c    |  32 +++++
>>  drivers/usb/gadget/function/uvc.h      |   2 +
>>  drivers/usb/gadget/function/uvc_v4l2.c | 176 +++++++++++++++++++++++++
>>  3 files changed, 210 insertions(+)
>>
>>diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>>index f4f6cf75930beb..7c416170b499e0 100644
>>--- a/drivers/usb/gadget/function/f_uvc.c
>>+++ b/drivers/usb/gadget/function/f_uvc.c
>>@@ -888,6 +888,7 @@ static void uvc_free(struct usb_function *f)
>>  	struct uvc_device *uvc = to_uvc(f);
>>  	struct f_uvc_opts *opts = container_of(f->fi, struct f_uvc_opts,
>>  					       func_inst);
>>+	config_item_put(&uvc->header->item);
>>  	--opts->refcnt;
>>  	kfree(uvc);
>>  }
>>@@ -941,6 +942,7 @@ 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 *streaming, *header, *h;
>>  	uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
>>  	if (uvc == NULL)
>>@@ -973,6 +975,36 @@ 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;
>>+
>>+	streaming = config_group_find_item(&opts->func_inst.group, "streaming");
>>+	if (!streaming) {
>>+		config_item_put(streaming);
>
>
>This shouldn't be necessary as it's a no-op if streaming is null
>
>>+		mutex_unlock(&opts->lock);
>>+		return ERR_PTR(-ENOMEM);
>
>
>Why ENOMEM? I wouldn't expect that error here...I think I'd expect 
>ENOENT. You also aren't freeing uvc here so that memory would be lost.
>
>
>>+	}
>>+
>>+	header = config_group_find_item(to_config_group(streaming), "header");
>>+	config_item_put(streaming);
>>+	if (!header) {
>>+		config_item_put(header);
>>+		mutex_unlock(&opts->lock);
>>+		return ERR_PTR(-ENOMEM);
>>+	}
>>+
>>+	h = config_group_find_item(to_config_group(header), "h");
>>+	config_item_put(header);
>>+	if (!h) {
>>+		config_item_put(h);
>>+		mutex_unlock(&opts->lock);
>>+		return ERR_PTR(-ENOMEM);
>>+	}
>
>
>Similar comments for these two error paths. Given the similarity of 
>those sections you could have something like;
>
>
>    streaming = config_group_find_item(&opts->func_inst.group, 
>"streaming");
>    if (!streaming)
>        goto err_config;
>
>    ... rest of the function ...
>
>    return &uvc->func;
>
>err_config:
>    mutex_unlock(&opts->lock);
>    kfree(uvc);
>    return -ENOENT; // or whatever is appropriate
>}
>
>>+
>>+	uvc->header = to_uvcg_streaming_header(h);
>>+	if (!uvc->header->linked) {
>>+		mutex_unlock(&opts->lock);
>>+		return ERR_PTR(-EBUSY);
>
>
>This path on the other hand should have config_item_put(h) I think, 
>and would also need to kfree(uvc).

All this makes totally sense. I will fix this patch and take also patch
4/6 and create a new series. The patches 5/6 and 6/6 will become a
separate series which I will send after the first part was merged. So we
can work us through the missing peaces step by step.

Regards,
Michael

>>+	}
>>+
>>  	++opts->refcnt;
>>  	mutex_unlock(&opts->lock);
>>diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>>index 58e383afdd4406..641cf2e7afaf6e 100644
>>--- a/drivers/usb/gadget/function/uvc.h
>>+++ b/drivers/usb/gadget/function/uvc.h
>>@@ -133,6 +133,8 @@ struct uvc_device {
>>  	bool func_connected;
>>  	wait_queue_head_t func_connected_queue;
>>+	struct uvcg_streaming_header *header;
>>+
>>  	/* Descriptors */
>>  	struct {
>>  		const struct uvc_descriptor_header * const *fs_control;
>>diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>>index 511f106f984375..63cb5a40306c75 100644
>>--- a/drivers/usb/gadget/function/uvc_v4l2.c
>>+++ b/drivers/usb/gadget/function/uvc_v4l2.c
>>@@ -18,12 +18,92 @@
>>  #include <media/v4l2-dev.h>
>>  #include <media/v4l2-event.h>
>>  #include <media/v4l2-ioctl.h>
>>+#include <media/v4l2-uvc.h>
>>  #include "f_uvc.h"
>>  #include "uvc.h"
>>  #include "uvc_queue.h"
>>  #include "uvc_video.h"
>>  #include "uvc_v4l2.h"
>>+#include "uvc_configfs.h"
>>+
>>+static struct uvc_format_desc *to_uvc_format(struct uvcg_format *uformat)
>>+{
>>+	char guid[16] = UVC_GUID_FORMAT_MJPEG;
>>+	struct uvc_format_desc *format;
>>+	struct uvcg_uncompressed *unc;
>>+
>>+	if (uformat->type == UVCG_UNCOMPRESSED) {
>>+		unc = to_uvcg_uncompressed(&uformat->group.cg_item);
>>+		if (!unc)
>>+			return ERR_PTR(-EINVAL);
>>+
>>+		memcpy(guid, unc->desc.guidFormat, sizeof(guid));
>>+	}
>>+
>>+	format = uvc_format_by_guid(guid);
>>+	if (!format)
>>+		return ERR_PTR(-EINVAL);
>>+
>>+	return format;
>>+}
>>+
>>+struct uvcg_format *find_format_by_index(struct uvc_device *uvc, int index)
>>+{
>>+	struct uvcg_format_ptr *format;
>>+	struct uvcg_format *uformat = NULL;
>>+	int i = 1;
>>+
>>+	list_for_each_entry(format, &uvc->header->formats, entry) {
>>+		if (index == i) {
>>+			uformat = format->fmt;
>>+			break;
>>+		}
>>+		i++;
>>+	}
>>+
>>+	return uformat;
>>+}
>>+
>>+struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
>>+				       struct uvcg_format *uformat,
>>+				       int index)
>>+{
>>+	struct uvcg_format_ptr *format;
>>+	struct uvcg_frame_ptr *frame;
>>+	struct uvcg_frame *uframe = NULL;
>>+
>>+	list_for_each_entry(format, &uvc->header->formats, entry) {
>>+		if (format->fmt->type != uformat->type)
>>+			continue;
>>+		list_for_each_entry(frame, &format->fmt->frames, entry) {
>>+			if (index == frame->frm->frame.b_frame_index) {
>>+				uframe = frame->frm;
>>+				break;
>>+			}
>>+		}
>>+	}
>>+
>>+	return uframe;
>>+}
>>+
>>+static struct uvcg_format *find_format_by_pix(struct uvc_device *uvc,
>>+					      u32 pixelformat)
>>+{
>>+	struct uvcg_format_ptr *format;
>>+	struct uvcg_format *uformat = NULL;
>>+
>>+	list_for_each_entry(format, &uvc->header->formats, entry) {
>>+		struct uvc_format_desc *fmtdesc = to_uvc_format(format->fmt);
>>+
>>+		if (fmtdesc->fcc == pixelformat) {
>>+			uformat = format->fmt;
>>+			break;
>>+		}
>>+	}
>>+
>>+	return uformat;
>>+}
>>  /* --------------------------------------------------------------------------
>>   * Requests handling
>>@@ -134,6 +214,99 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>>  	return 0;
>>  }
>>+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 *uformat = NULL;
>>+	struct uvcg_frame *uframe = NULL;
>>+	struct uvcg_frame_ptr *frame;
>>+
>>+	uformat = find_format_by_pix(uvc, fival->pixel_format);
>>+	if (!uformat)
>>+		return -EINVAL;
>>+
>>+	list_for_each_entry(frame, &uformat->frames, entry) {
>>+		if (frame->frm->frame.w_width == fival->width &&
>>+		    frame->frm->frame.w_height == fival->height) {
>>+			uframe = frame->frm;
>>+			break;
>>+		}
>>+	}
>>+	if (!uframe)
>>+		return -EINVAL;
>>+
>>+	if (fival->index >= uframe->frame.b_frame_interval_type)
>>+		return -EINVAL;
>>+
>>+	fival->discrete.numerator =
>>+		uframe->dw_frame_interval[fival->index];
>>+
>>+	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
>>+	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>>+	fival->discrete.denominator = 10000000;
>>+	v4l2_simplify_fraction(&fival->discrete.numerator,
>>+		&fival->discrete.denominator, 8, 333);
>>+
>>+	return 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 *uformat = NULL;
>>+	struct uvcg_frame *uframe = NULL;
>>+
>>+	uformat = find_format_by_pix(uvc, fsize->pixel_format);
>>+	if (!uformat)
>>+		return -EINVAL;
>>+
>>+	if (fsize->index >= uformat->num_frames)
>>+		return -EINVAL;
>>+
>>+	uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
>>+	if (!uframe)
>>+		return -EINVAL;
>>+
>>+	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>>+	fsize->discrete.width = uframe->frame.w_width;
>>+	fsize->discrete.height = uframe->frame.w_height;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+uvc_v4l2_enum_format(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 uvc_format_desc *fmtdesc;
>>+	struct uvcg_format *uformat;
>>+
>>+	if (f->index >= uvc->header->num_fmt)
>>+		return -EINVAL;
>>+
>>+	uformat = find_format_by_index(uvc, f->index + 1);
>>+	if (!uformat)
>>+		return -EINVAL;
>>+
>>+	if (uformat->type != UVCG_UNCOMPRESSED)
>>+		f->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>+
>>+	fmtdesc = to_uvc_format(uformat);
>>+	f->pixelformat = fmtdesc->fcc;
>>+
>>+	strscpy(f->description, fmtdesc->name, sizeof(f->description));
>>+	f->description[strlen(fmtdesc->name) - 1] = 0;
>>+
>>+	return 0;
>>+}
>>+
>>  static int
>>  uvc_v4l2_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
>>  {
>>@@ -300,6 +473,9 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>>  	.vidioc_querycap = uvc_v4l2_querycap,
>>  	.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_format,
>>  	.vidioc_reqbufs = uvc_v4l2_reqbufs,
>>  	.vidioc_querybuf = uvc_v4l2_querybuf,
>>  	.vidioc_qbuf = uvc_v4l2_qbuf,
>
Michael Grzeschik Sept. 9, 2022, 3:14 p.m. UTC | #5
On Fri, Sep 09, 2022 at 12:00:45PM +0100, Dan Scally wrote:
>Hello Michael - thanks for the set
>
>On 08/09/2022 20:47, Michael Grzeschik wrote:
>>This patch extends the v4l2 VIDIOCs for enum_format, enum_framesizes,
>>enum_frameintervals and set_fmt. The active host side configuration is
>>reported in these v4l2 interface functions. So the enum functions will
>>on set configuration return the currently set format/frmae/interval.
>>The set_format function has changed to be an noop since there is
>>no point for the kernel to set the currently decided format.
>>
>>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
>
>I actually get problems in uvc-gadget with this patch applied, when I 
>call v4l2-ctl --stream-mmap on the host side then on the gadget I get:
>
>
>/dev/video8: unable to queue buffer index 1/4 (22)
>[  336.655157] vb2-v4l2: [00000000ba5171ac] vb2_fill_vb2_v4l2_buffer: 
>plane parameters verification failed: -22
>
>
>And the reason for that verification failure is a mismatch in 
>__verify_length() between length which is 460800 and b->bytesused 
>which is 1843200. I set the format with v4l2-ctl --set-fmt-video 
>width=1280,height=720,pixelformat=YUYV so I'm expecting a size of 
>1843200. I'm setting the dwMaxVideoFrameBufferSize for that format to 
>1843200, so that setting doesn't seem to be reflected everywhere it 
>should be.

This is odd. I will look into this. For now, I am unsure where this
comes from.

I will work on patches 5/6 and 6/6 in a separate series. After this
series split, we can keep discussing solely about this new series in
question.

Thanks,
Michael

>>---
>>v1 -> v2:
>>    - fixed indentation of find_frame/format_by_index
>>    - fixed function name find_frm_by_size to find_frame_by_size
>>    - fixed indentation of _uvc_v4l2_try_fmt
>>    - fixed indentation in uvc_v4l2_enum_frameintervals
>>    - removed unneeded declaration of uvc_v4l2_get_bytesperline in uvc_v4l2.h
>>    - checked return values on config_group_find_item, handling refcount
>>    - fixed sizeof using variables instead of types
>>    - removed unsused def_format variable
>>    - wrting grp, hdr, fmt and frm in full
>>    - added proper ival handling
>>    - removed analyze_configfs function
>>    - added linked list of frames to uvcg_format
>>    - added functon find_frame_by_index
>>v2 -> v3:
>>    - fixed usage of u_uvc.h
>>    - removed unused variable i in _try_fmt
>>    - made uvc_v4l2_get_bytesperline static
>>v3 -> v4:
>>    - conditionally return current or all frames/formats/frameintervals on enum
>>    - dropped setting format and frame with set_format
>>    - combined try and set format function to one call
>>v4 -> v5:
>>    - fixed uninitialized return values reported by kernel test robot
>>    - added local video variable to uvc_v4l2_enum_frameintervals
>>v5 -> v6:
>>    -
>>v6 -> v7:
>>    - fixed unlocking in f_uvc uvc_alloc function
>>    - add uvc_get_frame_size function for sizeimage calculation
>>    - add fallback to frame.dw_max_video_frame_buffer_size
>>v7 -> v12:
>>    - moved the enum callbacks to a separate patch
>>    - rephrased the commit message
>>v7 -> v13:
>>    - moved the try_format callback to a separate patch
>>
>>  drivers/usb/gadget/function/f_uvc.c     |   4 +
>>  drivers/usb/gadget/function/uvc.h       |  20 +++-
>>  drivers/usb/gadget/function/uvc_queue.c |   2 +-
>>  drivers/usb/gadget/function/uvc_v4l2.c  | 117 ++++++++++++------------
>>  drivers/usb/gadget/function/uvc_video.c |  61 +++++++++++-
>>  5 files changed, 132 insertions(+), 72 deletions(-)
>>
>>diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>>index 7c416170b499e0..6f1138e819bdbf 100644
>>--- a/drivers/usb/gadget/function/f_uvc.c
>>+++ b/drivers/usb/gadget/function/f_uvc.c
>>@@ -327,6 +327,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>>  		if (uvc->video.ep)
>>  			usb_ep_disable(uvc->video.ep);
>>+		uvc->streamon = 0;
>>+
>>  		memset(&v4l2_event, 0, sizeof(v4l2_event));
>>  		v4l2_event.type = UVC_EVENT_STREAMOFF;
>>  		v4l2_event_queue(&uvc->vdev, &v4l2_event);
>>@@ -350,6 +352,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>>  			return ret;
>>  		usb_ep_enable(uvc->video.ep);
>>+		uvc->streamon = 1;
>>+
>>  		memset(&v4l2_event, 0, sizeof(v4l2_event));
>>  		v4l2_event.type = UVC_EVENT_STREAMON;
>>  		v4l2_event_queue(&uvc->vdev, &v4l2_event);
>>diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>>index 641cf2e7afaf6e..d32f34a7dbc423 100644
>>--- a/drivers/usb/gadget/function/uvc.h
>>+++ b/drivers/usb/gadget/function/uvc.h
>>@@ -90,11 +90,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 *cur_format;
>>+	struct uvcg_frame *cur_frame;
>>+	unsigned int cur_ival;
>>+
>>  	struct mutex mutex;	/* protects frame parameters */
>>  	unsigned int uvc_num_requests;
>>@@ -144,6 +143,8 @@ struct uvc_device {
>>  		const struct uvc_descriptor_header * const *ss_streaming;
>>  	} desc;
>>+	bool streamon;
>>+
>>  	unsigned int control_intf;
>>  	struct usb_ep *control_ep;
>>  	struct usb_request *control_req;
>>@@ -180,4 +181,13 @@ 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_get_frame_size(struct uvcg_format *uformat,
>>+		       struct uvcg_frame *uframe);
>>+
>>+extern struct uvcg_format *find_format_by_index(struct uvc_device *uvc,
>>+						int index);
>>+extern struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc,
>>+					      struct uvcg_format *uformat,
>>+					      int index);
>>+
>>  #endif /* _UVC_GADGET_H_ */
>>diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>>index ec500ee499eed1..3353d0566a59d5 100644
>>--- a/drivers/usb/gadget/function/uvc_queue.c
>>+++ b/drivers/usb/gadget/function/uvc_queue.c
>>@@ -52,7 +52,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>>  	*nplanes = 1;
>>-	sizes[0] = video->imagesize;
>>+	sizes[0] = uvc_get_frame_size(video->cur_format, video->cur_frame);
>>  	req_size = video->ep->maxpacket
>>  		 * max_t(unsigned int, video->ep->maxburst, 1)
>>diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>>index 761bc833d1e54f..bc5e2076c6b7e4 100644
>>--- a/drivers/usb/gadget/function/uvc_v4l2.c
>>+++ b/drivers/usb/gadget/function/uvc_v4l2.c
>>@@ -199,16 +199,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)
>>  {
>>@@ -229,56 +219,34 @@ uvc_v4l2_get_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_desc *fmtdesc;
>>-	fmt->fmt.pix.pixelformat = video->fcc;
>>-	fmt->fmt.pix.width = video->width;
>>-	fmt->fmt.pix.height = video->height;
>>+	fmtdesc = to_uvc_format(video->cur_format);
>>+
>>+	fmt->fmt.pix.pixelformat = fmtdesc->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 = video->bpp * video->width / 8;
>>-	fmt->fmt.pix.sizeimage = video->imagesize;
>>+	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(video->cur_format,
>>+							      video->cur_frame);
>>+	fmt->fmt.pix.sizeimage = uvc_get_frame_size(video->cur_format,
>>+						    video->cur_frame);
>>  	fmt->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>>  	fmt->fmt.pix.priv = 0;
>>  	return 0;
>>  }
>>+/* set_format is only allowed by the host side, so this is a noop */
>>  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)
>>-			break;
>>-	}
>>-	if (i == ARRAY_SIZE(uvc_formats)) {
>>-		uvcg_info(&uvc->func, "Unsupported format 0x%08x.\n",
>>-			  fmt->fmt.pix.pixelformat);
>>+	if (fmt->type != video->queue.queue.type)
>>  		return -EINVAL;
>>-	}
>>-
>>-	bpl = format->bpp * fmt->fmt.pix.width / 8;
>>-	imagesize = bpl ? bpl * fmt->fmt.pix.height : fmt->fmt.pix.sizeimage;
>>-
>>-	video->fcc = format->fcc;
>>-	video->bpp = format->bpp;
>>-	video->width = fmt->fmt.pix.width;
>>-	video->height = fmt->fmt.pix.height;
>>-	video->imagesize = imagesize;
>>-
>>-	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;
>>  	return 0;
>>  }
>>@@ -329,6 +297,7 @@ uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>>  {
>>  	struct video_device *vdev = video_devdata(file);
>>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>>+	struct uvc_video *video = &uvc->video;
>>  	struct uvcg_format *uformat = NULL;
>>  	struct uvcg_frame *uframe = NULL;
>>  	struct uvcg_frame_ptr *frame;
>>@@ -347,11 +316,19 @@ uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>>  	if (!uframe)
>>  		return -EINVAL;
>>-	if (fival->index >= uframe->frame.b_frame_interval_type)
>>-		return -EINVAL;
>>+	if (uvc->streamon) {
>>+		if (fival->index >= 1)
>>+			return -EINVAL;
>>-	fival->discrete.numerator =
>>-		uframe->dw_frame_interval[fival->index];
>>+		fival->discrete.numerator =
>>+			uframe->dw_frame_interval[video->cur_ival - 1];
>>+	} else {
>>+		if (fival->index >= uframe->frame.b_frame_interval_type)
>>+			return -EINVAL;
>>+
>>+		fival->discrete.numerator =
>>+			uframe->dw_frame_interval[fival->index];
>>+	}
>>  	/* TODO: handle V4L2_FRMIVAL_TYPE_STEPWISE */
>>  	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>>@@ -368,19 +345,28 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh,
>>  {
>>  	struct video_device *vdev = video_devdata(file);
>>  	struct uvc_device *uvc = video_get_drvdata(vdev);
>>+	struct uvc_video *video = &uvc->video;
>>  	struct uvcg_format *uformat = NULL;
>>  	struct uvcg_frame *uframe = NULL;
>>-	uformat = find_format_by_pix(uvc, fsize->pixel_format);
>>-	if (!uformat)
>>-		return -EINVAL;
>>+	if (uvc->streamon) {
>>+		if (fsize->index >= 1)
>>+			return -EINVAL;
>>-	if (fsize->index >= uformat->num_frames)
>>-		return -EINVAL;
>>+		uformat = video->cur_format;
>>+		uframe = video->cur_frame;
>>+	} else {
>>+		uformat = find_format_by_pix(uvc, fsize->pixel_format);
>>+		if (!uformat)
>>+			return -EINVAL;
>>-	uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
>>-	if (!uframe)
>>-		return -EINVAL;
>>+		if (fsize->index >= uformat->num_frames)
>>+			return -EINVAL;
>>+
>>+		uframe = find_frame_by_index(uvc, uformat, fsize->index + 1);
>>+		if (!uframe)
>>+			return -EINVAL;
>>+	}
>>  	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>>  	fsize->discrete.width = uframe->frame.w_width;
>>@@ -394,15 +380,24 @@ uvc_v4l2_enum_format(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 uvc_video *video = &uvc->video;
>>  	struct uvc_format_desc *fmtdesc;
>>  	struct uvcg_format *uformat;
>>-	if (f->index >= uvc->header->num_fmt)
>>-		return -EINVAL;
>>+	if (uvc->streamon) {
>>+		if (f->index >= 1)
>>+			return -EINVAL;
>>-	uformat = find_format_by_index(uvc, f->index + 1);
>>-	if (!uformat)
>>-		return -EINVAL;
>>+		uformat = video->cur_format;
>>+	} else {
>>+		if (f->index >= uvc->header->num_fmt)
>>+			return -EINVAL;
>>+
>>+		uformat = find_format_by_index(uvc, f->index + 1);
>>+		if (!uformat)
>>+			return -EINVAL;
>>+
>>+	}
>>  	if (uformat->type != UVCG_UNCOMPRESSED)
>>  		f->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>index c00ce0e91f5d5c..37867c93073418 100644
>>--- a/drivers/usb/gadget/function/uvc_video.c
>>+++ b/drivers/usb/gadget/function/uvc_video.c
>>@@ -19,6 +19,7 @@
>>  #include "uvc.h"
>>  #include "uvc_queue.h"
>>  #include "uvc_video.h"
>>+#include "uvc_configfs.h"
>>  /* --------------------------------------------------------------------------
>>   * Video codecs
>>@@ -490,21 +491,71 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>>  	return ret;
>>  }
>>+static int uvc_frame_default(struct uvcg_format *uformat)
>>+{
>>+	struct uvcg_uncompressed *u;
>>+	struct uvcg_mjpeg *m;
>>+
>>+	switch (uformat->type) {
>>+	case UVCG_UNCOMPRESSED:
>>+		u = to_uvcg_uncompressed(&uformat->group.cg_item);
>>+		if (u)
>>+			return u->desc.bDefaultFrameIndex;
>>+		break;
>>+	case UVCG_MJPEG:
>>+		m = to_uvcg_mjpeg(&uformat->group.cg_item);
>>+		if (m)
>>+			return m->desc.bDefaultFrameIndex;
>>+		break;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int uvc_default_frame_interval(struct uvc_video *video)
>>+{
>>+	int i;
>>+
>>+	for (i = 0; i < video->cur_frame->frame.b_frame_interval_type; i++) {
>>+		if (video->cur_frame->frame.dw_default_frame_interval ==
>>+			video->cur_frame->dw_frame_interval[i]) {
>>+			video->cur_ival = i + 1;
>>+			return i + 1;
>>+		}
>>+	}
>>+
>>+	/* fallback */
>>+	return 1;
>>+}
>>+
>>  /*
>>   * Initialize the UVC video stream.
>>   */
>>  int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>>  {
>>+	int iframe;
>>+
>>  	INIT_LIST_HEAD(&video->req_free);
>>  	spin_lock_init(&video->req_lock);
>>  	INIT_WORK(&video->pump, uvcg_video_pump);
>>+	if (list_empty(&uvc->header->formats))
>>+		return -EINVAL;
>>+
>>  	video->uvc = uvc;
>>-	video->fcc = V4L2_PIX_FMT_YUYV;
>>-	video->bpp = 16;
>>-	video->width = 320;
>>-	video->height = 240;
>>-	video->imagesize = 320 * 240 * 2;
>>+	video->cur_format = find_format_by_index(uvc, 1);
>>+	if (!video->cur_format)
>>+		return -EINVAL;
>>+
>>+	iframe = uvc_frame_default(video->cur_format);
>>+	if (!iframe)
>>+		return -EINVAL;
>>+
>>+	video->cur_frame = find_frame_by_index(uvc, video->cur_format, iframe);
>>+	if (!video->cur_frame)
>>+		return -EINVAL;
>>+
>>+	video->cur_ival = uvc_default_frame_interval(video);
>>  	/* Initialize the video buffers queue. */
>>  	uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,
>