Re: New USB core API to change interval and max packet size

2013-10-02 Thread Sarah Sharp
On Wed, Oct 02, 2013 at 12:16:04AM +0300, Xenia Ragiadakou wrote:
 On 10/01/2013 11:45 PM, Sarah Sharp wrote:
 Sure.  I would actually suggest you first finish up the patch to issue a
 configure endpoint if userspace wants to clear a halt, but the endpoint
 isn't actually halted.  Did your most current patch work?  I can't
 remember what the status was.
 
 Sarah Sharp
 
 Thanks for the clarification, I understand better now.
 As far as concerns the reset endpoint fix, I am not sure if it works
 I have to send an RFC so that it can be further tested but I have a
 lot of pending RFCs for xhci on the mailing list so i was waiting
 for those to be reviewed before sending new ones. Or it is not
 necessary to wait and just send the RFC based on current usb-next
 tree?

Go ahead and send that one as well.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: New USB core API to change interval and max packet size

2013-10-02 Thread Sarah Sharp
On Wed, Oct 02, 2013 at 10:22:52AM -0400, Alan Stern wrote:
 On Tue, 1 Oct 2013, Sarah Sharp wrote:
 
   When you say a new API, what do you mean? New functions in usbcore
   to be used by usb device drivers?
  
  Yes.  You would export the function in the USB core, and put a prototype
  in a USB include file (probably in include/linux/usb.h).  Let's say that
  function is called usb_change_ep_bandwidth.
  
  Drivers could call into that function when they needed to change either
  the bInterval or wMaxPacketSize of a particular endpoint.  This could be
  during the driver's probe function, or before switching alternate
  interface settings, or even after the alt setting is in place, but
  userspace dictates the driver use a different bandwidth.
  
  Drivers should pass usb_change_ep_bandwidth a pointer to the endpoint
  they need to change, along with the bInterval and wMaxPacketSize values
  they would like the endpoint to have.  Those values could be stored as
  new values in struct usb_host_endpoint.
  
  usb_change_ep_bandwidth would then call into the xHCI driver to drop the
  endpoint, re-add it, and then issue a bandwidth change.  The xHCI driver
  would have to be changed to look at the new fields in usb_host_endpoint,
  and set up the endpoint contexts with the interval and packet size from
  those fields, instead of the endpoint descriptor.
  
  We should probably set the new values in usb_host_endpoint to zero after
  the driver unbinds from the device.  Not sure if they should be reset
  after the driver switches interfaces.  I would have to see the use cases
  in the driver.
 
 We should consider this before rushing into a new API.

Yes, I agree. :)  That's why I'd like to see some cases in the media
drivers code where it could benefit from changing the interval or
maxpacket size, so that we can see what use cases we have.  Mauro, can
you point is to places in drivers that would need this?

 In particular, do we want to go around changing single endpoints, one 
 at a time?  Or do we want to change all the endpoints in an interface 
 at once?
 
 Given that a change to one endpoint may require the entire schedule to 
 be recomputed, it seems to make more sense to do all of them at once.  
 For example, drivers could call a routine to set the desired endpoint 
 parameters, and then the new parameters could take effect when the 
 driver calls usb_set_interface().

I think it makes sense to change several endpoints, and then ask the
host to recompute the schedule.  Perhaps the driver could issue several
calls to usb_change_ep_bandwidth and then ask the USB core to update the
host's schedule?

I'm not sure that usb_set_interface() is the right function for the new
parameters to take effect.  What if the driver is trying to modify the
current alt setting?  Would they need to call usb_set_interface() again?

 In any case, the question about what to do when the interface setting
 gets switched never really arises.  Each usb_host_endpoint structure is
 referenced from only one altsetting.  If the driver wants the new 
 parameters applied to an endpoint in several altsettings, it will have 
 to change each altsetting separately.

Ok, so it sounds like we want to keep the changes the endpoints across
alt setting changes.  But we still want to reset the values after the
driver unbinds, correct?

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


New USB core API to change interval and max packet size

2013-10-01 Thread Sarah Sharp
On Tue, Oct 01, 2013 at 10:01:08PM +0300, Xenia Ragiadakou wrote:
 Hi Sarah,
 
 I read the mail on 'possible conflict between xhci_hcd and a patched
 usbhid'.

