Message ID | 20220908194750.3750310-1-m.grzeschik@pengutronix.de |
---|---|
Headers | show |
Series | usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS | expand |
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,
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,
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) > { > /*
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, >
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, >