| Message ID | 20221003101627.144026-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete() | expand |
On Mon, Oct 03, 2022 at 11:16:27AM +0100, Daniel Scally wrote: >Calling uvcg_complete_buffer() from uvc_video_encode_isoc() sometimes >causes the final isoc packet for a video frame to be delayed long >enough to cause the USB controller to drop it. The first isoc packet >of the next video frame is then received by the host, which interprets >the toggled FID bit correctly such that the stream continues without >interruption, but the first frame will be missing the last isoc >packet's worth of bytes. > >To fix the issue delay the call to uvcg_complete_buffer() until the >usb_request's .complete() callback, as already happens when the data >is encoded via uvc_video_encode_isoc_sg(). > >Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >--- > drivers/usb/gadget/function/uvc_video.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >index c00ce0e91f5d..041819a655ed 100644 >--- a/drivers/usb/gadget/function/uvc_video.c >+++ b/drivers/usb/gadget/function/uvc_video.c >@@ -194,6 +194,7 @@ static void > uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > struct uvc_buffer *buf) > { >+ struct uvc_request *ureq = req->context; > void *mem = req->buf; > int len = video->req_size; > int ret; >@@ -213,7 +214,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > video->queue.buf_used = 0; > buf->state = UVC_BUF_STATE_DONE; > list_del(&buf->queue); >- uvcg_complete_buffer(&video->queue, buf); >+ ureq->last_buf = buf; > video->fid ^= UVC_STREAM_FID; > } > } >-- >2.34.1 Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Hi Dan, Looks like I did a superset of your change here: https://lore.kernel.org/all/20220926195307.110121-2-w36195@motorola.com/ I also included the uvc_video_encode_bulk() for consistency, even though it seems to be unused code. Out of curiosity, which setup did you test this on? I'm seeing issues on my devices with the dwc3 controller with some of the recent performance improvements (scatter/gather support and no_interrupt use). I've tried to include all relevant changes in my setup, but the issues are still present. Any input is appreciated, Dan On Mon, Oct 03, 2022 at 11:16:27AM +0100, Daniel Scally wrote: > Calling uvcg_complete_buffer() from uvc_video_encode_isoc() sometimes > causes the final isoc packet for a video frame to be delayed long > enough to cause the USB controller to drop it. The first isoc packet > of the next video frame is then received by the host, which interprets > the toggled FID bit correctly such that the stream continues without > interruption, but the first frame will be missing the last isoc > packet's worth of bytes. > > To fix the issue delay the call to uvcg_complete_buffer() until the > usb_request's .complete() callback, as already happens when the data > is encoded via uvc_video_encode_isoc_sg(). > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_video.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index c00ce0e91f5d..041819a655ed 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -194,6 +194,7 @@ static void > uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > struct uvc_buffer *buf) > { > + struct uvc_request *ureq = req->context; > void *mem = req->buf; > int len = video->req_size; > int ret; > @@ -213,7 +214,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > video->queue.buf_used = 0; > buf->state = UVC_BUF_STATE_DONE; > list_del(&buf->queue); > - uvcg_complete_buffer(&video->queue, buf); > + ureq->last_buf = buf; > video->fid ^= UVC_STREAM_FID; > } > } > -- > 2.34.1 >
Hi Dan On 03/10/2022 16:26, Dan Vacura wrote: > Hi Dan, > > Looks like I did a superset of your change here: > https://lore.kernel.org/all/20220926195307.110121-2-w36195@motorola.com/ Ah-ha nice, thanks for the heads up - I'll take a better look tomorrow, but yours is definitely more comprehensive :) > > I also included the uvc_video_encode_bulk() for consistency, even though > it seems to be unused code. > > Out of curiosity, which setup did you test this on? I'm seeing issues on > my devices with the dwc3 controller with some of the recent performance > improvements (scatter/gather support and no_interrupt use). I've tried > to include all relevant changes in my setup, but the issues are still > present. I'm testing with an imx8mp evaluation kit and a debix model A board (also with the imx8mp soc) as the gadget, both with a dwc3 controller. We also experience issues when it's using the scatter/gather API, to the extent that I added module parameters to set the support_sg variable false and exclude it. When using the sg code either the host or gadget would report a lot of missed isoc errors (manifesting on the host as uvcvideo reporting "USB isochronous frame lost" or on the gadget side as f_usb_uvc reporting "VS request completed with status -18") and no complete streams made it through to the host - particularly with streaming_maxpacket > 1024. Forcing the use of uvc_video_encode_isoc() instead resolved the majority of those problems. I did also try an rpi4 (dwc2) as the gadget and didn't hit those problems with that board. The problem this patch was solving was quite a bit different though, I hadn't considered them to be related. > > Any input is appreciated, > > Dan > > > On Mon, Oct 03, 2022 at 11:16:27AM +0100, Daniel Scally wrote: >> Calling uvcg_complete_buffer() from uvc_video_encode_isoc() sometimes >> causes the final isoc packet for a video frame to be delayed long >> enough to cause the USB controller to drop it. The first isoc packet >> of the next video frame is then received by the host, which interprets >> the toggled FID bit correctly such that the stream continues without >> interruption, but the first frame will be missing the last isoc >> packet's worth of bytes. >> >> To fix the issue delay the call to uvcg_complete_buffer() until the >> usb_request's .complete() callback, as already happens when the data >> is encoded via uvc_video_encode_isoc_sg(). >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> drivers/usb/gadget/function/uvc_video.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >> index c00ce0e91f5d..041819a655ed 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -194,6 +194,7 @@ static void >> uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, >> struct uvc_buffer *buf) >> { >> + struct uvc_request *ureq = req->context; >> void *mem = req->buf; >> int len = video->req_size; >> int ret; >> @@ -213,7 +214,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, >> video->queue.buf_used = 0; >> buf->state = UVC_BUF_STATE_DONE; >> list_del(&buf->queue); >> - uvcg_complete_buffer(&video->queue, buf); >> + ureq->last_buf = buf; >> video->fid ^= UVC_STREAM_FID; >> } >> } >> -- >> 2.34.1 >>
On Mon, Oct 03, 2022 at 11:16:27AM +0100, Daniel Scally wrote: > Calling uvcg_complete_buffer() from uvc_video_encode_isoc() sometimes > causes the final isoc packet for a video frame to be delayed long > enough to cause the USB controller to drop it. The first isoc packet > of the next video frame is then received by the host, which interprets > the toggled FID bit correctly such that the stream continues without > interruption, but the first frame will be missing the last isoc > packet's worth of bytes. > > To fix the issue delay the call to uvcg_complete_buffer() until the > usb_request's .complete() callback, as already happens when the data > is encoded via uvc_video_encode_isoc_sg(). > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_video.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) What commit id does this fix? Should it go to stable kernels? thanks, greg k-h
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index c00ce0e91f5d..041819a655ed 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -194,6 +194,7 @@ static void uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, struct uvc_buffer *buf) { + struct uvc_request *ureq = req->context; void *mem = req->buf; int len = video->req_size; int ret; @@ -213,7 +214,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, video->queue.buf_used = 0; buf->state = UVC_BUF_STATE_DONE; list_del(&buf->queue); - uvcg_complete_buffer(&video->queue, buf); + ureq->last_buf = buf; video->fid ^= UVC_STREAM_FID; } }
Calling uvcg_complete_buffer() from uvc_video_encode_isoc() sometimes causes the final isoc packet for a video frame to be delayed long enough to cause the USB controller to drop it. The first isoc packet of the next video frame is then received by the host, which interprets the toggled FID bit correctly such that the stream continues without interruption, but the first frame will be missing the last isoc packet's worth of bytes. To fix the issue delay the call to uvcg_complete_buffer() until the usb_request's .complete() callback, as already happens when the data is encoded via uvc_video_encode_isoc_sg(). Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- drivers/usb/gadget/function/uvc_video.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)