Messages in this thread | | | Date | Tue, 2 Apr 2024 09:45:42 +0200 | From | Michael Grzeschik <> | Subject | Re: [PATCH] usb: gadget: uvc: Improve error checking and tagging |
| |
On Tue, Mar 26, 2024 at 10:32:48AM +0100, Greg Kroah-Hartman wrote: >On Mon, Mar 25, 2024 at 12:32:30AM +0100, Michael Grzeschik wrote: >> Right now after one transfer was completed with EXDEV the currently >> encoded frame will get the UVC_STREAM_ERR tag attached. Since the >> complete and encode path are handling separate requests from different >> threads, there is no direct correspondence between the missed transfer >> of one request and the currently encoded request which might already >> belong to an completely different frame. >> >> When queueing requests into the hardware by calling ep_queue the >> underlying ringbuffer of the usb driver will be filled. However when >> one of these requests will have some issue while transfer the hardware >> will trigger an interrupt but will continue transferring the pending >> requests in the ringbuffer. This interrupt-latency will make it >> impossible to react in time to tag the fully enqueued frame with the >> UVC_STREAM_ERR in the header. >> >> This patch is also addressing this particular issue by delaying the >> transmit of the EOF/ERR tagged header by waiting for the last enqueued >> buffer of the frame to be completed. This way it is possible to react to >> send the EOF/ERR tag depending on the whole frame transfer status. >> >> As this is patch is adding latency to the enqueuing path of the frames >> we make this errorcheck optional by adding an extra module parameter. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/usb/gadget/function/f_uvc.c | 4 ++ >> drivers/usb/gadget/function/uvc.h | 3 ++ >> drivers/usb/gadget/function/uvc_video.c | 69 +++++++++++++++++++++++++++++---- >> 3 files changed, 68 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >> index 929666805bd23..6a7ca8ccaf360 100644 >> --- a/drivers/usb/gadget/function/f_uvc.c >> +++ b/drivers/usb/gadget/function/f_uvc.c >> @@ -33,6 +33,10 @@ unsigned int uvc_gadget_trace_param; >> module_param_named(trace, uvc_gadget_trace_param, uint, 0644); >> MODULE_PARM_DESC(trace, "Trace level bitmask"); >> >> +bool uvc_gadget_errorcheck_param = true; >> +module_param_named(errorcheck, uvc_gadget_errorcheck_param, bool, 0644); >> +MODULE_PARM_DESC(errorcheck, "Check and mark errors in the transfer of a frame"); > >I really really really really hate adding new module parameters as they >do not scale nor work properly for multiple devices and really, it's not >the 1990's anymore. > >Any way to make this debugging thing part of a debugfs interface or >worst case, a sysfs entry instead? > >Or why not just make it a tracing thing instead? > >But wait, you are fixing a real issue here, why is it an option at all? >Shouldn't this always be the case and the driver should always recover >in this way? Why would you not want this to be the default and only way >it operates?
Yes and no. I added the thange to be optional, since it is changing the amount of frames we will be able to transfer at once. However I have another set of changes in the queue, that is also taking the expected size and fps into the account of requst_size and therefor the real amount of bandwidth. That will indirectly will have the same result.
So this makes the option obsolete anyway. I will remove it in v2.
Thanks, Michael
-- 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 | [unhandled content-type:application/pgp-signature] | |