For reference to others:
http://marc.info/?l=linux-usbm=138064948726038w=2
http://marc.info/?l=linux-usbm=138065201426880w=2

 I looked in xhci and the problem arises in xhci_queue_intr_tx() when
 if (xhci_interval != ep_interval) {
 ...
 urb-interval = xhci_interval;
 }
 
 right?

Yes.  The underlying problem is that the xHCI host sets up the endpoint
contexts during the Configure Endpoint command, using the interval from
the device's endpoint descriptors.  It also uses the endpoint descriptor
wMaxPacketSize, which can be wrong as well.  If the device driver wants
to use a different urb-interval than is in the endpoint descriptor, the
xHCI driver will simply ignore it.

(I'm Ccing the linux-media list, as I've discussed some of these devices
with broken descriptors before.)

 When you say a new API, what do you mean? New functions in usbcore
 to be used by usb device drivers?

Yes.  You would export the function in the USB core, and put a prototype
in a USB include file (probably in include/linux/usb.h).  Let's say that
function is called usb_change_ep_bandwidth.

Drivers could call into that function when they needed to change either
the bInterval or wMaxPacketSize of a particular endpoint.  This could be
during the driver's probe function, or before switching alternate
interface settings, or even after the alt setting is in place, but
userspace dictates the driver use a different bandwidth.

Drivers should pass usb_change_ep_bandwidth a pointer to the endpoint
they need to change, along with the bInterval and wMaxPacketSize values
they would like the endpoint to have.  Those values could be stored as
new values in struct usb_host_endpoint.

usb_change_ep_bandwidth would then call into the xHCI driver to drop the
endpoint, re-add it, and then issue a bandwidth change.  The xHCI driver
would have to be changed to look at the new fields in usb_host_endpoint,
and set up the endpoint contexts with the interval and packet size from
those fields, instead of the endpoint descriptor.

We should probably set the new values in usb_host_endpoint to zero after
the driver unbinds from the device.  Not sure if they should be reset
after the driver switches interfaces.  I would have to see the use cases
in the driver.

 Here, it is needed to change the endpoint descriptors with the new
 value in urb so that xhci takes the correct value?
 I mean the fix should be made in usbcore and xhci shall remain intact?

No, we need to fix both the xHCI driver and the USB core.

 I have time to work on that but i 'm not sure that i understood.

Sure.  I would actually suggest you first finish up the patch to issue a
configure endpoint if userspace wants to clear a halt, but the endpoint
isn't actually halted.  Did your most current patch work?  I can't
remember what the status was.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: issue with uvcvideo and xhci

2012-12-12 Thread Sarah Sharp
On Wed, Dec 12, 2012 at 12:47:24PM +0100, Javier Martinez Canillas wrote:
 Hello,
 
 We have an issue when trying to use USB cameras on a particular machine using
 the latest mainline Linux 3.7 kernel. This is not a regression since the same
 issue is present with older kernels (i.e: 3.5).
 
 The cameras work fine when plugged to an USB2.0 port (using the EHCI HCD host
 controller driver) but they don't when using the USB3.0 port (using the xHCI
 HCD host controller driver).
 
 The machine's USB3.0 host controller is a NEC Corporation uPD720200 USB 3.0 
 Host
 Controller (rev 04).
 
 When enabling trace on the uvcvideo driver I see that most frames are lost:
 
 Dec 12 11:07:58 thinclient kernel: [ 4965.597637] uvcvideo: USB isochronous
 frame lost (-18).
 Dec 12 11:07:58 thinclient kernel: [ 4965.597642] uvcvideo: USB isochronous
 frame lost (-18).
 Dec 12 11:07:58 thinclient kernel: [ 4965.597647] uvcvideo: Marking buffer as
 bad (error bit set).
 Dec 12 11:07:58 thinclient kernel: [ 4965.597651] uvcvideo: Frame complete 
 (EOF
 found).
 Dec 12 11:07:58 thinclient kernel: [ 4965.597655] uvcvideo: EOF in empty 
 payload.
 Dec 12 11:07:58 thinclient kernel: [ 4965.597661] uvcvideo: Dropping payload
 (out of sync).
 Dec 12 11:07:58 thinclient kernel: [ 4965.813294] uvcvideo: frame 486 stats:
 0/2/8 packets, 0/0/8 pts
 
 The uvcvideo checks if urb-iso_frame_desc[i].status  0 on the
 uvc_video_decode_isoc() function (drivers/media/usb/uvc/uvc_video.c).
 
 I checked on the xhci driver and the only place where this error code (-EXDEV)
 is assigned to frame-status is inside the skip_isoc_td() function
 (drivers/usb/host/xhci-ring.c).
 
 At this point I'm not sure if this is a bug on the xhci driver, another quirk
 needed by the XHCI_NEC_HOST, a camera misconfiguration on the USB Video Class
 driver or a firmware/hardware bug.

