| Message ID | 20220907140254.2378109-5-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: use configfs entries for negotiation and v4l2 VIDIOCS | expand |
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@pengutronix.de> > > --- > 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);
Hi Laurent, On Wed, Sep 07, 2022 at 06:07:34PM +0300, Laurent Pinchart wrote: >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. The current case is to set configfs from userspace to ensure the host sees only what we configured. Then the userspace has to parse this configfs again, to be sure not to allow something on SET_CUR/GET_CUR that is not valid from configfs configuration. Since the configfs-Setup could be done from another application comming from somewhere in the userspace this limit will always stay. Since the kernel knows the configfs-setup it was given in the beginning it can ensure that SET_CUR/GET_CUR will only handle valid setups. When done in Kernel, we can also use simple v4l2 API calls like try_format to ask the driver what the host side did configure. So we can use simple abstracion-libs like gstreamer for this. I thought I already mentioned this all in the Cover-Letter of this series. Michael >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> 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 >
Hi Michael, On Wed, Sep 07, 2022 at 05:15:16PM +0200, Michael Grzeschik wrote: > On Wed, Sep 07, 2022 at 06:07:34PM +0300, Laurent Pinchart wrote: > > 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. > > The current case is to set configfs from userspace to ensure > the host sees only what we configured. Then the userspace has to parse > this configfs again, to be sure not to allow something on > SET_CUR/GET_CUR that is not valid from configfs configuration. Since the > configfs-Setup could be done from another application comming from > somewhere in the userspace this limit will always stay. That really depends on the architecture of the userspace stack, when the same userspace application configures the gadget and handles the runtime operations, it won't have to parse configfs. I'd also argue that in practical use cases, the application will likely need to know the list of formats and resolutions that are exposed by the gadget in order to prepare for supporting them properly (for instance, allocating buffers large enough to store the largest resolution is common when you want to lower stream start delays if your system is not memory-constrained). > Since the kernel knows the configfs-setup it was given in the beginning > it can ensure that SET_CUR/GET_CUR will only handle valid setups. > > When done in Kernel, we can also use simple v4l2 API calls like try_format to > ask the driver what the host side did configure. So we can use simple > abstracion-libs like gstreamer for this. This will cause trouble when extending support to UVC 1.5 as the complexity will grow on the kernel side. Furthermore, by handling the video probe and commit control in kernel space, you'd removing the possibility for userspace to decide on how to handle the bHint flags, or how to negociate dwFrameInterval key frame rate, compression quality, or the additional fields specific to UVC 1.5. This effectively hardcodes one particular policy in the driver, and that shouldn't be the role of the kernel. > I thought I already mentioned this all in the Cover-Letter of this > series. Sorry, I missed that. > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >> > >> --- > >> 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);
On Wed, Sep 07, 2022 at 06:32:09PM +0300, Laurent Pinchart wrote: >Hi Michael, > >On Wed, Sep 07, 2022 at 05:15:16PM +0200, Michael Grzeschik wrote: >> On Wed, Sep 07, 2022 at 06:07:34PM +0300, Laurent Pinchart wrote: >> > 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. >> >> The current case is to set configfs from userspace to ensure >> the host sees only what we configured. Then the userspace has to parse >> this configfs again, to be sure not to allow something on >> SET_CUR/GET_CUR that is not valid from configfs configuration. Since the >> configfs-Setup could be done from another application comming from >> somewhere in the userspace this limit will always stay. > >That really depends on the architecture of the userspace stack, when the >same userspace application configures the gadget and handles the runtime >operations, it won't have to parse configfs. I'd also argue that in >practical use cases, the application will likely need to know the list >of formats and resolutions that are exposed by the gadget in order to >prepare for supporting them properly (for instance, allocating buffers >large enough to store the largest resolution is common when you want to >lower stream start delays if your system is not memory-constrained). The userspace can for sure can do resolution and format parsing that is exposed by the gadget. Even more standardized, now with the simple v4l2 API, without having to parse the whole configfs again. >> Since the kernel knows the configfs-setup it was given in the beginning >> it can ensure that SET_CUR/GET_CUR will only handle valid setups. >> >> When done in Kernel, we can also use simple v4l2 API calls like try_format to >> ask the driver what the host side did configure. So we can use simple >> abstracion-libs like gstreamer for this. > >This will cause trouble when extending support to UVC 1.5 as the >complexity will grow on the kernel side. Furthermore, by handling the >video probe and commit control in kernel space, you'd removing the >possibility for userspace to decide on how to handle the bHint flags, or >how to negociate dwFrameInterval key frame rate, compression quality, or >the additional fields specific to UVC 1.5. This effectively hardcodes >one particular policy in the driver, and that shouldn't be the role of >the kernel. When you look closely, you will see that for later user cases, I decided the userspace appliaction will still be able to subscribe for DATA and SETUP. It will only fall back to the kernel side of operation if there is nobody that is able to operate. In those cases, it will take over. I tested this with your uvc-gadget appliaction. Since it subscribes more than STREAMON and STREAMOFF, it is still working like before with this patchstack applied. Regards, Michael
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);
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. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- 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(-)