linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Felipe Balbi <felipe.balbi@linux.intel.com>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	John Youn <John.Youn@synopsys.com>
Cc: "linux-usb\@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all
Date: Wed, 19 Jun 2019 09:28:30 +0300	[thread overview]
Message-ID: <87a7eef5rl.fsf@linux.intel.com> (raw)
In-Reply-To: <30102591E157244384E984126FC3CB4F63A1230F@us01wembx1.internal.synopsys.com>

[-- Attachment #1: Type: text/plain, Size: 4758 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> Would there be any obvious draw-back to going down this route? The thing
>>>>> is that, as it is, it seems like we will *always* have some corner case
>>>>> where we can't guarantee that we can even start a transfer since there's
>>>>> no upper-bound between XferNotReady and gadget driver finally queueing a
>>>>> request. Also, I can't simply read DSTS for the frame number because of
>>>>> top-most 2 bits.
>>>>>
>>>> For non-affected version of the IP, the xfernotready -> starttransfer
>>>> time will have to be off by more than a couple seconds for the driver
>>>> to produce an incorrect 16-bit frame number. If you're seeing errors
>>>> here, maybe we just need to code review the relevant sections to make
>>>> sure the 14/16-bit and rollover conditions are all handled correctly.
>>> I think what Felipe may see is some delay in the system that causes the
>>> SW to not handle XferNotReady event in time. We already have the "retry"
>>> method handle that to a certain extend.
>>>
>>>> But I can't think of any obvious drawbacks of the quirk, other than
>>>> doing some unnecessary work, which shouldn't produce any bad
>>>> side-effects. But we haven't really tested that.
>>>>
>>> The workaround for the isoc_quirk requires 2 tries sending
>>> START_TRANSFER command. This means that you have to account the delay of
>>> that command completion plus potentially 1 more END_TRANSFER completion.
>>> That's why the quirk gives a buffer of at least 4 uframes of the
>>> scheduled isoc frame. So, it cannot schedule immediately on the next
>>> uframe, that's one of the drawbacks.
>>>
>>>
>>> Hi Felipe,
>>>
>>> Since you're asking this, it means you're still seeing issue with your
>>> setup despite retrying to send START_TRANSFER command 5 times. What's
>>> the worse delay responding to XferNotReady you're seeing in your setup?
>> There's no upper-bound on how long the gadget will take to enqueue a
>> request. We see problems with UVC gadget all the time. It can take a lot
>> of time to decide to enqueue data.
>
> That's why there's a mechanism in the controller to return bus-expiry
> status to let the SW know if it had scheduled isoc too late. SW can do 2
> things: 1) re-schedule at a later timer or 2) send END_TRANSFER command
> to wait for the next XferNotReady to try again.

All of this is still rather flakey. Can I send consecutive END_TRANSFER
commands and get new XferNotReady at any moment? Consider this
situation:

. transfer started
. transfer completes with status Missed ISOC
. driver issues END_TRANSFER (as required by docs)
. XferNotReady fires
. driver updates current frame number
. several mS of nothing
. finally gadget enqueues a transfer
. Start Transfer command
. completes with Bus Expiry

Can I issue *another* END_TRANSFER at this point? I don't even have a
valid transfer resource since transfer wasn't started.

The best "workaround" I can think of would be to delay END_TRASFER until
ep_queue time.

>> Usually I hear this from folks using UVC gadget with a real sensor on
>> the background.
>>
>> I've seen gadget enqueueing as far as 20 intervals in the future. But
>> remember, there's no upper-bound. And that's the problem. If we could
>> just read the frame number from DSTS and use that, we wouldn't have any
>> issues. But since DSTS only contains 14 our of the 16 bits the
>> controller needs, then we can't really use that.
>
> You can create another quirk for devices that have this behavior to use
> frame number in DSTS instead.  As John had pointed out and mentioned, 
> this will only work if the service interval and the delay in the
> scheduling of isoc is within 2 seconds.

well, that's better than nothing, but there's no upper-bound for the
gadget driver, really.

> You will need to calculate this value along with BIT(15) and BIT(14) of
> XferNotReady for rollovers.
>
>>
>> To me, it seems like this part of the controller wasn't well
>> thought-out. These extra two bits, perhaps, should be internal to the
>> controller and SW should have no knowledge that they exist.
>
> These values are internal. SW should not have knowledge of it. This
> implementation will not follow the programming guide and should be used
> as a quirk for devices that are too slow to handle the XferNotReady
> event but want to schedule isoc immediately after handling the event.

They are *not* internal if SW needs to know that to start a transfer
properly it needs these extra two bits :-) What I meant to say was that
we should never have a 16-bit frame number. Only 14 bits. But in any
case, we can't change the HW now :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

  parent reply	other threads:[~2019-06-19  6:28 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-07  9:50 [RFC] Sorting out dwc3 ISOC endpoints once and for all Felipe Balbi
2019-06-17 17:37 ` John Youn
2019-06-17 19:08   ` Thinh Nguyen
2019-06-17 20:44     ` John Youn
2019-06-18  7:20     ` Felipe Balbi
2019-06-18 18:15       ` Thinh Nguyen
2019-06-18 19:58         ` Thinh Nguyen
2019-06-19  6:28         ` Felipe Balbi [this message]
2019-06-19 16:55           ` John Youn
2019-06-19 18:56           ` Thinh Nguyen
2019-06-20 17:58             ` Thinh Nguyen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a7eef5rl.fsf@linux.intel.com \
    --to=felipe.balbi@linux.intel.com \
    --cc=John.Youn@synopsys.com \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).