| Message ID | 20240403-uvc_request_length_by_interval-v4-0-ca22f334226e@pengutronix.de |
|---|---|
| Headers | show |
| Series | usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers | expand |
On Tue, Aug 13, 2024 at 11:09:31AM +0200, Michael Grzeschik wrote: > The uvc gadget driver is lacking the information which frame interval > was set by the host. We add this information by implementing the g_parm > and s_parm callbacks. As I've said countless times, this kind of hack is not the right way to pass information that the kernel has no use for between two userspace components. Please stop butchering this driver. > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > v3 -> v4: - > v2 -> v3: - > v1 -> v2: - > --- > drivers/usb/gadget/function/uvc.h | 1 + > drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index b3a5165ac70ec..f6bc58fb02b84 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -100,6 +100,7 @@ struct uvc_video { > unsigned int width; > unsigned int height; > unsigned int imagesize; > + unsigned int interval; > struct mutex mutex; /* protects frame parameters */ > > unsigned int uvc_num_requests; > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index de41519ce9aa0..392fb400aad14 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt) > return ret; > } > > +static int uvc_v4l2_g_parm(struct file *file, void *fh, > + struct v4l2_streamparm *parm) > +{ > + struct video_device *vdev = video_devdata(file); > + struct uvc_device *uvc = video_get_drvdata(vdev); > + struct uvc_video *video = &uvc->video; > + struct v4l2_fract timeperframe; > + > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) > + return -EINVAL; > + > + /* Return the actual frame period. */ > + timeperframe.numerator = video->interval; > + timeperframe.denominator = 10000000; > + v4l2_simplify_fraction(&timeperframe.numerator, > + &timeperframe.denominator, 8, 333); > + > + uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n", > + timeperframe.numerator, timeperframe.denominator, > + video->interval); > + > + parm->parm.output.timeperframe = timeperframe; > + parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME; > + > + return 0; > +} > + > +static int uvc_v4l2_s_parm(struct file *file, void *fh, > + struct v4l2_streamparm *parm) > +{ > + struct video_device *vdev = video_devdata(file); > + struct uvc_device *uvc = video_get_drvdata(vdev); > + struct uvc_video *video = &uvc->video; > + struct v4l2_fract timeperframe; > + > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) > + return -EINVAL; > + > + timeperframe = parm->parm.output.timeperframe; > + > + video->interval = v4l2_fraction_to_interval(timeperframe.numerator, > + timeperframe.denominator); > + > + uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n", > + timeperframe.numerator, timeperframe.denominator, > + video->interval); > + > + return 0; > +} > + > static int > uvc_v4l2_enum_frameintervals(struct file *file, void *fh, > struct v4l2_frmivalenum *fival) > @@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = { > .vidioc_dqbuf = uvc_v4l2_dqbuf, > .vidioc_streamon = uvc_v4l2_streamon, > .vidioc_streamoff = uvc_v4l2_streamoff, > + .vidioc_s_parm = uvc_v4l2_s_parm, > + .vidioc_g_parm = uvc_v4l2_g_parm, > .vidioc_subscribe_event = uvc_v4l2_subscribe_event, > .vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event, > .vidioc_default = uvc_v4l2_ioctl_default,
On Tue, Aug 13, 2024 at 11:09:30AM +0200, Michael Grzeschik wrote: > The uvc gadget driver is calculating the req_size on every > video_enable/alloc_request and is based on the fixed configfs parameters > maxpacket, maxburst and mult. > > As those parameters can not be changed once the gadget is started and > the same calculation is done already early on the > vb2_streamon/queue_setup path its save to remove one extra calculation > and reuse the calculation from uvc_queue_setup for the allocation step. Avoiding double calculations is good, but then don't compute the value in uvc_queue_setup(). That will also be called multiple times, and its timing will be controlled by userspace. Move it to a better location. > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > v3 -> v4: - > v2 -> v3: - > v1 -> v2: - > --- > drivers/usb/gadget/function/uvc_queue.c | 2 ++ > drivers/usb/gadget/function/uvc_video.c | 15 ++------------- > 2 files changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > index 7995dd3fef184..2414d78b031f4 100644 > --- a/drivers/usb/gadget/function/uvc_queue.c > +++ b/drivers/usb/gadget/function/uvc_queue.c > @@ -63,6 +63,8 @@ static int uvc_queue_setup(struct vb2_queue *vq, > */ > nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size); > nreq = clamp(nreq, 4U, 64U); > + > + video->req_size = req_size; > video->uvc_num_requests = nreq; > > return 0; > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index 259920ae36843..a6786beef91ad 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -480,7 +480,6 @@ uvc_video_free_requests(struct uvc_video *video) > INIT_LIST_HEAD(&video->ureqs); > INIT_LIST_HEAD(&video->req_free); > INIT_LIST_HEAD(&video->req_ready); > - video->req_size = 0; > return 0; > } > > @@ -488,16 +487,9 @@ static int > uvc_video_alloc_requests(struct uvc_video *video) > { > struct uvc_request *ureq; > - unsigned int req_size; > unsigned int i; > int ret = -ENOMEM; > > - BUG_ON(video->req_size); > - > - req_size = video->ep->maxpacket > - * max_t(unsigned int, video->ep->maxburst, 1) > - * (video->ep->mult); > - > for (i = 0; i < video->uvc_num_requests; i++) { > ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); > if (ureq == NULL) > @@ -507,7 +499,7 @@ uvc_video_alloc_requests(struct uvc_video *video) > > list_add_tail(&ureq->list, &video->ureqs); > > - ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); > + ureq->req_buffer = kmalloc(video->req_size, GFP_KERNEL); > if (ureq->req_buffer == NULL) > goto error; > > @@ -525,12 +517,10 @@ uvc_video_alloc_requests(struct uvc_video *video) > list_add_tail(&ureq->req->list, &video->req_free); > /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ > sg_alloc_table(&ureq->sgt, > - DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, > + DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN, > PAGE_SIZE) + 2, GFP_KERNEL); > } > > - video->req_size = req_size; > - > return 0; > > error: > @@ -663,7 +653,6 @@ uvcg_video_disable(struct uvc_video *video) > INIT_LIST_HEAD(&video->ureqs); > INIT_LIST_HEAD(&video->req_free); > INIT_LIST_HEAD(&video->req_ready); > - video->req_size = 0; > spin_unlock_irqrestore(&video->req_lock, flags); > > /*
On Tue, Aug 13, 2024 at 11:09:24AM +0200, Michael Grzeschik wrote: > This patch series is improving the size calculation and allocation of > the uvc requests. Using the selected frame duration of the stream it is > possible to calculate the number of requests based on the interval > length. > > It also precalculates the request length based on the actual per frame > size for compressed formats. > > For this calculations to work it was needed to rework the request > queueing by moving the encoding to one extra thread (in this case we > chose the qbuf) context. > > Next it was needed to move the actual request enqueueing to one extra > thread which is kept busy to fill the isoc queue in the udc. > > As a final step the series is increasing the minimum amount of > v4l2 buffers to 4 and allocates at least the amount of usb_requests > to store them in the usb gadgte isoc pipeline. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > Changes in v4: > - fixed exit path in uvc_enqueue_buffer on loop break > - Link to v3: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v3-0-4da7033dd488@pengutronix.de I just took v3 in my tree, should I drop them? thanks, greg k-h
On Tue, Aug 13, 2024 at 12:28:20PM +0300, Laurent Pinchart wrote: >On Tue, Aug 13, 2024 at 12:22:55PM +0300, Laurent Pinchart wrote: >> On Tue, Aug 13, 2024 at 11:09:31AM +0200, Michael Grzeschik wrote: >> > The uvc gadget driver is lacking the information which frame interval >> > was set by the host. We add this information by implementing the g_parm >> > and s_parm callbacks. >> >> As I've said countless times, this kind of hack is not the right way to >> pass information that the kernel has no use for between two userspace >> components. Please stop butchering this driver. > >Reading further patches in the series I see that you would like to get >more precise bandwidth information from userspace. That is fine, but I >don't think s_parm is the right option. This is not a regular V4L2 >driver, pass it the exat information it needs instead, through a >dedicated API that will provide all the needed data. We have an API, where we can handover the framerate. Why invent something new. All we need to fix is also patch you uvc-gadget implementation. What other API do you suggest then? Come up with something super new and special, only dedicated for UVC then? Regards, Michael >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> > --- >> > v3 -> v4: - >> > v2 -> v3: - >> > v1 -> v2: - >> > --- >> > drivers/usb/gadget/function/uvc.h | 1 + >> > drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++ >> > 2 files changed, 53 insertions(+) >> > >> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h >> > index b3a5165ac70ec..f6bc58fb02b84 100644 >> > --- a/drivers/usb/gadget/function/uvc.h >> > +++ b/drivers/usb/gadget/function/uvc.h >> > @@ -100,6 +100,7 @@ struct uvc_video { >> > unsigned int width; >> > unsigned int height; >> > unsigned int imagesize; >> > + unsigned int interval; >> > struct mutex mutex; /* protects frame parameters */ >> > >> > unsigned int uvc_num_requests; >> > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c >> > index de41519ce9aa0..392fb400aad14 100644 >> > --- a/drivers/usb/gadget/function/uvc_v4l2.c >> > +++ b/drivers/usb/gadget/function/uvc_v4l2.c >> > @@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt) >> > return ret; >> > } >> > >> > +static int uvc_v4l2_g_parm(struct file *file, void *fh, >> > + struct v4l2_streamparm *parm) >> > +{ >> > + struct video_device *vdev = video_devdata(file); >> > + struct uvc_device *uvc = video_get_drvdata(vdev); >> > + struct uvc_video *video = &uvc->video; >> > + struct v4l2_fract timeperframe; >> > + >> > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) >> > + return -EINVAL; >> > + >> > + /* Return the actual frame period. */ >> > + timeperframe.numerator = video->interval; >> > + timeperframe.denominator = 10000000; >> > + v4l2_simplify_fraction(&timeperframe.numerator, >> > + &timeperframe.denominator, 8, 333); >> > + >> > + uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n", >> > + timeperframe.numerator, timeperframe.denominator, >> > + video->interval); >> > + >> > + parm->parm.output.timeperframe = timeperframe; >> > + parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME; >> > + >> > + return 0; >> > +} >> > + >> > +static int uvc_v4l2_s_parm(struct file *file, void *fh, >> > + struct v4l2_streamparm *parm) >> > +{ >> > + struct video_device *vdev = video_devdata(file); >> > + struct uvc_device *uvc = video_get_drvdata(vdev); >> > + struct uvc_video *video = &uvc->video; >> > + struct v4l2_fract timeperframe; >> > + >> > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) >> > + return -EINVAL; >> > + >> > + timeperframe = parm->parm.output.timeperframe; >> > + >> > + video->interval = v4l2_fraction_to_interval(timeperframe.numerator, >> > + timeperframe.denominator); >> > + >> > + uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n", >> > + timeperframe.numerator, timeperframe.denominator, >> > + video->interval); >> > + >> > + return 0; >> > +} >> > + >> > static int >> > uvc_v4l2_enum_frameintervals(struct file *file, void *fh, >> > struct v4l2_frmivalenum *fival) >> > @@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = { >> > .vidioc_dqbuf = uvc_v4l2_dqbuf, >> > .vidioc_streamon = uvc_v4l2_streamon, >> > .vidioc_streamoff = uvc_v4l2_streamoff, >> > + .vidioc_s_parm = uvc_v4l2_s_parm, >> > + .vidioc_g_parm = uvc_v4l2_g_parm, >> > .vidioc_subscribe_event = uvc_v4l2_subscribe_event, >> > .vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event, >> > .vidioc_default = uvc_v4l2_ioctl_default, > >-- >Regards, > >Laurent Pinchart >
This patch series is improving the size calculation and allocation of the uvc requests. Using the selected frame duration of the stream it is possible to calculate the number of requests based on the interval length. It also precalculates the request length based on the actual per frame size for compressed formats. For this calculations to work it was needed to rework the request queueing by moving the encoding to one extra thread (in this case we chose the qbuf) context. Next it was needed to move the actual request enqueueing to one extra thread which is kept busy to fill the isoc queue in the udc. As a final step the series is increasing the minimum amount of v4l2 buffers to 4 and allocates at least the amount of usb_requests to store them in the usb gadgte isoc pipeline. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- Changes in v4: - fixed exit path in uvc_enqueue_buffer on loop break - Link to v3: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v3-0-4da7033dd488@pengutronix.de Changes in v3: - Added more patches necessary to properly rework the request queueing - Link to v2: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v2-0-12690f7a2eff@pengutronix.de Changes in v2: - added header size into calculation of request size - Link to v1: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v1-0-9436c4716233@pengutronix.de --- Michael Grzeschik (10): usb: gadget: uvc: always set interrupt on zero length requests usb: gadget: uvc: only enqueue zero length requests in potential underrun usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf usb: gadget: uvc: rework to enqueue in pump worker from encoded queue usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests usb: gadget: uvc: set req_size once when the vb2 queue is calculated usb: gadget: uvc: add g_parm and s_parm for frame interval usb: gadget: uvc: set req_size and n_requests based on the frame interval usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4 drivers/usb/gadget/function/f_uvc.c | 3 +- drivers/usb/gadget/function/uvc.h | 14 +- drivers/usb/gadget/function/uvc_queue.c | 52 +++++-- drivers/usb/gadget/function/uvc_queue.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 67 ++++++++- drivers/usb/gadget/function/uvc_video.c | 243 +++++++++++++------------------- drivers/usb/gadget/function/uvc_video.h | 1 + 7 files changed, 221 insertions(+), 160 deletions(-) --- base-commit: 38343be0bf9a7d7ef0d160da5f2db887a0e29b62 change-id: 20240403-uvc_request_length_by_interval-a7efd587d963 Best regards,