Received: by 2002:a25:7ec1:0:0:0:0:0 with SMTP id z184csp7057144ybc; Thu, 28 Nov 2019 10:03:50 -0800 (PST) X-Google-Smtp-Source: APXvYqxjN4AgDdavmHwewEhCEySbKyUa+tq4wAcXCqZ8u0SrIkIn3LBAZ3qIPSz7M4YKO1Qlc6MR X-Received: by 2002:a50:c343:: with SMTP id q3mr2792943edb.209.1574964230626; Thu, 28 Nov 2019 10:03:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1574964230; cv=none; d=google.com; s=arc-20160816; b=kYSX59OrQTHL9OgHWMjE2rTRsWJ1TRv/IdFmwO35VaXfXi59a7mA0xwV9fYFhld0Ib +fA5FYzHCIEb26NWoe6pXz2xv+J1FNGfdRCkxN0la8FhrIIPeJM8esOCfJ3Teq62M1jH s7WZDduMbz5OeoyKBPHSGbggi8sON/hJYg8PALd98LRoBlqRi3B2IdToR9zKYotY8otE ltzimhtwZMay745kX/TF1yBijdK8X05KO+3Ua4VVTZQrtZlJC+aGQovqMy3uU7QS4LmT yc6n2WlVHLR+h3Ihi4ndGOxPv7TPmX+O9aDuSKQ6h55uuJxGkPEDO/XWP8LWwQLKmig9 cuDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:mime-version:message-id:in-reply-to :subject:cc:to:from:date; bh=MHuOgyaxJRcoWIup8iwtFQVVoafVcDJrxnzjAIPlxhI=; b=KQtCo/+9NY7If/DQN6ex8QGk5op2csfPDC1MeohFV1uTtxtiFBuyGULGNSLej29HMk nBvWTVD+r8dbUdpoEegjkjU3J+wvmzvoTHGgkNeBj94yTna8gXhXMNXLuF7YsoDcigB6 JnRjiOmnBlU4/oGFKIZaargcl4pfxt7vjVxWjaWhjFJ4gYbDgu8opmuEXWzGabeSd6tu vlkd8YtXzr3XuJUOe7H5NOD3idnlsm5nAt3668un6ScJ5G6ddiGB05voRhnDpQ8MqNiy 44Ub2sKRW570NCrs7U7V2DeUwlA4n00H3QcVfzc9plg92RmQC433pdEoKrqSZ/i6MpiM K3Og== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id ce11si14830835edb.391.2019.11.28.10.03.25; Thu, 28 Nov 2019 10:03:50 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726670AbfK1R7z (ORCPT + 99 others); Thu, 28 Nov 2019 12:59:55 -0500 Received: from netrider.rowland.org ([192.131.102.5]:53785 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726582AbfK1R7y (ORCPT ); Thu, 28 Nov 2019 12:59:54 -0500 Received: (qmail 22060 invoked by uid 500); 28 Nov 2019 12:59:53 -0500 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 28 Nov 2019 12:59:53 -0500 Date: Thu, 28 Nov 2019 12:59:53 -0500 (EST) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Michael Olbrich cc: Thinh Nguyen , Felipe Balbi , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kernel@pengutronix.de" , Greg Kroah-Hartman Subject: Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late In-Reply-To: <20191128113633.5slzlehhwlmnc3zr@pengutronix.de> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 28 Nov 2019, Michael Olbrich wrote: > On Fri, Nov 15, 2019 at 04:06:10PM -0500, Alan Stern wrote: > > On Thu, 14 Nov 2019, Thinh Nguyen wrote: > > > > > Michael Olbrich wrote: > > > > > >>> How about changing the gadget driver instead? For frames where the UVC > > > >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so > > > >>> on in the example above), queue a zero-length request. Then there > > > >>> won't be any gaps in the isochronous packet stream. > > > >> What Alan suggests may work. Have you tried this? > > > > Yes and it works in general. There are however some problems with that > > > > approach that I want to avoid: > > > > > > > > 1. It adds extra overhead to handle the extra zero-length request. > > > > Especially for encoded video the available bandwidth can be quite a bit > > > > larger that what is actually used. I want to avoid that. > > > > This comment doesn't seem to make sense. If the available bandwidth is > > much _larger_ than what is actually used, what's the problem? You > > don't run into difficulties until the available bandwidth is too > > _small_. > > > > The extra overhead of a zero-length request should be pretty small. > > After all, the gadget expects to send a packet for every frame anyway, > > more or less. > > My current test-case is video frames with 450kB on average at 30fps. This > currently results in ~10 CPU load for the threaded interrupt handler. > At least in my test, filling the actual video data into the frame has very > little impact. So if I reserve 900kB to support occasionally larger video > frames, then I expect that this CPU load will almost double in all cases, > not just when the video frames are larger. This is the sort of thing you need to confirm by experimenting. It is not at all clear that doubling the interrupt rate will also double the CPU load, especially if half of the interrupts don't require the CPU to do much work. > > > > 2. The UVC gadget currently does no know how many zero-length request must > > > > added. So it needs fill all available request until a new video frame > > > > arrives. With the current 4 requests that is not a problem right now. But > > > > that does not scale for USB3 bandwidths. So one thing that I want to do is > > > > to queue many requests but only enable the interrupt for a few of than. > > > > From what I can tell from the code, the gadget framework and the dwc3 > > > > driver should already support this. > > > > This will result in extra latency. There is probably an acceptable > > > > trade-off with an acceptable interrupt load and latency. But I would like > > > > to avoid that if possible. > > > > There are two different situations to consider: > > > > In the middle of a video stream, latency isn't an issue. > > The gadget should expect to send a new packet for each frame, > > and it doesn't know what to put in that packet until it > > receives the video data or it knows there won't be any data. > > > > At the start of a video stream, latency can be an issue. But > > in this situation the gadget doesn't have to send 0-length > > requests until there actually is some data available. > > > > Either way, it should be okay. > > > > As far as interrupt load is concerned, I don't see how it relates to > > the issue of sending 0-length requests. > > Maybe I don't understand, how 0-length requests work. My current > understanding is, that they are queued like any other request. That's right. > If I want to reduce the number of interrupts then I need to queue more > requests and only ask for an interrupt for some of them. This means that > potentially a lot of 0-length requests requests are queued when a new video > frame arrives and this means extra latency for the frame. Let's say you ask for an interrupt for only even-numbered requests. Then there would be at most one 0-length request queued when a new video frame arrives. Processing that 0-length request should require very little CPU time (almost none) because it contains no data, so the extra latency would be negligible. > I think the worst-case latency is 2x the time between two interrupts. > So less interrupts mean more latency. What matters is the interrupt rate. If you double the rate at which transfers are queued but ask for an interrupt on only half of them, then the overall interrupt rate will remain the same and so will the average latency. > The stop/start transfer this patch implements, the video frame can be sent > immediately without any extra latency. The same would be true if you queued a request at the full isochronous rate. If video data is present, put it in the request; if not then queue a 0-length request. > > > Now, with UVC, it needs to communicate to the dwc3 driver that there > > > will be a gap after a certain request (and that the device is expecting > > > to send 0-length data). This is not a normal operation for isoc > > > transfer. You may need to introduce a new way for the function driver to > > > do that, possibly a new field in usb_request structure to indicate that. > > > However, this seems a little awkward. Maybe others can comment on this. > > I'm not sure how this is supposed to work. What exactly can the dwc3 driver > / hardware do to handle a gap? Are you talking about the driver on the gadget side or on the host side? The rules for the host-side driver are spelled out in the kerneldoc for usb_submit_urb(). As far as I know, there is no equivalent set of rules for the gadget-side drivers. > > Note that on the host side, there is a difference between receiving > > a 0-length packet and receiving no packet at all. As long as both the > > host and the gadget expect the isochronous stream to be running, there > > shouldn't be any gaps if you can avoid it. > > Huh, so how is this handled on other hardware? From what I can tell the UVC > gadget works with other drivers and I've not found any special handling for > this. Is there no packet sent or are 0-length packet generated implicitly > somewhere? I don't know. You can ask the UVC maintainer for more information; my guess is that it treats 0-length packets and missing packets the same. After all, what else could it do? Either way, there is no data. Alan Stern