Hi Janusz,
(good to see your email here)
Janusz Dziedzic <janusz.dziedzic@xxxxxxxxx> writes:
> On 19 May 2016 at 09:08, Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> Paul Zimmerman <pauldzim@xxxxxxxxx> writes:
>>> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>>>
>>>> If we're going to issue a Update Transfer command,
>>>> let's clear LST bit from previous TRB. This will let
>>>> us continue processing TRBs and convert previous IRQ
>>>> into XferInProgress, instead of XferComplete.
>>>>
>>>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>> ---
>>>> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++---------
>>>> 1 file changed, 23 insertions(+), 9 deletions(-)
>>>
>>> < snip >
>>>
>>>> + /*
>>>> + * When trying to issue Update Transfer, we can remove LST bit from
>>>> + * previous TRB and avoid a XferComplete
>>>> + */
>>>> + if (!starting) {
>>>> + trb = &dep->trb_pool[dep->trb_enqueue - 1];
>>>> + if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>>>> + trb->ctrl &= ~DWC3_TRB_CTRL_LST;
>>>
>>> Hi Felipe,
>>>
>>> This violates the DWC USB3 programming model. Once the HWO bit has been set
>>> and the transfer started, the host is not allowed to modify any of the
>>> fields in the TRB until the controller clears the HWO bit, or the transfer
>>> completes or is halted.
>>
>
> With this patch and rndis interface and highspeed I have (tested using iperf):
> 275Mbps for TCP for both directions (305Mbps for UDP).
>
> Without patch:
> 145Mbps for TCP
>
> This is quite interesting, while I understand we violate programming
> model here, but still have correct TCP traffic (also during MTP tests
> didn't hit any issue)/
right, you mentioned this to me off-list and I'm also a bit bummed out
that we loose such a good performance. BTW, did you see this with a deep
request queue too? Say, some 64 USB requests per endpoint? Can you share
trace output of iperf transfers? I'd like to make sure we're getting CSP
and ISP set properly.
To get the best output possible, make sure you cherry-pick "usb: dwc3:
trace: fully decode IRQ events" from my testing/next branch:
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=ec1c39e4f840a4f5012efac11ed6a7f18ced1d48
In fact, there are quite a few improvements to our tracepoints, if you
wanna pick them all, go for it :)
> And the improvement is also nice. Maybe HW should add something like
> APPEND_TRB which will set/clear this flag
> correctly in the HW, while HW know if this is already safe.
> This mean we still can improve dwc3 driver and get similar TP we have
> with this "wrong" patch.
I agree. If HW can give us an APPEND_TRB parameter to UPDATE_TRANSFER,
then we can make proper use of this.
John, can you check with your RTL engineers why it's a violation of
programming model to change LST while HWO is set? The only "problem" I
can see is that we could race with HW if it's already completing the
transfer when we decide to change LST.
--
balbi
Attachment:
signature.asc
Description: PGP signature
