From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com [209.85.210.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1989F3BBF2 for ; Tue, 4 Jun 2024 22:32:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717540340; cv=none; b=fSMNt7sZiPf7iPqdDjM5nOJN20F7Zat+aiysTyS55W0lClS8XcAgJp6wmJeK//JgNSc+qfOaSjO6a4P65Whv4GHTT4UY2W2hQxI6i8KDJAQKpukr3bt2JkiHA82ogeOfwurD5vJYty9bFm3j3L3uyCjLGEJrna3sMy80iZnSACQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717540340; c=relaxed/simple; bh=AsKB6DPmRmGCoakcfG6rcKKrmELOS0mxPRiLIBXDxiU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=PYUV44tHQoloBNvjazhGmUMpfi4Kxabn9oRMdjs0796szdxe0OADp7reKR+/+AQppFG93pRGwP17KGQAuxSUlebel8AEw9st2nwEv4OWwzeP3Qx6PAJWh+QNqA7HHy5GSRCcSZ6veMYeWHd8ARpUaOlx88ElmjinxCKGdq/Ny7U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=0vYPZsne; arc=none smtp.client-ip=209.85.210.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="0vYPZsne" Received: by mail-pf1-f172.google.com with SMTP id d2e1a72fcca58-703e5a09c11so277417b3a.2 for ; Tue, 04 Jun 2024 15:32:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1717540338; x=1718145138; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=eOcZRUFo+Zg79q4b2VwYjnq6VvA9LjCrSMhI7zwKOLc=; b=0vYPZsneGOIyBS+5Q+yUgxpHnwdlVSW35Due530aQBM8LDaDLHeih8O9cXB/ViAbad BVb3H+7YIYTvYYiMD6rZkh+k5Lspzxy5U4GSAISKi1GausGHzy7fCrjY6hIrmzrODn1U TSprS4Zz4niEXYu/NH50X/anwxRDjRFPsaSjdWaq4FqKuJcXOCKO59XETPyYhIGuImrH 2NGJlzSQMvcK3S+/JNW2MYXGe70wepFnDpkzQxk1zf4/BoA2xUJ8PdO9/3L43DATsle4 YBjG1b/uQA6ZvtD6vS5monF8P6IHMydyRSOmbxhdtHJbROzo8y+zPz/QMtOzDC82YqKa OahQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717540338; x=1718145138; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=eOcZRUFo+Zg79q4b2VwYjnq6VvA9LjCrSMhI7zwKOLc=; b=kIsAi1Qlem8qImcqr4roJWaBf0yM49M0/BvMiojQmM5Xe4jzDRW0g6YQ2NBlBZrJcG cPsVcIkUUw/zPuDsQwLJ3H3RbcOQB4SpPS1+SvgpcZofKUTIESrITCPjhpBGIYe22RHR m3uE8q/o1hrXQ5kuhRWoSmygQcmUavw46XdZCuR7xgzgH8p1Cqj6lM8/VlsaeHV0Vpxh xoi2uK6EhdhwraKmzC1886QjCmlWGXJ1i2pzBv8L1qJ2OvKs+DPxLIta8Oqswr8G+/Vo UANJG706TXTOgXeOirqDVd2amWHDxgmxVgO9kftSUqfr1shaP9K82OxCPcPWPKxm+sSy L/rw== X-Forwarded-Encrypted: i=1; AJvYcCWKy87ktfEvqmDcDH181UwRPsz7i5m2t36PX/aSeH/rb9GFIc0N2cWpKWFOa3UslTOQ1+LeV70lC0c0QU2hVYH5N+FNHDjpq578 X-Gm-Message-State: AOJu0Yy+KHkG7KyvLzdGQLU9qgQxa3t/vmEDz+9uRgsvAU7DRXbBB8pM H0LHQsLnityfh92+K/gnQUSLpvv5OnE7pBBY7EJvO1QrzjhC79gr3m8t6aYAkA== X-Google-Smtp-Source: AGHT+IHUk3udE+fZnFkbvaq+nAn6iapJhfQlFosMCRZi4PqIZSI1W8StjCpNIa99B4EhExbHG/V5FA== X-Received: by 2002:a05:6a00:23ca:b0:6f8:caf2:eead with SMTP id d2e1a72fcca58-703e59e4ba5mr820051b3a.22.1717540337911; Tue, 04 Jun 2024 15:32:17 -0700 (PDT) Received: from [192.168.60.239] (213.126.145.34.bc.googleusercontent.com. [34.145.126.213]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-70242c245absm7477337b3a.200.2024.06.04.15.32.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 04 Jun 2024 15:32:17 -0700 (PDT) Message-ID: <6b0421e0-8c5a-409b-8f34-3a041c838748@google.com> Date: Tue, 4 Jun 2024 15:32:15 -0700 Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 0/3] usb: gadget: uvc: allocate requests based on frame interval length and buffersize To: Michael Grzeschik Cc: Alan Stern , Laurent Pinchart , Daniel Scally , Greg Kroah-Hartman , Jayant Chowdhary , "etalvala@google.com" , Michael Riesch , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Thinh Nguyen References: <20240522014132.xlf7azgq2urfff2d@synopsys.com> <3f404a27-50e8-42c5-a497-b46751154613@rowland.harvard.edu> <20240522171640.iuol4672rnklc35g@synopsys.com> <0642b7a2-0982-4529-b742-3310f34d16b9@google.com> Content-Language: en-US From: Avichal Rakesh In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 5/29/24 14:24, Michael Grzeschik wrote: > On Tue, May 28, 2024 at 05:33:46PM -0700, Avichal Rakesh wrote: >> >> >> On 5/28/24 15:43, Michael Grzeschik wrote: >>> On Tue, May 28, 2024 at 02:27:34PM -0700, Avichal Rakesh wrote: >>>> >>>> >>>> On 5/28/24 13:22, Michael Grzeschik wrote: >>>>> On Tue, May 28, 2024 at 10:30:30AM -0700, Avichal Rakesh wrote: >>>>>> >>>>>> >>>>>> On 5/22/24 10:37, Michael Grzeschik wrote: >>>>>>> On Wed, May 22, 2024 at 05:17:02PM +0000, Thinh Nguyen wrote: >>>>> One option to be totally sure would be to resend the sentinel request to >>>>> be properly transmitted before starting the next frame. This resend >>>>> polling would probably include some extra zero-length requests. But also >>>>> if this resend keeps failing for n times, the driver should doubt there >>>>> is anything sane going on with the USB connection and bail out somehow. >>>>> >>>>> Since we try to tackle case (1) to avoid transmit errors and also avoid >>>>> creating late enqueued requests in the running isoc transfer, the over >>>>> all chance to trigger missed transfers should be minimal. >>>> >>>> Gotcha. It seems like the UVC gadget driver implicitly assumes that EOF >>>> flag will be used although the userspace application can technically >>>> make it optional. >>> >>> That is not all. The additional UVC_STREAM_ERR tag on the sentinel >>> request can be set optional by the host driver. But by spec the >>> userspace application has to drop the frame when the flag was set. >> >> Looking at the UVC specs, the ERR bit doesn't seem to refer to actual >> transmission error, only errors in frame generation (Section 4.3.1.7 >> of UVC 1.5 Class Specification). Maybe "data discontinuity" can be >> used but the examples given are bad media, and encoder issues, which >> suggests errors at higher level than the wire. > > Oh! That is a new perspective I did not consider. > > With the definition of UVC_STREAM_ERR by spec, the uvc_video driver > would in no case set this header bit for the current frame on its own? > Is that correct? It would indeed seem so. The way gadget driver is architected makes is impossible for the userspace application to notify the host of any errors. > >>> With my proposal this flag will be set, whenever we find out that >>> the currently transferred frame was erroneous. >>> >>>> Summarizing some of the discussions above: >>>> 1. UVC gadget driver should _not_ rely on the usb controller to >>>>   enqueue 0-length requests on UVC gadget drivers behalf; >>>> 2. However keeping up the backpressure to the controller means the >>>>   EOF request will be delayed behind all the zero-length requests. >>> >>> Exactly, this is why we have to somehow finetune the timedelay between >>> requests that trigger interrupts. And also monitor the amount of >>> requests currently enqueued in the hw ringbuffer. So that our drivers >>> enqueue dequeue mechanism is virtually adding only the minimum amount >>> of necessary zero-length requests in the hardware. This should be >>> possible. >>> >>> I am currently thinking through the remaining steps the pump worker has >>> to do on each wakeup to maintain the minimum threshold while waiting >>> with submitting requests that contain actual image payload. >>> >>>> Out of curiosity: What is wrong with letting the host rely on >>>> FID alone? Decoding the jpeg payload _should_ fail if any of the >>>> usb_requests containing the payload failed to transmit. >>> >>> This is not totally true. We saw partially rendered jpeg frames on the >>> host stream. How the host behaves with broken data is totally undefined >>> if the typical uvc flags EOF/ERR are not used as specified. Then think >>> about uncompressed formats. So relying on the transferred image format >>> to solve our problems is just as wrong as relying on the gadgets >>> hardware behavior. >> >> Do you know if the partially rendered frames were valid JPEGs, or >> if the host was simply making a best effort at displaying a broken >> JPEG? Perhaps the fix should go to the host instead? > > I can fully reproduce this with linux and windows hosts. For linux > machines I saw that the host was taking the FID change as a marker > to see the previous frame as ready and just rendered what got through. > This did not lead to garbage but only to partially displayed frames > with jpeg macroblock alignment. I was aware of linux doing so, but I only ever saw this behavior on Windows if there were a lot of invalid frames back to back. I am not super familiar with the guarantees of JPEG, but I suppose it is possible to have a "valid" JPEG with some middle blocks missing as long the EOI bits make it through? I am not sure how we go about solving that. > >> Following is my opinion, feel free to disagree (and correct me if >> something is factually incorrect): >> >> The fundamental issue here is that ISOC doesn't guarantee >> delivery of usb_requests or even basic data consistency upon delivery. >> So the gadget driver has no way to know the state of transmitted data. >> The gadget driver is notified of underruns but not of any other issues, >> and ideally we should never have an underrun if the zero-length >> backpressure is working as intended. >> >> So, UVC gadget driver can reduce the number of errors, but it'll never be >> able to guarantee that the data transmitted to the host isn't somehow >> corrupted or missing unless a more reliable mode of transmission >> (bulk, for example) is used. >> >> All of this to say: The host absolutely needs to be able to handle >> all sorts of invalid and broken payloads. How the host handles it >> might be undefined, but the host can never rely on perfect knowledge >> about the transmission state. In cases like these, where the underlying >> transport is unreliable, the burden of enforcing consistency moves up >> a layer, i.e. to the encoded payload in this case. So it is perfectly >> fine for the host to rely on the encoding to determine if the payload >> is corrupt and handle it accordingly. > > Right. > >> As for uncompressed format, you're correct that subtle corruptions >> may not be caught, but outright missing usb_requests can be easily >> checked by simply looking at the number of bytes in the payload. YUV >> frames are all of the same (predetermined) size for a given resolution. > > That was also my thought about five minutes after I did send you the > previous mail. So sure, this is no real issue for the host. > >> So my recommendation is the following: >> 1. Fix the bandwidth problem by splitting the encoded video frame >>   into more usb_requests (as your patch already does) making sure >>   there are enough free usb_request to encode the video frame in >>   one burst so we don't accidentally inflate the transmission >>   duration of a video frame by sneaking in zero-length requests in >>   the middle. > > Ack. This should already solve a lot of issues. > > For this I would still suggest to move the usb_ep_queue to be done in > the pump worker again. Its a bit back and forth, but IMHO its worth the > extra mile since only this way we would respect the dwc3 interrupt > threads assumption to run *very* short. The main reason for queuing the requests from the complete handler was to have a single point of usb_ep_queue call, which made reasoning through the locking simpler. But if you find a way to do so from the video_pump thread without making the locking a nightmare, then go for it! > >> 2. Unless there is an unusually high rate of transmission failures >>   when using the UVC gadget driver, it might be worth fixing the >>   host side driver to handle broken frames better instead (assuming >>   host is linux as well). > > Agreed, but this needs a separate scoped undestanding of the host side > behaviour over all layers. Agreed! > >> 2. Tighten up the error checking in UVC gadget driver -- We drop the >>   current frame whenever an EXDEV happens which is wrong. We should >>   only be dropping the current frame if the EXDEV corresponds to the >>   frame currently being encoded. > > What do you mean by drop? > > I would suggest to immediatly switch the uvc_buffer that is being > enqueued and start queueing prepared requests from the next buffers prep > list. As suggested, the idea is to have per uvc_buffer prep_list > requests which would make this task easy. Currently, if uvc gadget driver receives an EXDEV complete callback all it does is set the UVC_QUEUE_DROP_INCOMPLETE flag. So let's say that we receive an EXDEV for a usb_request containing data for video frame N. With how video_pump is currently configured, chances are that all usb_requests containing data for video frame N has already been queued to the controller. When the next video frame (N+1) comes in, video_pump's encode methods will look at the UVC_QUEUE_DROP_INCOMPLETE flag and incorrectly determine that "current" frame needs to be dropped, and stop encoding video frame N+1 even though the error was for video frame N. So the encode methods incorrectly drop video frame N+1 which isn't needed. The encode methods should only be dropping the video frame if we received an EXDEV for a usb_request for the video frame currently being encoded. I hope that makes sense! - Avi.