linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] uvc gadget performance issues
@ 2022-10-17 20:54 Dan Vacura
  2022-10-17 20:54 ` [PATCH] usb: gadget: uvc: fix dropped frame after missed isoc Dan Vacura
                   ` (5 more replies)
  0 siblings, 6 replies; 48+ messages in thread
From: Dan Vacura @ 2022-10-17 20:54 UTC (permalink / raw)
  To: linux-usb
  Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, Dan Vacura,
	Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
	Felipe Balbi, Michael Grzeschik, Paul Elder, linux-kernel,
	linux-doc

Hello uvc gadget developers,

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          |  4 ++
 drivers/usb/dwc3/core.h                       |  1 +
 drivers/usb/dwc3/gadget.c                     | 38 +++++++++++++------
 drivers/usb/gadget/function/f_uvc.c           |  5 +++
 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       | 27 +++++++++----
 10 files changed, 78 insertions(+), 25 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
@ 2022-10-18 20:49 Jeffrey Vanhoof
  2022-10-18 22:35 ` Thinh Nguyen
  0 siblings, 1 reply; 48+ messages in thread
From: Jeffrey Vanhoof @ 2022-10-18 20:49 UTC (permalink / raw)
  To: thinh.nguyen@synopsys.com
  Cc: balbi@kernel.org, corbet@lwn.net, dan.scally@ideasonboard.com,
	gregkh@linuxfoundation.org, laurent.pinchart@ideasonboard.com,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-usb@vger.kernel.org, m.grzeschik@pengutronix.de,
	paul.elder@ideasonboard.com, Jeffrey Vanhoof,
	stable@vger.kernel.org, Dan Vacura

From qjv001@qjv001-XeonWs Tue Oct 18 15:37:29 2022
From: qjv001 <qjv001@qjv001-XeonWs>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Subject: Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of
 release after missed isoc
References: <20221017205446.523796-1-w36195@motorola.com>
 <20221017205446.523796-3-w36195@motorola.com>
 <20221017213031.tqb575hdzli7jlbh@synopsys.com>
 <Y04K/HoUigF5FYBA@p1g3>
 <20221018184535.3g3sm35picdeuajs@synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221018184535.3g3sm35picdeuajs@synopsys.com>
X-Mutt-References: <20221018184535.3g3sm35picdeuajs@synopsys.com>
X-Mutt-Fcc: ~/sent
Status: RO
Date: Tue, 18 Oct 2022 15:37:29 -0500
Content-Length: 5434
Lines: 124

Hi Thinh,

On Tue, Oct 18, 2022 at 06:45:40PM +0000, Thinh Nguyen wrote:
> Hi Dan,
> 
> On Mon, Oct 17, 2022, Dan Vacura wrote:
> > Hi Thinh,
> > 
> > On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
> > > On Mon, Oct 17, 2022, 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.
> > > 
> > > This doesn't sound right. How did you determine that the hardware is
> > > still using the data associated with the TRB? Did you check the TRB's
> > > HWO bit?
> > 
> > The problem we're seeing was mentioned in the summary of this patch
> > series, issue #1. Basically, with the following patch
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/__;!!A4F2R9G_pg!aSNZ-IjMcPgL47A4NR5qp9qhVlP91UGTuCxej5NRTv8-FmTrMkKK7CjNToQQVEgtpqbKzLU2HXET9O226AEN$  
> > integrated a smmu panic is occurring on our Android device with the 5.15
> > kernel which is:
> > 
> >     <3>[  718.314900][  T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
> > 
> > The uvc gadget driver appears to be the first (and only) gadget that
> > uses the no_interrupt=1 logic, so this seems to be a new condition for
> > the dwc3 driver. In our configuration, we have up to 64 requests and the
> > no_interrupt=1 for up to 15 requests. The list size of dep->started_list
> > would get up to that amount when looping through to cleanup the
> > completed requests. From testing and debugging the smmu panic occurs
> > when a -EXDEV status shows up and right after
> > dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion
> > we had was the requests were getting returned to the gadget too early.
> 
> As I mentioned, if the status is updated to missed isoc, that means that
> the controller returned ownership of the TRB to the driver. At least for
> the particular request with -EXDEV, its TRBs are completed. I'm not
> clear on your conclusion.
> 
> Do we know where did the crash occur? Is it from dwc3 driver or from uvc
> driver, and at what line? It'd great if we can see the driver log.
>

To interject, what should happen in dwc3_gadget_ep_reclaim_completed_trb if the
IOC bit is not set (but the IMI bit is) and -EXDEV status is passed into it?
If the function returns 0, another attempt to reclaim may occur. If this
happens and the next request did have the HWO bit set, the function would
return 1 but dwc3_gadget_ep_cleanup_completed_request would still call
dwc3_gadget_giveback.

As a test (without this patch), I added a check to see if HWO bit was set in
dwc3_gadget_ep_cleanup_completed_requests(). If the usecase was ISOC and the
HWO bit was set I avoided calling dwc3_gadget_ep_cleanup_completed_request().
This seemed to also avoid the iommu related crash being seen.

Is there an issue in this area that needs to be corrected instead? Not having
interrupts set for each request may be causing some new issues to be uncovered.

As far as the crash seen without this patch, no good stacktrace is given. Line
provided for crash varied a bit, but tended to appear towards the end of
dwc3_stop_active_transfer() or dwc3_gadget_endpoint_trbs_complete().

Since dwc3_gadget_endpoint_trbs_complete() can be called from multiple
locations, I duplicated the function to help identify which path it was likely
being called from. At the time of the crashes seen,
dwc3_gadget_endpoint_transfer_in_progress() appeared to be the caller.

dwc3_gadget_endpoint_transfer_in_progress()
->dwc3_gadget_endpoint_trbs_complete() (crashed towards end of here)
->dwc3_stop_active_transfer() (sometimes crashed towards end of here)

I hope this clarifies things a bit.
 
> > 
> > > 
> > > The dwc3 driver would only give back the requests if the TRBs of the
> > > associated requests are completed or when the device is disconnected.
> > > If the TRB indicated missed isoc, that means that the TRB is completed
> > > and its status was updated.
> > 
> > Interesting, the device is not disconnected as we don't get the
> > -ESHUTDOWN status back and with this patch in place things continue
> > after a -EXDEV status is received.
> > 
> 
> Actually, minor correction here: a recent change
> b44c0e7fef51 ("usb: dwc3: gadget: conditionally remove requests")
> changed -ESHUTDOWN request status to -ECONNRESET when disable endpoint.
> This doesn't look right.
> 
> While disabling endpoint may also apply for other cases such as
> switching alternate interface in addition to disconnect, -ESHUTDOWN
> seems more fitting there.
> 
btw, we don't have "usb: dwc3: gadget: conditionally remove requests" in our baseline

> Hi Michael,
> 
> Can you help clarify for the change above? This changed the usage of
> requests. Now requests returned by disconnection won't be returned as
> -ESHUTDOWN.
> 
> > > 
> > > There's a special case which dwc3 may give back requests early is the
> > > case of the device disconnecting. The requests should be returned with
> > > -ESHUTDOWN, and the gadget driver shouldn't be re-using the requests on
> > > de-initialization anyway.
> > > 
> > > We should not issue End Transfer command just because of missed isoc. We
> > > may want issue End Transfer if the gadget driver is too slow and unable
> > > to feed requests in time (causing underrun and missed isoc) to resync
> > > with the host, but we already handle that.
> > 
> > Hmm, isn't that what happens when we get into this
> > condition in dwc3_gadget_endpoint_trbs_complete():
> > 
> > 	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> > 		list_empty(&dep->started_list) &&
> > 		(list_empty(&dep->pending_list) || status == -EXDEV))
> > 		dwc3_stop_active_transfer(dep, true, true);
> > 
> 
> Yes, it's being handled there.
> 
> > > 
> > > I'm still not clear what's the problem you're seeing. Do you have the
> > > crash log? Tracepoints?
> > > 
> > 
> > Appreciate the support!
> > 
> 
> Thanks,
> Thinh

Thanks,
Jeff

^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2024-03-08  2:47 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-17 20:54 [PATCH v3 0/6] uvc gadget performance issues Dan Vacura
2022-10-17 20:54 ` [PATCH] usb: gadget: uvc: fix dropped frame after missed isoc Dan Vacura
2022-10-18  1:50   ` Bagas Sanjaya
2022-10-18  2:15     ` Dan Vacura
2022-10-18  5:13       ` Greg Kroah-Hartman
2022-10-17 20:54 ` [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release " Dan Vacura
2022-10-17 21:30   ` Thinh Nguyen
2022-10-18  2:10     ` Dan Vacura
2022-10-18 18:45       ` Thinh Nguyen
2022-10-18 19:13         ` Michael Grzeschik
2022-10-18 22:45           ` Thinh Nguyen
2022-10-19  6:46             ` Michael Grzeschik
2024-02-22  0:02   ` Michael Grzeschik
2024-02-22  1:20     ` Thinh Nguyen
2024-02-27 21:01       ` Michael Grzeschik
2024-03-07  1:57         ` Thinh Nguyen
2024-03-07 16:15           ` Michael Grzeschik
2024-03-08  2:47             ` Thinh Nguyen
2022-10-17 20:54 ` [PATCH v3 3/6] usb: gadget: uvc: fix sg handling in error case Dan Vacura
2022-10-17 20:54 ` [PATCH v3 4/6] usb: gadget: uvc: fix sg handling during video encode Dan Vacura
2022-10-17 20:54 ` [PATCH v3 5/6] usb: gadget: uvc: make interrupt skip logic configurable Dan Vacura
2022-10-17 20:54 ` [PATCH v3 6/6] usb: gadget: uvc: add configfs option for sg support Dan Vacura
2022-10-18 13:27   ` Dan Scally
2022-10-18 14:04     ` Michael Grzeschik
2022-10-18 14:09       ` Dan Scally
2022-10-18 14:10       ` Dan Scally
2022-10-18 15:00         ` Dan Vacura
2022-10-18 14:32     ` Alan Stern
2022-10-18 15:14       ` Dan Vacura
2022-10-18 15:23         ` Alan Stern
2022-10-18 15:28         ` Michael Grzeschik
  -- strict thread matches above, loose matches on Subject: below --
2022-10-18 20:49 [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc Jeffrey Vanhoof
2022-10-18 22:35 ` Thinh Nguyen
2022-10-19  1:41   ` Jeff Vanhoof
2022-10-19  2:02     ` Thinh Nguyen
2022-10-19  7:40       ` Jeff Vanhoof
2022-10-19 19:08         ` Thinh Nguyen
2022-10-19 21:34           ` Jeff Vanhoof
2022-10-19 23:06             ` Thinh Nguyen
2022-10-20 16:47               ` Jeff Vanhoof
2022-10-20 20:53                 ` Jeff Vanhoof
2022-10-20 22:47                 ` Thinh Nguyen
2022-10-21  0:55                   ` Thinh Nguyen
2022-10-21  9:39                     ` Jeff Vanhoof
2022-10-21 16:43                       ` Thinh Nguyen
2022-10-21 18:28                         ` Jeff Vanhoof
2022-10-21 19:09                           ` Thinh Nguyen
2022-10-21 19:27                             ` Jeff Vanhoof

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).