Message-ID: <ZmdztaLmv5YWl3c1@pengutronix.de>
Date: Mon, 10 Jun 2024 23:44:21 +0200
From: Michael Grzeschik <mgr@...gutronix.de>
To: Avichal Rakesh <arakesh@...gle.com>
Cc: Alan Stern <stern@...land.harvard.edu>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Daniel Scally <dan.scally@...asonboard.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jayant Chowdhary <jchowdhary@...gle.com>,
	"etalvala@...gle.com" <etalvala@...gle.com>,
	Michael Riesch <michael.riesch@...fvision.net>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Subject: Re: [PATCH 0/3] usb: gadget: uvc: allocate requests based on frame
 interval length and buffersize

On Tue, Jun 04, 2024 at 03:32:15PM -0700, Avichal Rakesh wrote:
>
>
>On 5/29/24 14:24, Michael Grzeschik wrote:
>> On Tue, May 28, 2024 at 05:33:46PM -0700, Avichal Rakesh wrote:
>>>
>>>
>>> On 5/28/24 15:43, Michael Grzeschik wrote:
>>>> On Tue, May 28, 2024 at 02:27:34PM -0700, Avichal Rakesh wrote:
>>>>>
>>>>>
>>>>> On 5/28/24 13:22, Michael Grzeschik wrote:
>>>>>> On Tue, May 28, 2024 at 10:30:30AM -0700, Avichal Rakesh wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/22/24 10:37, Michael Grzeschik wrote:
>>>>>>>> On Wed, May 22, 2024 at 05:17:02PM +0000, Thinh Nguyen wrote:
>>>>>> One option to be totally sure would be to resend the sentinel request to
>>>>>> be properly transmitted before starting the next frame. This resend
>>>>>> polling would probably include some extra zero-length requests. But also
>>>>>> if this resend keeps failing for n times, the driver should doubt there
>>>>>> is anything sane going on with the USB connection and bail out somehow.
>>>>>>
>>>>>> Since we try to tackle case (1) to avoid transmit errors and also avoid
>>>>>> creating late enqueued requests in the running isoc transfer, the over
>>>>>> all chance to trigger missed transfers should be minimal.
>>>>>
>>>>> Gotcha. It seems like the UVC gadget driver implicitly assumes that EOF
>>>>> flag will be used although the userspace application can technically
>>>>> make it optional.
>>>>
>>>> That is not all. The additional UVC_STREAM_ERR tag on the sentinel
>>>> request can be set optional by the host driver. But by spec the
>>>> userspace application has to drop the frame when the flag was set.
>>>
>>> Looking at the UVC specs, the ERR bit doesn't seem to refer to actual
>>> transmission error, only errors in frame generation (Section 4.3.1.7
>>> of UVC 1.5 Class Specification). Maybe "data discontinuity" can be
>>> used but the examples given are bad media, and encoder issues, which
>>> suggests errors at higher level than the wire.
>>
>> Oh! That is a new perspective I did not consider.
>>
>> With the definition of UVC_STREAM_ERR by spec, the uvc_video driver
>> would in no case set this header bit for the current frame on its own?
>> Is that correct?
>
>It would indeed seem so. The way gadget driver is architected makes
>is impossible for the userspace application to notify the host of
>any errors.
>
>>
>>>> With my proposal this flag will be set, whenever we find out that
>>>> the currently transferred frame was erroneous.
>>>>
>>>>> Summarizing some of the discussions above:
>>>>> 1. UVC gadget driver should _not_ rely on the usb controller to
>>>>>   enqueue 0-length requests on UVC gadget drivers behalf;
>>>>> 2. However keeping up the backpressure to the controller means the
>>>>>   EOF request will be delayed behind all the zero-length requests.
>>>>
>>>> Exactly, this is why we have to somehow finetune the timedelay between
>>>> requests that trigger interrupts. And also monitor the amount of
>>>> requests currently enqueued in the hw ringbuffer. So that our drivers
>>>> enqueue dequeue mechanism is virtually adding only the minimum amount
>>>> of necessary zero-length requests in the hardware. This should be
>>>> possible.
>>>>
>>>> I am currently thinking through the remaining steps the pump worker has
>>>> to do on each wakeup to maintain the minimum threshold while waiting
>>>> with submitting requests that contain actual image payload.
>>>>
>>>>> Out of curiosity: What is wrong with letting the host rely on
>>>>> FID alone? Decoding the jpeg payload _should_ fail if any of the
>>>>> usb_requests containing the payload failed to transmit.
>>>>
>>>> This is not totally true. We saw partially rendered jpeg frames on the
>>>> host stream. How the host behaves with broken data is totally undefined
>>>> if the typical uvc flags EOF/ERR are not used as specified. Then think
>>>> about uncompressed formats. So relying on the transferred image format
>>>> to solve our problems is just as wrong as relying on the gadgets
>>>> hardware behavior.
>>>
>>> Do you know if the partially rendered frames were valid JPEGs, or
>>> if the host was simply making a best effort at displaying a broken
>>> JPEG? Perhaps the fix should go to the host instead?
>>
>> I can fully reproduce this with linux and windows hosts. For linux
>> machines I saw that the host was taking the FID change as a marker
>> to see the previous frame as ready and just rendered what got through.
>> This did not lead to garbage but only to partially displayed frames
>> with jpeg macroblock alignment.
>
>I was aware of linux doing so, but I only ever saw this behavior on
>Windows if there were a lot of invalid frames back to back.
>
>I am not super familiar with the guarantees of JPEG, but I suppose
>it is possible to have a "valid" JPEG with some middle blocks
>missing as long the EOI bits make it through? I am not sure how we
>go about solving that.

