Re: UVC Gadget Driver shows glitched frames with a Linux host

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Apr 20, 2023 at 10:31:24AM -0400, Alan Stern wrote:
> On Thu, Apr 20, 2023 at 02:31:25AM +0000, Thinh Nguyen wrote:
> > On Wed, Apr 19, 2023, Alan Stern wrote:
> > > On Wed, Apr 19, 2023 at 09:49:35PM +0000, Thinh Nguyen wrote:
> > > > By the usb spec, for IN direction, if there's no data available and the
> > > > host requests for data, then the device will send a zero-length data
> > > > packet.
> > > 
> > > I'm not aware of any such requirement in the USB-2 spec.  The closest I 
> > > can find is in section 5.6.4:
> > > 
> > > 	An isochronous IN endpoint must return a zero-length packet 
> > > 	whenever data is requested at a faster interval than the 
> > > 	specified interval and data is not available.
> > > 
> > > But this specifically refers only to situations where the host asks for 
> > > isochonous data more frequently than the endpoint descriptor's bInterval 
> > > describes.
> > 
> > This is from usb 3.2 section 4.4.8.2:
> > 
> > 	Note, if an endpoint has no isochronous data to transmit when
> > 	accessed by the host, it shall send a zero length packet in
> > 	response to the request for data.
> 
> Ah, but chapter 4 in the USB-3.2 spec describes only the SuperSpeed bus, 
> as mentioned in the first paragraph of section 4.1.  So the constraint 
> about sending 0-length isochronous packets when no data is available 
> applies only to SuperSpeed connections.  If a gadget is connected at 
> USB-2 speed, the constraint doesn't apply.
> 
> (And in fact, no matter what the connection speed is, there's always a 
> possibility that the first packet sent by the host to begin an 
> isochronous transfer might get lost or corrupted, in which case the 
> device would not send a reply in any case.)
> 
> > > >  This is what the dwc3 controller will do. So regardless whether
> > > > you prepare and queue a 0-length request or not, the host will receive
> > > > it.
> > > 
> > > Even so, whether the function driver submits these 0-length isochronous 
> > > requests makes a difference in terms of filling the slots in the 
> > > schedule and preventing later requests from becoming stale.
> > 
> > That's not my point. Avi concern was how the host may interpret 0-length
> > packet. My point was to note that it should behave the same as before.
> > Because even without sending 0-length requests, the controller would
> > already respond with 0-length packet already.
> 
> It would be good to confirm the reasoning by checking the source code 
> for the UVC host driver.  But I expect you are right.

The uvcvideo host driver should be fine. An isochronous frame descriptor
with actual_length set to 0 will cause the packet to be ignored. The
uvc_video_decode_isoc() function loops over all packets, and calls
uvc_video_decode_start() which starts with

	if (len < 2 || data[0] < 2 || data[0] > len) {
		stream->stats.frame.nb_invalid++;
		return -EINVAL;
	}

The -EINVAL return value causes the caller to ignore the packet.

We probably want to avoid increasing the counter of invalid packets
though, but the counter is used for debug purpose only, so it doesn't
affect operation negatively.

> > In fact, that's what the UVC driver should be doing, by maintaining the
> > request queue with dummy requests as suggested in the beginning.
> > 
> > Perhaps there was some misunderstanding. Sending 0-length request you
> > suggested is a perfectly good suggestion and UVC gadget driver should do
> > that instead of relying on the workaround in the dwc3 driver.
> 
> Agreed.

-- 
Regards,

Laurent Pinchart