On Sun, 26 Jul 2020 13:27:34 +0000
[email protected] wrote:
> On 2020/07/26 10:54, Marcus Glocker wrote:
> > On Sat, 25 Jul 2020 20:31:44 +0000
> > [email protected] wrote:
> >
> >> On 2020/07/25 18:10, Marcus Glocker wrote:
> >>> On Sun, Jul 19, 2020 at 02:12:21PM +0000, [email protected]
> >>> wrote:
> >>>> On 2020/07/19 11:25, Marcus Glocker wrote:
> >>>>> On Sun, 19 Jul 2020 02:25:30 +0000
> >>>>> [email protected] wrote:
> >>>>>
> >>>>>> hi,
> >>>>>>
> >>>>>> It works on AMD Bolton xHCI (78141022), Intel PCH (1e318086),
> >>>>>> and ASM1042 (10421b21).
> >>>>>> I simply play with ffplay -f v4l2 /dev/video0 to test.
> >>>>>
> >>>>> If your cam supports MJPEG it's good to add '-input_format
> >>>>> mjpeg' with higher resolutions like 1280x720, because that will
> >>>>> generated varying image sizes, which hit the 64k memory boundary
> >>>>> more often, and thus generate potentially more chained TDs.
> >>>>
> >>>> Thank you for useful information.
> >>>> My webcam supprots at most 640x480, but 1024 bytes/frame x (2+1)
> >>>> maxburst x 40 frames = 122880 bytes/xfer is enough to observe TD
> >>>> fragmentation.
> >>>>
> >>>>
> >>>>>> At this moment it does not work on VL805, but I have no idea.
> >>>>>> I'll investigate furthermore...
> >>>
> >>> Did you already had a chance to figure out something regarding the
> >>> issue you faced on your VL805 controller?
> >>>
> >>> I'm running the diff here since then on the Intel xHCI controller
> >>> and couldn't re-produce any errors using different uvideo(4) and
> >>> uaudio(4) devices.
> >>>
> >>
> >> No, yet -- all I know about this problem is VL805 genegates
> >> many MISSED_SRV Transfer Event for Isoch-IN pipe.
> >>
> >> xhci0: slot 3 missed srv with 123 TRB
> >> :
> >>
> >> Even if I increase UVIDEO_IXFERS in uvideo.h to 6, HC still detects
> >> MISSED_SRV. When I disable splitting TD, it works well.
> >> I added printf paddr in the event TRB but each paddr of MISSED_SRV
> >> is 0, that does not meet 4.10.3.2.
> >> Parameters in this endpoint context are
> >>
> >> xhci0: slot 3 dci 3 ival 0 mps 1024 maxb 2 mep 3072 atl 3072 mult 0
> >>
> >> looks sane.
> >
> > Hmm, I see.
> >
> > I currently have also no idea what exactly is causing the missed
> > service events. I was reading a little bit yesterday about the
> > VL805 and could find some statements where people say it's not
> > fully compliant with the xHCI specs, and in Linux it took some
> > cooperation with the vendor to make it work.
> >
> > One thing I still wanted to ask you to understand whether the
> > problem on your VL805 is only related with my last diff; Are the
> > multi-trb transfers working fine with your last diff on the VL805?
>
> On VL805 ffplay plays the movie sometimes smoothly, sometimes laggy.
> The multi-TRB transfer itself works on VL805 with your patch.
> Not all splitted TD fail to transfer. Successful splitted transfer
> works as intended.
> I think MISSED_SRV is caused by other reason, maybe isochronous
> scheduling problem.
> Thus, IMO your patch can be committed.
>
> VL805 also has same problem that AMD Bolton has.
> It may generate the 1. TRB event w/ cc = SHORT_PKT and
> remain = requested length (that is, transferred length = 0),
> but the 2. TRB w/ cc = SUCCESS and remain = 0.
> remain of 2. TRB should be given length, and CC should be SHORT_PKT.
> Your patch fixes this problem.
OK, that's what I wanted to understand.
I also have the impression that the MISSED_SRV issue on the VL805 is
related to another problem which we should trace separately from the
multi-trb problem. Thanks for that useful feedback.
Attached the latest version of my patch including all the inputs
received (mostly related to malloc/free).
Patrick, Martin, would you be fine to OK that?
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 19 Jul 2020 06:51:58 -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,16 @@ 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));
+ usbd_dma_contig_free(&sc->sc_bus, &ring->dma);
+ return (ENOMEM);
+ }
+
xhci_ring_reset(sc, ring);
return (0);
@@ -1710,6 +1736,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 +1749,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 +1835,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 19 Jul 2020 06:51:58 -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;
