[PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[Daniel Scally]

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;
 	}
 }
-- 
2.34.1

Re: [PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[Dan Scally]

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;
>   	}
>   }

Re: [PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[Greg KH]

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

Re: [PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[Dan Scally]

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

Re: [PATCH v2] uvc: gadget: uvc: Defer uvcg_complete_buffer() until .complete()

[Dan Scally]

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