| Message ID | 20221019124535.2712902-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete() | expand |
Hi All
I forgot to add a Fixes tag to this at the time, but I think that this
patch:
Fixes: cdda479f15cd ("USB gadget: video class function driver")
The problem it's fixing has as far as I can tell been around since then,
though Michael did patch the uvc_video_encode_isoc_sg() function much
more recently in 9b969f93bcef ("usb: gadget: uvc: giveback vb2 buffer on
req complete"), and this patch relies on the change in Michael's.
On 19/10/2022 13:45, 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(). For consistency's sake the
> same change is applied to uvc_video_encode_bulk().
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>
> Changes in v2:
>
> - Applied the same change to uvc_video_encode_bulk() for consistency
>
> @Dan - In the end I thought this is probably worth separating from your "usb:
> gadget: uvc: fix sg handling in error case" patch, since it fixes a separate
> issue by itself. I _think_ they're separable but I wasn't experiencing the
> problem you were so I can't test that - let me know if I'm wrong.
>
> @Michael - I dropped your R-b since I made the change to uvc_video_encode_bulk()
> too, didn't want to jump the gun :)
>
> drivers/usb/gadget/function/uvc_video.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index c00ce0e91f5d..42bd4dd1d4a9 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -87,6 +87,7 @@ static void
> uvc_video_encode_bulk(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;
> @@ -113,7 +114,7 @@ uvc_video_encode_bulk(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;
>
> video->payload_size = 0;
> @@ -194,6 +195,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 +215,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;
> }
> }
On Wed, Oct 19, 2022 at 01:45:35PM +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(). For consistency's sake the > same change is applied to uvc_video_encode_bulk(). > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > > Changes in v2: > > - Applied the same change to uvc_video_encode_bulk() for consistency > > @Dan - In the end I thought this is probably worth separating from your "usb: > gadget: uvc: fix sg handling in error case" patch, since it fixes a separate > issue by itself. I _think_ they're separable but I wasn't experiencing the > problem you were so I can't test that - let me know if I'm wrong. > > @Michael - I dropped your R-b since I made the change to uvc_video_encode_bulk() > too, didn't want to jump the gun :) Does not apply to my tree anymore :( Can you rebase against the usb-linus branch of usb.git and resend? thanks, greg k-h
Hi Greg On 09/11/2022 10:46, Greg KH wrote: > On Wed, Oct 19, 2022 at 01:45:35PM +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(). For consistency's sake the >> same change is applied to uvc_video_encode_bulk(). >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> >> Changes in v2: >> >> - Applied the same change to uvc_video_encode_bulk() for consistency >> >> @Dan - In the end I thought this is probably worth separating from your "usb: >> gadget: uvc: fix sg handling in error case" patch, since it fixes a separate >> issue by itself. I _think_ they're separable but I wasn't experiencing the >> problem you were so I can't test that - let me know if I'm wrong. >> >> @Michael - I dropped your R-b since I made the change to uvc_video_encode_bulk() >> too, didn't want to jump the gun :) > > Does not apply to my tree anymore :( > > Can you rebase against the usb-linus branch of usb.git and resend? Sure thing > > thanks, > > greg k-h
Hi Greg On 09/11/2022 10:46, Greg KH wrote: > On Wed, Oct 19, 2022 at 01:45:35PM +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(). For consistency's sake the >> same change is applied to uvc_video_encode_bulk(). >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> >> Changes in v2: >> >> - Applied the same change to uvc_video_encode_bulk() for consistency >> >> @Dan - In the end I thought this is probably worth separating from your "usb: >> gadget: uvc: fix sg handling in error case" patch, since it fixes a separate >> issue by itself. I _think_ they're separable but I wasn't experiencing the >> problem you were so I can't test that - let me know if I'm wrong. >> >> @Michael - I dropped your R-b since I made the change to uvc_video_encode_bulk() >> too, didn't want to jump the gun :) > > Does not apply to my tree anymore :( > > Can you rebase against the usb-linus branch of usb.git and resend? Ah, you've got a patch in there already that is a superset of this change (The one mentioned in my @Dan above), so there's nothing to do; that's why it doesn't apply. > > thanks, > > greg k-h
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index c00ce0e91f5d..42bd4dd1d4a9 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -87,6 +87,7 @@ static void uvc_video_encode_bulk(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; @@ -113,7 +114,7 @@ uvc_video_encode_bulk(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; video->payload_size = 0; @@ -194,6 +195,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 +215,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(). For consistency's sake the same change is applied to uvc_video_encode_bulk(). Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- Changes in v2: - Applied the same change to uvc_video_encode_bulk() for consistency @Dan - In the end I thought this is probably worth separating from your "usb: gadget: uvc: fix sg handling in error case" patch, since it fixes a separate issue by itself. I _think_ they're separable but I wasn't experiencing the problem you were so I can't test that - let me know if I'm wrong. @Michael - I dropped your R-b since I made the change to uvc_video_encode_bulk() too, didn't want to jump the gun :) drivers/usb/gadget/function/uvc_video.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)