It's a known performance issue, although it's not clear whether it's on
the xHCI driver side or the host controller side.  When an interface
setting is enabled where the isochronous endpoint requires two
additional transfers per service interval, the NEC host controller
starts reporting many missed service intervals.  The xHCI driver then
finds all the frame buffers that were skipped and marks them with the
-EXDEV status.

An error status of Missed Service Interval means the host controller
could not access the transfer memory fast enough through the PCI bus to
service the endpoint in time.  It could be a host hardware issue, or it
could be software slowing down the system to a crawl.  I lean towards a
software issue since, as you said, the Windows driver works fine.
(Although who knows what NEC quirks the Windows driver is working
around...)

The NEC xHCI host controller is a 0.96 revision, which doesn't support
the Block Event Interrupt (BEI) flag which cuts down on the number of
interrupts per URB submitted.  So the xHCI driver's interrupt routine
gets called on every single service interval, rather than being called
once per URB.

Since the Linux xHCI driver isn't really optimized for performance yet,
the interrupt handler is probably pretty slow and could cause delays in
submitting future URBs.  The high amount of interrupts is probably
causing other systems to be starved, possibly leading to the xHCI host
controller not being able to access memory fast enough to service the
endpoint.

 The cameras are reported to work on the same machine but using another 
 operating
 system (Windows).

Windows probably uses Event Data TRBs to cut the interrupts down to one
per URB.  It would take a major effort to make the xHCI driver use Event
Data TRBs.

 I was wondering if you can give me some pointers on how to be sure what's the
 issue or if this rings any bells to you.

I don't have time to work on performance issues right now, as I have
several other critical bugs (mostly around failed S3/S4).  However, if
you want to try to fix this issue yourself, I suggest you run perf and
see where the bottle necks in the xHCI interrupt handler are.

I suspect that part of it is that the interrupt handler reads the xHCI
status register.  That PCI register read is pretty costly, and it's not
necessary since 99% of the time the host controller is going to report
an OK status.  And there's no guarantee that when the host does have an
error that it will set a bad status.

But without an analysis by perf, we won't really know where the
bottlenecks are.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: USB mini-summit at LinuxCon Vancouver

2011-08-09 Thread Sarah Sharp
On Tue, Aug 09, 2011 at 09:52:44AM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/08/2011 07:58 PM, Sarah Sharp wrote:
 On Fri, Aug 05, 2011 at 09:45:56AM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/05/2011 12:56 AM, Greg KH wrote:
 On Thu, Aug 04, 2011 at 07:21:47PM -0300, Mauro Carvalho Chehab wrote:
 I think it is important to separate oranges from apples here, there are
 at least 3 different problem classes which all seem to have gotten thrown
 onto a pile here:
 
 1) The reason Mauro suggested having some discussion on this at the
 USB summit is because of a discussion about dual mode cameras on the
 linux media list.
 ...
 3) Re-direction of usb devices to virtual machines. This works by using
 the userspace usbfs interface from qemu / vmware / virtualbox / whatever.
 The basics of this work fine, but it lacks proper locking / safeguards
 for when a vm takes over a usb device from the in kernel driver.
 
 Hi Hans and Mauro,
 
 We have do room in the schedule for the USB mini-summit for this
 discussion, since the schedule is still pretty flexible.  The
 preliminary schedule is up here:
 
 http://userweb.kernel.org/~sarah/linuxcon-usb-minisummit.html
 
 I think it's best to discuss the VM redirection in the afternoon when
 some of the KVM folks join us after Hans' talk on USB redirection over
 the network.
 
 
 That seems best to me too. I'm available at both proposed time slots,
 and I would like to join both discussions.
 
 It sounds like we need a separate topic for the dual mode cameras and TV
 tuners.  Mauro, do you want to lead that discussion in the early morning
 (in a 9:30 to 10:30 slot) or in the late afternoon (in a 15:30 to 16:30
 slot)?  I want to be sure we have all the video/media developers who are
 interested in this topic present, and I don't know if they will be going
 to the KVM forum.
 
 I would really like to see the dual mode camera and TV tuner discussion
 separated. They are 2 different issues AFAIK.

