uvcvideo failure under xHCI

uvcvideo failure under xHCI

[Sarah Sharp]

Hi Laurent,

I think this issue has been happening for a while now, but my recent
patches to remove most of the xHCI debugging have finally allowed me to
use a webcam under xHCI with debugging on.  Unfortunately, it doesn't
work very well.

When I plug in a webcam under an xHCI host controller in 3.0-rc3+
(basically top of Greg's usb-linus branch) with xHCI debugging turned
on, the host controller occasionally cannot keep up with the isochronous
transfers, and it tells the xHCI driver that it had to "skip" several
microframes of transfers.  These "Missed Service Intervals" aren't
supposed to be fatal errors, just an indication that something was
hogging the PCI memory bandwidth.

The xHCI driver then sets the URB's status to -EXDEV, to indicate that
some of the iso_frame_desc transferred, and sets at least one frame's
status to -EXDEV:

static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
                        struct xhci_transfer_event *event,
                        struct xhci_virt_ep *ep, int *status)
{
        struct xhci_ring *ep_ring;
        struct urb_priv *urb_priv;
        struct usb_iso_packet_descriptor *frame;
        int idx;

        ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));
        urb_priv = td->urb->hcpriv;
        idx = urb_priv->td_cnt; 
        frame = &td->urb->iso_frame_desc[idx];
        
        /* The transfer is partly done */
        *status = -EXDEV;
        frame->status = -EXDEV;
        
        /* calc actual length */
        frame->actual_length = 0;
        
        /* Update ring dequeue pointer */
        while (ep_ring->dequeue != td->last_trb)
                inc_deq(xhci, ep_ring, false);
        inc_deq(xhci, ep_ring, false);
        
        return finish_td(xhci, td, NULL, event, ep, status, true);
}

The urb->status causes uvcvideo code in uvc_status.c:uvc_status_complete() to
fail with the message:

Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status (-18) in video completion handler.

It doesn't resubmit the isochronous URB in that case, and the userspace
video freezes.  If I restart the application, the video comes back until
the next Missed Service Interval event from the xHCI driver.  Ideally,
the video driver would just resubmit the URB, and the xHCI host
controller would complete transfers as best it can.  I think the frames
with -EXDEV status should be treated like short transfers.

I've grepped through drivers/media/video, and it seems like none of the
drivers handle the -EXDEV status.  What should the xHCI driver be
setting the URB's status and frame status to when the xHCI host
controller skips over transfers?  -EREMOTEIO?

Or does it need to set the URB's status to zero, but only set the
individual frame status to -EXDEV?