It is even worse. Since we don't necessary need the EOF tag set but the
host will draw the content that it got after the FID has changed. It is
always possible that an frame that was errornous and therefor dropped
on the sendin side, will be shown on the host to the last macroblock it
received. So these partially drawn frames are more common then expected.

>>> Following is my opinion, feel free to disagree (and correct me if
>>> something is factually incorrect):
>>>
>>> The fundamental issue here is that ISOC doesn't guarantee
>>> delivery of usb_requests or even basic data consistency upon delivery.
>>> So the gadget driver has no way to know the state of transmitted data.
>>> The gadget driver is notified of underruns but not of any other issues,
>>> and ideally we should never have an underrun if the zero-length
>>> backpressure is working as intended.
>>>
>>> So, UVC gadget driver can reduce the number of errors, but it'll never be
>>> able to guarantee that the data transmitted to the host isn't somehow
>>> corrupted or missing unless a more reliable mode of transmission
>>> (bulk, for example) is used.
>>>
>>> All of this to say: The host absolutely needs to be able to handle
>>> all sorts of invalid and broken payloads. How the host handles it
>>> might be undefined, but the host can never rely on perfect knowledge
>>> about the transmission state. In cases like these, where the underlying
>>> transport is unreliable, the burden of enforcing consistency moves up
>>> a layer, i.e. to the encoded payload in this case. So it is perfectly
>>> fine for the host to rely on the encoding to determine if the payload
>>> is corrupt and handle it accordingly.
>>
>> Right.
>>
>>> As for uncompressed format, you're correct that subtle corruptions
>>> may not be caught, but outright missing usb_requests can be easily
>>> checked by simply looking at the number of bytes in the payload. YUV
>>> frames are all of the same (predetermined) size for a given resolution.
>>
>> That was also my thought about five minutes after I did send you the
>> previous mail. So sure, this is no real issue for the host.
>>
>>> So my recommendation is the following:
>>> 1. Fix the bandwidth problem by splitting the encoded video frame
>>>   into more usb_requests (as your patch already does) making sure
>>>   there are enough free usb_request to encode the video frame in
>>>   one burst so we don't accidentally inflate the transmission
>>>   duration of a video frame by sneaking in zero-length requests in
>>>   the middle.
>>
>> Ack. This should already solve a lot of issues.
>>
>> For this I would still suggest to move the usb_ep_queue to be done in
>> the pump worker again. Its a bit back and forth, but IMHO its worth the
>> extra mile since only this way we would respect the dwc3 interrupt
>> threads assumption to run *very* short.
>
>The main reason for queuing the requests from the complete handler
>was to have a single point of usb_ep_queue call, which made reasoning
>through the locking simpler. But if you find a way to do so from
>the video_pump thread without making the locking a nightmare, then go
>for it!
>
>>
>>> 2. Unless there is an unusually high rate of transmission failures
>>>   when using the UVC gadget driver, it might be worth fixing the
>>>   host side driver to handle broken frames better instead (assuming
>>>   host is linux as well).
>>
>> Agreed, but this needs a separate scoped undestanding of the host side
>> behaviour over all layers.
>
>Agreed!
>
>>
>>> 2. Tighten up the error checking in UVC gadget driver -- We drop the
>>>   current frame whenever an EXDEV happens which is wrong. We should
>>>   only be dropping the current frame if the EXDEV corresponds to the
>>>   frame currently being encoded.
>>
>> What do you mean by drop?
>>
>> I would suggest to immediatly switch the uvc_buffer that is being
>> enqueued and start queueing prepared requests from the next buffers prep
>> list. As suggested, the idea is to have per uvc_buffer prep_list
>> requests which would make this task easy.
>
>Currently, if uvc gadget driver receives an EXDEV complete callback
>all it does is set the UVC_QUEUE_DROP_INCOMPLETE flag.
>
>So let's say that we receive an EXDEV for a usb_request containing data
>for video frame N. With how video_pump is currently configured, chances
>are that all usb_requests containing data for video frame N has already
>been queued to the controller.
>
>When the next video frame (N+1) comes in, video_pump's encode methods
>will look at the UVC_QUEUE_DROP_INCOMPLETE flag and incorrectly
>determine that "current" frame needs to be dropped, and stop encoding
>video frame N+1 even though the error was for video frame N. So the
>encode methods incorrectly drop video frame N+1 which isn't needed.
>
>The encode methods should only be dropping the video frame if we
>received an EXDEV for a usb_request for the video frame currently
>being encoded.
>
>I hope that makes sense!

This totally makes sense. I just wanted to make sure that this does not
involve any UVC_STREAM_ERR tagging from your understanding.

I totally agree with this concept. So now we "only" have to implement
this. :)

First I will review and update my patches that will increase the amount
of requests per frame.

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists