Hi Alan, On Monday 01 July 2013 15:39:46 Alan Stern wrote: > On Mon, 1 Jul 2013, Laurent Pinchart wrote: > > > What about error recovery for insane systems? If we do get a 50-ms gap > > > in the data stream, what's the best way for the UVC driver to learn this > > > has happens and attempt to carry on? > > > > When isochronous packets are lost video frames get corrupted. UVC doesn't > > provide sequence numbers for packets, so there's no way to know exactly > > how many packets have been lost. > > > > However, several synchronization methods are available for the driver to > > detect frame boundaries (namely an End-Of-Frame bit and a Frame ID bit > > that toggles for every frame in isochronous packet headers). The driver > > already handles those bits and marks video frames as complete when the EOF > > bit or an FID transition is detected. For uncompressed formats the driver > > then checks whether the received data size matches the frame size, and > > marks the frame as bad if it doesn't. That check can't be performed for > > compressed formats as the frame size is then variable. > > > > It would thus be helpful to receive a notification when a gap in the data > > stream is detected, through the URB status field for instance. The driver > > could then mark the frame being received as faulty and reset its state > > machine to resynchronize to the next frame boundary. > > One possibility is to set urb->status to -EXDEV if an underrun has caused > the entire URB to be too late. Currently, under those conditions the HCD > rejects the URB submission with a -EXDEV error code, which doesn't seem to > be the right approach. Drivers don't expect isochronous _submissions_ to > fail, although they are prepared to see failure statuses for isochronous > _completions_. > > Of course, it has been true all along that the status fields in the URB's > individual usb_iso_packet_descriptor structures indicate any errors. But > HCDs tend to set urb->status to 0 always, for isochronous URBs. Is there > any advantage to setting urb->status to -EXDEV, given that we already set > urb->iso_frame_desc[i].status to -EXDEV for each i? Not that I see, for the uvcvideo driver at least. > (There's also an urb->error_count field, which reports how many of the > isochronous packets got an error. It is hardly used; I think we could > remove it.) > > This boils down to three possibilities for how to handle URBs that were > submitted too late -- that is, so late that all the time slots for all > the URB's packets are known to have expired already: > > (1) Reject the submission with -EXDEV -- this is what we do now. > > (2) Accept the submission, but have the URB complete immediately > with urb->status and all the packet statuses set to -EXDEV. > > (3) Accept the submission, but have the URB complete immediately > with urb->status set to 0 and all the packet statuses set to > -EXDEV. > > For (1), the only way to recover is to submit an URB with URB_ISO_ASAP > set. This is essentially the same as shutting down the stream and > restarting it. > > For (2) and (3), the stream's "next" time slot value would be updated > in the usual way. For example, if 10 slots had expired and the driver > was submitting URBs containing 4 packets each, then the first and > second URBs would complete right away with errors, and the first two > packets of the third URB would get errors, but the last two packets of > the third URB would be assigned to the two upcoming time slots and > would be treated normally. Thus recovery would be automatic, at the > cost of "wasting" two URBs. Since we expect underruns to be rare, > maybe this is acceptable. > > With (2) or (3), the driver could also recover right away by setting > the URB_ISO_ASAP flag on its next URB, but then synchronization would > be lost. You wouldn't want to do this if synchronization matters. > But if it doesn't, the driver could simply set URB_ISO_ASAP on every > URB to avoid worrying about the problem -- the flag would have no > effect in the absence of underruns. That's what the uvcvideo driver does. This could be changed, but I don't have time to investigate at the moment. (Tested) patches are of course welcome :-) > (The main reason the current code uses (1) is because (2) and (3) > require completion to occur _during_ submission. Resubmissions would > thus be nested, consuming excessive stack space, and there would be a > deadlock if the completion handler tried to acquire a lock that was > held during submission. When a tasklet is used for completions, none > of these problems arise.) > > Any preference? Clemens, what do you think? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