Sarah Sharp

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Wed, Jun 15, 2011 at 06:39:57PM -0700, Sarah Sharp wrote:
> When I plug in a webcam under an xHCI host controller in 3.0-rc3+
> (basically top of Greg's usb-linus branch) with xHCI debugging turned
> on, the host controller occasionally cannot keep up with the isochronous
> transfers, and it tells the xHCI driver that it had to "skip" several
> microframes of transfers.  These "Missed Service Intervals" aren't
> supposed to be fatal errors, just an indication that something was
> hogging the PCI memory bandwidth.
> 
> The xHCI driver then sets the URB's status to -EXDEV, to indicate that
> some of the iso_frame_desc transferred, and sets at least one frame's
> status to -EXDEV:
...
> The urb->status causes uvcvideo code in uvc_status.c:uvc_status_complete() to
> fail with the message:
> 
> Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status (-18) in video completion handler.
...
> I've grepped through drivers/media/video, and it seems like none of the
> drivers handle the -EXDEV status.  What should the xHCI driver be
> setting the URB's status and frame status to when the xHCI host
> controller skips over transfers?  -EREMOTEIO?
> 
> Or does it need to set the URB's status to zero, but only set the
> individual frame status to -EXDEV?

Ok, looking at both EHCI and UHCI, they seem to set the urb->status to
zero, regardless of what they set the frame descriptor field to.

Alan, does that seem correct?

I've created a patch to do the same thing in the xHCI driver, and I seem
to be getting consistent video with xHCI debugging turned on, despite
lots of Missed Service Interval events.

Sarah Sharp

Re: uvcvideo failure under xHCI

[Laurent Pinchart]

Hi Sarah,

On Thursday 16 June 2011 04:59:57 Sarah Sharp wrote:
> On Wed, Jun 15, 2011 at 06:39:57PM -0700, Sarah Sharp wrote:
> > When I plug in a webcam under an xHCI host controller in 3.0-rc3+
> > (basically top of Greg's usb-linus branch) with xHCI debugging turned
> > on, the host controller occasionally cannot keep up with the isochronous
> > transfers, and it tells the xHCI driver that it had to "skip" several
> > microframes of transfers.  These "Missed Service Intervals" aren't
> > supposed to be fatal errors, just an indication that something was
> > hogging the PCI memory bandwidth.
> > 
> > The xHCI driver then sets the URB's status to -EXDEV, to indicate that
> > some of the iso_frame_desc transferred, and sets at least one frame's
> 
> > status to -EXDEV:
> ...
> 
> > The urb->status causes uvcvideo code in
> > uvc_status.c:uvc_status_complete() to fail with the message:
> > 
> > Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status
> > (-18) in video completion handler.
> 
> ...
> 
> > I've grepped through drivers/media/video, and it seems like none of the
> > drivers handle the -EXDEV status.  What should the xHCI driver be
> > setting the URB's status and frame status to when the xHCI host
> > controller skips over transfers?  -EREMOTEIO?
> > 
> > Or does it need to set the URB's status to zero, but only set the
> > individual frame status to -EXDEV?
> 
> Ok, looking at both EHCI and UHCI, they seem to set the urb->status to
> zero, regardless of what they set the frame descriptor field to.
> 
> Alan, does that seem correct?

According to Documentation/usb/error-codes.txt, host controller drivers should 
set the status to -EXDEV. However, no device drivers seem to handle that, 
probably because the EHCI/UHCI drivers don't use that error code.

Drivers are clearly out of sync with the documentation, so we should fix one 
of them.

> I've created a patch to do the same thing in the xHCI driver, and I seem
> to be getting consistent video with xHCI debugging turned on, despite
> lots of Missed Service Interval events.

-- 
Regards,

Laurent Pinchart

Re: uvcvideo failure under xHCI

[Andy Walls]

Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:

>Hi Laurent,
>
>I think this issue has been happening for a while now, but my recent
>patches to remove most of the xHCI debugging


have finally allowed me to
>use a webcam under xHCI with debugging on.  Unfortunately, it doesn't
>work very well.
>
>When I plug in a webcam under an xHCI host controller in 3.0-rc3+
>(basically top of Greg's usb-linus branch) with xHCI debugging turned
>on, the host controller occasionally cannot keep up with the
>isochronous
>transfers, and it tells the xHCI driver that it had to "skip" several
>microframes of transfers.  These "Missed Service Intervals" aren't
>supposed to be fatal errors, just an indication that something was
>hogging the PCI memory bandwidth.
>
>The xHCI driver then sets the URB's status to -EXDEV, to indicate that
>some of the iso_frame_desc transferred, and sets at least one frame's
>status to -EXDEV:
>
>static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
>                        struct xhci_transfer_event *event,
>                        struct xhci_virt_ep *ep, int *status)
>{
>        struct xhci_ring *ep_ring;
>        struct urb_priv *urb_priv;
>        struct usb_iso_packet_descriptor *frame;
>        int idx;
>
>   ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));
>        urb_priv = td->urb->hcpriv;
>        idx = urb_priv->td_cnt; 
>        frame = &td->urb->iso_frame_desc[idx];
>        
>        /* The transfer is partly done */
>        *status = -EXDEV;
>        frame->status = -EXDEV;
>        
>        /* calc actual length */
>        frame->actual_length = 0;
>        
>        /* Update ring dequeue pointer */
>        while (ep_ring->dequeue != td->last_trb)
>                inc_deq(xhci, ep_ring, false);
>        inc_deq(xhci, ep_ring, false);
>        
>        return finish_td(xhci, td, NULL, event, ep, status, true);
>}
>
>The urb->status causes uvcvideo code in
>uvc_status.c:uvc_status_complete() to
>fail with the message:
>
>Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status
>(-18) in video completion handler.
>
>It doesn't resubmit the isochronous URB in that case, and the userspace
>video freezes.  If I restart the application, the video comes back
>until
>the next Missed Service Interval event from the xHCI driver.  Ideally,
>the video driver would just resubmit the URB, and the xHCI host
>controller would complete transfers as best it can.  I think the frames
>with -EXDEV status should be treated like short transfers.
>
>I've grepped through drivers/media/video, and it seems like none of the
>drivers handle the -EXDEV status.  What should the xHCI driver be
>setting the URB's status and frame status to when the xHCI host
>controller skips over transfers?  -EREMOTEIO?
>
>Or does it need to set the URB's status to zero, but only set the
>individual frame status to -EXDEV?
>
>Sarah Sharp
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Video drivers don't handle EXDEV probably because it is an error code about filesystem links:

http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html#tag_02_03


Regards,
Andy

Re: uvcvideo failure under xHCI

[Alan Stern]

On Thu, 16 Jun 2011, Laurent Pinchart wrote:

> Hi Sarah,
> 
> On Thursday 16 June 2011 04:59:57 Sarah Sharp wrote:
> > On Wed, Jun 15, 2011 at 06:39:57PM -0700, Sarah Sharp wrote:
> > > When I plug in a webcam under an xHCI host controller in 3.0-rc3+
> > > (basically top of Greg's usb-linus branch) with xHCI debugging turned
> > > on, the host controller occasionally cannot keep up with the isochronous
> > > transfers, and it tells the xHCI driver that it had to "skip" several
> > > microframes of transfers.  These "Missed Service Intervals" aren't
> > > supposed to be fatal errors, just an indication that something was
> > > hogging the PCI memory bandwidth.
> > > 
> > > The xHCI driver then sets the URB's status to -EXDEV, to indicate that
> > > some of the iso_frame_desc transferred, and sets at least one frame's
> > 
> > > status to -EXDEV:
> > ...
> > 
> > > The urb->status causes uvcvideo code in
> > > uvc_status.c:uvc_status_complete() to fail with the message:
> > > 
> > > Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status
> > > (-18) in video completion handler.
> > 
> > ...
> > 
> > > I've grepped through drivers/media/video, and it seems like none of the
> > > drivers handle the -EXDEV status.  What should the xHCI driver be
> > > setting the URB's status and frame status to when the xHCI host
> > > controller skips over transfers?  -EREMOTEIO?
> > > 
> > > Or does it need to set the URB's status to zero, but only set the
> > > individual frame status to -EXDEV?
> > 
> > Ok, looking at both EHCI and UHCI, they seem to set the urb->status to
> > zero, regardless of what they set the frame descriptor field to.
> > 
> > Alan, does that seem correct?

The description of the behavior of ehci-hcd and uhci-hcd is correct.  
ohci-hcd behaves the same way too.  And they all agree with the 
behavior described in the kerneldoc for struct urb in 
include/linux/usb.h.

> According to Documentation/usb/error-codes.txt, host controller drivers should 
> set the status to -EXDEV. However, no device drivers seem to handle that, 
> probably because the EHCI/UHCI drivers don't use that error code.
> 
> Drivers are clearly out of sync with the documentation, so we should fix one 
> of them.

Under the circumstances, the documentation file should be changed.  
Sarah, can you do that along with the change to xhci-hcd?

Alan Stern

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Thu, Jun 16, 2011 at 10:35:49AM -0400, Alan Stern wrote:
> On Thu, 16 Jun 2011, Laurent Pinchart wrote:
> > On Thursday 16 June 2011 04:59:57 Sarah Sharp wrote:
> > > On Wed, Jun 15, 2011 at 06:39:57PM -0700, Sarah Sharp wrote:
> > > > I've grepped through drivers/media/video, and it seems like none of the
> > > > drivers handle the -EXDEV status.  What should the xHCI driver be
> > > > setting the URB's status and frame status to when the xHCI host
> > > > controller skips over transfers?  -EREMOTEIO?
> > > > 
> > > > Or does it need to set the URB's status to zero, but only set the
> > > > individual frame status to -EXDEV?
> > > 
> > > Ok, looking at both EHCI and UHCI, they seem to set the urb->status to
> > > zero, regardless of what they set the frame descriptor field to.
> > > 
> > > Alan, does that seem correct?
> 
> The description of the behavior of ehci-hcd and uhci-hcd is correct.  
> ohci-hcd behaves the same way too.  And they all agree with the 
> behavior described in the kerneldoc for struct urb in 
> include/linux/usb.h.

Ah, you mean this bit?

 * @status: This is read in non-iso completion functions to get the
 *      status of the particular request.  ISO requests only use it
 *      to tell whether the URB was unlinked; detailed status for
 *      each frame is in the fields of the iso_frame-desc.


> > According to Documentation/usb/error-codes.txt, host controller drivers should 
> > set the status to -EXDEV. However, no device drivers seem to handle that, 
> > probably because the EHCI/UHCI drivers don't use that error code.
> > 
> > Drivers are clearly out of sync with the documentation, so we should fix one 
> > of them.
> 
> Under the circumstances, the documentation file should be changed.  
> Sarah, can you do that along with the change to xhci-hcd?

Sure.  It feels like there should be a note about which values
isochronous URBs might have in the urb->status field.  The USB core is
the only one that would be setting those, so which values would it set?
uvcvideo tests for these error codes:

        case -ENOENT:           /* usb_kill_urb() called. */
        case -ECONNRESET:       /* usb_unlink_urb() called. */
        case -ESHUTDOWN:        /* The endpoint is being disabled. */
        case -EPROTO:           /* Device is disconnected (reported by some
                                 * host controller). */

Are there any others.

Sarah Sharp

Re: uvcvideo failure under xHCI

[Alan Stern]

On Thu, 16 Jun 2011, Sarah Sharp wrote:

> > > > Alan, does that seem correct?
> > 
> > The description of the behavior of ehci-hcd and uhci-hcd is correct.  
> > ohci-hcd behaves the same way too.  And they all agree with the 
> > behavior described in the kerneldoc for struct urb in 
> > include/linux/usb.h.
> 
> Ah, you mean this bit?
> 
>  * @status: This is read in non-iso completion functions to get the
>  *      status of the particular request.  ISO requests only use it
>  *      to tell whether the URB was unlinked; detailed status for
>  *      each frame is in the fields of the iso_frame-desc.

Right.  There's also some more near the end:

 * Completion Callbacks:
 *
 * The completion callback is made in_interrupt(), and one of the first
 * things that a completion handler should do is check the status field.
 * The status field is provided for all URBs.  It is used to report
 * unlinked URBs, and status for all non-ISO transfers.  It should not
 * be examined before the URB is returned to the completion handler.

> > Under the circumstances, the documentation file should be changed.  
> > Sarah, can you do that along with the change to xhci-hcd?
> 
> Sure.  It feels like there should be a note about which values
> isochronous URBs might have in the urb->status field.  The USB core is
> the only one that would be setting those, so which values would it set?
> uvcvideo tests for these error codes:
> 
>         case -ENOENT:           /* usb_kill_urb() called. */
>         case -ECONNRESET:       /* usb_unlink_urb() called. */
>         case -ESHUTDOWN:        /* The endpoint is being disabled. */
>         case -EPROTO:           /* Device is disconnected (reported by some
>                                  * host controller). */
> 
> Are there any others.

-EREMOTEIO, in the unlikely event that URB_SHORT_NOT_OK is set, but no
others.

And I wasn't aware of that last one...  Host controller drivers should
report -ESHUTDOWN to mean the device has been disconnected, not
-EPROTO.  But usually HCD don't take these events into account when
determining URB status codes.

Alan Stern

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Thu, Jun 16, 2011 at 01:39:43PM -0400, Alan Stern wrote:
> On Thu, 16 Jun 2011, Sarah Sharp wrote:
> 
> > > > > Alan, does that seem correct?
> > > 
> > > The description of the behavior of ehci-hcd and uhci-hcd is correct.  
> > > ohci-hcd behaves the same way too.  And they all agree with the 
> > > behavior described in the kerneldoc for struct urb in 
> > > include/linux/usb.h.
> > 
> > Ah, you mean this bit?
> > 
> >  * @status: This is read in non-iso completion functions to get the
> >  *      status of the particular request.  ISO requests only use it
> >  *      to tell whether the URB was unlinked; detailed status for
> >  *      each frame is in the fields of the iso_frame-desc.
> 
> Right.  There's also some more near the end:
> 
>  * Completion Callbacks:
>  *
>  * The completion callback is made in_interrupt(), and one of the first
>  * things that a completion handler should do is check the status field.
>  * The status field is provided for all URBs.  It is used to report
>  * unlinked URBs, and status for all non-ISO transfers.  It should not
>  * be examined before the URB is returned to the completion handler.
> 
> > > Under the circumstances, the documentation file should be changed.  
> > > Sarah, can you do that along with the change to xhci-hcd?
> > 
> > Sure.  It feels like there should be a note about which values
> > isochronous URBs might have in the urb->status field.  The USB core is
> > the only one that would be setting those, so which values would it set?
> > uvcvideo tests for these error codes:
> > 
> >         case -ENOENT:           /* usb_kill_urb() called. */
> >         case -ECONNRESET:       /* usb_unlink_urb() called. */
> >         case -ESHUTDOWN:        /* The endpoint is being disabled. */
> >         case -EPROTO:           /* Device is disconnected (reported by some
> >                                  * host controller). */
> > 
> > Are there any others.
> 
> -EREMOTEIO, in the unlikely event that URB_SHORT_NOT_OK is set, but no
> others.

Are you saying that the USB core will only set -EREMOTEIO for
isochronous URBs?  Or do you mean that in addition to the status values
that uvcvideo checks, the USB core can also set -EREMOTEIO?

> And I wasn't aware of that last one...  Host controller drivers should
> report -ESHUTDOWN to mean the device has been disconnected, not
> -EPROTO.  But usually HCD don't take these events into account when
> determining URB status codes.

The xHCI driver will return -ESHUTDOWN as a status for URBs when the
host controller is dying.

Sarah Sharp

Re: uvcvideo failure under xHCI

[Alan Stern]

On Thu, 16 Jun 2011, Sarah Sharp wrote:

> > > Sure.  It feels like there should be a note about which values
> > > isochronous URBs might have in the urb->status field.  The USB core is
> > > the only one that would be setting those, so which values would it set?
> > > uvcvideo tests for these error codes:
> > > 
> > >         case -ENOENT:           /* usb_kill_urb() called. */
> > >         case -ECONNRESET:       /* usb_unlink_urb() called. */
> > >         case -ESHUTDOWN:        /* The endpoint is being disabled. */
> > >         case -EPROTO:           /* Device is disconnected (reported by some
> > >                                  * host controller). */
> > > 
> > > Are there any others.
> > 
> > -EREMOTEIO, in the unlikely event that URB_SHORT_NOT_OK is set, but no
> > others.
> 
> Are you saying that the USB core will only set -EREMOTEIO for
> isochronous URBs?  Or do you mean that in addition to the status values
> that uvcvideo checks, the USB core can also set -EREMOTEIO?

The latter.  However, if uvcvideo never sets the URB_SHORT_NOT_OK flag 
then usbcore will never set urb->status to -EREMOTEIO.

> > And I wasn't aware of that last one...  Host controller drivers should
> > report -ESHUTDOWN to mean the device has been disconnected, not
> > -EPROTO.  But usually HCD don't take these events into account when
> > determining URB status codes.
> 
> The xHCI driver will return -ESHUTDOWN as a status for URBs when the
> host controller is dying.

That's appropriate.  But nobody should ever set an isochronous URB's
status field to -EPROTO, no matter whether the device is connected or
not and no matter whether the host controller is alive or not.

Alan Stern

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> That's appropriate.  But nobody should ever set an isochronous URB's
> status field to -EPROTO, no matter whether the device is connected or
> not and no matter whether the host controller is alive or not.

But the individual frame status be set to -EPROTO, correct?  That's what
Alex was told to do when an isochronous TD had a completion code of
"Incompatible Device Error".

Sarah Sharp

Re: uvcvideo failure under xHCI

[Alan Stern]

On Thu, 16 Jun 2011, Sarah Sharp wrote:

> On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > That's appropriate.  But nobody should ever set an isochronous URB's
> > status field to -EPROTO, no matter whether the device is connected or
> > not and no matter whether the host controller is alive or not.
> 
> But the individual frame status be set to -EPROTO, correct?  That's what
> Alex was told to do when an isochronous TD had a completion code of
> "Incompatible Device Error".

Right.  -EPROTO is a perfectly reasonable code for a frame's status.  
But not for an isochronous URB's status.  There's no reason for 
uvcvideo to test for it.

Alan Stern

Re: uvcvideo failure under xHCI

[Laurent Pinchart]

On Thursday 16 June 2011 22:20:22 Alan Stern wrote:
> On Thu, 16 Jun 2011, Sarah Sharp wrote:
> > On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > > That's appropriate.  But nobody should ever set an isochronous URB's
> > > status field to -EPROTO, no matter whether the device is connected or
> > > not and no matter whether the host controller is alive or not.
> > 
> > But the individual frame status be set to -EPROTO, correct?  That's what
> > Alex was told to do when an isochronous TD had a completion code of
> > "Incompatible Device Error".
> 
> Right.  -EPROTO is a perfectly reasonable code for a frame's status.
> But not for an isochronous URB's status.  There's no reason for
> uvcvideo to test for it.

The uvcvideo driver tests for -EPROTO for interrupt URBs only. For isochronous 
URBs it tests for -ENOENT, -ECONNRESET and -ESHUTDOWN.

-- 
Regards,

Laurent Pinchart

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Fri, Jun 17, 2011 at 10:18:39AM +0200, Laurent Pinchart wrote:
> On Thursday 16 June 2011 22:20:22 Alan Stern wrote:
> > On Thu, 16 Jun 2011, Sarah Sharp wrote:
> > > On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > > > That's appropriate.  But nobody should ever set an isochronous URB's
> > > > status field to -EPROTO, no matter whether the device is connected or
> > > > not and no matter whether the host controller is alive or not.
> > > 
> > > But the individual frame status be set to -EPROTO, correct?  That's what
> > > Alex was told to do when an isochronous TD had a completion code of
> > > "Incompatible Device Error".
> > 
> > Right.  -EPROTO is a perfectly reasonable code for a frame's status.
> > But not for an isochronous URB's status.  There's no reason for
> > uvcvideo to test for it.
> 
> The uvcvideo driver tests for -EPROTO for interrupt URBs only. For isochronous 
> URBs it tests for -ENOENT, -ECONNRESET and -ESHUTDOWN.

So is uvc_status_complete() shared between interrupt and isochronous
URBs then?

Sarah Sharp

Re: uvcvideo failure under xHCI

[Laurent Pinchart]

Hi Sarah,

On Friday 17 June 2011 18:46:20 Sarah Sharp wrote:
> On Fri, Jun 17, 2011 at 10:18:39AM +0200, Laurent Pinchart wrote:
> > On Thursday 16 June 2011 22:20:22 Alan Stern wrote:
> > > On Thu, 16 Jun 2011, Sarah Sharp wrote:
> > > > On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > > > > That's appropriate.  But nobody should ever set an isochronous
> > > > > URB's status field to -EPROTO, no matter whether the device is
> > > > > connected or not and no matter whether the host controller is
> > > > > alive or not.
> > > > 
> > > > But the individual frame status be set to -EPROTO, correct?  That's
> > > > what Alex was told to do when an isochronous TD had a completion
> > > > code of "Incompatible Device Error".
> > > 
> > > Right.  -EPROTO is a perfectly reasonable code for a frame's status.
> > > But not for an isochronous URB's status.  There's no reason for
> > > uvcvideo to test for it.
> > 
> > The uvcvideo driver tests for -EPROTO for interrupt URBs only. For
> > isochronous URBs it tests for -ENOENT, -ECONNRESET and -ESHUTDOWN.
> 
> So is uvc_status_complete() shared between interrupt and isochronous
> URBs then?

No, uvc_status_complete() handles status URBs (interrupt only), and 
uvc_video_complete() handles video URBs (isochronous or bulk, depending on the 
device).

-- 
Regards,

Laurent Pinchart

Re: uvcvideo failure under xHCI

[Sarah Sharp]

On Fri, Jun 17, 2011 at 07:01:10PM +0200, Laurent Pinchart wrote:
> Hi Sarah,
> 
> On Friday 17 June 2011 18:46:20 Sarah Sharp wrote:
> > On Fri, Jun 17, 2011 at 10:18:39AM +0200, Laurent Pinchart wrote:
> > > On Thursday 16 June 2011 22:20:22 Alan Stern wrote:
> > > > On Thu, 16 Jun 2011, Sarah Sharp wrote:
> > > > > On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > > > > > That's appropriate.  But nobody should ever set an isochronous
> > > > > > URB's status field to -EPROTO, no matter whether the device is
> > > > > > connected or not and no matter whether the host controller is
> > > > > > alive or not.
> > > > > 
> > > > > But the individual frame status be set to -EPROTO, correct?  That's
> > > > > what Alex was told to do when an isochronous TD had a completion
> > > > > code of "Incompatible Device Error".
> > > > 
> > > > Right.  -EPROTO is a perfectly reasonable code for a frame's status.
> > > > But not for an isochronous URB's status.  There's no reason for
> > > > uvcvideo to test for it.
> > > 
> > > The uvcvideo driver tests for -EPROTO for interrupt URBs only. For
> > > isochronous URBs it tests for -ENOENT, -ECONNRESET and -ESHUTDOWN.
> > 
> > So is uvc_status_complete() shared between interrupt and isochronous
> > URBs then?
> 
> No, uvc_status_complete() handles status URBs (interrupt only), and 
> uvc_video_complete() handles video URBs (isochronous or bulk, depending on the 
> device).

Huh, that's very odd then.  I could have sworn I was getting missed
service interval events (which are only for isochronous transfers) and
then seeing the "Non-zero" message.  And the userspace video definitely
froze before my patch and did not freeze after the patch was applied.
I'll have to look more closely at the logs.

Sarah Sharp

Re: uvcvideo failure under xHCI

[Laurent Pinchart]

Hi Sarah,

On Friday 17 June 2011 20:19:01 Sarah Sharp wrote:
> On Fri, Jun 17, 2011 at 07:01:10PM +0200, Laurent Pinchart wrote:
> > On Friday 17 June 2011 18:46:20 Sarah Sharp wrote:
> > > On Fri, Jun 17, 2011 at 10:18:39AM +0200, Laurent Pinchart wrote:
> > > > On Thursday 16 June 2011 22:20:22 Alan Stern wrote:
> > > > > On Thu, 16 Jun 2011, Sarah Sharp wrote:
> > > > > > On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote:
> > > > > > > That's appropriate.  But nobody should ever set an isochronous
> > > > > > > URB's status field to -EPROTO, no matter whether the device is
> > > > > > > connected or not and no matter whether the host controller is
> > > > > > > alive or not.
> > > > > > 
> > > > > > But the individual frame status be set to -EPROTO, correct? 
> > > > > > That's what Alex was told to do when an isochronous TD had a
> > > > > > completion code of "Incompatible Device Error".
> > > > > 
> > > > > Right.  -EPROTO is a perfectly reasonable code for a frame's
> > > > > status. But not for an isochronous URB's status.  There's no
> > > > > reason for uvcvideo to test for it.
> > > > 
> > > > The uvcvideo driver tests for -EPROTO for interrupt URBs only. For
> > > > isochronous URBs it tests for -ENOENT, -ECONNRESET and -ESHUTDOWN.
> > > 
> > > So is uvc_status_complete() shared between interrupt and isochronous
> > > URBs then?
> > 
> > No, uvc_status_complete() handles status URBs (interrupt only), and
> > uvc_video_complete() handles video URBs (isochronous or bulk, depending
> > on the device).
> 
> Huh, that's very odd then.  I could have sworn I was getting missed
> service interval events (which are only for isochronous transfers) and
> then seeing the "Non-zero" message.  And the userspace video definitely
> froze before my patch and did not freeze after the patch was applied.
> I'll have to look more closely at the logs.

The log excerpt message you posted was

Jun 15 17:37:11 talon kernel: [  117.987769] uvcvideo: Non-zero status (-18) 
in video completion handler.

That's printed by uvc_video_complete(). uvc_status_complete() would have 
printed "Non-zero status (-18) in status completion handler". There's nothing 
wrong with your analysis of the problem, you just picked the wrong uvcvideo 
function.

-- 
Regards,

Laurent Pinchart