Date: Sun, 4 May 2003 14:54:33 -0700 (PDT) From: "Bruce R. Montague" <brucem@cruzio.com> To: freebsd-hackers@freebsd.org Cc: n_hibma@freebsd.org Subject: patch to make ohci isochronous xfers work. Message-ID: <200305042154.h44LsXIf000523@cruzio.com>
next in thread | raw e-mail | index | archive | help
Hi, I have fixed isochronous usb transfers to work
under FreeBSD 5.0 -current when using the ohci
controller (dev/usb/ohci.c), at least to the degree
required to support the Dlink DSB-C100 web-camera
and the "vid" image capture app. Patches attached.
Can any usb/ohci committers evaluate this?
The FreeBSD usb stack is basically shared between
all BSDs. I'm sending this patch message to just
about everyone that I've corresponded with about
*BSD usb source, since I don't seem to know of a
`one-stop' active BSD usb e-mail list.
The ohci.c code as it stands has a long-outstanding
bug that precludes sustained isochronous transfers
from ever working. Isochronous transfers are basically
transfers that last forever, from open to close on
the stream, basically a continuous dma input. The
real definition of "isoc" here is that it's OK to
simply drop data without reporting any errors :).
You can use isoc to maintain a ring buffer of data,
such as audio or video, that can tolerate drops.
The core problem is that after an interrupt, the
interrupt handler allocates resources for the next
isoc I/O iteration, but then these resources are
immediately freed because the I/O is considered done.
Another problem is that the hardware is never
started, because the bit in the listhead that the
controller looks at to see if it should process the
isoc list is never set. This might have been
deliberate, because it was known not to work.
The specific problem was that "xfer->hcpriv" is used
in "ohci_device_isoc_enter()" to anchor a list of
allocated "sitd" structures that are constructed to
map the next I/O transfer that the "xfer" structure
will be used for, but this allocation is almost
immediately followed by a call to "ohci_device_isoc_done()"
which frees all the sitd's on the "xfer->hcpriv"
list and then does a "xfer->hcpriv = NULL;". The
result is that only the initial UGEN_NISOREQS that
were created by the initial device open can work.
After these active operations complete, the driver
stalls, as it in effect never allocates new sitds.
The above calls are performed by the "usbdi.c"
"usb_transfer_complete()" routine (not in the ohci
driver proper), which is itself called by the ohci
interrupt handler.
A couple of other problems have also been fixed which
make it possible to reliably abort and close the
isoc pipe.
The patch is for FreeBSD -current code as of about
10-april-2003, however, it doesn't look like this
file has been touched in any cvs for over two months.
The OpenBSD and NetBSD webcvs ohci.c files look
similar, but not identical to FreeBSD -current. Their
isoc implementaions appear to contain the same bug
as in FreeBSD. I have _not_ tested this under FreeBSD
4-stable.
I'm not claiming this is a perfect solution; perhaps
the entire isoc code in ohci.c needs to be restructured.
However, with this patch the "vid" image capture app
can be placed in a loop that "runs forever".
This patch includes two of the unrelated splx()
patches that were reported by jordbaer@mac.com on
30-april-2003 (the others already appear to be in
the FreeBSD-current cvs).
Can whoever is responsible for various commits
associated with "dev/usb/ohci.c" please evaluate
this? This bug has been "outstanding" for a good
while, is hard to pin down, and keeps all but the
most trivial isoc operations from being used with
the ohci (specifically, web-cams won't work). Thanks!
- bruce
--- ../ohci_5_10apr/ohci.c Sat May 3 15:37:04 2003
+++ ohci.c Sun May 4 12:33:18 2003
@@ -635,6 +635,7 @@
OHCI_ITD_ALIGN, &dma);
if (err)
return (NULL);
+ s = splusb();
for(i = 0; i < OHCI_SITD_CHUNK; i++) {
offs = i * OHCI_SITD_SIZE;
sitd = KERNADDR(&dma, offs);
@@ -642,6 +643,7 @@
sitd->nextitd = sc->sc_freeitds;
sc->sc_freeitds = sitd;
}
+ splx(s);
}
s = splusb();
@@ -974,6 +976,18 @@
ohci_freex(struct usbd_bus *bus, usbd_xfer_handle xfer)
{
struct ohci_softc *sc = (struct ohci_softc *)bus;
+ struct ohci_xfer *oxfer = (struct ohci_xfer *)xfer;
+ ohci_soft_itd_t *sitd;
+
+ if (oxfer->ohci_xfer_flags & OHCI_ISOC_DIRTY) {
+ sitd = xfer->hcpriv;
+ if (sitd) {
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd == NULL ) break;
+ ohci_free_sitd(sc, sitd);
+ }
+ }
+ }
#ifdef DIAGNOSTIC
if (xfer->busy_free != XFER_BUSY) {
@@ -1380,7 +1394,9 @@
xfer->actlen += len;
if (std->flags & OHCI_CALL_DONE) {
xfer->status = USBD_NORMAL_COMPLETION;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
ohci_free_std(sc, std);
} else {
@@ -1420,7 +1436,10 @@
xfer->status = USBD_STALLED;
else
xfer->status = USBD_IOERROR;
+
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
}
@@ -1445,6 +1464,9 @@
/* Handled by abort routine. */
continue;
}
+ if (xfer->pipe) {
+ if (xfer->pipe->aborting) continue; /*Ignore.*/
+ }
#ifdef DIAGNOSTIC
if (sitd->isdone)
printf("ohci_softintr: sitd=%p is done\n", sitd);
@@ -1460,12 +1482,16 @@
/* XXX update frlengths with actual length */
/* XXX xfer->actlen = actlen; */
xfer->status = USBD_NORMAL_COMPLETION;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
} else {
/* XXX Do more */
xfer->status = USBD_IOERROR;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
}
@@ -3223,8 +3249,9 @@
ohci_softc_t *sc = (ohci_softc_t *)dev->bus;
ohci_soft_ed_t *sed = opipe->sed;
struct iso *iso = &opipe->u.iso;
+ struct ohci_xfer *oxfer = (struct ohci_xfer *)xfer;
ohci_soft_itd_t *sitd, *nsitd;
- ohci_physaddr_t buf, offs, noffs, bp0;
+ ohci_physaddr_t buf, offs, noffs, bp0, tdphys;
int i, ncur, nframes;
int s;
@@ -3242,6 +3269,24 @@
iso->next));
}
+ if (xfer->hcpriv) {
+ sitd = xfer->hcpriv;
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if (sitd == NULL) break;
+ ohci_free_sitd(sc, sitd); /* Free ITDs in prev xfer*/
+ }
+ if (sitd == NULL) {
+ sitd = ohci_alloc_sitd(sc);
+ if (sitd == NULL) panic( "cant alloc isoc" );
+ opipe->tail.itd = sitd;
+ tdphys = sitd->physaddr;
+ sed->ed.ed_flags |= htole32(OHCI_ED_SKIP); /* Stop*/
+ sed->ed.ed_headp =
+ sed->ed.ed_tailp = htole32(tdphys);
+ sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* Start.*/
+ }
+ }
+
sitd = opipe->tail.itd;
buf = DMAADDR(&xfer->dmabuf, 0);
bp0 = OHCI_PAGE(buf);
@@ -3310,6 +3355,8 @@
xfer->status = USBD_IN_PROGRESS;
+ oxfer->ohci_xfer_flags |= OHCI_ISOC_DIRTY;
+
#ifdef USB_DEBUG
if (ohcidebug > 5) {
DPRINTF(("ohci_device_isoc_enter: frame=%d\n",
@@ -3340,6 +3387,8 @@
{
struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe;
ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus;
+ ohci_soft_ed_t *sed;
+ int s;
DPRINTFN(5,("ohci_device_isoc_start: xfer=%p\n", xfer));
@@ -3353,6 +3402,11 @@
/* XXX anything to do? */
+ s = splusb();
+ sed = opipe->sed; /* Turn off ED skip-bit to start processing */
+ sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* ED's ITD list.*/
+ splx(s);
+
return (USBD_IN_PROGRESS);
}
@@ -3391,11 +3445,14 @@
return;
}
#endif
- for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd ) { /* Only if xfer has ITDs. */
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd == NULL ) break;
#ifdef DIAGNOSTIC
DPRINTFN(1,("abort sets done sitd=%p\n", sitd));
sitd->isdone = 1;
#endif
+ }
}
splx(s);
@@ -3407,7 +3464,9 @@
/* Run callback. */
usb_transfer_complete(xfer);
- sed->ed.ed_headp = htole32(sitd->physaddr); /* unlink TDs */
+ if ( sitd ) { /* Only if there is an ITD... */
+ sed->ed.ed_headp = htole32(sitd->physaddr); /* unlink TDs */
+ }
sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* remove hardware skip */
splx(s);
@@ -3416,21 +3475,23 @@
void
ohci_device_isoc_done(usbd_xfer_handle xfer)
{
- struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe;
- ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus;
- ohci_soft_itd_t *sitd, *nsitd;
-
- DPRINTFN(1,("ohci_device_isoc_done: xfer=%p\n", xfer));
+/* This null routine corresponds to non-isoc "done()" routines
+ * that free the stds associated with an xfer after a completed
+ * xfer interrupt. However, in the case of isoc transfers, the
+ * sitds associated with the transfer have already been processed
+ * and reallocated for the next iteration by "ohci_device_isoc_transfer()".
+ *
+ * Routine "usb_transfer_complete()" is called at the end of every
+ * relevant usb interrupt. "usb_transfer_complete()" indirectly
+ * calls 1) "ohci_device_isoc_transfer()" (which keeps pumping the
+ * pipeline by setting up the next transfer iteration) and 2) then
+ * calls "ohci_device_isoc_done()". Isoc transfers have not been
+ * working for the ohci usb because this routine was trashing the
+ * xfer set up for the next iteration (thus, only the first
+ * UGEN_NISOREQS xfers outstanding on an open would work). Perhaps
+ * this could all be re-factored, but that's another pass...
+ */
- for (sitd = xfer->hcpriv;
- !(sitd->flags & OHCI_CALL_DONE);
- sitd = nsitd) {
- nsitd = sitd->nextitd;
- DPRINTFN(1,("ohci_device_isoc_done: free sitd=%p\n", sitd));
- ohci_free_sitd(sc, sitd);
- }
- ohci_free_sitd(sc, sitd);
- xfer->hcpriv = NULL;
}
usbd_status
@@ -3456,11 +3517,20 @@
{
struct ohci_pipe *opipe = (struct ohci_pipe *)pipe;
ohci_softc_t *sc = (ohci_softc_t *)pipe->device->bus;
+ ohci_soft_ed_t *sed;
DPRINTF(("ohci_device_isoc_close: pipe=%p\n", pipe));
- ohci_close_pipe(pipe, sc->sc_isoc_head);
+
+ sed = opipe->sed;
+ sed->ed.ed_flags |= htole32(OHCI_ED_SKIP); /* Stop device. */
+
+ ohci_close_pipe(pipe, sc->sc_isoc_head); /* Stop isoc list, free ED.*/
+
+ /* up to NISOREQs xfers still outstanding. */
+
+
#ifdef DIAGNOSTIC
opipe->tail.itd->isdone = 1;
#endif
- ohci_free_sitd(sc, opipe->tail.itd);
+ ohci_free_sitd(sc, opipe->tail.itd); /* Next `avail free' sitd.*/
}
--- ../ohci_5_10apr/ohcivar.h Sat May 3 15:37:11 2003
+++ ohcivar.h Sun May 4 12:33:22 2003
@@ -149,7 +149,9 @@
struct ohci_xfer {
struct usbd_xfer xfer;
struct usb_task abort_task;
+ u_int32_t ohci_xfer_flags;
};
+#define OHCI_ISOC_DIRTY 0x01
#define OXFER(xfer) ((struct ehci_xfer *)(xfer))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200305042154.h44LsXIf000523>