Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late

[Thinh Nguyen]

Michael Olbrich wrote:
> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote:
>> Alan Stern wrote:
>>> On Wed, 13 Nov 2019, Michael Olbrich wrote:
>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote:
>>>>> Michael Olbrich wrote:
>>>>>> Currently, most gadget drivers handle isoc transfers on a best effort
>>>>>> bases: If the request queue runs empty, then there will simply be gaps in
>>>>>> the isoc data stream.
>>>>>>
>>>>>> The UVC gadget depends on this behaviour. It simply provides new requests
>>>>>> when video frames are available and assumes that they are sent as soon as
>>>>>> possible.
>>>>>>
>>>>>> The dwc3 gadget currently works differently: It assumes that there is a
>>>>>> contiguous stream of requests without any gaps. If a request is too late,
>>>>>> then it is dropped by the hardware.
>>>>>> For the UVC gadget this means that a live stream stops after the first
>>>>>> frame because all following requests are late.
>>>>> Can you explain little more how UVC gadget fails?
>>>>> dwc3 controller expects a steady stream of data otherwise it will result
>>>>> in missed_isoc status, and it should be fine as long as new requests are
>>>>> queued. The controller doesn't just drop the request unless there's some
>>>>> other failure.
>>>> UVC (with a live stream) does not fill the complete bandwidth of an
>>>> isochronous endpoint. Let's assume for the example that one video frame
>>>> fills 3 requests. Because it is a live stream, there will be a gap between
>>>> video frames. This is unavoidable, especially for compressed video. So the
>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14
>>>> 15 and so on.
>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9
>>>> 10 11 12. So except for the fist few requests, all are late and result in a
>>>> missed_isoc. I tried to just ignore the missed_isoc but that did not work
>>>> for me. I only received the first frame at the other end.
>>>> Maybe I missing something here, i don't have access to the hardware
>>>> documentation, so I can only guess from the existing driver.
>> The reason I asked is because your patch doesn't seem to address the
>> actual issue.
>>
>> For the 2 checks you do here
>> 1. There are currently no requests queued in the hardware
>> 2. The current frame number provided by DSTS does not match the frame
>>   ÂÂÂ number returned by the last transfer.
>>
>> For #1, it's already done in the dwc3 driver. (check
>> dwc3_gadget_endpoint_transfer_in_progress())
> But that's only after a isoc_missed occurred. What exactly does that mean?
> Was the request transferred or not? My tests suggest that it was not
> transferred, so I wanted to catch this before it happens.

Missed_isoc status means that the controller did not move all the data 
in an interval.

>
>> For #2, it's unlikely that DSTS current frame number will match with the
>> XferNotReady's frame number. So this check doesn't mean much.
> The frame number is also updated for each "Transfer In Progress" interrupt.
> If they match, then there a new request can still be queued successfully.
> Without this I got unnecessary stop/start transfers in the middle of a
> video frame. But maybe something else was wrong here. I'd need to recheck.

The reason they may not match is 1) the frame_number is only updated 
after the software handles the XferInProgress interrupt. Depends on 
system latency, that value may not be updated at the time that we check 
the frame_number.
2) This check doesn't work if the service interval is greater than 1 
uframe. That is, it doesn't have to match exactly the time to be 
consider not late. Though, the second reason can easily be fixed.


>
>> So I'm still wondering how does this patch help resolve your issue.
> With this patch, the transfer is restarted for every video frame.
> Otherwise, I just get a lot of isoc_missed and ignoring those did not help.
> No valid data arrived after the first video frame.
>
>>> How about changing the gadget driver instead?  For frames where the UVC
>>> gadget knows no video frame data is available (numbers 4, 8, 12, and so
>>> on in the example above), queue a zero-length request.  Then there
>>> won't be any gaps in the isochronous packet stream.
>> What Alan suggests may work. Have you tried this?
> Yes and it works in general. There are however some problems with that
> approach that I want to avoid:
>
> 1. It adds extra overhead to handle the extra zero-length request.
> Especially for encoded video the available bandwidth can be quite a bit
> larger that what is actually used. I want to avoid that.
>
> 2. The UVC gadget currently does no know how many zero-length request must
> added. So it needs fill all available request until a new video frame
> arrives. With the current 4 requests that is not a problem right now. But
> that does not scale for USB3 bandwidths. So one thing that I want to do is
> to queue many requests but only enable the interrupt for a few of than.
>  From what I can tell from the code, the gadget framework and the dwc3
> driver should already support this.
> This will result in extra latency. There is probably an acceptable
> trade-off with an acceptable interrupt load and latency. But I would like
> to avoid that if possible.
>
> Regards,
> Michael


I think I understand the problem you're trying to solve now.

The dwc3 driver does not know that there's a gap until after a new 
request was queued, which then it will send an END_TRANSFER command and 
dequeue all the requests to restart the transfer due to missed_isoc.
We do this because the dwc3 driver does not know whether the new request 
is actually stale data, and we should not change this behavior.

Now, with UVC, it needs to communicate to the dwc3 driver that there 
will be a gap after a certain request (and that the device is expecting 
to send 0-length data). This is not a normal operation for isoc 
transfer. You may need to introduce a new way for the function driver to 
do that, possibly a new field in usb_request structure to indicate that. 
However, this seems a little awkward. Maybe others can comment on this.

BR,
Thinh