Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1321397 > unrolled thread
| Started by | Douglas Anderson <dianders@chromium.org> |
|---|---|
| First post | 2016-01-29 03:30 +0100 |
| Last post | 2016-02-03 19:30 +0100 |
| Articles | 18 on this page of 38 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v6 0/22] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 09/22] usb: dwc2: host: Giveback URB in tasklet context Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 06/22] usb: dwc2: host: fix split transfer schedule sequence Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 15/22] usb: dwc2: host: Split code out to make dwc2_do_reserve() Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 22/22] usb: dwc2: host: If using uframe scheduler, end splits better Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 11/22] usb: dwc2: host: There's not really a TT for the root hub Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 11/22] usb: dwc2: host: There's not really a TT for the root hub Kever Yang <kever.yang@rock-chips.com> - 2016-01-31 10:30 +0100
[PATCH v6 02/22] usb: dwc2: host: Get aligned DMA in a more supported way Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 10/22] usb: dwc2: host: Properly set the HFIR Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 10/22] usb: dwc2: host: Properly set the HFIR Kever Yang <kever.yang@rock-chips.com> - 2016-01-31 10:30 +0100
Re: [PATCH v6 10/22] usb: dwc2: host: Properly set the HFIR Doug Anderson <dianders@chromium.org> - 2016-01-31 23:20 +0100
[PATCH v6 13/22] usb: dwc2: host: Rename some fields in struct dwc2_qh Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 04/22] usb: dwc2: host: Avoid use of chan->qh after qh freed Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 16/22] usb: dwc2: host: Add scheduler logging for missed SOFs Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 01/22] usb: dwc2: rockchip: Make the max_transfer_size automatic Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 17/22] usb: dwc2: host: Manage frame nums better in scheduler Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 17/22] usb: dwc2: host: Manage frame nums better in scheduler Doug Anderson <dianders@chromium.org> - 2016-02-03 21:40 +0100
[PATCH v6 05/22] usb: dwc2: host: Always add to the tail of queues Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 19/22] usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 03/22] usb: dwc2: host: Set host_rx_fifo_size to 525 for rk3066 Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 07/22] usb: dwc2: host: Add scheduler tracing Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 08/22] usb: dwc2: host: Add a delay before releasing periodic bandwidth Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame Kever Yang <kever.yang@rock-chips.com> - 2016-02-02 08:50 +0100
Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame Doug Anderson <dianders@chromium.org> - 2016-02-02 23:50 +0100
Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame Kever Yang <kever.yang@rock-chips.com> - 2016-02-03 08:50 +0100
[PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Kever Yang <kever.yang@rock-chips.com> - 2016-01-31 10:40 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Doug Anderson <dianders@chromium.org> - 2016-01-31 23:10 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Kever Yang <kever.yang@rock-chips.com> - 2016-02-01 04:40 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Doug Anderson <dianders@chromium.org> - 2016-02-01 05:40 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Doug Anderson <dianders@chromium.org> - 2016-02-02 01:40 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Kever Yang <kever.yang@rock-chips.com> - 2016-02-02 08:10 +0100
Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time Doug Anderson <dianders@chromium.org> - 2016-02-03 00:40 +0100
[PATCH v6 14/22] usb: dwc2: host: Reorder things in hcd_queue.c Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
[PATCH v6 12/22] usb: dwc2: host: Use periodic interrupt even with DMA Douglas Anderson <dianders@chromium.org> - 2016-01-29 03:30 +0100
Re: [PATCH v6 0/22] usb: dwc2: host: Fix and speed up all the stuff, especially with splits John Youn <John.Youn@synopsys.com> - 2016-02-03 01:00 +0100
Re: [PATCH v6 0/22] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Doug Anderson <dianders@chromium.org> - 2016-02-03 19:30 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 07/22] usb: dwc2: host: Add scheduler tracing |
| Message-ID | <qW6YX-4gM-45@gated-at.bofh.it> |
| In reply to | #1321397 |
In preparation for future changes to the scheduler let's add some
tracing that makes it easy for us to see what's happening. By default
this tracing will be off.
By changing "core.h" you can easily trace to ftrace, the console, or
nowhere.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
Changes in v6:
- Add Kever's Reviewed-by.
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.
Changes in v5: None
Changes in v4:
- Retooled scheduler tracing a bit, so left off John's Ack from v3.
Changes in v3: None
Changes in v2: None
drivers/usb/dwc2/core.h | 20 ++++++++++++++++++++
drivers/usb/dwc2/hcd.h | 5 +++++
drivers/usb/dwc2/hcd_intr.c | 6 +++++-
drivers/usb/dwc2/hcd_queue.c | 24 +++++++++++++++++++++++-
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 538cf38af0e4..18f9e4045643 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -44,6 +44,26 @@
#include <linux/usb/phy.h>
#include "hw.h"
+/*
+ * Suggested defines for tracers:
+ * - no_printk: Disable tracing
+ * - pr_info: Print this info to the console
+ * - trace_printk: Print this info to trace buffer (good for verbose logging)
+ */
+
+#define DWC2_TRACE_SCHEDULER no_printk
+#define DWC2_TRACE_SCHEDULER_VB no_printk
+
+/* Detailed scheduler tracing, but won't overwhelm console */
+#define dwc2_sch_dbg(hsotg, fmt, ...) \
+ DWC2_TRACE_SCHEDULER(pr_fmt("%s: SCH: " fmt), \
+ dev_name(hsotg->dev), ##__VA_ARGS__)
+
+/* Verbose scheduler tracing */
+#define dwc2_sch_vdbg(hsotg, fmt, ...) \
+ DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt), \
+ dev_name(hsotg->dev), ##__VA_ARGS__)
+
static inline u32 dwc2_readl(const void __iomem *addr)
{
u32 value = __raw_readl(addr);
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 1b46e2e617cc..809bc4ff9116 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -563,6 +563,11 @@ static inline u16 dwc2_frame_num_inc(u16 frame, u16 inc)
return (frame + inc) & HFNUM_MAX_FRNUM;
}
+static inline u16 dwc2_frame_num_dec(u16 frame, u16 dec)
+{
+ return (frame + HFNUM_MAX_FRNUM + 1 - dec) & HFNUM_MAX_FRNUM;
+}
+
static inline u16 dwc2_full_frame_num(u16 frame)
{
return (frame & HFNUM_MAX_FRNUM) >> 3;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 577c91096a51..5d25a5ec9736 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -138,13 +138,17 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
while (qh_entry != &hsotg->periodic_sched_inactive) {
qh = list_entry(qh_entry, struct dwc2_qh, qh_list_entry);
qh_entry = qh_entry->next;
- if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number))
+ if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number)) {
+ dwc2_sch_vdbg(hsotg, "QH=%p ready fn=%04x, sch=%04x\n",
+ qh, hsotg->frame_number, qh->sched_frame);
+
/*
* Move QH to the ready list to be executed next
* (micro)frame
*/
list_move_tail(&qh->qh_list_entry,
&hsotg->periodic_sched_ready);
+ }
}
tr_type = dwc2_hcd_select_transactions(hsotg);
if (tr_type != DWC2_TRANSACTION_NONE)
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index bc632a72f611..0e9faa75593c 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -113,6 +113,9 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
qh->sched_frame = dwc2_frame_num_inc(hsotg->frame_number,
SCHEDULE_SLOP);
qh->interval = urb->interval;
+ dwc2_sch_dbg(hsotg, "QH=%p init sch=%04x, fn=%04x, int=%#x\n",
+ qh, qh->sched_frame, hsotg->frame_number,
+ qh->interval);
#if 0
/* Increase interrupt polling rate for debugging */
if (qh->ep_type == USB_ENDPOINT_XFER_INT)
@@ -126,6 +129,11 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
qh->interval *= 8;
qh->sched_frame |= 0x7;
qh->start_split_frame = qh->sched_frame;
+ dwc2_sch_dbg(hsotg,
+ "QH=%p init*8 sch=%04x, fn=%04x, int=%#x\n",
+ qh, qh->sched_frame, hsotg->frame_number,
+ qh->interval);
+
}
dev_dbg(hsotg->dev, "interval=%d\n", qh->interval);
}
@@ -482,6 +490,8 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
if (frame >= 0) {
qh->sched_frame &= ~0x7;
qh->sched_frame |= (frame & 7);
+ dwc2_sch_dbg(hsotg, "QH=%p sched_p sch=%04x, uf=%d\n",
+ qh, qh->sched_frame, frame);
}
if (status > 0)
@@ -583,10 +593,16 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
if (!dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number) &&
!hsotg->frame_number) {
+ u16 new_frame;
+
dev_dbg(hsotg->dev,
"reset frame number counter\n");
- qh->sched_frame = dwc2_frame_num_inc(hsotg->frame_number,
+ new_frame = dwc2_frame_num_inc(hsotg->frame_number,
SCHEDULE_SLOP);
+
+ dwc2_sch_vdbg(hsotg, "QH=%p reset sch=%04x=>%04x\n",
+ qh, qh->sched_frame, new_frame);
+ qh->sched_frame = new_frame;
}
/* Add the new QH to the appropriate schedule */
@@ -652,6 +668,7 @@ static void dwc2_sched_periodic_split(struct dwc2_hsotg *hsotg,
int sched_next_periodic_split)
{
u16 incr;
+ u16 old_frame = qh->sched_frame;
if (sched_next_periodic_split) {
qh->sched_frame = frame_number;
@@ -677,6 +694,11 @@ static void dwc2_sched_periodic_split(struct dwc2_hsotg *hsotg,
qh->sched_frame |= 0x7;
qh->start_split_frame = qh->sched_frame;
}
+
+ dwc2_sch_vdbg(hsotg, "QH=%p next(%d) fn=%04x, sch=%04x=>%04x (%+d)\n",
+ qh, sched_next_periodic_split, frame_number, old_frame,
+ qh->sched_frame,
+ dwc2_frame_num_dec(qh->sched_frame, old_frame));
}
/*
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 08/22] usb: dwc2: host: Add a delay before releasing periodic bandwidth |
| Message-ID | <qW6YY-4gM-51@gated-at.bofh.it> |
| In reply to | #1321397 |
We'd like to be able to use HCD_BH in order to speed up the dwc2 host
interrupt handler quite a bit. However, according to the kernel doc for
usb_submit_urb() (specifically the part about "Reserved Bandwidth
Transfers"), we need to keep a reservation active as long as a device
driver keeps submitting. That was easy to do when we gave back the URB
in the interrupt context: we just looked at when our queue was empty and
released the reserved bandwidth then. ...but now we need a little more
complexity.
We'll follow EHCI's lead in commit 9118f9eb4f1e ("USB: EHCI: improve
interrupt qh unlink") and add a 5ms delay. Since we don't have a whole
timer infrastructure in dwc2, we'll just add a timer per QH. The
overhead for this is very small.
Note that the dwc2 scheduler is pretty broken (see future patches to fix
it). This patch attempts to replicate all old behavior and just add the
proper delay.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.
Changes in v5: None
Changes in v4:
- Moved periodic bandwidth release delay patch earlier again.
Changes in v3:
- Moved periodic bandwidth release delay patch later in the series.
Changes in v2:
- Periodic bandwidth release delay new for V2
drivers/usb/dwc2/hcd.h | 6 ++
drivers/usb/dwc2/hcd_queue.c | 237 +++++++++++++++++++++++++++++++++----------
2 files changed, 187 insertions(+), 56 deletions(-)
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 809bc4ff9116..79473ea35bd6 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -215,6 +215,7 @@ enum dwc2_transaction_type {
/**
* struct dwc2_qh - Software queue head structure
*
+ * @hsotg: The HCD state structure for the DWC OTG controller
* @ep_type: Endpoint type. One of the following values:
* - USB_ENDPOINT_XFER_CONTROL
* - USB_ENDPOINT_XFER_BULK
@@ -252,13 +253,16 @@ enum dwc2_transaction_type {
* @n_bytes: Xfer Bytes array. Each element corresponds to a transfer
* descriptor and indicates original XferSize value for the
* descriptor
+ * @unreserve_timer: Timer for releasing periodic reservation.
* @tt_buffer_dirty True if clear_tt_buffer_complete is pending
+ * @unreserve_pending: True if we planned to unreserve but haven't yet.
*
* A Queue Head (QH) holds the static characteristics of an endpoint and
* maintains a list of transfers (QTDs) for that endpoint. A QH structure may
* be entered in either the non-periodic or periodic schedule.
*/
struct dwc2_qh {
+ struct dwc2_hsotg *hsotg;
u8 ep_type;
u8 ep_is_in;
u16 maxp;
@@ -281,7 +285,9 @@ struct dwc2_qh {
dma_addr_t desc_list_dma;
u32 desc_list_sz;
u32 *n_bytes;
+ struct timer_list unreserve_timer;
unsigned tt_buffer_dirty:1;
+ unsigned unreserve_pending:1;
};
/**
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 0e9faa75593c..b9e4867e1afd 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -53,6 +53,94 @@
#include "core.h"
#include "hcd.h"
+/* Wait this long before releasing periodic reservation */
+#define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
+
+/**
+ * dwc2_do_unreserve() - Actually release the periodic reservation
+ *
+ * This function actually releases the periodic bandwidth that was reserved
+ * by the given qh.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh: QH for the periodic transfer.
+ */
+static void dwc2_do_unreserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+ assert_spin_locked(&hsotg->lock);
+
+ WARN_ON(!qh->unreserve_pending);
+
+ /* No more unreserve pending--we're doing it */
+ qh->unreserve_pending = false;
+
+ if (WARN_ON(!list_empty(&qh->qh_list_entry)))
+ list_del_init(&qh->qh_list_entry);
+
+ /* Update claimed usecs per (micro)frame */
+ hsotg->periodic_usecs -= qh->usecs;
+
+ if (hsotg->core_params->uframe_sched > 0) {
+ int i;
+
+ for (i = 0; i < 8; i++) {
+ hsotg->frame_usecs[i] += qh->frame_usecs[i];
+ qh->frame_usecs[i] = 0;
+ }
+ } else {
+ /* Release periodic channel reservation */
+ hsotg->periodic_channels--;
+ }
+}
+
+/**
+ * dwc2_unreserve_timer_fn() - Timer function to release periodic reservation
+ *
+ * According to the kernel doc for usb_submit_urb() (specifically the part about
+ * "Reserved Bandwidth Transfers"), we need to keep a reservation active as
+ * long as a device driver keeps submitting. Since we're using HCD_BH to give
+ * back the URB we need to give the driver a little bit of time before we
+ * release the reservation. This worker is called after the appropriate
+ * delay.
+ *
+ * @work: Pointer to a qh unreserve_work.
+ */
+static void dwc2_unreserve_timer_fn(unsigned long data)
+{
+ struct dwc2_qh *qh = (struct dwc2_qh *)data;
+ struct dwc2_hsotg *hsotg = qh->hsotg;
+ unsigned long flags;
+
+ /*
+ * Wait for the lock, or for us to be scheduled again. We
+ * could be scheduled again if:
+ * - We started executing but didn't get the lock yet.
+ * - A new reservation came in, but cancel didn't take effect
+ * because we already started executing.
+ * - The timer has been kicked again.
+ * In that case cancel and wait for the next call.
+ */
+ while (!spin_trylock_irqsave(&hsotg->lock, flags)) {
+ if (timer_pending(&qh->unreserve_timer))
+ return;
+ }
+
+ /*
+ * Might be no more unreserve pending if:
+ * - We started executing but didn't get the lock yet.
+ * - A new reservation came in, but cancel didn't take effect
+ * because we already started executing.
+ *
+ * We can't put this in the loop above because unreserve_pending needs
+ * to be accessed under lock, so we can only check it once we got the
+ * lock.
+ */
+ if (qh->unreserve_pending)
+ dwc2_do_unreserve(hsotg, qh);
+
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+}
+
/**
* dwc2_qh_init() - Initializes a QH structure
*
@@ -71,6 +159,9 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
dev_vdbg(hsotg->dev, "%s()\n", __func__);
/* Initialize QH */
+ qh->hsotg = hsotg;
+ setup_timer(&qh->unreserve_timer, dwc2_unreserve_timer_fn,
+ (unsigned long)qh);
qh->ep_type = dwc2_hcd_get_pipe_type(&urb->pipe_info);
qh->ep_is_in = dwc2_hcd_is_pipe_in(&urb->pipe_info) ? 1 : 0;
@@ -240,6 +331,15 @@ struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
*/
void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
{
+ /* Make sure any unreserve work is finished. */
+ if (del_timer_sync(&qh->unreserve_timer)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ dwc2_do_unreserve(hsotg, qh);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+ }
+
if (qh->desc_list)
dwc2_hcd_qh_free_ddma(hsotg, qh);
kfree(qh);
@@ -477,51 +577,74 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
{
int status;
- if (hsotg->core_params->uframe_sched > 0) {
- int frame = -1;
-
- status = dwc2_find_uframe(hsotg, qh);
- if (status == 0)
- frame = 7;
- else if (status > 0)
- frame = status - 1;
-
- /* Set the new frame up */
- if (frame >= 0) {
- qh->sched_frame &= ~0x7;
- qh->sched_frame |= (frame & 7);
- dwc2_sch_dbg(hsotg, "QH=%p sched_p sch=%04x, uf=%d\n",
- qh, qh->sched_frame, frame);
+ status = dwc2_check_max_xfer_size(hsotg, qh);
+ if (status) {
+ dev_dbg(hsotg->dev,
+ "%s: Channel max transfer size too small for periodic transfer\n",
+ __func__);
+ return status;
+ }
+
+ /* Cancel pending unreserve; if canceled OK, unreserve was pending */
+ if (del_timer(&qh->unreserve_timer))
+ WARN_ON(!qh->unreserve_pending);
+
+ /*
+ * Only need to reserve if there's not an unreserve pending, since if an
+ * unreserve is pending then by definition our old reservation is still
+ * valid. Unreserve might still be pending even if we didn't cancel if
+ * dwc2_unreserve_timer_fn() already started. Code in the timer handles
+ * that case.
+ */
+ if (!qh->unreserve_pending) {
+ if (hsotg->core_params->uframe_sched > 0) {
+ int frame = -1;
+
+ status = dwc2_find_uframe(hsotg, qh);
+ if (status == 0)
+ frame = 7;
+ else if (status > 0)
+ frame = status - 1;
+
+ /* Set the new frame up */
+ if (frame >= 0) {
+ qh->sched_frame &= ~0x7;
+ qh->sched_frame |= (frame & 7);
+ dwc2_sch_dbg(hsotg,
+ "QH=%p sched_p sch=%04x, uf=%d\n",
+ qh, qh->sched_frame, frame);
+ }
+
+ if (status > 0)
+ status = 0;
+ } else {
+ status = dwc2_periodic_channel_available(hsotg);
+ if (status) {
+ dev_info(hsotg->dev,
+ "%s: No host channel available for periodic transfer\n",
+ __func__);
+ return status;
+ }
+
+ status = dwc2_check_periodic_bandwidth(hsotg, qh);
}
- if (status > 0)
- status = 0;
- } else {
- status = dwc2_periodic_channel_available(hsotg);
if (status) {
- dev_info(hsotg->dev,
- "%s: No host channel available for periodic transfer\n",
- __func__);
+ dev_dbg(hsotg->dev,
+ "%s: Insufficient periodic bandwidth for periodic transfer\n",
+ __func__);
return status;
}
- status = dwc2_check_periodic_bandwidth(hsotg, qh);
- }
+ if (hsotg->core_params->uframe_sched <= 0)
+ /* Reserve periodic channel */
+ hsotg->periodic_channels++;
- if (status) {
- dev_dbg(hsotg->dev,
- "%s: Insufficient periodic bandwidth for periodic transfer\n",
- __func__);
- return status;
+ /* Update claimed usecs per (micro)frame */
+ hsotg->periodic_usecs += qh->usecs;
}
- status = dwc2_check_max_xfer_size(hsotg, qh);
- if (status) {
- dev_dbg(hsotg->dev,
- "%s: Channel max transfer size too small for periodic transfer\n",
- __func__);
- return status;
- }
+ qh->unreserve_pending = 0;
if (hsotg->core_params->dma_desc_enable > 0)
/* Don't rely on SOF and start in ready schedule */
@@ -531,13 +654,6 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
list_add_tail(&qh->qh_list_entry,
&hsotg->periodic_sched_inactive);
- if (hsotg->core_params->uframe_sched <= 0)
- /* Reserve periodic channel */
- hsotg->periodic_channels++;
-
- /* Update claimed usecs per (micro)frame */
- hsotg->periodic_usecs += qh->usecs;
-
return status;
}
@@ -551,22 +667,31 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg,
struct dwc2_qh *qh)
{
- int i;
+ bool did_modify;
- list_del_init(&qh->qh_list_entry);
+ assert_spin_locked(&hsotg->lock);
- /* Update claimed usecs per (micro)frame */
- hsotg->periodic_usecs -= qh->usecs;
+ /*
+ * Schedule the unreserve to happen in a little bit. Cases here:
+ * - Unreserve worker might be sitting there waiting to grab the lock.
+ * In this case it will notice it's been schedule again and will
+ * quit.
+ * - Unreserve worker might not be scheduled.
+ *
+ * We should never already be scheduled since dwc2_schedule_periodic()
+ * should have canceled the scheduled unreserve timer (hence the
+ * warning on did_modify).
+ *
+ * We add + 1 to the timer to guarantee that at least 1 jiffy has
+ * passed (otherwise if the jiffy counter might tick right after we
+ * read it and we'll get no delay).
+ */
+ did_modify = mod_timer(&qh->unreserve_timer,
+ jiffies + DWC2_UNRESERVE_DELAY + 1);
+ WARN_ON(did_modify);
+ qh->unreserve_pending = 1;
- if (hsotg->core_params->uframe_sched > 0) {
- for (i = 0; i < 8; i++) {
- hsotg->frame_usecs[i] += qh->frame_usecs[i];
- qh->frame_usecs[i] = 0;
- }
- } else {
- /* Release periodic channel reservation */
- hsotg->periodic_channels--;
- }
+ list_del_init(&qh->qh_list_entry);
}
/**
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame |
| Message-ID | <qW6YX-4gM-49@gated-at.bofh.it> |
| In reply to | #1321397 |
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 <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
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);
+
+ /*
+ * Roughly estimate bus time for everything in the periodic
+ * queue + our new transfer. This is "rough" because we're
+ * using a function that makes takes into account IN/OUT
+ * and INT/ISO and we're just slamming in one value for all
+ * transfers. This should be an over-estimate and that should
+ * be OK, but we can probably tighten it.
+ */
+ xfer_ns = usb_calc_bus_time(host_speed, false, false,
+ chan->xfer_len + bytes_in_fifo);
+ xfer_us = NS_TO_US(xfer_ns);
+
+ /* See what frame number we'll be at by the time we finish */
+ frame_number = dwc2_hcd_get_future_frame_number(hsotg, xfer_us);
+
+ /* This is when we were scheduled to be on the wire */
+ wire_frame = dwc2_frame_num_inc(chan->qh->next_active_frame, 1);
+
+ /*
+ * If we'd finish _after_ the frame we're scheduled in then
+ * it's hopeless. Just schedule right away and hope for the
+ * best. Note that it _might_ be wise to call back into the
+ * scheduler to pick a better frame, but this is better than
+ * nothing.
+ */
+ if (dwc2_frame_num_gt(frame_number, wire_frame)) {
+ dwc2_sch_vdbg(hsotg,
+ "QH=%p EO MISS fr=%04x=>%04x (%+d)\n",
+ chan->qh, wire_frame, frame_number,
+ dwc2_frame_num_dec(frame_number,
+ wire_frame));
+ wire_frame = frame_number;
+
+ /*
+ * We picked a different frame number; communicate this
+ * back to the scheduler so it doesn't try to schedule
+ * another in the same frame.
+ *
+ * Remember that next_active_frame is 1 before the wire
+ * frame.
+ */
+ chan->qh->next_active_frame =
+ dwc2_frame_num_dec(frame_number, 1);
+ }
+
+ if (wire_frame & 1)
*hcchar |= HCCHAR_ODDFRM;
+ else
+ *hcchar &= ~HCCHAR_ODDFRM;
}
}
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 3abb34a5fc5b..5f909747b5a4 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -985,6 +985,14 @@ static int dwc2_next_periodic_start(struct dwc2_hsotg *hsotg,
* and next_active_frame are always 1 frame before we want things
* to be active and we assume we can still get scheduled in the
* current frame number.
+ * - It's possible for start_active_frame (now incremented) to be
+ * next_active_frame if we got an EO MISS (even_odd miss) which
+ * basically means that we detected there wasn't enough time for
+ * the last packet and dwc2_hc_set_even_odd_frame() rescheduled us
+ * at the last second. We want to make sure we don't schedule
+ * another transfer for the same frame. My test webcam doesn't seem
+ * terribly upset by missing a transfer but really doesn't like when
+ * we do two transfers in the same frame.
* - Some misses are expected. Specifically, in order to work
* perfectly dwc2 really needs quite spectacular interrupt latency
* requirements. It needs to be able to handle its interrupts
@@ -995,7 +1003,8 @@ static int dwc2_next_periodic_start(struct dwc2_hsotg *hsotg,
* guarantee that a system will have interrupt latency < 125 us, so
* we have to be robust to some misses.
*/
- if (dwc2_frame_num_gt(prev_frame_number, qh->start_active_frame)) {
+ if (qh->start_active_frame == qh->next_active_frame ||
+ dwc2_frame_num_gt(prev_frame_number, qh->start_active_frame)) {
u16 ideal_start = qh->start_active_frame;
/* Adjust interval as per gcd with plan length. */
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Kever Yang <kever.yang@rock-chips.com> |
|---|---|
| Date | 2016-02-02 08:50 +0100 |
| Subject | Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame |
| Message-ID | <qXDSO-7AO-11@gated-at.bofh.it> |
| In reply to | #1321421 |
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 <dianders@chromium.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> 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);
> +
> + /*
> + * Roughly estimate bus time for everything in the periodic
> + * queue + our new transfer. This is "rough" because we're
> + * using a function that makes takes into account IN/OUT
> + * and INT/ISO and we're just slamming in one value for all
> + * transfers. This should be an over-estimate and that should
> + * be OK, but we can probably tighten it.
> + */
> + xfer_ns = usb_calc_bus_time(host_speed, false, false,
> + chan->xfer_len + bytes_in_fifo);
> + xfer_us = NS_TO_US(xfer_ns);
> +
> + /* See what frame number we'll be at by the time we finish */
> + frame_number = dwc2_hcd_get_future_frame_number(hsotg, xfer_us);
> +
> + /* This is when we were scheduled to be on the wire */
> + wire_frame = dwc2_frame_num_inc(chan->qh->next_active_frame, 1);
> +
> + /*
> + * If we'd finish _after_ the frame we're scheduled in then
> + * it's hopeless. Just schedule right away and hope for the
> + * best. Note that it _might_ be wise to call back into the
> + * scheduler to pick a better frame, but this is better than
> + * nothing.
> + */
> + if (dwc2_frame_num_gt(frame_number, wire_frame)) {
> + dwc2_sch_vdbg(hsotg,
> + "QH=%p EO MISS fr=%04x=>%04x (%+d)\n",
> + chan->qh, wire_frame, frame_number,
> + dwc2_frame_num_dec(frame_number,
> + wire_frame));
> + wire_frame = frame_number;
> +
> + /*
> + * We picked a different frame number; communicate this
> + * back to the scheduler so it doesn't try to schedule
> + * another in the same frame.
> + *
> + * Remember that next_active_frame is 1 before the wire
> + * frame.
> + */
> + chan->qh->next_active_frame =
> + dwc2_frame_num_dec(frame_number, 1);
> + }
> +
> + if (wire_frame & 1)
> *hcchar |= HCCHAR_ODDFRM;
> + else
> + *hcchar &= ~HCCHAR_ODDFRM;
> }
> }
>
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 3abb34a5fc5b..5f909747b5a4 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -985,6 +985,14 @@ static int dwc2_next_periodic_start(struct dwc2_hsotg *hsotg,
> * and next_active_frame are always 1 frame before we want things
> * to be active and we assume we can still get scheduled in the
> * current frame number.
> + * - It's possible for start_active_frame (now incremented) to be
> + * next_active_frame if we got an EO MISS (even_odd miss) which
> + * basically means that we detected there wasn't enough time for
> + * the last packet and dwc2_hc_set_even_odd_frame() rescheduled us
> + * at the last second. We want to make sure we don't schedule
> + * another transfer for the same frame. My test webcam doesn't seem
> + * terribly upset by missing a transfer but really doesn't like when
> + * we do two transfers in the same frame.
> * - Some misses are expected. Specifically, in order to work
> * perfectly dwc2 really needs quite spectacular interrupt latency
> * requirements. It needs to be able to handle its interrupts
> @@ -995,7 +1003,8 @@ static int dwc2_next_periodic_start(struct dwc2_hsotg *hsotg,
> * guarantee that a system will have interrupt latency < 125 us, so
> * we have to be robust to some misses.
> */
> - if (dwc2_frame_num_gt(prev_frame_number, qh->start_active_frame)) {
> + if (qh->start_active_frame == qh->next_active_frame ||
> + dwc2_frame_num_gt(prev_frame_number, qh->start_active_frame)) {
> u16 ideal_start = qh->start_active_frame;
>
> /* Adjust interval as per gcd with plan length. */
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-02-02 23:50 +0100 |
| Subject | Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame |
| Message-ID | <qXRVM-1tU-17@gated-at.bofh.it> |
| In reply to | #1323856 |
Kever,
On Mon, Feb 1, 2016 at 11:46 PM, Kever Yang <kever.yang@rock-chips.com> 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
<https://chromium-review.googlesource.com/325451> 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) fn=32e9, sch=32e8=>32e9 (+1) miss=0
87.815391 QH=e2fd7880 IMM ready fn=32e9, nxt=32e9
87.815491 QH=e2fd7880 next(0) fn=32ea, sch=32e9=>32ea (+1) miss=0
87.815493 QH=e2fd7880 IMM ready fn=32ea, nxt=32ea
87.815635 QH=e2fd7880 next(0) fn=32eb, sch=32ea=>32eb (+1) miss=0
87.815638 QH=e2fd7880 IMM ready fn=32eb, nxt=32eb
Note that with my TEST-ONLY patch the old way is still _slightly_
different in that I still communicate back to the scheduler with:
chan->qh->next_active_frame = now_frame;
The old code didn't used to do that. If I don't do that then you
you'll just stay in an inconsistent state for a while where things are
going on the wire 1 frame later than we think they are.
Also note that above you can see that the new way is indeed able to
schedule things in the current microframe. Looking one line at a
time:
87.815012 QH=e2fd7880 IMM ready fn=32e6, nxt=32e6
QH e2fd7880 is going straight to the ready queue. Actual frame number
in hardware is 32e6. next_active_frame = 32e6 which means we ideally
want to give it to hardware in 32e6 and wire frame is 32e7.
87.815220 QH=e2fd7880 next(0) fn=32e8, sch=32e6=>32e7 (+1) miss=0
87.815222 QH=e2fd7880 IMM ready fn=32e8, nxt=32e7
Frame number in hardware is now 32e8. We'd like to give the next
transfer to hardware in 32e7 to transfer on the wire at 32e8, but
that's obviously impossible. We will try to give it right away.
87.815230 QH=e2fd7880, wire=32e8, old_wire=32e9, EO diff (use NEW)
Showing a difference in the old way. We'll choose "even" to have the
packet go on the wire (expecting 32e8).
87.815278 QH=e2fd7880 next(0) fn=32e8, sch=32e7=>32e8 (+1) miss=0
87.815280 QH=e2fd7880 IMM ready fn=32e8, nxt=32e8
We got a response back and are ready to schedule the next transfer and
it's still 32e8! That means that transfer must have happened (as
expected) in 32e8. Whew! Give the next transfer to hardware hoping
for 32e9 wire.
87.815390 QH=e2fd7880 next(0) fn=32e9, sch=32e8=>32e9 (+1) miss=0
Now at hardware 32e9 and ready to schedule the next...
> 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.
The docbook is obviously way too terse here, but the above experiment
shows that the hardware is designed in the only sane way that it could
be designed.
Why do I say that this is the only sane way for the hardware to work?
I think all the following is true (please correct any errors):
A) HW only lets you specify even/odd which means you choose between
two frame to send the packet. Two possible ways HW could be
implemented: "sane" way means you can send a packet in frame "x" and
"x + 1". "insane" way means you can send a packet in frame "x + 1"
and "x + 2" but not frame "x"
B) In some cases (especially with regards to SPLIT transfers), we need
to use the result of a transfer in uFrame "x" to decide what to do
about uFrame "x + 1". Specifically for IN transfers I think we can't
know for sure whether we'll get back all of our data in uFrame "x" or
whether we'll only get part of the data and need uFrame "x + 1".
C) It's possible to schedule 100us worth of periodic transfers in one
125us uFrame.
D) We can't know the result of a transfer until that transfer is done.
So above basically means that we might have a periodic transfer where
we get the result of the transfer 100us into a uFrame. We've now got
to quickly queue up the transfer for the next uFrame. If hardware was
designed in the "insane" way then we'd need an interrupt latency of <
25 us since once the frame ticked we'd no longer be able to schedule.
If hardware was designed in the "sane" way then we'd "only" need an
interrupt latency of 125 us since we could continue to schedule even
partway through the current frame.
Also note that if there's any chance that a periodic transfer ends
later than 100 us into a frame (like if a non-periodic transfer snuck
in there because we were out of periodic channels) then the above
problem becomes even more extreme.
-Doug
[toc] | [prev] | [next] | [standalone]
| From | Kever Yang <kever.yang@rock-chips.com> |
|---|---|
| Date | 2016-02-03 08:50 +0100 |
| Subject | Re: [PATCH v6 20/22] usb: dwc2: host: Properly set even/odd frame |
| Message-ID | <qY0ml-7ok-1@gated-at.bofh.it> |
| In reply to | #1324703 |
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 <kever.yang@rock-chips.com> 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
> <https://chromium-review.googlesource.com/325451> 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) fn=32e9, sch=32e8=>32e9 (+1) miss=0
> 87.815391 QH=e2fd7880 IMM ready fn=32e9, nxt=32e9
> 87.815491 QH=e2fd7880 next(0) fn=32ea, sch=32e9=>32ea (+1) miss=0
> 87.815493 QH=e2fd7880 IMM ready fn=32ea, nxt=32ea
> 87.815635 QH=e2fd7880 next(0) fn=32eb, sch=32ea=>32eb (+1) miss=0
> 87.815638 QH=e2fd7880 IMM ready fn=32eb, nxt=32eb
>
>
> Note that with my TEST-ONLY patch the old way is still _slightly_
> different in that I still communicate back to the scheduler with:
>
> chan->qh->next_active_frame = now_frame;
>
> The old code didn't used to do that. If I don't do that then you
> you'll just stay in an inconsistent state for a while where things are
> going on the wire 1 frame later than we think they are.
>
>
> Also note that above you can see that the new way is indeed able to
> schedule things in the current microframe. Looking one line at a
> time:
>
>
> 87.815012 QH=e2fd7880 IMM ready fn=32e6, nxt=32e6
>
> QH e2fd7880 is going straight to the ready queue. Actual frame number
> in hardware is 32e6. next_active_frame = 32e6 which means we ideally
> want to give it to hardware in 32e6 and wire frame is 32e7.
>
>
> 87.815220 QH=e2fd7880 next(0) fn=32e8, sch=32e6=>32e7 (+1) miss=0
> 87.815222 QH=e2fd7880 IMM ready fn=32e8, nxt=32e7
>
> Frame number in hardware is now 32e8. We'd like to give the next
> transfer to hardware in 32e7 to transfer on the wire at 32e8, but
> that's obviously impossible. We will try to give it right away.
>
>
> 87.815230 QH=e2fd7880, wire=32e8, old_wire=32e9, EO diff (use NEW)
>
> Showing a difference in the old way. We'll choose "even" to have the
> packet go on the wire (expecting 32e8).
>
>
> 87.815278 QH=e2fd7880 next(0) fn=32e8, sch=32e7=>32e8 (+1) miss=0
> 87.815280 QH=e2fd7880 IMM ready fn=32e8, nxt=32e8
>
> We got a response back and are ready to schedule the next transfer and
> it's still 32e8! That means that transfer must have happened (as
> expected) in 32e8. Whew! Give the next transfer to hardware hoping
> for 32e9 wire.
>
>
> 87.815390 QH=e2fd7880 next(0) fn=32e9, sch=32e8=>32e9 (+1) miss=0
>
> Now at hardware 32e9 and ready to schedule the next...
>
>
>
>> 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.
> The docbook is obviously way too terse here, but the above experiment
> shows that the hardware is designed in the only sane way that it could
> be designed.
>
> Why do I say that this is the only sane way for the hardware to work?
> I think all the following is true (please correct any errors):
>
> A) HW only lets you specify even/odd which means you choose between
> two frame to send the packet. Two possible ways HW could be
> implemented: "sane" way means you can send a packet in frame "x" and
> "x + 1". "insane" way means you can send a packet in frame "x + 1"
> and "x + 2" but not frame "x"
>
> B) In some cases (especially with regards to SPLIT transfers), we need
> to use the result of a transfer in uFrame "x" to decide what to do
> about uFrame "x + 1". Specifically for IN transfers I think we can't
> know for sure whether we'll get back all of our data in uFrame "x" or
> whether we'll only get part of the data and need uFrame "x + 1".
>
> C) It's possible to schedule 100us worth of periodic transfers in one
> 125us uFrame.
>
> D) We can't know the result of a transfer until that transfer is done.
>
>
> So above basically means that we might have a periodic transfer where
> we get the result of the transfer 100us into a uFrame. We've now got
> to quickly queue up the transfer for the next uFrame. If hardware was
> designed in the "insane" way then we'd need an interrupt latency of <
> 25 us since once the frame ticked we'd no longer be able to schedule.
> If hardware was designed in the "sane" way then we'd "only" need an
> interrupt latency of 125 us since we could continue to schedule even
> partway through the current frame.
>
> Also note that if there's any chance that a periodic transfer ends
> later than 100 us into a frame (like if a non-periodic transfer snuck
> in there because we were out of periodic channels) then the above
> problem becomes even more extreme.
>
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
[toc] | [prev] | [next] | [standalone]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qW6YY-4gM-59@gated-at.bofh.it> |
| In reply to | #1321397 |
In dwc2_hcd_qh_deactivate() we will put some things on the
periodic_sched_ready list. These things won't be taken off the ready
list until the next SOF, which might be a little late. Let's put them
on right away.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.
Changes in v5: None
Changes in v4:
- Schedule periodic right away if it's time new for v4.
Changes in v3: None
Changes in v2: None
drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 9b3c435339ee..3abb34a5fc5b 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
* Note: we purposely use the frame_number from the "hsotg" structure
* since we know SOF interrupt will handle future frames.
*/
- if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
+ if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) {
+ enum dwc2_transaction_type tr_type;
+
+ /*
+ * We're bypassing the SOF handler which is normally what puts
+ * us on the ready list because we're in a hurry and need to
+ * try to catch up.
+ */
+ dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, nxt=%04x\n",
+ qh, frame_number, qh->next_active_frame);
list_move_tail(&qh->qh_list_entry,
&hsotg->periodic_sched_ready);
- else
+
+ tr_type = dwc2_hcd_select_transactions(hsotg);
+ if (tr_type != DWC2_TRANSACTION_NONE)
+ dwc2_hcd_queue_transactions(hsotg, tr_type);
+ } else {
list_move_tail(&qh->qh_list_entry,
&hsotg->periodic_sched_inactive);
+ }
}
/**
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Kever Yang <kever.yang@rock-chips.com> |
|---|---|
| Date | 2016-01-31 10:40 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qWWEa-1hh-19@gated-at.bofh.it> |
| In reply to | #1321422 |
Doug,
On 01/29/2016 10:20 AM, Douglas Anderson wrote:
> In dwc2_hcd_qh_deactivate() we will put some things on the
> periodic_sched_ready list. These things won't be taken off the ready
> list until the next SOF, which might be a little late. Let's put them
> on right away.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> Changes in v6:
> - Add Heiko's Tested-by.
> - Add Stefan's Tested-by.
>
> Changes in v5: None
> Changes in v4:
> - Schedule periodic right away if it's time new for v4.
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 9b3c435339ee..3abb34a5fc5b 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
> * Note: we purposely use the frame_number from the "hsotg" structure
> * since we know SOF interrupt will handle future frames.
> */
> - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
> + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) {
> + enum dwc2_transaction_type tr_type;
> +
> + /*
> + * We're bypassing the SOF handler which is normally what puts
> + * us on the ready list because we're in a hurry and need to
> + * try to catch up.
> + */
> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, nxt=%04x\n",
> + qh, frame_number, qh->next_active_frame);
> list_move_tail(&qh->qh_list_entry,
> &hsotg->periodic_sched_ready);
> - else
> +
> + tr_type = dwc2_hcd_select_transactions(hsotg);
Do we need to add select_transactions call here? If we get into this
function in interrupt
and once we put the qh in ready queue, the qh can be handled in this
frame again by the
later function call of dwc_hcd_select_transactions, so what we need to
to here is put
it in ready list instead of inactive queue, and wait for the schedule.
Thanks,
- Kever
> + if (tr_type != DWC2_TRANSACTION_NONE)
> + dwc2_hcd_queue_transactions(hsotg, tr_type);
> + } else {
> list_move_tail(&qh->qh_list_entry,
> &hsotg->periodic_sched_inactive);
> + }
> }
>
> /**
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-31 23:10 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qX8lY-1mU-13@gated-at.bofh.it> |
| In reply to | #1322593 |
Kever,
On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Doug,
>
>
> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>
>> In dwc2_hcd_qh_deactivate() we will put some things on the
>> periodic_sched_ready list. These things won't be taken off the ready
>> list until the next SOF, which might be a little late. Let's put them
>> on right away.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>> Changes in v6:
>> - Add Heiko's Tested-by.
>> - Add Stefan's Tested-by.
>>
>> Changes in v5: None
>> Changes in v4:
>> - Schedule periodic right away if it's time new for v4.
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>> index 9b3c435339ee..3abb34a5fc5b 100644
>> --- a/drivers/usb/dwc2/hcd_queue.c
>> +++ b/drivers/usb/dwc2/hcd_queue.c
>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>> *hsotg, struct dwc2_qh *qh,
>> * Note: we purposely use the frame_number from the "hsotg"
>> structure
>> * since we know SOF interrupt will handle future frames.
>> */
>> - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>> + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>> {
>> + enum dwc2_transaction_type tr_type;
>> +
>> + /*
>> + * We're bypassing the SOF handler which is normally what
>> puts
>> + * us on the ready list because we're in a hurry and need
>> to
>> + * try to catch up.
>> + */
>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>> nxt=%04x\n",
>> + qh, frame_number, qh->next_active_frame);
>> list_move_tail(&qh->qh_list_entry,
>> &hsotg->periodic_sched_ready);
>> - else
>> +
>> + tr_type = dwc2_hcd_select_transactions(hsotg);
>
> Do we need to add select_transactions call here? If we get into this
> function in interrupt
> and once we put the qh in ready queue, the qh can be handled in this frame
> again by the
> later function call of dwc_hcd_select_transactions, so what we need to to
> here is put
> it in ready list instead of inactive queue, and wait for the schedule.
I'm not sure I understand. Can you restate?
I'll try to explain more in the meantime...
Both before and after my change, this function would place something
on the ready queue if the next_active_frame <= the frame number as of
last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on
the inactive queue. Assuming that the previous change ("usb: dwc2:
host: Manage frame nums better in scheduler") worked properly then
next_active_frame shouldn't be less than (hsotg->frame_number - 1).
Remember that next_active_frame is always 1 before the wire frame, so
if "next_active_frame == hsotg->frame_number - 1" it means that we
need to get the transfer on the wire _right away_. If
"next_active_frame == hsotg->frame_number" the transfer doesn't need
to go on the wire right away, but since dwc2 can be prepped one frame
in advance it doesn't hurt to give it to the hardware right away if
there's space.
As I understand it, if we stick something on the ready queue it won't
generally get looked at until the next SOF interrupt. That means
we'll be too late if "next_active_frame == hsotg->frame_number - 1"
and we'll possibly be too late (depending on interrupt latency) if
"next_active_frame == hsotg->frame_number"
Note that before my series, there were more places than just the SOF
interrupt that would update hsotg->frame_number (see "usb: dwc2: host:
Manage frame nums better in scheduler" for fix). Also before my
series (specially "usb: dwc2: host: Manage frame nums better in
scheduler") we used the actual current frame number when doing
comparisons. Also before my series (specifically "usb: dwc2: host:
Properly set even/odd frame") we didn't really place things in the
frame that they were scheduled in anyway.
Also note that I believe that when dwc2_hcd_qh_deactivate() is called
our spinlock is held which means that the SOF interrupt either ran
before our function or won't run till after it.
>
>> + if (tr_type != DWC2_TRANSACTION_NONE)
>> + dwc2_hcd_queue_transactions(hsotg, tr_type);
>> + } else {
>> list_move_tail(&qh->qh_list_entry,
>> &hsotg->periodic_sched_inactive);
>> + }
>> }
>> /**
>
>
>
[toc] | [prev] | [next] | [standalone]
| From | Kever Yang <kever.yang@rock-chips.com> |
|---|---|
| Date | 2016-02-01 04:40 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qXdvk-4X4-5@gated-at.bofh.it> |
| In reply to | #1322748 |
Doug,
On 02/01/2016 06:09 AM, Doug Anderson wrote:
> Kever,
>
> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Doug,
>>
>>
>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>> periodic_sched_ready list. These things won't be taken off the ready
>>> list until the next SOF, which might be a little late. Let's put them
>>> on right away.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> Changes in v6:
>>> - Add Heiko's Tested-by.
>>> - Add Stefan's Tested-by.
>>>
>>> Changes in v5: None
>>> Changes in v4:
>>> - Schedule periodic right away if it's time new for v4.
>>>
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>> *hsotg, struct dwc2_qh *qh,
>>> * Note: we purposely use the frame_number from the "hsotg"
>>> structure
>>> * since we know SOF interrupt will handle future frames.
>>> */
>>> - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>>> + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
>>> {
>>> + enum dwc2_transaction_type tr_type;
>>> +
>>> + /*
>>> + * We're bypassing the SOF handler which is normally what
>>> puts
>>> + * us on the ready list because we're in a hurry and need
>>> to
>>> + * try to catch up.
>>> + */
>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>> nxt=%04x\n",
>>> + qh, frame_number, qh->next_active_frame);
>>> list_move_tail(&qh->qh_list_entry,
>>> &hsotg->periodic_sched_ready);
>>> - else
>>> +
>>> + tr_type = dwc2_hcd_select_transactions(hsotg);
>> Do we need to add select_transactions call here? If we get into this
>> function in interrupt
>> and once we put the qh in ready queue, the qh can be handled in this frame
>> again by the
>> later function call of dwc_hcd_select_transactions, so what we need to to
>> here is put
>> it in ready list instead of inactive queue, and wait for the schedule.
> I'm not sure I understand. Can you restate?
>
>
> I'll try to explain more in the meantime...
>
> Both before and after my change, this function would place something
> on the ready queue if the next_active_frame <= the frame number as of
> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on
> the inactive queue. Assuming that the previous change ("usb: dwc2:
> host: Manage frame nums better in scheduler") worked properly then
> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
> Remember that next_active_frame is always 1 before the wire frame, so
> if "next_active_frame == hsotg->frame_number - 1" it means that we
> need to get the transfer on the wire _right away_. If
> "next_active_frame == hsotg->frame_number" the transfer doesn't need
> to go on the wire right away, but since dwc2 can be prepped one frame
> in advance it doesn't hurt to give it to the hardware right away if
> there's space.
>
> As I understand it, if we stick something on the ready queue it won't
> generally get looked at until the next SOF interrupt. That means
> we'll be too late if "next_active_frame == hsotg->frame_number - 1"
> and we'll possibly be too late (depending on interrupt latency) if
> "next_active_frame == hsotg->frame_number"
>
I understand this patch and agree with your point of schedule the
periodic right away instead of at least next frame.
My point is, there are only two call to dwc2_hcd_qh_deactivate(), from
dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need
to do the schedule for dequeue, and there is one
dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(),
maybe we don't need another dwc2_hcd_select_transactions() here.
I think the duration from this point to the function call of
dwc2_hcd_select_transactions()
in dwc2_release_channel() will be the main factor for us to decide if
we need to add a function call of dwc2_hcd_select_transactions() here.
Thanks,
- Kever
> Note that before my series, there were more places than just the SOF
> interrupt that would update hsotg->frame_number (see "usb: dwc2: host:
> Manage frame nums better in scheduler" for fix). Also before my
> series (specially "usb: dwc2: host: Manage frame nums better in
> scheduler") we used the actual current frame number when doing
> comparisons. Also before my series (specifically "usb: dwc2: host:
> Properly set even/odd frame") we didn't really place things in the
> frame that they were scheduled in anyway.
>
>
> Also note that I believe that when dwc2_hcd_qh_deactivate() is called
> our spinlock is held which means that the SOF interrupt either ran
> before our function or won't run till after it.
>
>>> + if (tr_type != DWC2_TRANSACTION_NONE)
>>> + dwc2_hcd_queue_transactions(hsotg, tr_type);
>>> + } else {
>>> list_move_tail(&qh->qh_list_entry,
>>> &hsotg->periodic_sched_inactive);
>>> + }
>>> }
>>> /**
>>
>>
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-02-01 05:40 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qXero-5EF-7@gated-at.bofh.it> |
| In reply to | #1322807 |
Kever,
On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Doug,
>
>
> On 02/01/2016 06:09 AM, Doug Anderson wrote:
>>
>> Kever,
>>
>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com>
>> wrote:
>>>
>>> Doug,
>>>
>>>
>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>>>
>>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>>> periodic_sched_ready list. These things won't be taken off the ready
>>>> list until the next SOF, which might be a little late. Let's put them
>>>> on right away.
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> ---
>>>> Changes in v6:
>>>> - Add Heiko's Tested-by.
>>>> - Add Stefan's Tested-by.
>>>>
>>>> Changes in v5: None
>>>> Changes in v4:
>>>> - Schedule periodic right away if it's time new for v4.
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
>>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>>> *hsotg, struct dwc2_qh *qh,
>>>> * Note: we purposely use the frame_number from the "hsotg"
>>>> structure
>>>> * since we know SOF interrupt will handle future frames.
>>>> */
>>>> - if (dwc2_frame_num_le(qh->next_active_frame,
>>>> hsotg->frame_number))
>>>> + if (dwc2_frame_num_le(qh->next_active_frame,
>>>> hsotg->frame_number))
>>>> {
>>>> + enum dwc2_transaction_type tr_type;
>>>> +
>>>> + /*
>>>> + * We're bypassing the SOF handler which is normally
>>>> what
>>>> puts
>>>> + * us on the ready list because we're in a hurry and
>>>> need
>>>> to
>>>> + * try to catch up.
>>>> + */
>>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>>> nxt=%04x\n",
>>>> + qh, frame_number, qh->next_active_frame);
>>>> list_move_tail(&qh->qh_list_entry,
>>>> &hsotg->periodic_sched_ready);
>>>> - else
>>>> +
>>>> + tr_type = dwc2_hcd_select_transactions(hsotg);
>>>
>>> Do we need to add select_transactions call here? If we get into this
>>> function in interrupt
>>> and once we put the qh in ready queue, the qh can be handled in this
>>> frame
>>> again by the
>>> later function call of dwc_hcd_select_transactions, so what we need to to
>>> here is put
>>> it in ready list instead of inactive queue, and wait for the schedule.
>>
>> I'm not sure I understand. Can you restate?
>>
>>
>> I'll try to explain more in the meantime...
>>
>> Both before and after my change, this function would place something
>> on the ready queue if the next_active_frame <= the frame number as of
>> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on
>> the inactive queue. Assuming that the previous change ("usb: dwc2:
>> host: Manage frame nums better in scheduler") worked properly then
>> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
>> Remember that next_active_frame is always 1 before the wire frame, so
>> if "next_active_frame == hsotg->frame_number - 1" it means that we
>> need to get the transfer on the wire _right away_. If
>> "next_active_frame == hsotg->frame_number" the transfer doesn't need
>> to go on the wire right away, but since dwc2 can be prepped one frame
>> in advance it doesn't hurt to give it to the hardware right away if
>> there's space.
>>
>> As I understand it, if we stick something on the ready queue it won't
>> generally get looked at until the next SOF interrupt. That means
>> we'll be too late if "next_active_frame == hsotg->frame_number - 1"
>> and we'll possibly be too late (depending on interrupt latency) if
>> "next_active_frame == hsotg->frame_number"
>>
> I understand this patch and agree with your point of schedule the
> periodic right away instead of at least next frame.
> My point is, there are only two call to dwc2_hcd_qh_deactivate(), from
> dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need
> to do the schedule for dequeue, and there is one
> dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(),
> maybe we don't need another dwc2_hcd_select_transactions() here.
>
> I think the duration from this point to the function call of
> dwc2_hcd_select_transactions()
> in dwc2_release_channel() will be the main factor for us to decide if
> we need to add a function call of dwc2_hcd_select_transactions() here.
Oh, now I get what you're saying!
A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
dwc2_hcd_qh_deactivate()
...and always in that case we'll do a select / queue, so we don't need it there.
B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're
not continuing a split so timing isn't quite as urgent, but you still
might have an INT or ISOC packet that's scheduled with an interval of
1. We still might want to schedule right away if there are remaining
QTDs, right?
-Doug
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-02-02 01:40 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qXxaG-2zt-19@gated-at.bofh.it> |
| In reply to | #1322821 |
Kever,
On Sun, Jan 31, 2016 at 8:36 PM, Doug Anderson <dianders@chromium.org> wrote:
> Kever,
>
> On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Doug,
>>
>>
>> On 02/01/2016 06:09 AM, Doug Anderson wrote:
>>>
>>> Kever,
>>>
>>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com>
>>> wrote:
>>>>
>>>> Doug,
>>>>
>>>>
>>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>>>>
>>>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>>>> periodic_sched_ready list. These things won't be taken off the ready
>>>>> list until the next SOF, which might be a little late. Let's put them
>>>>> on right away.
>>>>>
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>> ---
>>>>> Changes in v6:
>>>>> - Add Heiko's Tested-by.
>>>>> - Add Stefan's Tested-by.
>>>>>
>>>>> Changes in v5: None
>>>>> Changes in v4:
>>>>> - Schedule periodic right away if it's time new for v4.
>>>>>
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>>
>>>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
>>>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>>>> *hsotg, struct dwc2_qh *qh,
>>>>> * Note: we purposely use the frame_number from the "hsotg"
>>>>> structure
>>>>> * since we know SOF interrupt will handle future frames.
>>>>> */
>>>>> - if (dwc2_frame_num_le(qh->next_active_frame,
>>>>> hsotg->frame_number))
>>>>> + if (dwc2_frame_num_le(qh->next_active_frame,
>>>>> hsotg->frame_number))
>>>>> {
>>>>> + enum dwc2_transaction_type tr_type;
>>>>> +
>>>>> + /*
>>>>> + * We're bypassing the SOF handler which is normally
>>>>> what
>>>>> puts
>>>>> + * us on the ready list because we're in a hurry and
>>>>> need
>>>>> to
>>>>> + * try to catch up.
>>>>> + */
>>>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>>>> nxt=%04x\n",
>>>>> + qh, frame_number, qh->next_active_frame);
>>>>> list_move_tail(&qh->qh_list_entry,
>>>>> &hsotg->periodic_sched_ready);
>>>>> - else
>>>>> +
>>>>> + tr_type = dwc2_hcd_select_transactions(hsotg);
>>>>
>>>> Do we need to add select_transactions call here? If we get into this
>>>> function in interrupt
>>>> and once we put the qh in ready queue, the qh can be handled in this
>>>> frame
>>>> again by the
>>>> later function call of dwc_hcd_select_transactions, so what we need to to
>>>> here is put
>>>> it in ready list instead of inactive queue, and wait for the schedule.
>>>
>>> I'm not sure I understand. Can you restate?
>>>
>>>
>>> I'll try to explain more in the meantime...
>>>
>>> Both before and after my change, this function would place something
>>> on the ready queue if the next_active_frame <= the frame number as of
>>> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on
>>> the inactive queue. Assuming that the previous change ("usb: dwc2:
>>> host: Manage frame nums better in scheduler") worked properly then
>>> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
>>> Remember that next_active_frame is always 1 before the wire frame, so
>>> if "next_active_frame == hsotg->frame_number - 1" it means that we
>>> need to get the transfer on the wire _right away_. If
>>> "next_active_frame == hsotg->frame_number" the transfer doesn't need
>>> to go on the wire right away, but since dwc2 can be prepped one frame
>>> in advance it doesn't hurt to give it to the hardware right away if
>>> there's space.
>>>
>>> As I understand it, if we stick something on the ready queue it won't
>>> generally get looked at until the next SOF interrupt. That means
>>> we'll be too late if "next_active_frame == hsotg->frame_number - 1"
>>> and we'll possibly be too late (depending on interrupt latency) if
>>> "next_active_frame == hsotg->frame_number"
>>>
>> I understand this patch and agree with your point of schedule the
>> periodic right away instead of at least next frame.
>> My point is, there are only two call to dwc2_hcd_qh_deactivate(), from
>> dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need
>> to do the schedule for dequeue, and there is one
>> dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(),
>> maybe we don't need another dwc2_hcd_select_transactions() here.
>>
>> I think the duration from this point to the function call of
>> dwc2_hcd_select_transactions()
>> in dwc2_release_channel() will be the main factor for us to decide if
>> we need to add a function call of dwc2_hcd_select_transactions() here.
>
> Oh, now I get what you're saying!
>
> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
> dwc2_hcd_qh_deactivate()
> ...and always in that case we'll do a select / queue, so we don't need it there.
>
> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
>
> ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're
> not continuing a split so timing isn't quite as urgent, but you still
> might have an INT or ISOC packet that's scheduled with an interval of
> 1. We still might want to schedule right away if there are remaining
> QTDs, right?
I ran out of time to fully test today, but I couldn't actually get a
case where we needed to schedule right away for B). ...so given your
point about the the select / queue already present in case A, we could
probably just drop this patch ("usb: dwc2: host: Schedule periodic
right away if it's time") and if we can find a case where it's needed
in case B we can add the select / queue there.
Sound OK? I'll try to do more testing tomorrow...
-Doug
[toc] | [prev] | [next] | [standalone]
| From | Kever Yang <kever.yang@rock-chips.com> |
|---|---|
| Date | 2016-02-02 08:10 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qXDg6-7hC-3@gated-at.bofh.it> |
| In reply to | #1323678 |
Doug,
On 02/02/2016 08:36 AM, Doug Anderson wrote:
> Kever,
>
> On Sun, Jan 31, 2016 at 8:36 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Kever,
>>
>> On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> Doug,
>>>
>>>
>>> On 02/01/2016 06:09 AM, Doug Anderson wrote:
>>>> Kever,
>>>>
>>>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com>
>>>> wrote:
>>>>> Doug,
>>>>>
>>>>>
>>>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote:
>>>>>> In dwc2_hcd_qh_deactivate() we will put some things on the
>>>>>> periodic_sched_ready list. These things won't be taken off the ready
>>>>>> list until the next SOF, which might be a little late. Let's put them
>>>>>> on right away.
>>>>>>
>>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>> ---
>>>>>> Changes in v6:
>>>>>> - Add Heiko's Tested-by.
>>>>>> - Add Stefan's Tested-by.
>>>>>>
>>>>>> Changes in v5: None
>>>>>> Changes in v4:
>>>>>> - Schedule periodic right away if it's time new for v4.
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
>>>>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++--
>>>>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>>>>>> index 9b3c435339ee..3abb34a5fc5b 100644
>>>>>> --- a/drivers/usb/dwc2/hcd_queue.c
>>>>>> +++ b/drivers/usb/dwc2/hcd_queue.c
>>>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg
>>>>>> *hsotg, struct dwc2_qh *qh,
>>>>>> * Note: we purposely use the frame_number from the "hsotg"
>>>>>> structure
>>>>>> * since we know SOF interrupt will handle future frames.
>>>>>> */
>>>>>> - if (dwc2_frame_num_le(qh->next_active_frame,
>>>>>> hsotg->frame_number))
>>>>>> + if (dwc2_frame_num_le(qh->next_active_frame,
>>>>>> hsotg->frame_number))
>>>>>> {
>>>>>> + enum dwc2_transaction_type tr_type;
>>>>>> +
>>>>>> + /*
>>>>>> + * We're bypassing the SOF handler which is normally
>>>>>> what
>>>>>> puts
>>>>>> + * us on the ready list because we're in a hurry and
>>>>>> need
>>>>>> to
>>>>>> + * try to catch up.
>>>>>> + */
>>>>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x,
>>>>>> nxt=%04x\n",
>>>>>> + qh, frame_number, qh->next_active_frame);
>>>>>> list_move_tail(&qh->qh_list_entry,
>>>>>> &hsotg->periodic_sched_ready);
>>>>>> - else
>>>>>> +
>>>>>> + tr_type = dwc2_hcd_select_transactions(hsotg);
>>>>> Do we need to add select_transactions call here? If we get into this
>>>>> function in interrupt
>>>>> and once we put the qh in ready queue, the qh can be handled in this
>>>>> frame
>>>>> again by the
>>>>> later function call of dwc_hcd_select_transactions, so what we need to to
>>>>> here is put
>>>>> it in ready list instead of inactive queue, and wait for the schedule.
>>>> I'm not sure I understand. Can you restate?
>>>>
>>>>
>>>> I'll try to explain more in the meantime...
>>>>
>>>> Both before and after my change, this function would place something
>>>> on the ready queue if the next_active_frame <= the frame number as of
>>>> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on
>>>> the inactive queue. Assuming that the previous change ("usb: dwc2:
>>>> host: Manage frame nums better in scheduler") worked properly then
>>>> next_active_frame shouldn't be less than (hsotg->frame_number - 1).
>>>> Remember that next_active_frame is always 1 before the wire frame, so
>>>> if "next_active_frame == hsotg->frame_number - 1" it means that we
>>>> need to get the transfer on the wire _right away_. If
>>>> "next_active_frame == hsotg->frame_number" the transfer doesn't need
>>>> to go on the wire right away, but since dwc2 can be prepped one frame
>>>> in advance it doesn't hurt to give it to the hardware right away if
>>>> there's space.
>>>>
>>>> As I understand it, if we stick something on the ready queue it won't
>>>> generally get looked at until the next SOF interrupt. That means
>>>> we'll be too late if "next_active_frame == hsotg->frame_number - 1"
>>>> and we'll possibly be too late (depending on interrupt latency) if
>>>> "next_active_frame == hsotg->frame_number"
>>>>
>>> I understand this patch and agree with your point of schedule the
>>> periodic right away instead of at least next frame.
>>> My point is, there are only two call to dwc2_hcd_qh_deactivate(), from
>>> dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need
>>> to do the schedule for dequeue, and there is one
>>> dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(),
>>> maybe we don't need another dwc2_hcd_select_transactions() here.
>>>
>>> I think the duration from this point to the function call of
>>> dwc2_hcd_select_transactions()
>>> in dwc2_release_channel() will be the main factor for us to decide if
>>> we need to add a function call of dwc2_hcd_select_transactions() here.
>> Oh, now I get what you're saying!
>>
>> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
>> dwc2_hcd_qh_deactivate()
>> ...and always in that case we'll do a select / queue, so we don't need it there.
>>
>> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
>>
>> ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're
>> not continuing a split so timing isn't quite as urgent, but you still
>> might have an INT or ISOC packet that's scheduled with an interval of
>> 1. We still might want to schedule right away if there are remaining
>> QTDs, right?
> I ran out of time to fully test today, but I couldn't actually get a
> case where we needed to schedule right away for B). ...so given your
> point about the the select / queue already present in case A, we could
> probably just drop this patch ("usb: dwc2: host: Schedule periodic
> right away if it's time") and if we can find a case where it's needed
> in case B we can add the select / queue there.
>
> Sound OK? I'll try to do more testing tomorrow...
Yes, we don't get a case we need to schedule right away for case B).
For INT or ISOC packet, I can recall I have seen somewhere but I can find
it now, the synchronous transfer is happen in the next uframe instead of
the uframe
when the host channel initialized, so there is no difference of setting the
host channel register sooner or later inside the same frame.
Which means the existent code should be OK for case A).
We can drop this patch before we have the exact use case.
Thanks,
- Kever
>
> -Doug
>
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-02-03 00:40 +0100 |
| Subject | Re: [PATCH v6 18/22] usb: dwc2: host: Schedule periodic right away if it's time |
| Message-ID | <qXSI9-29S-3@gated-at.bofh.it> |
| In reply to | #1323843 |
Kever,
On Mon, Feb 1, 2016 at 11:04 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> Oh, now I get what you're saying!
>>>
>>> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() ->
>>> dwc2_hcd_qh_deactivate()
>>> ...and always in that case we'll do a select / queue, so we don't need it
>>> there.
>>>
>>> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate()
>>>
>>> ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're
>>> not continuing a split so timing isn't quite as urgent, but you still
>>> might have an INT or ISOC packet that's scheduled with an interval of
>>> 1. We still might want to schedule right away if there are remaining
>>> QTDs, right?
>>
>> I ran out of time to fully test today, but I couldn't actually get a
>> case where we needed to schedule right away for B). ...so given your
>> point about the the select / queue already present in case A, we could
>> probably just drop this patch ("usb: dwc2: host: Schedule periodic
>> right away if it's time") and if we can find a case where it's needed
>> in case B we can add the select / queue there.
>>
>> Sound OK? I'll try to do more testing tomorrow...
>
> Yes, we don't get a case we need to schedule right away for case B).
>
> For INT or ISOC packet, I can recall I have seen somewhere but I can find
> it now, the synchronous transfer is happen in the next uframe instead of the
> uframe
> when the host channel initialized, so there is no difference of setting the
> host channel register sooner or later inside the same frame.
> Which means the existent code should be OK for case A).
>
> We can drop this patch before we have the exact use case.
I put in some printouts and I finally did manage to find a place where
we needed to queue things up in dwc2_hcd_urb_dequeue(). I saw:
314.587916: QH=d9535340 next(0) fn=2a52, sch=2a51=>2a52 (+1) miss=0
314.588040: QH=d9535340 next(0) fn=2a53, sch=2a52=>2a53 (+1) miss=0
314.588162: QH=d9535340 next(0) fn=2a54, sch=2a53=>2a54 (+1) miss=0
314.588299: QH=d9535340 next(0) fn=2a55, sch=2a54=>2a55 (+1) miss=0
314.588304: QH=d9535340 queue in dwc2_hcd_urb_dequeue
314.588363: QH=d9535340 next(0) fn=2a55, sch=2a55=>2a56 (+1) miss=0
314.588413: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea380 ready
fn=2a56, nxt=2a56
314.588414: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e73ccc40 ready
fn=2a56, nxt=2a56
314.588415: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea8c0 ready
fn=2a56, nxt=2a56
It's not something that's terribly common. It's fine to just drop
this patch, or I can replace it with
<https://chromium-review.googlesource.com/325540>.
-Doug
[toc] | [prev] | [next] | [standalone]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 14/22] usb: dwc2: host: Reorder things in hcd_queue.c |
| Message-ID | <qW6YY-4gM-55@gated-at.bofh.it> |
| In reply to | #1321397 |
This no-op change just reorders a few functions in hcd_queue.c in order
to prepare for future changes. Motivations here:
The functions dwc2_hcd_qh_free() and dwc2_hcd_qh_create() are exported
functions. They are not called within the file. That means that they
should be near the bottom so that they can easily call static helpers.
The function dwc2_qh_init() is only called by dwc2_hcd_qh_create() and
should move near the bottom with it.
The only reason that the dwc2_unreserve_timer_fn() timer function (and
its subroutine dwc2_do_unreserve()) were so high in the file was that
they needed to be above dwc2_qh_init(). Now that dwc2_qh_init() has
been moved down it can be moved down a bit. A later patch will split
the reserve code out of dwc2_schedule_periodic() and the reserve
function should be near the unreserve function. The reserve function
needs to be below dwc2_find_uframe() since it calls that.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.
Changes in v5: None
Changes in v4:
- Reorder things in hcd_queue.c new for v4.
Changes in v3: None
Changes in v2: None
drivers/usb/dwc2/hcd_queue.c | 600 +++++++++++++++++++++----------------------
1 file changed, 300 insertions(+), 300 deletions(-)
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 39f4de6279f8..8a2067bc1e62 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -57,295 +57,6 @@
#define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
/**
- * dwc2_do_unreserve() - Actually release the periodic reservation
- *
- * This function actually releases the periodic bandwidth that was reserved
- * by the given qh.
- *
- * @hsotg: The HCD state structure for the DWC OTG controller
- * @qh: QH for the periodic transfer.
- */
-static void dwc2_do_unreserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
-{
- assert_spin_locked(&hsotg->lock);
-
- WARN_ON(!qh->unreserve_pending);
-
- /* No more unreserve pending--we're doing it */
- qh->unreserve_pending = false;
-
- if (WARN_ON(!list_empty(&qh->qh_list_entry)))
- list_del_init(&qh->qh_list_entry);
-
- /* Update claimed usecs per (micro)frame */
- hsotg->periodic_usecs -= qh->host_us;
-
- if (hsotg->core_params->uframe_sched > 0) {
- int i;
-
- for (i = 0; i < 8; i++) {
- hsotg->frame_usecs[i] += qh->frame_usecs[i];
- qh->frame_usecs[i] = 0;
- }
- } else {
- /* Release periodic channel reservation */
- hsotg->periodic_channels--;
- }
-}
-
-/**
- * dwc2_unreserve_timer_fn() - Timer function to release periodic reservation
- *
- * According to the kernel doc for usb_submit_urb() (specifically the part about
- * "Reserved Bandwidth Transfers"), we need to keep a reservation active as
- * long as a device driver keeps submitting. Since we're using HCD_BH to give
- * back the URB we need to give the driver a little bit of time before we
- * release the reservation. This worker is called after the appropriate
- * delay.
- *
- * @work: Pointer to a qh unreserve_work.
- */
-static void dwc2_unreserve_timer_fn(unsigned long data)
-{
- struct dwc2_qh *qh = (struct dwc2_qh *)data;
- struct dwc2_hsotg *hsotg = qh->hsotg;
- unsigned long flags;
-
- /*
- * Wait for the lock, or for us to be scheduled again. We
- * could be scheduled again if:
- * - We started executing but didn't get the lock yet.
- * - A new reservation came in, but cancel didn't take effect
- * because we already started executing.
- * - The timer has been kicked again.
- * In that case cancel and wait for the next call.
- */
- while (!spin_trylock_irqsave(&hsotg->lock, flags)) {
- if (timer_pending(&qh->unreserve_timer))
- return;
- }
-
- /*
- * Might be no more unreserve pending if:
- * - We started executing but didn't get the lock yet.
- * - A new reservation came in, but cancel didn't take effect
- * because we already started executing.
- *
- * We can't put this in the loop above because unreserve_pending needs
- * to be accessed under lock, so we can only check it once we got the
- * lock.
- */
- if (qh->unreserve_pending)
- dwc2_do_unreserve(hsotg, qh);
-
- spin_unlock_irqrestore(&hsotg->lock, flags);
-}
-
-/**
- * dwc2_qh_init() - Initializes a QH structure
- *
- * @hsotg: The HCD state structure for the DWC OTG controller
- * @qh: The QH to init
- * @urb: Holds the information about the device/endpoint needed to initialize
- * the QH
- */
-#define SCHEDULE_SLOP 10
-static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
- struct dwc2_hcd_urb *urb)
-{
- int dev_speed, hub_addr, hub_port;
- char *speed, *type;
-
- dev_vdbg(hsotg->dev, "%s()\n", __func__);
-
- /* Initialize QH */
- qh->hsotg = hsotg;
- setup_timer(&qh->unreserve_timer, dwc2_unreserve_timer_fn,
- (unsigned long)qh);
- qh->ep_type = dwc2_hcd_get_pipe_type(&urb->pipe_info);
- qh->ep_is_in = dwc2_hcd_is_pipe_in(&urb->pipe_info) ? 1 : 0;
-
- qh->data_toggle = DWC2_HC_PID_DATA0;
- qh->maxp = dwc2_hcd_get_mps(&urb->pipe_info);
- INIT_LIST_HEAD(&qh->qtd_list);
- INIT_LIST_HEAD(&qh->qh_list_entry);
-
- /* FS/LS Endpoint on HS Hub, NOT virtual root hub */
- dev_speed = dwc2_host_get_speed(hsotg, urb->priv);
-
- dwc2_host_hub_info(hsotg, urb->priv, &hub_addr, &hub_port);
-
- if ((dev_speed == USB_SPEED_LOW || dev_speed == USB_SPEED_FULL) &&
- hub_addr != 0 && hub_addr != 1) {
- dev_vdbg(hsotg->dev,
- "QH init: EP %d: TT found at hub addr %d, for port %d\n",
- dwc2_hcd_get_ep_num(&urb->pipe_info), hub_addr,
- hub_port);
- qh->do_split = 1;
- }
-
- if (qh->ep_type == USB_ENDPOINT_XFER_INT ||
- qh->ep_type == USB_ENDPOINT_XFER_ISOC) {
- /* Compute scheduling parameters once and save them */
- u32 hprt, prtspd;
-
- /* Todo: Account for split transfers in the bus time */
- int bytecount =
- dwc2_hb_mult(qh->maxp) * dwc2_max_packet(qh->maxp);
-
- qh->host_us = NS_TO_US(usb_calc_bus_time(qh->do_split ?
- USB_SPEED_HIGH : dev_speed, qh->ep_is_in,
- qh->ep_type == USB_ENDPOINT_XFER_ISOC,
- bytecount));
-
- /* Ensure frame_number corresponds to the reality */
- hsotg->frame_number = dwc2_hcd_get_frame_number(hsotg);
- /* Start in a slightly future (micro)frame */
- qh->next_active_frame = dwc2_frame_num_inc(hsotg->frame_number,
- SCHEDULE_SLOP);
- qh->host_interval = urb->interval;
- dwc2_sch_dbg(hsotg, "QH=%p init nxt=%04x, fn=%04x, int=%#x\n",
- qh, qh->next_active_frame, hsotg->frame_number,
- qh->host_interval);
-#if 0
- /* Increase interrupt polling rate for debugging */
- if (qh->ep_type == USB_ENDPOINT_XFER_INT)
- qh->host_interval = 8;
-#endif
- hprt = dwc2_readl(hsotg->regs + HPRT0);
- prtspd = (hprt & HPRT0_SPD_MASK) >> HPRT0_SPD_SHIFT;
- if (prtspd == HPRT0_SPD_HIGH_SPEED &&
- (dev_speed == USB_SPEED_LOW ||
- dev_speed == USB_SPEED_FULL)) {
- qh->host_interval *= 8;
- qh->next_active_frame |= 0x7;
- qh->start_split_frame = qh->next_active_frame;
- dwc2_sch_dbg(hsotg,
- "QH=%p init*8 nxt=%04x, fn=%04x, int=%#x\n",
- qh, qh->next_active_frame,
- hsotg->frame_number, qh->host_interval);
-
- }
- dev_dbg(hsotg->dev, "interval=%d\n", qh->host_interval);
- }
-
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH Initialized\n");
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - qh = %p\n", qh);
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Device Address = %d\n",
- dwc2_hcd_get_dev_addr(&urb->pipe_info));
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Endpoint %d, %s\n",
- dwc2_hcd_get_ep_num(&urb->pipe_info),
- dwc2_hcd_is_pipe_in(&urb->pipe_info) ? "IN" : "OUT");
-
- qh->dev_speed = dev_speed;
-
- switch (dev_speed) {
- case USB_SPEED_LOW:
- speed = "low";
- break;
- case USB_SPEED_FULL:
- speed = "full";
- break;
- case USB_SPEED_HIGH:
- speed = "high";
- break;
- default:
- speed = "?";
- break;
- }
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Speed = %s\n", speed);
-
- switch (qh->ep_type) {
- case USB_ENDPOINT_XFER_ISOC:
- type = "isochronous";
- break;
- case USB_ENDPOINT_XFER_INT:
- type = "interrupt";
- break;
- case USB_ENDPOINT_XFER_CONTROL:
- type = "control";
- break;
- case USB_ENDPOINT_XFER_BULK:
- type = "bulk";
- break;
- default:
- type = "?";
- break;
- }
-
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Type = %s\n", type);
-
- if (qh->ep_type == USB_ENDPOINT_XFER_INT) {
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - usecs = %d\n",
- qh->host_us);
- dev_vdbg(hsotg->dev, "DWC OTG HCD QH - interval = %d\n",
- qh->host_interval);
- }
-}
-
-/**
- * dwc2_hcd_qh_create() - Allocates and initializes a QH
- *
- * @hsotg: The HCD state structure for the DWC OTG controller
- * @urb: Holds the information about the device/endpoint needed
- * to initialize the QH
- * @atomic_alloc: Flag to do atomic allocation if needed
- *
- * Return: Pointer to the newly allocated QH, or NULL on error
- */
-struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
- struct dwc2_hcd_urb *urb,
- gfp_t mem_flags)
-{
- struct dwc2_qh *qh;
-
- if (!urb->priv)
- return NULL;
-
- /* Allocate memory */
- qh = kzalloc(sizeof(*qh), mem_flags);
- if (!qh)
- return NULL;
-
- dwc2_qh_init(hsotg, qh, urb);
-
- if (hsotg->core_params->dma_desc_enable > 0 &&
- dwc2_hcd_qh_init_ddma(hsotg, qh, mem_flags) < 0) {
- dwc2_hcd_qh_free(hsotg, qh);
- return NULL;
- }
-
- return qh;
-}
-
-/**
- * dwc2_hcd_qh_free() - Frees the QH
- *
- * @hsotg: HCD instance
- * @qh: The QH to free
- *
- * QH should already be removed from the list. QTD list should already be empty
- * if called from URB Dequeue.
- *
- * Must NOT be called with interrupt disabled or spinlock held
- */
-void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
-{
- /* Make sure any unreserve work is finished. */
- if (del_timer_sync(&qh->unreserve_timer)) {
- unsigned long flags;
-
- spin_lock_irqsave(&hsotg->lock, flags);
- dwc2_do_unreserve(hsotg, qh);
- spin_unlock_irqrestore(&hsotg->lock, flags);
- }
-
- if (qh->desc_list)
- dwc2_hcd_qh_free_ddma(hsotg, qh);
- kfree(qh);
-}
-
-/**
* dwc2_periodic_channel_available() - Checks that a channel is available for a
* periodic transfer
*
@@ -518,19 +229,104 @@ static int dwc2_find_multi_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
static int dwc2_find_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
{
- int ret;
+ int ret;
+
+ if (qh->dev_speed == USB_SPEED_HIGH) {
+ /* if this is a hs transaction we need a full frame */
+ ret = dwc2_find_single_uframe(hsotg, qh);
+ } else {
+ /*
+ * if this is a fs transaction we may need a sequence
+ * of frames
+ */
+ ret = dwc2_find_multi_uframe(hsotg, qh);
+ }
+ return ret;
+}
+
+/**
+ * dwc2_do_unreserve() - Actually release the periodic reservation
+ *
+ * This function actually releases the periodic bandwidth that was reserved
+ * by the given qh.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh: QH for the periodic transfer.
+ */
+static void dwc2_do_unreserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+ assert_spin_locked(&hsotg->lock);
+
+ WARN_ON(!qh->unreserve_pending);
+
+ /* No more unreserve pending--we're doing it */
+ qh->unreserve_pending = false;
+
+ if (WARN_ON(!list_empty(&qh->qh_list_entry)))
+ list_del_init(&qh->qh_list_entry);
+
+ /* Update claimed usecs per (micro)frame */
+ hsotg->periodic_usecs -= qh->host_us;
+
+ if (hsotg->core_params->uframe_sched > 0) {
+ int i;
+
+ for (i = 0; i < 8; i++) {
+ hsotg->frame_usecs[i] += qh->frame_usecs[i];
+ qh->frame_usecs[i] = 0;
+ }
+ } else {
+ /* Release periodic channel reservation */
+ hsotg->periodic_channels--;
+ }
+}
+
+/**
+ * dwc2_unreserve_timer_fn() - Timer function to release periodic reservation
+ *
+ * According to the kernel doc for usb_submit_urb() (specifically the part about
+ * "Reserved Bandwidth Transfers"), we need to keep a reservation active as
+ * long as a device driver keeps submitting. Since we're using HCD_BH to give
+ * back the URB we need to give the driver a little bit of time before we
+ * release the reservation. This worker is called after the appropriate
+ * delay.
+ *
+ * @work: Pointer to a qh unreserve_work.
+ */
+static void dwc2_unreserve_timer_fn(unsigned long data)
+{
+ struct dwc2_qh *qh = (struct dwc2_qh *)data;
+ struct dwc2_hsotg *hsotg = qh->hsotg;
+ unsigned long flags;
- if (qh->dev_speed == USB_SPEED_HIGH) {
- /* if this is a hs transaction we need a full frame */
- ret = dwc2_find_single_uframe(hsotg, qh);
- } else {
- /*
- * if this is a fs transaction we may need a sequence
- * of frames
- */
- ret = dwc2_find_multi_uframe(hsotg, qh);
+ /*
+ * Wait for the lock, or for us to be scheduled again. We
+ * could be scheduled again if:
+ * - We started executing but didn't get the lock yet.
+ * - A new reservation came in, but cancel didn't take effect
+ * because we already started executing.
+ * - The timer has been kicked again.
+ * In that case cancel and wait for the next call.
+ */
+ while (!spin_trylock_irqsave(&hsotg->lock, flags)) {
+ if (timer_pending(&qh->unreserve_timer))
+ return;
}
- return ret;
+
+ /*
+ * Might be no more unreserve pending if:
+ * - We started executing but didn't get the lock yet.
+ * - A new reservation came in, but cancel didn't take effect
+ * because we already started executing.
+ *
+ * We can't put this in the loop above because unreserve_pending needs
+ * to be accessed under lock, so we can only check it once we got the
+ * lock.
+ */
+ if (qh->unreserve_pending)
+ dwc2_do_unreserve(hsotg, qh);
+
+ spin_unlock_irqrestore(&hsotg->lock, flags);
}
/**
@@ -695,6 +491,210 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg,
}
/**
+ * dwc2_qh_init() - Initializes a QH structure
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh: The QH to init
+ * @urb: Holds the information about the device/endpoint needed to initialize
+ * the QH
+ */
+#define SCHEDULE_SLOP 10
+static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
+ struct dwc2_hcd_urb *urb)
+{
+ int dev_speed, hub_addr, hub_port;
+ char *speed, *type;
+
+ dev_vdbg(hsotg->dev, "%s()\n", __func__);
+
+ /* Initialize QH */
+ qh->hsotg = hsotg;
+ setup_timer(&qh->unreserve_timer, dwc2_unreserve_timer_fn,
+ (unsigned long)qh);
+ qh->ep_type = dwc2_hcd_get_pipe_type(&urb->pipe_info);
+ qh->ep_is_in = dwc2_hcd_is_pipe_in(&urb->pipe_info) ? 1 : 0;
+
+ qh->data_toggle = DWC2_HC_PID_DATA0;
+ qh->maxp = dwc2_hcd_get_mps(&urb->pipe_info);
+ INIT_LIST_HEAD(&qh->qtd_list);
+ INIT_LIST_HEAD(&qh->qh_list_entry);
+
+ /* FS/LS Endpoint on HS Hub, NOT virtual root hub */
+ dev_speed = dwc2_host_get_speed(hsotg, urb->priv);
+
+ dwc2_host_hub_info(hsotg, urb->priv, &hub_addr, &hub_port);
+
+ if ((dev_speed == USB_SPEED_LOW || dev_speed == USB_SPEED_FULL) &&
+ hub_addr != 0 && hub_addr != 1) {
+ dev_vdbg(hsotg->dev,
+ "QH init: EP %d: TT found at hub addr %d, for port %d\n",
+ dwc2_hcd_get_ep_num(&urb->pipe_info), hub_addr,
+ hub_port);
+ qh->do_split = 1;
+ }
+
+ if (qh->ep_type == USB_ENDPOINT_XFER_INT ||
+ qh->ep_type == USB_ENDPOINT_XFER_ISOC) {
+ /* Compute scheduling parameters once and save them */
+ u32 hprt, prtspd;
+
+ /* Todo: Account for split transfers in the bus time */
+ int bytecount =
+ dwc2_hb_mult(qh->maxp) * dwc2_max_packet(qh->maxp);
+
+ qh->host_us = NS_TO_US(usb_calc_bus_time(qh->do_split ?
+ USB_SPEED_HIGH : dev_speed, qh->ep_is_in,
+ qh->ep_type == USB_ENDPOINT_XFER_ISOC,
+ bytecount));
+
+ /* Ensure frame_number corresponds to the reality */
+ hsotg->frame_number = dwc2_hcd_get_frame_number(hsotg);
+ /* Start in a slightly future (micro)frame */
+ qh->next_active_frame = dwc2_frame_num_inc(hsotg->frame_number,
+ SCHEDULE_SLOP);
+ qh->host_interval = urb->interval;
+ dwc2_sch_dbg(hsotg, "QH=%p init nxt=%04x, fn=%04x, int=%#x\n",
+ qh, qh->next_active_frame, hsotg->frame_number,
+ qh->host_interval);
+#if 0
+ /* Increase interrupt polling rate for debugging */
+ if (qh->ep_type == USB_ENDPOINT_XFER_INT)
+ qh->host_interval = 8;
+#endif
+ hprt = dwc2_readl(hsotg->regs + HPRT0);
+ prtspd = (hprt & HPRT0_SPD_MASK) >> HPRT0_SPD_SHIFT;
+ if (prtspd == HPRT0_SPD_HIGH_SPEED &&
+ (dev_speed == USB_SPEED_LOW ||
+ dev_speed == USB_SPEED_FULL)) {
+ qh->host_interval *= 8;
+ qh->next_active_frame |= 0x7;
+ qh->start_split_frame = qh->next_active_frame;
+ dwc2_sch_dbg(hsotg,
+ "QH=%p init*8 nxt=%04x, fn=%04x, int=%#x\n",
+ qh, qh->next_active_frame,
+ hsotg->frame_number, qh->host_interval);
+
+ }
+ dev_dbg(hsotg->dev, "interval=%d\n", qh->host_interval);
+ }
+
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH Initialized\n");
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - qh = %p\n", qh);
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Device Address = %d\n",
+ dwc2_hcd_get_dev_addr(&urb->pipe_info));
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Endpoint %d, %s\n",
+ dwc2_hcd_get_ep_num(&urb->pipe_info),
+ dwc2_hcd_is_pipe_in(&urb->pipe_info) ? "IN" : "OUT");
+
+ qh->dev_speed = dev_speed;
+
+ switch (dev_speed) {
+ case USB_SPEED_LOW:
+ speed = "low";
+ break;
+ case USB_SPEED_FULL:
+ speed = "full";
+ break;
+ case USB_SPEED_HIGH:
+ speed = "high";
+ break;
+ default:
+ speed = "?";
+ break;
+ }
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Speed = %s\n", speed);
+
+ switch (qh->ep_type) {
+ case USB_ENDPOINT_XFER_ISOC:
+ type = "isochronous";
+ break;
+ case USB_ENDPOINT_XFER_INT:
+ type = "interrupt";
+ break;
+ case USB_ENDPOINT_XFER_CONTROL:
+ type = "control";
+ break;
+ case USB_ENDPOINT_XFER_BULK:
+ type = "bulk";
+ break;
+ default:
+ type = "?";
+ break;
+ }
+
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - Type = %s\n", type);
+
+ if (qh->ep_type == USB_ENDPOINT_XFER_INT) {
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - usecs = %d\n",
+ qh->host_us);
+ dev_vdbg(hsotg->dev, "DWC OTG HCD QH - interval = %d\n",
+ qh->host_interval);
+ }
+}
+
+/**
+ * dwc2_hcd_qh_create() - Allocates and initializes a QH
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @urb: Holds the information about the device/endpoint needed
+ * to initialize the QH
+ * @atomic_alloc: Flag to do atomic allocation if needed
+ *
+ * Return: Pointer to the newly allocated QH, or NULL on error
+ */
+struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
+ struct dwc2_hcd_urb *urb,
+ gfp_t mem_flags)
+{
+ struct dwc2_qh *qh;
+
+ if (!urb->priv)
+ return NULL;
+
+ /* Allocate memory */
+ qh = kzalloc(sizeof(*qh), mem_flags);
+ if (!qh)
+ return NULL;
+
+ dwc2_qh_init(hsotg, qh, urb);
+
+ if (hsotg->core_params->dma_desc_enable > 0 &&
+ dwc2_hcd_qh_init_ddma(hsotg, qh, mem_flags) < 0) {
+ dwc2_hcd_qh_free(hsotg, qh);
+ return NULL;
+ }
+
+ return qh;
+}
+
+/**
+ * dwc2_hcd_qh_free() - Frees the QH
+ *
+ * @hsotg: HCD instance
+ * @qh: The QH to free
+ *
+ * QH should already be removed from the list. QTD list should already be empty
+ * if called from URB Dequeue.
+ *
+ * Must NOT be called with interrupt disabled or spinlock held
+ */
+void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+ /* Make sure any unreserve work is finished. */
+ if (del_timer_sync(&qh->unreserve_timer)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ dwc2_do_unreserve(hsotg, qh);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+ }
+
+ if (qh->desc_list)
+ dwc2_hcd_qh_free_ddma(hsotg, qh);
+ kfree(qh);
+}
+
+/**
* dwc2_hcd_qh_add() - Adds a QH to either the non periodic or periodic
* schedule if it is not already in the schedule. If the QH is already in
* the schedule, no action is taken.
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | Douglas Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-01-29 03:30 +0100 |
| Subject | [PATCH v6 12/22] usb: dwc2: host: Use periodic interrupt even with DMA |
| Message-ID | <qW6YY-4gM-57@gated-at.bofh.it> |
| In reply to | #1321397 |
The old code in dwc2_process_periodic_channels() would only enable the
"periodic empty" interrupt if we weren't using DMA. That wasn't right
since we can still get into cases where we have small FIFOs even on
systems that have DMA (the rk3288 is a prime example).
Let's always enable/disable the "periodic empty" when appropriate. As
part of this:
* Always call dwc2_process_periodic_channels() even if there's nothing
in periodic_sched_assigned (we move the queue empty check so we still
avoid the extra work). That will make extra certain that we will
properly disable the "periodic empty" interrupt even if there's
nothing queued up.
* Move the enable of "periodic empty" due to non-empty
periodic_sched_assigned to be for slave mode (non-DMA mode) only.
Presumably this was the original intention of the check for DMA since
it seems to match the comments above where in slave mode we leave
things on the assigned queue.
Note that even before this change slave mode didn't work for me, so I
can't say for sure that my understanding of slave mode is correct.
However, this shouldn't change anything for slave mode so if slave mode
worked for someone in the past it ought to still work.
With this change, I no longer get constant misses reported by my other
debugging code (and with future patches) when I've got:
* Rockchip rk3288 Chromebook, using port ff540000
-> Pluggable 7-port Hub with Charging (powered)
-> Microsoft Wireless Keyboard 2000 in port 1.
-> Das Keyboard in port 2.
-> Jabra Speaker in port 3
-> Logitech, Inc. Webcam C600 in port 4
-> Microsoft Sidewinder X6 Keyboard in port 5
...and I'm playing music on the USB speaker and capturing video from the
webcam.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
---
Changes in v6:
- Add Heiko's Tested-by.
- Add Stefan's Tested-by.
Changes in v5: None
Changes in v4:
- Use periodic interrupt even with DMA new for v4.
Changes in v3: None
Changes in v2: None
drivers/usb/dwc2/hcd.c | 71 +++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 39 deletions(-)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 40558478a192..fd731347daf7 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1109,10 +1109,14 @@ static void dwc2_process_periodic_channels(struct dwc2_hsotg *hsotg)
u32 fspcavail;
u32 gintmsk;
int status;
- int no_queue_space = 0;
- int no_fifo_space = 0;
+ bool no_queue_space = false;
+ bool no_fifo_space = false;
u32 qspcavail;
+ /* If empty list then just adjust interrupt enables */
+ if (list_empty(&hsotg->periodic_sched_assigned))
+ goto exit;
+
if (dbg_perio())
dev_vdbg(hsotg->dev, "Queue periodic transactions\n");
@@ -1190,42 +1194,32 @@ static void dwc2_process_periodic_channels(struct dwc2_hsotg *hsotg)
}
}
- if (hsotg->core_params->dma_enable <= 0) {
- tx_status = dwc2_readl(hsotg->regs + HPTXSTS);
- qspcavail = (tx_status & TXSTS_QSPCAVAIL_MASK) >>
- TXSTS_QSPCAVAIL_SHIFT;
- fspcavail = (tx_status & TXSTS_FSPCAVAIL_MASK) >>
- TXSTS_FSPCAVAIL_SHIFT;
- if (dbg_perio()) {
- dev_vdbg(hsotg->dev,
- " P Tx Req Queue Space Avail (after queue): %d\n",
- qspcavail);
- dev_vdbg(hsotg->dev,
- " P Tx FIFO Space Avail (after queue): %d\n",
- fspcavail);
- }
-
- if (!list_empty(&hsotg->periodic_sched_assigned) ||
- no_queue_space || no_fifo_space) {
- /*
- * May need to queue more transactions as the request
- * queue or Tx FIFO empties. Enable the periodic Tx
- * FIFO empty interrupt. (Always use the half-empty
- * level to ensure that new requests are loaded as
- * soon as possible.)
- */
- gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+exit:
+ if (no_queue_space || no_fifo_space ||
+ (hsotg->core_params->dma_enable <= 0 &&
+ !list_empty(&hsotg->periodic_sched_assigned))) {
+ /*
+ * May need to queue more transactions as the request
+ * queue or Tx FIFO empties. Enable the periodic Tx
+ * FIFO empty interrupt. (Always use the half-empty
+ * level to ensure that new requests are loaded as
+ * soon as possible.)
+ */
+ gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+ if (!(gintmsk & GINTSTS_PTXFEMP)) {
gintmsk |= GINTSTS_PTXFEMP;
dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
- } else {
- /*
- * Disable the Tx FIFO empty interrupt since there are
- * no more transactions that need to be queued right
- * now. This function is called from interrupt
- * handlers to queue more transactions as transfer
- * states change.
- */
- gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+ }
+ } else {
+ /*
+ * Disable the Tx FIFO empty interrupt since there are
+ * no more transactions that need to be queued right
+ * now. This function is called from interrupt
+ * handlers to queue more transactions as transfer
+ * states change.
+ */
+ gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+ if (gintmsk & GINTSTS_PTXFEMP) {
gintmsk &= ~GINTSTS_PTXFEMP;
dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
}
@@ -1372,9 +1366,8 @@ void dwc2_hcd_queue_transactions(struct dwc2_hsotg *hsotg,
dev_vdbg(hsotg->dev, "Queue Transactions\n");
#endif
/* Process host channels associated with periodic transfers */
- if ((tr_type == DWC2_TRANSACTION_PERIODIC ||
- tr_type == DWC2_TRANSACTION_ALL) &&
- !list_empty(&hsotg->periodic_sched_assigned))
+ if (tr_type == DWC2_TRANSACTION_PERIODIC ||
+ tr_type == DWC2_TRANSACTION_ALL)
dwc2_process_periodic_channels(hsotg);
/* Process host channels associated with non-periodic transfers */
--
2.7.0.rc3.207.g0ac5344
[toc] | [prev] | [next] | [standalone]
| From | John Youn <John.Youn@synopsys.com> |
|---|---|
| Date | 2016-02-03 01:00 +0100 |
| Subject | Re: [PATCH v6 0/22] usb: dwc2: host: Fix and speed up all the stuff, especially with splits |
| Message-ID | <qXT1w-2h3-9@gated-at.bofh.it> |
| In reply to | #1321397 |
On 1/28/2016 6:20 PM, Douglas Anderson wrote: > This is a bit of catchall series for all the bug fix and performance > patches I've been working on over the last few months. Note that for > dwc2 we need to do LOTS in software and need super low interrupt > latency, so most performance improvements actually fix real bugs. > > Patches are structured to start with no-brainer stuff that could be > applied ASAP, especially things I've already gotten Acks for. Things > get slightly more RFC / RFT like as we get farther down the series. > Anything that can be landed sooner rather than later (especially those > Acked long ago) would help in re-posts (I'm not biased, of course). > Hi Doug, I've yet to review this, but just wanted to let you know that we've started on it and also testing. We'll get back to you with some feedback and results soon. We had also been looking at some of these same and related issues so we want to make sure everything we've done is compatible with your changes and is still working ok too. Regards, John
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-02-03 19:30 +0100 |
| Subject | Re: [PATCH v6 0/22] usb: dwc2: host: Fix and speed up all the stuff, especially with splits |
| Message-ID | <qYalH-5xv-1@gated-at.bofh.it> |
| In reply to | #1324748 |
John, On Tue, Feb 2, 2016 at 3:57 PM, John Youn <John.Youn@synopsys.com> wrote: > On 1/28/2016 6:20 PM, Douglas Anderson wrote: >> This is a bit of catchall series for all the bug fix and performance >> patches I've been working on over the last few months. Note that for >> dwc2 we need to do LOTS in software and need super low interrupt >> latency, so most performance improvements actually fix real bugs. >> >> Patches are structured to start with no-brainer stuff that could be >> applied ASAP, especially things I've already gotten Acks for. Things >> get slightly more RFC / RFT like as we get farther down the series. >> Anything that can be landed sooner rather than later (especially those >> Acked long ago) would help in re-posts (I'm not biased, of course). >> > > Hi Doug, > > I've yet to review this, but just wanted to let you know that we've > started on it and also testing. We'll get back to you with some > feedback and results soon. > > We had also been looking at some of these same and related issues so > we want to make sure everything we've done is compatible with your > changes and is still working ok too. Great, thanks for the reply. It's very helpful to know that you're looking at it even if you haven't had time to do a full review yet. Note: if any of my patches are wrong or redundant to patches that you've developed and tested, I'm happy to take your patches instead. ;) Note that patches 1 - 11 have already landed in our tree and thus are getting additional exposure and testing. IMHO those are all ready for prime time. Assuming there are no huge issues, it would be handy if those 11 patches landed as-is (or almost as-is), but that's just me being selfish so I don't need to revert / reland new versions. :-P Also note that the period of time where I can devote this much time to dwc2 is coming soon to an end since I have to go on to work on other things. If there are major issues or easy fixes I will certainly be able to help with those things (I'll still be around), but I won't be able to devote days to tracking down weird problems or testing rewrites. ;-) -Doug
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web