Hey, On Sat, 18 Jul 2020 00:14:32 +0000 [email protected] wrote:
> hi, > > On 2020/07/15 21:28, [email protected] wrote: > > hi, > > > > The patch works well on Intel PCH xhci, but not on AMD Bolton xHCI. > > I'll investigate this problem. > > Bolton xHCI sometimes raises following events. > (I added printf Completion Code.) > > #246 cc 13 remain 0 type 5 origlen 2048 frlengths[6] 2048 > #247 cc 1 remain 0 type 1 origlen 1024 frlengths[6] 3072 > > It seems this behavior violates the spec. 4.10.1.1. > When the remain of 1. TRB is 0, CC should be SUCCESS, not SHORT_PKT, > and HC should not raise interrupt. > The problem is how many length 2. TRB transferred. > If real CC of 1. TRB = SUCCESS, we should add 2. TRB length to > frlengths. If SHORT_PKT, we should keep frlengths as is. > Actually with the latter assumption I can see video smoothly w/o > errors. > > I changed your patch to skip adding to frlengths and actlen > if previous TRB is processed AND remain == 0. Interesting! Thanks for tracing this down. So in general we can say that for a chained TD, if the first TRB is SHORT, we can simply skip the second TRB. To make the code a bit more understandable, and use the new trb_processed flag to control this, I made another diff which keeps track whether the TRB was not processed, processed, or processed short. It also includes the malloc NULL check and replaces malloc by mallocarray as suggested by tb@. Does this diff also work for you on your AMD xhci? Index: xhci.c =================================================================== RCS file: /cvs/src/sys/dev/usb/xhci.c,v retrieving revision 1.116 diff -u -p -u -p -r1.116 xhci.c --- xhci.c 30 Jun 2020 10:21:59 -0000 1.116 +++ xhci.c 18 Jul 2020 14:20:28 -0000 @@ -82,7 +82,7 @@ void xhci_event_xfer(struct xhci_softc * int xhci_event_xfer_generic(struct xhci_softc *, struct usbd_xfer *, struct xhci_pipe *, uint32_t, int, uint8_t, uint8_t, uint8_t); int xhci_event_xfer_isoc(struct usbd_xfer *, struct xhci_pipe *, - uint32_t, int); + uint32_t, int, uint8_t); void xhci_event_command(struct xhci_softc *, uint64_t); void xhci_event_port_change(struct xhci_softc *, uint64_t, uint32_t); int xhci_pipe_init(struct xhci_softc *, struct usbd_pipe *); @@ -800,7 +800,7 @@ xhci_event_xfer(struct xhci_softc *sc, u return; break; case UE_ISOCHRONOUS: - if (xhci_event_xfer_isoc(xfer, xp, remain, trb_idx)) + if (xhci_event_xfer_isoc(xfer, xp, remain, trb_idx, code)) return; break; default: @@ -927,13 +927,23 @@ xhci_event_xfer_generic(struct xhci_soft int xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xhci_pipe *xp, - uint32_t remain, int trb_idx) + uint32_t remain, int trb_idx, uint8_t code) { struct usbd_xfer *skipxfer; struct xhci_xfer *xx = (struct xhci_xfer *)xfer; - int trb0_idx, frame_idx = 0; + int trb0_idx, frame_idx = 0, skip_trb = 0; KASSERT(xx->index >= 0); + + switch (code) { + case XHCI_CODE_SHORT_XFER: + xp->ring.trb_processed[trb_idx] = TRB_PROCESSED_SHORT; + break; + default: + xp->ring.trb_processed[trb_idx] = TRB_PROCESSED_YES; + break; + } + trb0_idx = ((xx->index + xp->ring.ntrb) - xx->ntrb) % (xp->ring.ntrb - 1); @@ -958,15 +968,21 @@ xhci_event_xfer_isoc(struct usbd_xfer *x trb0_idx = xp->ring.ntrb - 2; else trb0_idx = trb_idx - 1; - if (xfer->frlengths[frame_idx] == 0) { + if (xp->ring.trb_processed[trb0_idx] == TRB_PROCESSED_NO) { xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32( xp->ring.trbs[trb0_idx].trb_status)); + } else if (xp->ring.trb_processed[trb0_idx] == + TRB_PROCESSED_SHORT) { + skip_trb = 1; } } - xfer->frlengths[frame_idx] += - XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain; - xfer->actlen += xfer->frlengths[frame_idx]; + if (!skip_trb) { + xfer->frlengths[frame_idx] += + XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - + remain; + xfer->actlen += xfer->frlengths[frame_idx]; + } if (xx->index != trb_idx) return (1); @@ -1702,6 +1718,15 @@ xhci_ring_alloc(struct xhci_softc *sc, s ring->ntrb = ntrb; + ring->trb_processed = + mallocarray(ring->ntrb, sizeof(*ring->trb_processed), M_DEVBUF, + M_NOWAIT); + if (ring->trb_processed == NULL) { + printf("%s: unable to allocate ring trb_processed array\n", + DEVNAME(sc)); + return (ENOMEM); + } + xhci_ring_reset(sc, ring); return (0); @@ -1710,6 +1735,8 @@ xhci_ring_alloc(struct xhci_softc *sc, s void xhci_ring_free(struct xhci_softc *sc, struct xhci_ring *ring) { + free(ring->trb_processed, M_DEVBUF, + ring->ntrb * sizeof(*ring->trb_processed)); usbd_dma_contig_free(&sc->sc_bus, &ring->dma); } @@ -1721,6 +1748,8 @@ xhci_ring_reset(struct xhci_softc *sc, s size = ring->ntrb * sizeof(struct xhci_trb); memset(ring->trbs, 0, size); + memset(ring->trb_processed, 0, + ring->ntrb * sizeof(*ring->trb_processed)); ring->index = 0; ring->toggle = XHCI_TRB_CYCLE; @@ -1805,6 +1834,8 @@ xhci_ring_produce(struct xhci_softc *sc, ring->index = 0; ring->toggle ^= 1; } + + ring->trb_processed[(trb - &ring->trbs[0])] = TRB_PROCESSED_NO; return (trb); } Index: xhcivar.h =================================================================== RCS file: /cvs/src/sys/dev/usb/xhcivar.h,v retrieving revision 1.11 diff -u -p -u -p -r1.11 xhcivar.h --- xhcivar.h 6 Oct 2019 17:30:00 -0000 1.11 +++ xhcivar.h 18 Jul 2020 14:20:28 -0000 @@ -44,6 +44,12 @@ struct xhci_xfer { struct xhci_ring { struct xhci_trb *trbs; +enum { + TRB_PROCESSED_NO, + TRB_PROCESSED_YES, + TRB_PROCESSED_SHORT +}; + uint8_t *trb_processed; size_t ntrb; struct usbd_dma_info dma;
