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.
> 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;
>
