On Tue, 28 Jul 2020 11:42:43 +0200 Martin Pieuchot <[email protected]> wrote:
> On 26/07/20(Sun) 16:23, Marcus Glocker wrote: > > 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? > > The logic looks fine. I'd suggest you move the trb_processed array to > the xhci_pipe structure. Command and Event rings do not need it, > right? This should also allow you to get rid of the malloc/free by > always using XHCI_MAX_XFER elements. Good point and true. I wasn't sure initially whether we could make use of this on other rings, but in the meantime I also believe this will only be required for xfer rings. I moved it to xhci_pipe. Gets the patch quite compact :-) 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 28 Jul 2020 10:38:49 -0000 @@ -73,6 +73,12 @@ struct xhci_pipe { int halted; size_t free_trbs; int skip; +enum { + TRB_PROCESSED_NO, + TRB_PROCESSED_YES, + TRB_PROCESSED_SHORT +}; + uint8_t trb_processed[XHCI_MAX_XFER]; }; int xhci_reset(struct xhci_softc *); @@ -82,7 +88,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 +806,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 +933,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->trb_processed[trb_idx] = TRB_PROCESSED_SHORT; + break; + default: + xp->trb_processed[trb_idx] = TRB_PROCESSED_YES; + break; + } + trb0_idx = ((xx->index + xp->ring.ntrb) - xx->ntrb) % (xp->ring.ntrb - 1); @@ -958,15 +974,20 @@ 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->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->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); @@ -1835,6 +1856,8 @@ xhci_xfer_get_trb(struct xhci_softc *sc, xx->ntrb += 1; break; } + + xp->trb_processed[xp->ring.index] = TRB_PROCESSED_NO; return (xhci_ring_produce(sc, &xp->ring)); }
