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

[PATCH] 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().

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

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

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>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dan Vacura]

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
> 

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

[Dan Scally]

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

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

[Greg KH]

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