Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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+gIAAPESAgAEc0ACAAIVuAIDmOjMAgAEfnQCAAV5tAIAAMTkAgBAZiYCAARofAA==
Date:   Tue, 21 Apr 2020 23:41:05 +0000
Message-ID: <d87513ec-c429-7ec2-aac7-d54e83a596ab@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>
 <431d2faa-b073-287c-62ec-557d7d021c48@synopsys.com>
 <20200410220336.GB19563@pengutronix.de>
 <295ff41f-f287-e2c8-7c33-1c225e9b76b5@synopsys.com>
 <20200421065118.GB2338@pengutronix.de>
In-Reply-To: <20200421065118.GB2338@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.7.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: <79DAD694EFEC9441B492CEB0DB048629@namprd12.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e966679e-31b4-424b-c28c-08d7e64d757d
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Apr 2020 23:41:05.2961
 (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: XEzZxhC2r9uMzAvYSF/DZ9FHb/+gcZiqL7XsjAOJp2uNX2wcoiEKGfODD1C92EDaTyn4laZoT59npd/LkARI0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3271
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:
> Hi,
>
> On Sat, Apr 11, 2020 at 12:59:47AM +0000, Thinh Nguyen wrote:
>> Hi,
>>
>> Michael Grzeschik wrote:
>>> On Fri, Apr 10, 2020 at 01:09:23AM +0000, Thinh Nguyen wrote:
>>>> 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
>>>>>>>>>>>> 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=20
>>>>>>>>>>>> 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=20
>>>>>>>>>> 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=20
>>>>>>>>>> 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=20
>>>>>>>> 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=20
>>>>>>> that
>>>>>>> mean?
>>>>>>> Was the request transferred or not? My tests suggest that it was=20
>>>>>>> not
>>>>>>> transferred, so I wanted to catch this before it happens.
>>>>>>
>>>>>> Missed_isoc status means that the controller did not move all the=20
>>>>>> 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 t=
he
>>>>> XferInProgress Interrupt will be produced, with the missed status=20
>>>>> 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
>>>> (with specific control bits). For IN direction, if the controller is
>>>> starved of TRB, it will send a ZLP if the host requests for data.
>>>
>>> Which control bits are those? ISOC-First, Chain and Last bits?
>>>
>>> I see the Scatter-Gather preparation is using these pattern.
>>
>> The IOC bit. You can check the programming guide for more detail on how
>> to setup the TRBs, but what we have in dwc3 is fine.
>
> OK.
>
>>>>>>>> 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.
>>>>>
>>>>> In the empty trb case, after the Hardware has send enough zero=20
>>>>> packets
>>>>> this
>>>>> active transfer has to be stopped with endtransfer cmd. Because every
>>>>> next
>>>>> update transfer on that active transfer will likely lead to further
>>>>> 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
>>>> controller for every interval. If it's late, then the controller will
>>>> consider that data "missed_isoc".
>>>>
>>>> In your case, the UVC driver seems to queue requests to the controller
>>>> driver as if it is bulk requests, and the UVC expects those data to go
>>>> out at the time it queues. To achieve what UVC needs, then you may=20
>>>> want
>>>> to issue END_TRANSFER command before the next burst of data. This way,
>>>> the controller can restart the isoc endpoint and not consider the next
>>>> video frame data late. There are some corner cases that you need to
>>>> watch out for. If you're going for this route, we can look further.
>>>
>>> Right, for now the drivers is doing:
>>>
>>> - Strart Transfer (with the right frame number) once.
>>>
>>> - Update Transfer On every ep_queue with the corresponding TRB
>>> =A0setting CHN =3D 0, IOC =3D 1, First-ISOC =3D 1
>>>
>>> - End Transfer is somehow not handled right for this case.
>>>
>>> See my first comment. I think dwc3_prepare_one_trb_sg does the proper
>>> chain
>>> handling already.
>>>
>>>> Also, you'd need provide a way for the UVC to communicate to the=20
>>>> dwc3 to
>>>> let it know to expect the next burst of data.
>>>
>>> Can the UVC not just enqueue one big sg-request, which will represent
>>> one burst
>>> and not one TRB. Also that is=A0 what the SG code already seem to handl=
e
>>> right.
>>
>> Do you need SG? What size does video class driver setup its isoc URB? If
>> it's superspeed, max is 48K, and depending on the type of platform
>> you're running UVC on, you can setup a single 48K buffer per request if
>> you want to do that. However, it's probably not a good idea since many
>> host controllers can't even handle even close to 48K/uframe.
>
> We wan't to transfer uncached 4K Video frames via UVC. So Scatter-Gather
> is a must anyway.
>
>> What I was saying is if UVC knows when to wait for the next video data,
>> it can tell dwc3 to stop the isoc endpoint before queuing the next video
>> data in a set of requests. If UVC doesn't know that, then it needs to
>> tell dwc3 to change its handling of isoc requests.
>
> In function/gadget/uvc_video.c we take a buffer and pump it into many
> available requests as we find. On the way the driver can drain that
> video frame queue, in that case we could stop the transfer. I have
> prepared a patch where we have only one source, queueing new requests,
> so this could work.
>
> For now to limit the interrupt load on the dwc3 driver we already set
> the request no_interrupt on every nth request, and make sure the
> interrupt is on the very last one of the frame. But with that we still
> sometimes run into missed transfers as the queued trb list somehow ends
> up with a TRB not having the IOC bit set.
>
>>>>> 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
>>>>> *dep,
>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 const struct dwc3_event_depevt *=
event)
>>>>> {
>>>>> ...
>>>>>
>>>>> =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->start=
ed_list))
>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 st=
op =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
>>>>> trb queue
>>>>> after I stoped the tansfer when the started_list was empty right=20
>>>>> after
>>>>> ep_cleanup_completed_requests has moved all trbs out of the queue.
>>>>>
>>>>> These Interrupts appeared right after the ENDTRANSFER cmd was send.
>>>>> (But I
>>>>> could no verify this every time)
>>>>
>>>> If END_TRANSFER command completes, then you should not see
>>>> XferInProgress event. The next event should ber XferNotReady.
>>>
>>> Right. This also stops, after the Command Complete Interrupt arrives.
>>>
>>>>> Anyways in that case these Interrupts are not useful anymore, as I
>>>>> already
>>>>> implied the same stop, with ENDTRANSFER after we know that there are
>>>>> no other
>>>>> trbs in the chain.
>>>>>
>>>>
>>>> Just curious, do you know if there's a reason for UVC to behave this
>>>> way? Seems like what it's trying to do is more for bulk. Maybe it=20
>>>> wants
>>>> bandwidth priority perhaps?
>>>
>>> I don't know, probably it was more likely for USB 2.0 controllers to
>>> be handled
>>> this way.
>>>
>>> As mentioned the current uvc code is also working when we add this
>>> changes.
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index ec357f64f319..a5dc44f2e9d8 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2629,6 +2629,9 @@ static void
>>> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>>
>>> =A0=A0=A0=A0=A0=A0 dwc3_gadget_ep_cleanup_completed_requests(dep, event=
, status);
>>>
>>> +=A0=A0=A0=A0=A0=A0 if (list_empty(&dep->started_list))
>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 stop =3D true;
>>> +
>>
>> You should check the pending list too and either drop them or prepare
>> those requests (maybe too late). This happens when there's no available
>> TRB but UVC queues more requests.
>> Also, make sure this change only applies for isoc.
>
> I will send the patches for that so we can discuss the right handling
> for that in a separate thread.

Sure, we can review further then.

BR,
Thinh