Ok, great, I've put both topics in the morning, in separate slots.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: USB mini-summit at LinuxCon Vancouver

2011-08-08 Thread Sarah Sharp
On Fri, Aug 05, 2011 at 09:45:56AM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/05/2011 12:56 AM, Greg KH wrote:
 On Thu, Aug 04, 2011 at 07:21:47PM -0300, Mauro Carvalho Chehab wrote:
 I think it is important to separate oranges from apples here, there are
 at least 3 different problem classes which all seem to have gotten thrown
 onto a pile here:
 
 1) The reason Mauro suggested having some discussion on this at the
 USB summit is because of a discussion about dual mode cameras on the
 linux media list.
...
 3) Re-direction of usb devices to virtual machines. This works by using
 the userspace usbfs interface from qemu / vmware / virtualbox / whatever.
 The basics of this work fine, but it lacks proper locking / safeguards
 for when a vm takes over a usb device from the in kernel driver.

Hi Hans and Mauro,

We have do room in the schedule for the USB mini-summit for this
discussion, since the schedule is still pretty flexible.  The
preliminary schedule is up here:

http://userweb.kernel.org/~sarah/linuxcon-usb-minisummit.html

I think it's best to discuss the VM redirection in the afternoon when
some of the KVM folks join us after Hans' talk on USB redirection over
the network.

It sounds like we need a separate topic for the dual mode cameras and TV
tuners.  Mauro, do you want to lead that discussion in the early morning
(in a 9:30 to 10:30 slot) or in the late afternoon (in a 15:30 to 16:30
slot)?  I want to be sure we have all the video/media developers who are
interested in this topic present, and I don't know if they will be going
to the KVM forum.

Link PM was originally slated for the 10am slot, but since we're missing
several people for that discussion (Alan Stern, Andiry Xu, and any of
the Intel folks who did the Moorestown USB 2.0 LPM), I think it will be
more useful to have that discussion on-list or at another conference.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: USB mini-summit at LinuxCon Vancouver

2011-08-08 Thread Sarah Sharp
On Mon, Aug 08, 2011 at 01:23:56PM -0500, Theodore Kilgore wrote:
 
 
 On Mon, 8 Aug 2011, Sarah Sharp wrote:
 
  On Fri, Aug 05, 2011 at 09:45:56AM +0200, Hans de Goede wrote:
   Hi,
   
   On 08/05/2011 12:56 AM, Greg KH wrote:
   On Thu, Aug 04, 2011 at 07:21:47PM -0300, Mauro Carvalho Chehab wrote:
   I think it is important to separate oranges from apples here, there are
   at least 3 different problem classes which all seem to have gotten thrown
   onto a pile here:
   
   1) The reason Mauro suggested having some discussion on this at the
   USB summit is because of a discussion about dual mode cameras on the
   linux media list.
  ...
   3) Re-direction of usb devices to virtual machines. This works by using
   the userspace usbfs interface from qemu / vmware / virtualbox / whatever.
   The basics of this work fine, but it lacks proper locking / safeguards
   for when a vm takes over a usb device from the in kernel driver.
  
  Hi Hans and Mauro,
  
  We have do room in the schedule for the USB mini-summit for this
  discussion, since the schedule is still pretty flexible.  The
  preliminary schedule is up here:
  
  http://userweb.kernel.org/~sarah/linuxcon-usb-minisummit.html
  
  I think it's best to discuss the VM redirection in the afternoon when
  some of the KVM folks join us after Hans' talk on USB redirection over
  the network.
  
  It sounds like we need a separate topic for the dual mode cameras and TV
  tuners.  Mauro, do you want to lead that discussion in the early morning
  (in a 9:30 to 10:30 slot) or in the late afternoon (in a 15:30 to 16:30
  slot)?  I want to be sure we have all the video/media developers who are
  interested in this topic present, and I don't know if they will be going
  to the KVM forum.
 
 Sarah,
 
 Alas. I would suspect that I am one of the people most interested in the 
 topic of dual-mode cameras, since I have worked on supporting them both in 
 libgphoto2 and in the kernel. But I teach in a university for a living, 
 and the first classes of Fall Semester 2011 start on August 17 in Auburn, 
 Alabama. Knowing this, I decided, months ago, that I simply could not 
 attend a conference which starts on August 16 in Vancouver.
 
 So, after starting all of the current mailing-list discussion on the topic 
 I will not be at the conference. I can only hope that those who do attend 
 will keep me current about what gets discussed.

No worries, I'll be taking notes and post them to the Linux USB and
Linux media lists.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 0/2] USB: EHCI: Allow users to override 80% max periodic bandwidth

2011-06-30 Thread Sarah Sharp
On Fri, Jun 24, 2011 at 08:48:06PM +0400, Kirill Smelkov wrote:
 
 Changes since v1:
 
 
  - dropped RFC status as this seems like the sort of feature somebody might
reasonably want to use -- if they know exactly what they're doing;
 
  - new preparatory patch (1/2) which moves already-in-there sysfs code into
ehci-sysfs.c;
 
  - moved uframe_periodic_max parameter from module option to sysfs attribute,
so that it can be set per controller and at runtime, added validity checks;
 
  - clarified a bit bandwith analysis for 96% max periodic setup as noticed by
Alan Stern;
 
  - clarified patch description saying that set in stone 80% max periodic is
specified by USB 2.0;

Have you tested this patch by maxing out this bandwidth on various
types of host controllers?  It's entirely possible that you'll run into
vendor-specific bugs if you try to pack the schedule with isochronous
transfers.  I don't think any hardware designer would seriously test or
validate their hardware with a schedule that is basically a violation of
the USB bus spec (more than 80% for periodic transfers).

But if Alan is fine with giving users a way to shoot themselves in the
foot, and it's disabled by default, then I don't particularly mind this
patch.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-17 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-17 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-16 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-16 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-16 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


uvcvideo failure under xHCI

2011-06-15 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: uvcvideo failure under xHCI

2011-06-15 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: USB mini-summit at LinuxCon Vancouver

2011-06-10 Thread Sarah Sharp
On Thu, Jun 09, 2011 at 08:18:05PM -0700, Greg KH wrote:
 On Thu, Jun 09, 2011 at 05:21:03PM -0700, Sarah Sharp wrote:
  Topic 1
  ---
  
  The KVM folks suggested that it would be good to get USB and
  virtualization developers together to talk about how to virtualize the
  xHCI host controller.  The xHCI spec architect worked closely with
  VMWare to get some extra goodies in the spec to help virtualization, and
  I'd like to see the other virtualization developers take advantage of
  that.  I'd also like us to hash out any issues they have been finding in
  the USB core or xHCI driver during the virtualization effort.
 
 Do people really want to virtualize the whole xHCI controller, or just
 specific ports or devices to the guest operating system?

A host OS could chose to virtualize the whole xHCI controller if it
wanted to.  That's part of the reason why xHCI does all the bandwidth
checking in hardware, not in software.

 If just specific ports, would something like usbip be better for virtual
 machines, with the USB traffic going over the network connection between
 the guest/host?

It could be done that way too.  But that doesn't help if you're trying
to run Windows under Linux, right?  Only if all the guest OSes use the
same USB IP protocol then it would work.

  Hope to see you there!
 
 Thanks for putting this together.
 
 Do we need to sign up somewhere to give the organizers a sense of how
 many people will be attending?

I private ack by email would be great.  Or you can ack by facebook:
https://www.facebook.com/event.php?eid=229532200405657  I could add the
event in upcoming.yahoo.com if anyone uses that.

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


USB mini-summit at LinuxCon Vancouver

2011-06-09 Thread Sarah Sharp
I'm pleased to announce a USB mini-summit at LinuxCon Vancouver.

What:   USB mini-summit
When:   Tuesday, August 16th, all day
Where:  At the conference venue, room TBD pending confirmation from
Angela Brown.

Proposed topics include USB virtualization, and improved bandwidth APIs
between the USB core and drivers (especially webcam drivers).  See the
detailed topic list below.  Anyone is also welcome to propose or show up
with a USB related topic.  MUSB?  USB 3.0 gadget drivers?  USB-IP?

The USB mini-summit does overlap with the virtualization mini-summit by
a day, but I'm hoping we can schedule talks so some of the
virtualization folks can make it to the USB mini-summit.  The other
option was on Friday during the conference which was not ideal.

Proposed topics:

Topic 1
---

The KVM folks suggested that it would be good to get USB and
virtualization developers together to talk about how to virtualize the
xHCI host controller.  The xHCI spec architect worked closely with
VMWare to get some extra goodies in the spec to help virtualization, and
I'd like to see the other virtualization developers take advantage of
that.  I'd also like us to hash out any issues they have been finding in
the USB core or xHCI driver during the virtualization effort.


Topic 2
---

I'd also like to get the V4L and audio developers who work with USB
devices together with the core USB folks to talk about bandwidth
management under xHCI.

One of the issues is that since the xHCI hardware does bandwidth
management, not the xHCI driver, a schedule that will take too much
bandwidth will get rejected much sooner than any USB driver currently
expects (during a call to usb_set_interface).  This poses issues, since
most USB video drivers negotiate the video size and frame rate after
they call usb_set_interface, so they don't know whether they can fall
back to a less bandwidth-intensive setting.  Currently, they just submit
URBs with less and less bandwidth until one interval setting gets
accepted that won't work under xHCI.

A second issue is that that some drivers need less bandwidth than the
device advertises, and the xHCI driver currently uses whatever periodic
interval the device advertises in its descriptors.  This is not what the
video/audio driver wants, especially in the case of buggy high speed
devices that advertise the interval in frames, not microframes.  There
needs to be some way for the drivers to communicate their bandwidth
needs to the USB core.  We've known about this issue for a while, and I
think it's time to get everyone in the same room and hash out an API.

(I will send out an API proposal later this month.)

Hope to see you there!

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Status of the patches under review (85 patches) and some misc notes about the devel procedures

2010-05-10 Thread Sarah Sharp
On Sat, May 08, 2010 at 03:45:41PM -0700, Mauro Carvalho Chehab wrote:
 Jean-Francois Moine wrote:
  On Fri, 7 May 2010 09:39:16 -0300
  Mauro Carvalho Chehab mche...@redhat.com wrote:
  
 == Gspca patches - Waiting Jean-Francois Moine
  moin...@free.fr submission/review == 
 
  Feb,24 2010: gspca pac7302: add USB PID range based on
  heuristics   http://patchwork.kernel.org/patch/81612
  May, 3 2010: gspca: Try a less bandwidth-intensive alt
  setting. http://patchwork.kernel.org/patch/96514
  
  Hello Mauro,
  
  I don't think the patch about pac7302 should be applied.
 
  
  The patch about the gspca main is in my last git pull request.
 
 (c/c Sarah)
 
 I also didn't like this patch strategy. It seems a sort of workaround
 for xHCI, instead of a proper fix.
 
 I'll mark this patch as rejected, and wait for a proper fix.

This isn't a work around for a bug in the xHCI host.  The bandwidth
checking is a feature.  It allows the host to reject a new interface if
other devices are already taking up too much of the bus bandwidth.  I
expect that all drivers that use interrupt or isochronous will have to
fall back to a different alternate interface setting if they can.

Now, Alan Stern and I have been talking about making a different API for
drivers to request a specific polling rate and frame list length for an
endpoint.  However, I expect that the call would have to be either
before or part of the call to usb_set_interface(), because of how the
xHCI hardware tracks endpoints.  So even if we had the ideal interface,
the drivers would still need code like this to fall back to
less-bandwidth intensive alternate settings.

Is there a different way you think we should handle running out of
bandwidth?

Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] gspca: Try a less bandwidth-intensive alt setting.

2010-05-03 Thread Sarah Sharp
Under OHCI, UHCI, and EHCI, if an alternate interface setting took too
much of the bus bandwidth, then submit_urb() would fail.  The xHCI host
controller does bandwidth checking when the alternate interface setting is
installed, so usb_set_interface() can fail.  If it does, try the next
alternate interface setting.

Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com
Tested-by:  Andiry Xu andiry...@amd.com
---
 drivers/media/video/gspca/gspca.c |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c 
b/drivers/media/video/gspca/gspca.c
index 222af47..6de3117 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -643,6 +643,7 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev 
*gspca_dev)
xfer = gspca_dev-cam.bulk ? USB_ENDPOINT_XFER_BULK
   : USB_ENDPOINT_XFER_ISOC;
i = gspca_dev-alt; /* previous alt setting */
+find_alt:
if (gspca_dev-cam.reverse_alts) {
while (++i  gspca_dev-nbalt) {
ep = alt_xfer(intf-altsetting[i], xfer);
@@ -666,10 +667,11 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev 
*gspca_dev)
if (gspca_dev-nbalt  1) {
gspca_input_destroy_urb(gspca_dev);
ret = usb_set_interface(gspca_dev-dev, gspca_dev-iface, i);
-   if (ret  0) {
-   err(set alt %d err %d, i, ret);
-   ep = NULL;
-   }
+   /* xHCI hosts will reject set interface requests
+* if they take too much bandwidth, so try again.
+*/
+   if (ret  0)
+   goto find_alt;
gspca_input_create_urb(gspca_dev);
}
return ep;
-- 
1.6.3.3

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


xHCI bandwidth error with USB webcam

2010-04-12 Thread Sarah Sharp
I've been trying out the patches to enable isochronous transfers under
xHCI, and they work fine on my USB speaker.  However, I've been having
trouble getting my high speed USB webcam to work.  The NEC Express Card
I have rejects the first alternate setting that the uvcvideo driver
tries to install (altsetting 11), saying that it takes too much
bandwidth.  This happens even when I plug the device directly into the
roothub with no other devices plugged in.

I would like to know if this is correct behavior for the host, as I
can't believe a device would advertise an alternate setting that took up
too much bandwidth by itself.  Device descriptors and a log snippet are
attached.


The other problem is that uvcvideo then gives up on the device when
installing the alt setting fails, rather than trying the next less
resource-intensive alternate setting.  The past, submit_urb() might fail
if there wasn't enough bandwidth for the isochronous transfers, but
under an xHCI host controller, it will fail sooner, when
usb_set_interface() is called.  That needs to be fixed in all the USB
video drivers.

I figured out how to patch the gspca driver, but not uvcvideo.  The
patch looks a bit hackish; can with experience with that driver look it
over?  Can anyone tell me where to look for the usb_set_interface() in
uvcvideo?

Sarah Sharp

8--

From 0e6bc81b178364ee9771c64a06ab006588c73ae6 Mon Sep 17 00:00:00 2001
From: Sarah Sharp sarah.a.sh...@linux.intel.com
Date: Mon, 12 Apr 2010 11:23:46 -0700

Subject: [PATCH] gspca: Try a less bandwidth-intensive alt setting.

Under OHCI, UHCI, and EHCI, if an alternate interface setting took too
much of the bus bandwidth, then submit_urb() would fail.  The xHCI host
controller does bandwidth checking when the alternate interface setting is
installed, so usb_set_interface() can fail.  If it does, try the next
alternate interface setting.

Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com
---
 drivers/media/video/gspca/gspca.c |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c 
b/drivers/media/video/gspca/gspca.c
index 222af47..6de3117 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -643,6 +643,7 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev 
*gspca_dev)
xfer = gspca_dev-cam.bulk ? USB_ENDPOINT_XFER_BULK
   : USB_ENDPOINT_XFER_ISOC;
i = gspca_dev-alt; /* previous alt setting */
+find_alt:
if (gspca_dev-cam.reverse_alts) {
while (++i  gspca_dev-nbalt) {
ep = alt_xfer(intf-altsetting[i], xfer);
@@ -666,10 +667,11 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev 
*gspca_dev)
if (gspca_dev-nbalt  1) {
gspca_input_destroy_urb(gspca_dev);
ret = usb_set_interface(gspca_dev-dev, gspca_dev-iface, i);
-   if (ret  0) {
-   err(set alt %d err %d, i, ret);
-   ep = NULL;
-   }
+   /* xHCI hosts will reject set interface requests
+* if they take too much bandwidth, so try again.
+*/
+   if (ret  0)
+   goto find_alt;
gspca_input_create_urb(gspca_dev);
}
return ep;
-- 
1.6.3.3


Bus 009 Device 002: ID 046d:09a5 Logitech, Inc. 
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass  239 Miscellaneous Device
  bDeviceSubClass 2 ?
  bDeviceProtocol 1 Interface Association
  bMaxPacketSize064
  idVendor   0x046d Logitech, Inc.
  idProduct  0x09a5 
  bcdDevice0.06
  iManufacturer   0 
  iProduct0 
  iSerial 2 0A539160
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 1183
bNumInterfaces  4
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0x80
  (Bus Powered)
MaxPower  500mA
Interface Association:
  bLength 8
  bDescriptorType11
  bFirstInterface 0
  bInterfaceCount 2
  bFunctionClass 14 Video
  bFunctionSubClass   3 Video Interface Collection
  bFunctionProtocol   0 
  iFunction   0 
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass14 Video
  bInterfaceSubClass  1 Video Control
  bInterfaceProtocol  0 
  iInterface  0 
  VideoControl Interface Descriptor:
bLength13
bDescriptorType