Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To:     Michael Grzeschik <mgr@pengutronix.de>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     Alan Stern <stern@rowland.harvard.edu>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Felipe Balbi <balbi@kernel.org>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc
 request is queued too late
Thread-Topic: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc
 request is queued too late
Thread-Index: AQHVmKSAwOKFEHeaw0mottJUNN0pEqeIevUAgABCiACAAII+gIAAPESAgAEc0ACAAIVuAIDmOjMAgAEfnQA=
Date:   Fri, 10 Apr 2020 01:09:23 +0000
Message-ID: <431d2faa-b073-287c-62ec-557d7d021c48@synopsys.com>
References: <Pine.LNX.4.44L0.1911131036340.1558-100000@iolanthe.rowland.org>
 <587b0adf-b71d-6fde-407b-46089ed5d695@synopsys.com>
 <20191114121422.qtvyom6nytzwoy2e@pengutronix.de>
 <6d4b87c8-5aca-18cb-81db-a8d2fd4bd86e@synopsys.com>
 <20200409075958.GA19563@pengutronix.de>
In-Reply-To: <20200409075958.GA19563@pengutronix.de>
Accept-Language: en-US
Content-Language: en-US
user-agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
received-spf: None (protection.outlook.com: synopsys.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="iso-8859-15"
Content-ID: <84B48088BF64E44CAFB3371691F74C8A@namprd12.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 50ce774a-bedf-42e5-1ff3-08d7dcebce50
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Apr 2020 01:09:23.1038
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Ewz2Hc4huWacBwyH1Qoz3dA69wzNH23tUyNAtX9Sn7TKV9vRuqxWbs74WvkZ8sT2gFDlzpdFd0inCpaluZF6IQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3526
X-OriginatorOrg: synopsys.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

Michael Grzeschik wrote:
> On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote:
>> 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=20
>>>>>>>> effort
>>>>>>>> bases: If the request queue runs empty, then there will simply=20
>>>>>>>> be gaps in
>>>>>>>> the isoc data stream.
>>>>>>>>
>>>>>>>> The UVC gadget depends on this behaviour. It simply provides=20
>>>>>>>> new requests
>>>>>>>> when video frames are available and assumes that they are sent=20
>>>>>>>> as soon as
>>>>>>>> possible.
>>>>>>>>
>>>>>>>> The dwc3 gadget currently works differently: It assumes that=20
>>>>>>>> there is a
>>>>>>>> contiguous stream of requests without any gaps. If a request is=20
>>>>>>>> too late,
>>>>>>>> then it is dropped by the hardware.
>>>>>>>> For the UVC gadget this means that a live stream stops after=20
>>>>>>>> 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=20
>>>>>>> will result
>>>>>>> in missed_isoc status, and it should be fine as long as new=20
>>>>>>> requests are
>>>>>>> queued. The controller doesn't just drop the request unless=20
>>>>>>> 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=20
>>>>>> frame
>>>>>> fills 3 requests. Because it is a live stream, there will be a=20
>>>>>> gap between
>>>>>> video frames. This is unavoidable, especially for compressed=20
>>>>>> video. So the
>>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9=20
>>>>>> 10 11 13 14
>>>>>> 15 and so on.
>>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4=20
>>>>>> 5 6 7 8 9
>>>>>> 10 11 12. So except for the fist few requests, all are late and=20
>>>>>> result in a
>>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did=20
>>>>>> 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
>>>> =A0 =A0=A0=A0 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=20
>>> 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.
>
> I read in some Processor documentation that in case the host tries to
> fetch data from the client and no active TRB (HWO=3D1) is available the
> XferInProgress Interrupt will be produced, with the missed status set.
> This is done because the hardware will produce zero length packets
> on its own, to keep the stream running.

The controller only generates XferInProgress if it had processed a TRB=20
(with specific control bits). For IN direction, if the controller is=20
starved of TRB, it will send a ZLP if the host requests for data.


>
>>>> For #2, it's unlikely that DSTS current frame number will match=20
>>>> with the
>>>> XferNotReady's frame number. So this check doesn't mean much.
>>> The frame number is also updated for each "Transfer In Progress"=20
>>> interrupt.
>>> If they match, then there a new request can still be queued=20
>>> 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=20
>>> 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.
>
> In the empty trb case, after the Hardware has send enough zero packets=20
> this
> active transfer has to be stopped with endtransfer cmd. Because every=20
> next
> update transfer on that active transfer will likely lead to further=20
> missed
> transfers, as the newly updated trb will be handled to late anyway.

The controller is expecting the function driver to feed TRBs to the=20
controller for every interval. If it's late, then the controller will=20
consider that data "missed_isoc".

In your case, the UVC driver seems to queue requests to the controller=20
driver as if it is bulk requests, and the UVC expects those data to go=20
out at the time it queues. To achieve what UVC needs, then you may want=20
to issue END_TRANSFER command before the next burst of data. This way,=20
the controller can restart the isoc endpoint and not consider the next=20
video frame data late. There are some corner cases that you need to=20
watch out for. If you're going for this route, we can look further.

Also, you'd need provide a way for the UVC to communicate to the dwc3 to=20
let it know to expect the next burst of data.

>
>
> The odd thing here is, that I don't see the refered XferInProgress
> Interrupts with the missed event, when the started_list is empty.

See my first comment.

>
> But this would be the only case to fall into this condition and handle it
> properly. Like alredy assumed in the following code:
>
> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep=20
> *dep,
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 const struct dwc3_event_depevt *even=
t)
> {
> ...
>
> =A0=A0=A0=A0=A0=A0 if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 status =3D -EXDEV;
>
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (list_empty(&dep->started_l=
ist))
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 stop =
=3D true;
> =A0=A0=A0=A0=A0=A0 }
>
> ...
>
> =A0=A0=A0=A0=A0=A0 if (stop)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 dwc3_stop_active_transfer(dep,=
 true, true);
> ...
> }
>
> In fact I did sometimes see these XferInProgress Interrupts on empty=20
> trb queue
> after I stoped the tansfer when the started_list was empty right after
> ep_cleanup_completed_requests has moved all trbs out of the queue.
>
> These Interrupts appeared right after the ENDTRANSFER cmd was send.=20
> (But I
> could no verify this every time)

If END_TRANSFER command completes, then you should not see=20
XferInProgress event. The next event should ber XferNotReady.

>
> Anyways in that case these Interrupts are not useful anymore, as I=20
> already
> implied the same stop, with ENDTRANSFER after we know that there are=20
> no other
> trbs in the chain.
>

Just curious, do you know if there's a reason for UVC to behave this=20
way? Seems like what it's trying to do is more for bulk. Maybe it wants=20
bandwidth priority perhaps?

BR,
Thinh