Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame

2016-02-02 Thread Doug Anderson 
Kever,

On Mon, Feb 1, 2016 at 11:46 PM, Kever Yang  wrote:
> Doug,
>
>
> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>
>> When setting up ISO and INT transfers dwc2 needs to specify whether the
>> transfer is for an even or an odd frame (or microframe if the controller
>> is running in high speed mode).
>>
>> The controller appears to use this as a simple way to figure out if a
>> transfer should happen right away (in the current microframe) or should
>> happen at the start of the next microframe.  Said another way:
>>
>> - If you set "odd" and the current frame number is odd it appears that
>>the controller will try to transfer right away.  Same thing if you set
>>"even" and the current frame number is even.
>> - If the oddness you set and the oddness of the frame number are
>>_different_, the transfer will be delayed until the frame number
>>changes.
>>
>> As I understand it, the above technique allows you to plan ahead of time
>> where possible by always working on the next frame.  ...but it still
>> allows you to properly respond immediately to things that happened in
>> the previous frame.
>>
>> The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
>> It always looked at the frame number and setup the transfer to happen in
>> the next frame.  In some cases that meant that certain transactions
>> would be transferred in the wrong frame.
>>
>> We'll try our best to set the even / odd to do the transfer in the
>> scheduled frame.  If that fails then we'll do an ugly "schedule ASAP".
>> We'll also modify the scheduler code to handle this and not try to
>> schedule a second transfer for the same frame.
>>
>> Note that this change relies on the work to redo the microframe
>> scheduler.  It can work atop ("usb: dwc2: host: Manage frame nums better
>> in scheduler") but it works even better after ("usb: dwc2: host: Totally
>> redo the microframe scheduler").
>>
>> With this change my stressful USB test (USB webcam + USB audio +
>> keyboards) has less audio crackling than before.
>
> Seems this really help for your case?

Yes, I believe it does.  Of course my test case is pretty "black box"
for the most part in that I play music on youtube while having a
webcam open and several USB input devices connected.  I then try to
decide whether I hear more static or less static.  ...clearly a less
subjective test would be better...

* I tried with http://crosreview.com/325451 (see below) and I hear
more static with "use_old = true" than with "use_old = "false".

* I tried with this entire patch reverted and I hear about the same
static as with "use_old = true".

Note that counting reported MISS lines from my logging also shows that
the new code is better...


> Do you check if the transfer can happen right in the current frame? I know
> it's
> quite difficult to check it, but this changes what I know for the dwc core
> schedule the transaction.

Yes.  I just tried again, too.  I coded up
 and included it.  I
then opened up a USB webcam.

With things set to the old way:

  115.355370  QH=dc6ba8c0 next(0) fn=10cb, sch=10ca=>10cb (+1) miss=0
  115.355373  QH=dc6ba8c0 IMM ready fn=10cb, nxt=10cb
  115.355518  QH=dc6ba8c0 next(0) fn=10cc, sch=10cb=>10cc (+1) miss=0
  115.355522  QH=dc6ba8c0 IMM ready fn=10cc, nxt=10cc
  115.355637  QH=dc6ba8c0 next(0) fn=10cd, sch=10cc=>10cd (+1) miss=0
  115.355641  QH=dc6ba8c0 IMM ready fn=10cd, nxt=10cd
  115.355857  QH=dc6ba8c0 next(0) fn=10ce, sch=10cd=>10ce (+1) miss=0
  115.355859  QH=dc6ba8c0 IMM ready fn=10ce, nxt=10ce
  115.355867  QH=dc6ba8c0, wire=10cf, old_wire=10d0, EO diff (use OLD)
  115.355870  QH=dc6ba8c0 EO MISS w/ old (10ce != 10cf)
  115.356037  QH=dc6ba8c0 next(0) fn=10d0, sch=10cf=>10d0 (+1) miss=1 MISS
  115.356039  QH=dc6ba8c0 IMM ready fn=10d0, nxt=10d0
  115.356169  QH=dc6ba8c0 next(0) fn=10d1, sch=10d0=>10d1 (+1) miss=0
  115.356170  QH=dc6ba8c0 IMM ready fn=10d1, nxt=10d1
  115.356269  QH=dc6ba8c0 next(0) fn=10d2, sch=10d1=>10d2 (+1) miss=0
  115.356273  QH=dc6ba8c0 IMM ready fn=10d2, nxt=10d2
  115.356404  QH=dc6ba8c0 next(0) fn=10d3, sch=10d2=>10d3 (+1) miss=0
  115.356407  QH=dc6ba8c0 IMM ready fn=10d3, nxt=10d3

With the new way:

   87.814741  QH=e2fd7880 next(0) fn=32e4, sch=32e3=>32e4 (+1) miss=0
   87.814744  QH=e2fd7880 IMM ready fn=32e4, nxt=32e4
   87.814858  QH=e2fd7880 next(0) fn=32e5, sch=32e4=>32e5 (+1) miss=0
   87.814862  QH=e2fd7880 IMM ready fn=32e5, nxt=32e5
   87.815010  QH=e2fd7880 next(0) fn=32e6, sch=32e5=>32e6 (+1) miss=0
   87.815012  QH=e2fd7880 IMM ready fn=32e6, nxt=32e6
   87.815220  QH=e2fd7880 next(0) fn=32e8, sch=32e6=>32e7 (+1) miss=0
   87.815222  QH=e2fd7880 IMM ready fn=32e8, nxt=32e7
   87.815230  QH=e2fd7880, wire=32e8, old_wire=32e9, EO diff (use NEW)
   87.815278  QH=e2fd7880 next(0) fn=32e8, sch=32e7=>32e8 (+1) miss=0
   87.815280  QH=e2fd7880 IMM ready fn=32e8, nxt=32e8
   87.815390  QH=e2fd7880 next(0)

Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame

2016-02-02 Thread Kever Yang 

Doug,

Thanks for your detail debug information, pls add my Reviewed-by for 
this patch.


Thanks,
- Kever
On 02/03/2016 06:47 AM, Doug Anderson wrote:

Kever,

On Mon, Feb 1, 2016 at 11:46 PM, Kever Yang  wrote:

Doug,


On 01/29/2016 10:20 AM, Douglas Anderson wrote:

When setting up ISO and INT transfers dwc2 needs to specify whether the
transfer is for an even or an odd frame (or microframe if the controller
is running in high speed mode).

The controller appears to use this as a simple way to figure out if a
transfer should happen right away (in the current microframe) or should
happen at the start of the next microframe.  Said another way:

- If you set "odd" and the current frame number is odd it appears that
the controller will try to transfer right away.  Same thing if you set
"even" and the current frame number is even.
- If the oddness you set and the oddness of the frame number are
_different_, the transfer will be delayed until the frame number
changes.

As I understand it, the above technique allows you to plan ahead of time
where possible by always working on the next frame.  ...but it still
allows you to properly respond immediately to things that happened in
the previous frame.

The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
It always looked at the frame number and setup the transfer to happen in
the next frame.  In some cases that meant that certain transactions
would be transferred in the wrong frame.

We'll try our best to set the even / odd to do the transfer in the
scheduled frame.  If that fails then we'll do an ugly "schedule ASAP".
We'll also modify the scheduler code to handle this and not try to
schedule a second transfer for the same frame.

Note that this change relies on the work to redo the microframe
scheduler.  It can work atop ("usb: dwc2: host: Manage frame nums better
in scheduler") but it works even better after ("usb: dwc2: host: Totally
redo the microframe scheduler").

With this change my stressful USB test (USB webcam + USB audio +
keyboards) has less audio crackling than before.

Seems this really help for your case?

Yes, I believe it does.  Of course my test case is pretty "black box"
for the most part in that I play music on youtube while having a
webcam open and several USB input devices connected.  I then try to
decide whether I hear more static or less static.  ...clearly a less
subjective test would be better...

* I tried with http://crosreview.com/325451 (see below) and I hear
more static with "use_old = true" than with "use_old = "false".

* I tried with this entire patch reverted and I hear about the same
static as with "use_old = true".

Note that counting reported MISS lines from my logging also shows that
the new code is better...



Do you check if the transfer can happen right in the current frame? I know
it's
quite difficult to check it, but this changes what I know for the dwc core
schedule the transaction.

Yes.  I just tried again, too.  I coded up
 and included it.  I
then opened up a USB webcam.

With things set to the old way:

   115.355370  QH=dc6ba8c0 next(0) fn=10cb, sch=10ca=>10cb (+1) miss=0
   115.355373  QH=dc6ba8c0 IMM ready fn=10cb, nxt=10cb
   115.355518  QH=dc6ba8c0 next(0) fn=10cc, sch=10cb=>10cc (+1) miss=0
   115.355522  QH=dc6ba8c0 IMM ready fn=10cc, nxt=10cc
   115.355637  QH=dc6ba8c0 next(0) fn=10cd, sch=10cc=>10cd (+1) miss=0
   115.355641  QH=dc6ba8c0 IMM ready fn=10cd, nxt=10cd
   115.355857  QH=dc6ba8c0 next(0) fn=10ce, sch=10cd=>10ce (+1) miss=0
   115.355859  QH=dc6ba8c0 IMM ready fn=10ce, nxt=10ce
   115.355867  QH=dc6ba8c0, wire=10cf, old_wire=10d0, EO diff (use OLD)
   115.355870  QH=dc6ba8c0 EO MISS w/ old (10ce != 10cf)
   115.356037  QH=dc6ba8c0 next(0) fn=10d0, sch=10cf=>10d0 (+1) miss=1 MISS
   115.356039  QH=dc6ba8c0 IMM ready fn=10d0, nxt=10d0
   115.356169  QH=dc6ba8c0 next(0) fn=10d1, sch=10d0=>10d1 (+1) miss=0
   115.356170  QH=dc6ba8c0 IMM ready fn=10d1, nxt=10d1
   115.356269  QH=dc6ba8c0 next(0) fn=10d2, sch=10d1=>10d2 (+1) miss=0
   115.356273  QH=dc6ba8c0 IMM ready fn=10d2, nxt=10d2
   115.356404  QH=dc6ba8c0 next(0) fn=10d3, sch=10d2=>10d3 (+1) miss=0
   115.356407  QH=dc6ba8c0 IMM ready fn=10d3, nxt=10d3

With the new way:

87.814741  QH=e2fd7880 next(0) fn=32e4, sch=32e3=>32e4 (+1) miss=0
87.814744  QH=e2fd7880 IMM ready fn=32e4, nxt=32e4
87.814858  QH=e2fd7880 next(0) fn=32e5, sch=32e4=>32e5 (+1) miss=0
87.814862  QH=e2fd7880 IMM ready fn=32e5, nxt=32e5
87.815010  QH=e2fd7880 next(0) fn=32e6, sch=32e5=>32e6 (+1) miss=0
87.815012  QH=e2fd7880 IMM ready fn=32e6, nxt=32e6
87.815220  QH=e2fd7880 next(0) fn=32e8, sch=32e6=>32e7 (+1) miss=0
87.815222  QH=e2fd7880 IMM ready fn=32e8, nxt=32e7
87.815230  QH=e2fd7880, wire=32e8, old_wire=32e9, EO diff (use NEW)
87.815278  QH=e2fd7880 next(0) fn=32e8, sch=32e7=>32e8 (+1) miss=0
87.815280

Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame

2016-02-01 Thread Kever Yang 

Doug,

On 01/29/2016 10:20 AM, Douglas Anderson wrote:

When setting up ISO and INT transfers dwc2 needs to specify whether the
transfer is for an even or an odd frame (or microframe if the controller
is running in high speed mode).

The controller appears to use this as a simple way to figure out if a
transfer should happen right away (in the current microframe) or should
happen at the start of the next microframe.  Said another way:

- If you set "odd" and the current frame number is odd it appears that
   the controller will try to transfer right away.  Same thing if you set
   "even" and the current frame number is even.
- If the oddness you set and the oddness of the frame number are
   _different_, the transfer will be delayed until the frame number
   changes.

As I understand it, the above technique allows you to plan ahead of time
where possible by always working on the next frame.  ...but it still
allows you to properly respond immediately to things that happened in
the previous frame.

The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
It always looked at the frame number and setup the transfer to happen in
the next frame.  In some cases that meant that certain transactions
would be transferred in the wrong frame.

We'll try our best to set the even / odd to do the transfer in the
scheduled frame.  If that fails then we'll do an ugly "schedule ASAP".
We'll also modify the scheduler code to handle this and not try to
schedule a second transfer for the same frame.

Note that this change relies on the work to redo the microframe
scheduler.  It can work atop ("usb: dwc2: host: Manage frame nums better
in scheduler") but it works even better after ("usb: dwc2: host: Totally
redo the microframe scheduler").

With this change my stressful USB test (USB webcam + USB audio +
keyboards) has less audio crackling than before.

Seems this really help for your case?

Do you check if the transfer can happen right in the current frame? I 
know it's

quite difficult to check it, but this changes what I know for the dwc core
schedule the transaction.

In dwc_otgbook, Interrupt OUT Transactions(also similar for Int IN, Iso 
IN/OUT)

in DMA Mode, the normal Interrupt OUT operation says:
The DWC_otg host attempts to send out the OUT token in the beginning of next
odd frame/microframe.

So I'm confuse about if the dwc core can do the transaction at the same 
frame

of host channel initialized or not.

Thanks,
- Kever


Signed-off-by: Douglas Anderson 
Tested-by: Heiko Stuebner 
Tested-by: Stefan Wahren 
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.

Changes in v5: None
Changes in v4:
- Properly set even/odd frame new for v4.

Changes in v3: None
Changes in v2: None

  drivers/usb/dwc2/core.c  | 92 +++-
  drivers/usb/dwc2/hcd_queue.c | 11 +-
  2 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a5db20f12ee4..c143f26bd9d9 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1703,9 +1703,97 @@ static void dwc2_hc_set_even_odd_frame(struct dwc2_hsotg 
*hsotg,
  {
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
-   /* 1 if _next_ frame is odd, 0 if it's even */
-   if (!(dwc2_hcd_get_frame_number(hsotg) & 0x1))
+   int host_speed;
+   int xfer_ns;
+   int xfer_us;
+   int bytes_in_fifo;
+   u16 fifo_space;
+   u16 frame_number;
+   u16 wire_frame;
+
+   /*
+* Try to figure out if we're an even or odd frame. If we set
+* even and the current frame number is even the the transfer
+* will happen immediately.  Similar if both are odd. If one is
+* even and the other is odd then the transfer will happen when
+* the frame number ticks.
+*
+* There's a bit of a balancing act to get this right.
+* Sometimes we may want to send data in the current frame (AK
+* right away).  We might want to do this if the frame number
+* _just_ ticked, but we might also want to do this in order
+* to continue a split transaction that happened late in a
+* microframe (so we didn't know to queue the next transfer
+* until the frame number had ticked).  The problem is that we
+* need a lot of knowledge to know if there's actually still
+* time to send things or if it would be better to wait until
+* the next frame.
+*
+* We can look at how much time is left in the current frame
+* and make a guess about whether we'll have time to

[PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame

2016-01-28 Thread Douglas Anderson 
When setting up ISO and INT transfers dwc2 needs to specify whether the
transfer is for an even or an odd frame (or microframe if the controller
is running in high speed mode).

The controller appears to use this as a simple way to figure out if a
transfer should happen right away (in the current microframe) or should
happen at the start of the next microframe.  Said another way:

- If you set "odd" and the current frame number is odd it appears that
  the controller will try to transfer right away.  Same thing if you set
  "even" and the current frame number is even.
- If the oddness you set and the oddness of the frame number are
  _different_, the transfer will be delayed until the frame number
  changes.

As I understand it, the above technique allows you to plan ahead of time
where possible by always working on the next frame.  ...but it still
allows you to properly respond immediately to things that happened in
the previous frame.

The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
It always looked at the frame number and setup the transfer to happen in
the next frame.  In some cases that meant that certain transactions
would be transferred in the wrong frame.

We'll try our best to set the even / odd to do the transfer in the
scheduled frame.  If that fails then we'll do an ugly "schedule ASAP".
We'll also modify the scheduler code to handle this and not try to
schedule a second transfer for the same frame.

Note that this change relies on the work to redo the microframe
scheduler.  It can work atop ("usb: dwc2: host: Manage frame nums better
in scheduler") but it works even better after ("usb: dwc2: host: Totally
redo the microframe scheduler").

With this change my stressful USB test (USB webcam + USB audio +
keyboards) has less audio crackling than before.

Signed-off-by: Douglas Anderson 
Tested-by: Heiko Stuebner 
Tested-by: Stefan Wahren 
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.

Changes in v5: None
Changes in v4:
- Properly set even/odd frame new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.c  | 92 +++-
 drivers/usb/dwc2/hcd_queue.c | 11 +-
 2 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a5db20f12ee4..c143f26bd9d9 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1703,9 +1703,97 @@ static void dwc2_hc_set_even_odd_frame(struct dwc2_hsotg 
*hsotg,
 {
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
-   /* 1 if _next_ frame is odd, 0 if it's even */
-   if (!(dwc2_hcd_get_frame_number(hsotg) & 0x1))
+   int host_speed;
+   int xfer_ns;
+   int xfer_us;
+   int bytes_in_fifo;
+   u16 fifo_space;
+   u16 frame_number;
+   u16 wire_frame;
+
+   /*
+* Try to figure out if we're an even or odd frame. If we set
+* even and the current frame number is even the the transfer
+* will happen immediately.  Similar if both are odd. If one is
+* even and the other is odd then the transfer will happen when
+* the frame number ticks.
+*
+* There's a bit of a balancing act to get this right.
+* Sometimes we may want to send data in the current frame (AK
+* right away).  We might want to do this if the frame number
+* _just_ ticked, but we might also want to do this in order
+* to continue a split transaction that happened late in a
+* microframe (so we didn't know to queue the next transfer
+* until the frame number had ticked).  The problem is that we
+* need a lot of knowledge to know if there's actually still
+* time to send things or if it would be better to wait until
+* the next frame.
+*
+* We can look at how much time is left in the current frame
+* and make a guess about whether we'll have time to transfer.
+* We'll do that.
+*/
+
+   /* Get speed host is running at */
+   host_speed = (chan->speed != USB_SPEED_HIGH &&
+ !chan->do_split) ? chan->speed : USB_SPEED_HIGH;
+
+   /* See how many bytes are in the periodic FIFO right now */
+   fifo_space = (dwc2_readl(hsotg->regs + HPTXSTS) &
+ TXSTS_FSPCAVAIL_MASK) >> TXSTS_FSPCAVAIL_SHIFT;
+   bytes_in_fifo = sizeof(u32) *
+   (hsotg->core_params->host_perio_tx_fifo_size -
+fifo_space);
+
+