Hi Michael, Thank you for the patch. On Wed, Sep 07, 2022 at 04:02:54PM +0200, Michael Grzeschik wrote: > The Hostside format selection is currently only done in userspace, as > the events for SET_CUR and GET_CUR are always moved to the application > layer. Since the v4l2 device parses the configfs data, the format > negotiation can be done in the kernel. This patch adds the functions to > set the current configuration while continuing to forward all unknown > events to the userspace level. Why do you think this is better done in kernel space, given that userspace has to process the event anyway ? It's more complexity in the kernel side, for little added value as far as I can see. It will also make it more difficult to handle different UVC versions (in particular UVC 1.5). I'd rather not go in this direction if there's no clear benefit. > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > > --- > v1 -> v2: > - fixed the commit message > - changed pr_debug to pr_err in events_process_data > - aligned many indentations > - simplified uvc_events_process_data > - fixed uvc_fill_streaming_control calls in uvcg_video_init > - added setup_subscribed to decide if userspace takes over on EOPNOTSUPP > - added data_subscribed to decide if userspace takes over on EOPNOTSUPP > - removed duplicate send_response > - wrting fmt and frm in full > v2 -> v3: > - added find_format_index to set the right probe > v3 -> v4: > - add function find_ival_index and use for cur_ival > - fix swapped frame and format in uvc_events_process_data on uvc_fill_streaming_control > - set proper resp.length on ep0 complete > - dropped setting cur_probe on set_format since function was removed > - added locking around getting correspondent cur_{frame,format,ival} > v4 -> v5: > - fixed sparse errors reported by kernel test robot > v5 -> v6: > - fixed the handling in uvc_function_ep0_complete after events_process_data > v6 -> v7: > - set dwMaxPayloadTransferSize unconditionally from streaming_maxpacket > - fixed check for interface with masking for 0xff > v7 -> v8: > - > > drivers/usb/gadget/function/f_uvc.c | 237 +++++++++++++++++++++++- > drivers/usb/gadget/function/uvc.h | 19 ++ > drivers/usb/gadget/function/uvc_v4l2.c | 66 ++++++- > drivers/usb/gadget/function/uvc_video.c | 12 +- > 4 files changed, 322 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index a8dcf5f01f16de..fc3fabc47018cd 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -16,7 +16,6 @@ > #include <linux/string.h> > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > -#include <linux/usb/g_uvc.h> > #include <linux/usb/video.h> > #include <linux/vmalloc.h> > #include <linux/wait.h> > @@ -204,21 +203,228 @@ static const struct usb_descriptor_header * const uvc_ss_streaming[] = { > * Control requests > */ > > +void uvc_fill_streaming_control(struct uvc_device *uvc, > + struct uvc_streaming_control *ctrl, > + int iframe, int iformat, unsigned int ival) > +{ > + struct f_uvc_opts *opts; > + struct uvcg_format *uformat; > + struct uvcg_frame *uframe; > + > + /* Restrict the iformat, iframe and ival to valid values. Negative > + * values for ifrmat and iframe will result in the maximum valid value > + * being selected > + */ > + iformat = clamp((unsigned int)iformat, 1U, > + (unsigned int)uvc->header->num_fmt); > + uformat = find_format_by_index(uvc, iformat); > + if (!uformat) > + return; > + > + iframe = clamp((unsigned int)iframe, 1U, > + (unsigned int)uformat->num_frames); > + uframe = find_frame_by_index(uvc, uformat, iframe); > + if (!uframe) > + return; > + > + ival = clamp((unsigned int)ival, 1U, > + (unsigned int)uframe->frame.b_frame_interval_type); > + if (!uframe->dw_frame_interval[ival - 1]) > + return; > + > + opts = fi_to_f_uvc_opts(uvc->func.fi); > + > + memset(ctrl, 0, sizeof(*ctrl)); > + > + ctrl->bmHint = 1; > + ctrl->bFormatIndex = iformat; > + ctrl->bFrameIndex = iframe; > + ctrl->dwFrameInterval = uframe->dw_frame_interval[ival - 1]; > + ctrl->dwMaxVideoFrameSize = > + uframe->frame.dw_max_video_frame_buffer_size; > + > + ctrl->dwMaxPayloadTransferSize = opts->streaming_maxpacket; > + ctrl->bmFramingInfo = 3; > + ctrl->bPreferedVersion = 1; > + ctrl->bMaxVersion = 1; > +} > + > +static int uvc_events_process_data(struct uvc_device *uvc, > + struct usb_request *req) > +{ > + struct uvc_video *video = &uvc->video; > + struct uvc_streaming_control *target; > + struct uvc_streaming_control *ctrl; > + struct uvcg_frame *uframe; > + struct uvcg_format *uformat; > + > + switch (video->control) { > + case UVC_VS_PROBE_CONTROL: > + pr_debug("setting probe control, length = %d\n", req->actual); > + target = &video->probe; > + break; > + > + case UVC_VS_COMMIT_CONTROL: > + pr_debug("setting commit control, length = %d\n", req->actual); > + target = &video->commit; > + break; > + > + default: > + pr_err("setting unknown control, length = %d\n", req->actual); > + return -EOPNOTSUPP; > + } > + > + ctrl = (struct uvc_streaming_control *)req->buf; > + > + uvc_fill_streaming_control(uvc, target, ctrl->bFrameIndex, > + ctrl->bFormatIndex, ctrl->dwFrameInterval); > + > + if (video->control == UVC_VS_COMMIT_CONTROL) { > + uformat = find_format_by_index(uvc, target->bFormatIndex); > + if (!uformat) > + return -EINVAL; > + > + uframe = find_frame_by_index(uvc, uformat, ctrl->bFrameIndex); > + if (!uframe) > + return -EINVAL; > + > + spin_lock(&video->frame_lock); > + > + video->cur_frame = uframe; > + video->cur_format = uformat; > + video->cur_ival = find_ival_index(uframe, ctrl->dwFrameInterval); > + > + spin_unlock(&video->frame_lock); > + } > + > + return 0; > +} > + > +static void > +uvc_events_process_streaming(struct uvc_device *uvc, uint8_t req, uint8_t cs, > + struct uvc_request_data *resp) > +{ > + struct uvc_streaming_control *ctrl; > + > + pr_debug("streaming request (req %02x cs %02x)\n", req, cs); > + > + if (cs != UVC_VS_PROBE_CONTROL && cs != UVC_VS_COMMIT_CONTROL) > + return; > + > + ctrl = (struct uvc_streaming_control *)&resp->data; > + resp->length = sizeof(*ctrl); > + > + switch (req) { > + case UVC_SET_CUR: > + uvc->video.control = cs; > + resp->length = 34; > + break; > + > + case UVC_GET_CUR: > + if (cs == UVC_VS_PROBE_CONTROL) > + memcpy(ctrl, &uvc->video.probe, sizeof(*ctrl)); > + else > + memcpy(ctrl, &uvc->video.commit, sizeof(*ctrl)); > + break; > + > + case UVC_GET_MIN: > + case UVC_GET_MAX: > + case UVC_GET_DEF: > + if (req == UVC_GET_MAX) > + uvc_fill_streaming_control(uvc, ctrl, -1, -1, UINT_MAX); > + else > + uvc_fill_streaming_control(uvc, ctrl, 1, 1, 0); > + break; > + > + case UVC_GET_RES: > + memset(ctrl, 0, sizeof(*ctrl)); > + break; > + > + case UVC_GET_LEN: > + resp->data[0] = 0x00; > + resp->data[1] = 0x22; > + resp->length = 2; > + break; > + > + case UVC_GET_INFO: > + resp->data[0] = 0x03; > + resp->length = 1; > + break; > + } > +} > + > +static int > +uvc_events_process_class(struct uvc_device *uvc, > + const struct usb_ctrlrequest *ctrl, > + struct uvc_request_data *resp) > +{ > + unsigned int interface = le16_to_cpu(ctrl->wIndex) & 0xff; > + > + if ((ctrl->bRequestType & USB_RECIP_MASK) != USB_RECIP_INTERFACE) > + return -EINVAL; > + > + if (interface == uvc->control_intf) > + return -EOPNOTSUPP; > + else if (interface == uvc->streaming_intf) > + uvc_events_process_streaming(uvc, ctrl->bRequest, > + le16_to_cpu(ctrl->wValue) >> 8, > + resp); > + > + return 0; > +} > + > +static int > +uvc_events_process_setup(struct uvc_device *uvc, > + const struct usb_ctrlrequest *ctrl, > + struct uvc_request_data *resp) > +{ > + uvc->video.control = 0; > + > + pr_debug("bRequestType %02x bRequest %02x wValue %04x wIndex %04x wLength %04x\n", > + ctrl->bRequestType, ctrl->bRequest, ctrl->wValue, > + ctrl->wIndex, ctrl->wLength); > + > + switch (ctrl->bRequestType & USB_TYPE_MASK) { > + case USB_TYPE_STANDARD: > + return -EOPNOTSUPP; > + > + case USB_TYPE_CLASS: > + return uvc_events_process_class(uvc, ctrl, resp); > + > + default: > + break; > + } > + > + return 0; > +} > + > static void > uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req) > { > struct uvc_device *uvc = req->context; > struct v4l2_event v4l2_event; > struct uvc_event *uvc_event = (void *)&v4l2_event.u.data; > + int ret; > > if (uvc->event_setup_out) { > uvc->event_setup_out = 0; > > - memset(&v4l2_event, 0, sizeof(v4l2_event)); > - v4l2_event.type = UVC_EVENT_DATA; > - uvc_event->data.length = req->actual; > - memcpy(&uvc_event->data.data, req->buf, req->actual); > - v4l2_event_queue(&uvc->vdev, &v4l2_event); > + ret = uvc_events_process_data(uvc, req); > + /* If we have a real error on process */ > + if (ret == -EINVAL) { > + struct uvc_request_data resp; > + > + memset(&resp, 0, sizeof(resp)); > + resp.length = -EL2HLT; > + > + uvc_send_response(uvc, &resp); > + } else if (ret == -EOPNOTSUPP && uvc->data_subscribed) { > + memset(&v4l2_event, 0, sizeof(v4l2_event)); > + v4l2_event.type = UVC_EVENT_DATA; > + uvc_event->data.length = req->actual; > + memcpy(&uvc_event->data.data, req->buf, req->actual); > + v4l2_event_queue(&uvc->vdev, &v4l2_event); > + } > } > } > > @@ -228,6 +434,8 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) > struct uvc_device *uvc = to_uvc(f); > struct v4l2_event v4l2_event; > struct uvc_event *uvc_event = (void *)&v4l2_event.u.data; > + struct uvc_request_data resp; > + int ret = 0; > > if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) { > uvcg_info(f, "invalid request type\n"); > @@ -245,6 +453,23 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) > uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN); > uvc->event_length = le16_to_cpu(ctrl->wLength); > > + memset(&resp, 0, sizeof(resp)); > + resp.length = -EL2HLT; > + > + ret = uvc_events_process_setup(uvc, ctrl, &resp); > + /* If we have no error on process */ > + if (!ret) > + return uvc_send_response(uvc, &resp); > + > + /* If we have a real error on process */ > + if (ret != -EOPNOTSUPP) > + return ret; > + > + /* If we have -EOPNOTSUPP */ > + if (!uvc->setup_subscribed) > + return uvc_send_response(uvc, &resp); > + > + /* If we have setup subscribed */ > memset(&v4l2_event, 0, sizeof(v4l2_event)); > v4l2_event.type = UVC_EVENT_SETUP; > memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req)); > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 8a71d0c4abbcd5..cdfd46f3a2861c 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -13,6 +13,8 @@ > #include <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/usb/composite.h> > +#include <linux/usb/g_uvc.h> > +#include <linux/usb/video.h> > #include <linux/videodev2.h> > #include <linux/wait.h> > > @@ -95,6 +97,12 @@ struct uvc_video { > unsigned int cur_ival; > > struct mutex mutex; /* protects frame parameters */ > + spinlock_t frame_lock; > + > + struct uvc_streaming_control probe; > + struct uvc_streaming_control commit; > + > + int control; > > unsigned int uvc_num_requests; > > @@ -131,6 +139,8 @@ struct uvc_device { > struct uvc_video video; > bool func_connected; > wait_queue_head_t func_connected_queue; > + bool setup_subscribed; > + bool data_subscribed; > > struct uvcg_streaming_header *header; > > @@ -187,5 +197,14 @@ extern struct uvcg_format *find_format_by_index(struct uvc_device *uvc, > extern struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc, > struct uvcg_format *uformat, > int index); > +extern int find_format_index(struct uvc_device *uvc, > + struct uvcg_format *uformat); > +extern int find_ival_index(struct uvcg_frame *uframe, int dwFrameInterval); > +extern void uvc_fill_streaming_control(struct uvc_device *uvc, > + struct uvc_streaming_control *ctrl, > + int iframe, int iformat, > + unsigned int ival); > +extern int uvc_send_response(struct uvc_device *uvc, > + struct uvc_request_data *data); > > #endif /* _UVC_GADGET_H_ */ > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index 3311e98a040537..e4c4bebd2ee860 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -90,6 +90,33 @@ struct uvcg_format *find_format_by_index(struct uvc_device *uvc, int index) > return uformat; > } > > +int find_format_index(struct uvc_device *uvc, struct uvcg_format *uformat) > +{ > + struct uvcg_format_ptr *format; > + int i = 1; > + > + list_for_each_entry(format, &uvc->header->formats, entry) { > + if (uformat == format->fmt) > + return i; > + i++; > + } > + > + return 0; > +} > + > +int find_ival_index(struct uvcg_frame *uframe, int dwFrameInterval) > +{ > + int i; > + > + for (i = 0; i < uframe->frame.b_frame_interval_type; i++) { > + if (dwFrameInterval == uframe->dw_frame_interval[i]) > + return i + 1; > + } > + > + /* fallback */ > + return 1; > +} > + > struct uvcg_frame *find_frame_by_index(struct uvc_device *uvc, > struct uvcg_format *uformat, > int index) > @@ -178,8 +205,7 @@ static struct uvcg_frame *find_closest_frame_by_size(struct uvc_device *uvc, > * Requests handling > */ > > -static int > -uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data) > +int uvc_send_response(struct uvc_device *uvc, struct uvc_request_data *data) > { > struct usb_composite_dev *cdev = uvc->func.config->cdev; > struct usb_request *req = uvc->control_req; > @@ -221,6 +247,8 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt) > struct uvc_video *video = &uvc->video; > struct uvc_format_desc *fmtdesc; > > + spin_lock(&video->frame_lock); > + > fmtdesc = to_uvc_format(video->cur_format); > > fmt->fmt.pix.pixelformat = fmtdesc->fcc; > @@ -231,6 +259,8 @@ uvc_v4l2_get_format(struct file *file, void *fh, struct v4l2_format *fmt) > fmt->fmt.pix.sizeimage = video->imagesize; > fmt->fmt.pix.priv = 0; > > + spin_unlock(&video->frame_lock); > + > return 0; > } > > @@ -255,6 +285,7 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt) > struct uvc_video *video = &uvc->video; > struct uvcg_format *uformat; > struct uvcg_frame *uframe; > + int iformat; > u8 *fcc; > > if (fmt->type != video->queue.queue.type) > @@ -270,6 +301,10 @@ uvc_v4l2_try_format(struct file *file, void *fh, struct v4l2_format *fmt) > if (!uformat) > return -EINVAL; > > + iformat = find_format_index(uvc, uformat); > + if (!iformat) > + return -EINVAL; > + > uframe = find_closest_frame_by_size(uvc, uformat, > fmt->fmt.pix.width, fmt->fmt.pix.height); > if (!uframe) > @@ -314,8 +349,12 @@ uvc_v4l2_enum_frameintervals(struct file *file, void *fh, > if (fival->index >= 1) > return -EINVAL; > > + spin_lock(&video->frame_lock); > + > fival->discrete.numerator = > uframe->dw_frame_interval[video->cur_ival - 1]; > + > + spin_unlock(&video->frame_lock); > } else { > if (fival->index >= uframe->frame.b_frame_interval_type) > return -EINVAL; > @@ -347,8 +386,12 @@ uvc_v4l2_enum_framesizes(struct file *file, void *fh, > if (fsize->index >= 1) > return -EINVAL; > > + spin_lock(&video->frame_lock); > + > uformat = video->cur_format; > uframe = video->cur_frame; > + > + spin_unlock(&video->frame_lock); > } else { > uformat = find_format_by_pix(uvc, fsize->pixel_format); > if (!uformat) > @@ -382,7 +425,11 @@ uvc_v4l2_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f) > if (f->index >= 1) > return -EINVAL; > > + spin_lock(&video->frame_lock); > + > uformat = video->cur_format; > + > + spin_unlock(&video->frame_lock); > } else { > if (f->index >= uvc->header->num_fmt) > return -EINVAL; > @@ -506,14 +553,20 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh, > if (sub->type < UVC_EVENT_FIRST || sub->type > UVC_EVENT_LAST) > return -EINVAL; > > - if (sub->type == UVC_EVENT_SETUP && uvc->func_connected) > + if (sub->type == UVC_EVENT_STREAMON && uvc->func_connected) > return -EBUSY; > > ret = v4l2_event_subscribe(fh, sub, 2, NULL); > if (ret < 0) > return ret; > > - if (sub->type == UVC_EVENT_SETUP) { > + if (sub->type == UVC_EVENT_SETUP) > + uvc->setup_subscribed = true; > + > + if (sub->type == UVC_EVENT_DATA) > + uvc->data_subscribed = true; > + > + if (sub->type == UVC_EVENT_STREAMON) { > uvc->func_connected = true; > handle->is_uvc_app_handle = true; > uvc_function_connect(uvc); > @@ -543,7 +596,10 @@ uvc_v4l2_unsubscribe_event(struct v4l2_fh *fh, > if (ret < 0) > return ret; > > - if (sub->type == UVC_EVENT_SETUP && handle->is_uvc_app_handle) { > + if (sub->type == UVC_EVENT_SETUP) > + uvc->setup_subscribed = false; > + > + if (sub->type == UVC_EVENT_STREAMON && handle->is_uvc_app_handle) { > uvc_v4l2_disable(uvc); > handle->is_uvc_app_handle = false; > } > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index 37867c93073418..c20d832c4a0b8a 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -533,10 +533,11 @@ static int uvc_default_frame_interval(struct uvc_video *video) > */ > int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) > { > - int iframe; > + int iframe, iformat; > > INIT_LIST_HEAD(&video->req_free); > spin_lock_init(&video->req_lock); > + spin_lock_init(&video->frame_lock); > INIT_WORK(&video->pump, uvcg_video_pump); > > if (list_empty(&uvc->header->formats)) > @@ -547,6 +548,10 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) > if (!video->cur_format) > return -EINVAL; > > + iformat = find_format_index(uvc, video->cur_format); > + if (!iformat) > + return -EINVAL; > + > iframe = uvc_frame_default(video->cur_format); > if (!iframe) > return -EINVAL; > @@ -557,6 +562,11 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) > > video->cur_ival = uvc_default_frame_interval(video); > > + uvc_fill_streaming_control(uvc, &video->probe, iframe, iformat, > + video->cur_ival); > + uvc_fill_streaming_control(uvc, &video->commit, iframe, iformat, > + video->cur_ival); > + > /* Initialize the video buffers queue. */ > uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent, > V4L2_BUF_TYPE_VIDEO_OUTPUT, &video->mutex); -- Regards, Laurent Pinchart