Message-ID: <6b0421e0-8c5a-409b-8f34-3a041c838748@google.com>
Date: Tue, 4 Jun 2024 15:32:15 -0700
From: Avichal Rakesh <arakesh@...gle.com>
To: Michael Grzeschik <mgr@...gutronix.de>
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 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.

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


- Avi.

Powered by blists - more mailing lists