| Message ID | 20221018215044.765044-1-w36195@motorola.com |
|---|---|
| Headers | show |
| Series | uvc gadget performance issues | expand |
Hi Greg, On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote: > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote: > > From: Jeff Vanhoof <qjv001@motorola.com> > > > > arm-smmu related crashes seen after a Missed ISOC interrupt when > > no_interrupt=1 is used. This can happen if the hardware is still using > > the data associated with a TRB after the usb_request's ->complete call > > has been made. Instead of immediately releasing a request when a Missed > > ISOC interrupt has occurred, this change will add logic to cancel the > > request instead where it will eventually be released when the > > END_TRANSFER command has completed. This logic is similar to some of the > > cleanup done in dwc3_gadget_ep_dequeue. > > > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com> > > Co-developed-by: Dan Vacura <w36195@motorola.com> > > Signed-off-by: Dan Vacura <w36195@motorola.com> > > --- > > V1 -> V3: > > - no change, new patch in series > > V3 -> V4: > > - no change > > I need an ack from the dwc3 maintainer before I can take this one. > > thanks, > > greg k-h Thinh has rejected this version of the patch. He has provided an alternative implementation which has been testing well for us so far. Either Thinh or Dan will formalize this patch within the next few days. The latest proposed changes are: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index dfaf9ac24c4f..50287437d6de 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if (event->status & DEPEVT_STATUS_SHORT && !chain) return 1; + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain) + return 1; + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) || (trb->ctrl & DWC3_TRB_CTRL_LST)) return 1; @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, struct scatterlist *s; unsigned int num_queued = req->num_queued_sgs; unsigned int i; + bool missed_isoc = false; int ret = 0; for_each_sg(sg, s, num_queued, i) { @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, req->sg = sg_next(s); req->num_queued_sgs--; + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC) + missed_isoc = true; + ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req, trb, event, status, true); if (ret) break; } + if (missed_isoc) + ret = 1; + return ret; } Thanks, Jeff
Hi Thinh, On Mon, Oct 24, 2022 at 10:47:53PM +0000, Thinh Nguyen wrote: >On Sat, Oct 22, 2022, Jeff Vanhoof wrote: >> Hi Greg, >> >> On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote: >> > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote: >> > > From: Jeff Vanhoof <qjv001@motorola.com> >> > > >> > > arm-smmu related crashes seen after a Missed ISOC interrupt when >> > > no_interrupt=1 is used. This can happen if the hardware is still using >> > > the data associated with a TRB after the usb_request's ->complete call >> > > has been made. Instead of immediately releasing a request when a Missed >> > > ISOC interrupt has occurred, this change will add logic to cancel the >> > > request instead where it will eventually be released when the >> > > END_TRANSFER command has completed. This logic is similar to some of the >> > > cleanup done in dwc3_gadget_ep_dequeue. >> > > >> > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status") >> > > Cc: <stable@vger.kernel.org> >> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com> >> > > Co-developed-by: Dan Vacura <w36195@motorola.com> >> > > Signed-off-by: Dan Vacura <w36195@motorola.com> >> > > --- >> > > V1 -> V3: >> > > - no change, new patch in series >> > > V3 -> V4: >> > > - no change >> > >> > I need an ack from the dwc3 maintainer before I can take this one. >> > >> > thanks, >> > >> > greg k-h >> >> Thinh has rejected this version of the patch. He has provided an alternative >> implementation which has been testing well for us so far. Either Thinh or Dan >> will formalize this patch within the next few days. >> The latest proposed changes are: >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index dfaf9ac24c4f..50287437d6de 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> if (event->status & DEPEVT_STATUS_SHORT && !chain) >> return 1; >> >> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain) >> + return 1; >> + >> if ((trb->ctrl & DWC3_TRB_CTRL_IOC) || >> (trb->ctrl & DWC3_TRB_CTRL_LST)) >> return 1; >> @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, >> struct scatterlist *s; >> unsigned int num_queued = req->num_queued_sgs; >> unsigned int i; >> + bool missed_isoc = false; >> int ret = 0; >> >> for_each_sg(sg, s, num_queued, i) { >> @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, >> req->sg = sg_next(s); >> req->num_queued_sgs--; >> >> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC) >> + missed_isoc = true; >> + >> ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req, >> trb, event, status, true); >> if (ret) >> break; >> } >> >> + if (missed_isoc) >> + ret = 1; >> + >> return ret; >> } >> >> > >That's just a debug patch. I'll send out proper fix patches. Ping! While digging out this thread, I did not find any followup patch for this suggestion. Did it hit the mailinglist anywhere? If not, will you send one? Regards, Michael
Hello uvc gadget developers, V4 series updated with comments to ("usb: gadget: uvc: add configfs option for sg support") and extra stable cherry-picks dropped in ("usb: gadget: uvc: fix dropped frame after missed isoc") since a request was made to stable separately for those. V3 series updated with fixes for the two issues discussed below, plus fixes for the configfs interrupt patch. V2 series with added patches to disable these performance features at the userspace level for devices that don't work well with the UDC hw, i.e. dwc3 in this case. Also included are updates to comments for the v1 patch. Original note: I'm working on a 5.15.41 based kernel on a qcom chipset with the dwc3 controller and I'm encountering two problems related to the recent performance improvement changes: https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-5-m.grzeschik@pengutronix.de/ and https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/ If I revert these two changes, then I have much improved stability and a transmission problem I'm seeing is gone. Has there been any success from others on 5.15 with this uvc improvement and any recommendations for my current problems? Those being: 1) a smmu panic, snippet here: <3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3! <3>[ 718.314994][ T803] arm-smmu 15000000.apps-smmu: FAR = 0x00000000efe60800 <3>[ 718.315023][ T803] arm-smmu 15000000.apps-smmu: PAR = 0x0000000000000000 <3>[ 718.315048][ T803] arm-smmu 15000000.apps-smmu: FSR = 0x40000402 [TF R SS ] <3>[ 718.315074][ T803] arm-smmu 15000000.apps-smmu: FSYNR0 = 0x5f0003 <3>[ 718.315096][ T803] arm-smmu 15000000.apps-smmu: FSYNR1 = 0xaa02 <3>[ 718.315117][ T803] arm-smmu 15000000.apps-smmu: context bank# = 0x1b <3>[ 718.315141][ T803] arm-smmu 15000000.apps-smmu: TTBR0 = 0x001b0000c2a92000 <3>[ 718.315165][ T803] arm-smmu 15000000.apps-smmu: TTBR1 = 0x001b000000000000 <3>[ 718.315192][ T803] arm-smmu 15000000.apps-smmu: SCTLR = 0x0a5f00e7 ACTLR = 0x00000003 <3>[ 718.315245][ T803] arm-smmu 15000000.apps-smmu: CBAR = 0x0001f300 <3>[ 718.315274][ T803] arm-smmu 15000000.apps-smmu: MAIR0 = 0xf404ff44 MAIR1 = 0x0000efe4 <3>[ 718.315297][ T803] arm-smmu 15000000.apps-smmu: SID = 0x40 <3>[ 718.315318][ T803] arm-smmu 15000000.apps-smmu: Client info: BID=0x5, PID=0xa, MID=0x2 <3>[ 718.315377][ T803] arm-smmu 15000000.apps-smmu: soft iova-to-phys=0x0000000000000000 I can reduce this panic with the proposed patch, but it still happens until I disable the "req->no_interrupt = 1" logic. 2) The frame is not fully transmitted in dwc3 with sg support enabled. There seems to be a mapping limit I'm seeing where only the roughly first 70% of the total frame is sent. Interestingly, if I allocate a larger size for the buffer upfront, in uvc_queue_setup(), like sizes[0] = video->imagesize * 3. Then the issue rarely happens. For example, when I do YUYV I see green, uninitialized data, at the bottom part of the frame. If I do MJPG with smaller filled sizes, the transmission is fine. +-------------------------+ | | | | | | | Good data | | | | | | | +-------------------------+ |xxxxxxxxxxxxxxxxxxxxxxxxx| |xxxx Bad data xxxxxxxxx| |xxxxxxxxxxxxxxxxxxxxxxxxx| +-------------------------+ Dan Vacura (4): usb: gadget: uvc: fix dropped frame after missed isoc usb: gadget: uvc: fix sg handling in error case usb: gadget: uvc: make interrupt skip logic configurable usb: gadget: uvc: add configfs option for sg support Jeff Vanhoof (2): usb: dwc3: gadget: cancel requests instead of release after missed isoc usb: gadget: uvc: fix sg handling during video encode .../ABI/testing/configfs-usb-gadget-uvc | 2 + Documentation/usb/gadget-testing.rst | 6 +++ drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 38 +++++++++++++------ drivers/usb/gadget/function/f_uvc.c | 4 ++ drivers/usb/gadget/function/u_uvc.h | 2 + drivers/usb/gadget/function/uvc.h | 2 + drivers/usb/gadget/function/uvc_configfs.c | 4 ++ drivers/usb/gadget/function/uvc_queue.c | 18 +++++++-- drivers/usb/gadget/function/uvc_video.c | 28 ++++++++++---- 10 files changed, 81 insertions(+), 24 deletions(-)