New USB core API to change interval and max packet size

New USB core API to change interval and max packet size

[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-usb&m=138064948726038&w=2
http://marc.info/?l=linux-usb&m=138065201426880&w=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

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

[Xenia Ragiadakou]

On 10/01/2013 11:45 PM, Sarah Sharp wrote:
> 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-usb&m=138064948726038&w=2
> http://marc.info/?l=linux-usb&m=138065201426880&w=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

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?

regards,
ksenia

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

[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

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

[Mauro Carvalho Chehab]

Hi Sarah,

Em Tue, 1 Oct 2013 13:45:54 -0700
Sarah Sharp <sarah.a.sharp@linux.intel.com> escreveu:

> 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-usb&m=138064948726038&w=2
> http://marc.info/?l=linux-usb&m=138065201426880&w=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.

Let me see if I understand the changes at the media drivers. So, please
correct me if I got it wrong.

I'm yet to get any USB 3.0 media device, although it is common to connect
an USB 1.1 or USB 2.0 device on a USB 3.0 host port.

So, for example, on this device:

	Bus 003 Device 002: ID 2040:6600 Hauppauge 
	Device Descriptor:
	  bLength                18
	  bDescriptorType         1
	  bcdUSB               2.00
	  bDeviceClass            0 (Defined at Interface level)
	  bDeviceSubClass         0 
	  bDeviceProtocol         0 
	  bMaxPacketSize0        64
	  idVendor           0x2040 Hauppauge
	  idProduct          0x6600 
	  bcdDevice            0.69
	  iManufacturer          16 
	  iProduct               32 HVR900H
	  iSerial                64 4031932745
...
	      Endpoint Descriptor:
	        bLength                 7
	        bDescriptorType         5
	        bEndpointAddress     0x83  EP 3 IN
	        bmAttributes            3
	          Transfer Type            Interrupt
	          Synch Type               None
	          Usage Type               Data
	        wMaxPacketSize     0x0004  1x 4 bytes
	        bInterval               1
...

connected via this BUS device:

	Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
	Device Descriptor:
	  bLength                18
	  bDescriptorType         1
	  bcdUSB               2.00
	  bDeviceClass            9 Hub
	  bDeviceSubClass         0 Unused
	  bDeviceProtocol         1 Single TT
	  bMaxPacketSize0        64
	  idVendor           0x1d6b Linux Foundation
	  idProduct          0x0002 2.0 root hub
	  bcdDevice            3.11
	  iManufacturer           3 Linux 3.11.2-201.fc19.x86_64 xhci_hcd
	  iProduct                2 xHCI Host Controller
	  iSerial                 1 0000:00:14.0

In such situation, and assuming that the USB tables are correct, there's
nothing that needs to be done there, as bInterval/wMaxPacketSize are
correct for USB 2.0.

So, there's no need to call usb_change_ep_bandwidth().

If so, then usb_change_ep_bandwidth() as a quirk, if bInterval
or wMaxPacketSize were improperly filled.

Right?
-- 

Cheers,
Mauro

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

[Alan Stern]

On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote:

> Let me see if I understand the changes at the media drivers. So, please
> correct me if I got it wrong.
> 
> I'm yet to get any USB 3.0 media device, although it is common to connect
> an USB 1.1 or USB 2.0 device on a USB 3.0 host port.
> 
> So, for example, on this device:

> ...
> 	      Endpoint Descriptor:
> 	        bLength                 7
> 	        bDescriptorType         5
> 	        bEndpointAddress     0x83  EP 3 IN
> 	        bmAttributes            3
> 	          Transfer Type            Interrupt
> 	          Synch Type               None
> 	          Usage Type               Data
> 	        wMaxPacketSize     0x0004  1x 4 bytes
> 	        bInterval               1
> ...
> 
> connected via this BUS device:

...

> In such situation, and assuming that the USB tables are correct, there's
> nothing that needs to be done there, as bInterval/wMaxPacketSize are
> correct for USB 2.0.
> 
> So, there's no need to call usb_change_ep_bandwidth().

That's right.

> If so, then usb_change_ep_bandwidth() as a quirk, if bInterval
> or wMaxPacketSize were improperly filled.
> 
> Right?

Or if the values are correct, but the driver wants to use something 
different for its own reasons (for example, to get lower latency or 
because it knows that it will never use packets as large as the 
descriptor allows).  Right.

Alan Stern

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

[Mauro Carvalho Chehab]

Em Wed, 02 Oct 2013 11:00:13 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> escreveu:

> On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote:
> 
> > Let me see if I understand the changes at the media drivers. So, please
> > correct me if I got it wrong.
> > 
> > I'm yet to get any USB 3.0 media device, although it is common to connect
> > an USB 1.1 or USB 2.0 device on a USB 3.0 host port.
> > 
> > So, for example, on this device:
> 
> > ...
> > 	      Endpoint Descriptor:
> > 	        bLength                 7
> > 	        bDescriptorType         5
> > 	        bEndpointAddress     0x83  EP 3 IN
> > 	        bmAttributes            3
> > 	          Transfer Type            Interrupt
> > 	          Synch Type               None
> > 	          Usage Type               Data
> > 	        wMaxPacketSize     0x0004  1x 4 bytes
> > 	        bInterval               1
> > ...
> > 
> > connected via this BUS device:
> 
> ...
> 
> > In such situation, and assuming that the USB tables are correct, there's
> > nothing that needs to be done there, as bInterval/wMaxPacketSize are
> > correct for USB 2.0.
> > 
> > So, there's no need to call usb_change_ep_bandwidth().
> 
> That's right.
> 
> > If so, then usb_change_ep_bandwidth() as a quirk, if bInterval
> > or wMaxPacketSize were improperly filled.
> > 
> > Right?
> 
> Or if the values are correct, but the driver wants to use something 
> different for its own reasons (for example, to get lower latency or 
> because it knows that it will never use packets as large as the 
> descriptor allows).  Right.

Ok, so, in this case, usb_change_ep_bandwidth() could be called
just before usb_alloc_urb(), in order to make it to use the packet
size that would be expected for that kind of ISOC traffic that
userspace indirectly selected, by adjusting the streaming
video resolution selected, right?

Regards,
Mauro

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

[Alan Stern]

On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote:

> > > So, there's no need to call usb_change_ep_bandwidth().
> > 
> > That's right.
> > 
> > > If so, then usb_change_ep_bandwidth() as a quirk, if bInterval
> > > or wMaxPacketSize were improperly filled.
> > > 
> > > Right?
> > 
> > Or if the values are correct, but the driver wants to use something 
> > different for its own reasons (for example, to get lower latency or 
> > because it knows that it will never use packets as large as the 
> > descriptor allows).  Right.
> 
> Ok, so, in this case, usb_change_ep_bandwidth() could be called
> just before usb_alloc_urb(), in order to make it to use the packet
> size that would be expected for that kind of ISOC traffic that
> userspace indirectly selected, by adjusting the streaming
> video resolution selected, right?

We haven't decided on the final API yet.  However, note that
usb_alloc_urb() doesn't depend on the packet size.  It requires you to
specify only the number of packets, not their sizes.  Therefore it
doesn't matter whether you call usb_change_ep_bandwidth() before or
after usb_alloc_urb().

Alan Stern

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

[Mauro Carvalho Chehab]

Em Wed, 02 Oct 2013 12:38:14 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> escreveu:

> On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote:
> 
> > > > So, there's no need to call usb_change_ep_bandwidth().
> > > 
> > > That's right.
> > > 
> > > > If so, then usb_change_ep_bandwidth() as a quirk, if bInterval
> > > > or wMaxPacketSize were improperly filled.
> > > > 
> > > > Right?
> > > 
> > > Or if the values are correct, but the driver wants to use something 
> > > different for its own reasons (for example, to get lower latency or 
> > > because it knows that it will never use packets as large as the 
> > > descriptor allows).  Right.
> > 
> > Ok, so, in this case, usb_change_ep_bandwidth() could be called
> > just before usb_alloc_urb(), in order to make it to use the packet
> > size that would be expected for that kind of ISOC traffic that
> > userspace indirectly selected, by adjusting the streaming
> > video resolution selected, right?
> 
> We haven't decided on the final API yet.  However, note that
> usb_alloc_urb() doesn't depend on the packet size.  It requires you to
> specify only the number of packets, not their sizes.  Therefore it
> doesn't matter whether you call usb_change_ep_bandwidth() before or
> after usb_alloc_urb().

Sure, but, at least on almost all V4L2 drivers, the number of packets
and their sizes should be calculated to be able to receive all URBs
needed to store a complete image frame (that generally arrives on 
every 1/60 Hz or 1/50 Hz - depending on the frames per second rate).

On those drivers, the transfer_buffer is allocated at the same loop 
where usb_alloc_urb() is called.

So, it makes sense for them to specify the bandwidth parameters at
the function that calls usb_alloc_coherent() and usb_alloc_urb().

Regards,
Mauro

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

[Alan Stern]

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.

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().

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.

Alan Stern

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

[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

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

[Alan Stern]

On Wed, 2 Oct 2013, Sarah Sharp wrote:

> > 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?

That's what I had in mind.  Note that usb_set_interface() already
updates the 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?

Yes.  I can see two disadvantages:

	You don't want to call usb_set_interface() if there are any 
	transfers in progress for endpoints in that interface -- even 
	endpoints whose bandwidth you aren't trying to change.  The
	transfers would get shut down.

	usb_set_interface() communicates with the device, which isn't
	necessary if you're merely updating the host's schedule.

The alternatives are either a separate function to do the schedule
update, or else pass usb_change_ep_bandwidth() an array or list of
endpoint info and have it do all the updates at once.  I think a
separate function would be easier for drivers to use.

> > 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.

No, just the opposite.  That was the point I was trying to make.  Any
changes to ep5 in altsetting 0 (for example) will have no effect on ep1
in altsetting 1, because the two altsettings reference the endpoint by
two separate usb_host_endpoint structures.

Furthermore, it generally doesn't make sense to apply a single change 
across multiple altsettings.  An isochronous endpoint, for example, 
might have maxpacket = 500 in one altsetting and 900 in another.  When 
you reduce the 500 to 400, that doesn't mean you also want to reduce 
the 900 to 400.

>  But we still want to reset the values after the
> driver unbinds, correct?

Absolutely.  When the next driver binds, it should get the default
values (i.e., the ones stored in the descriptors).  This would be
analogous to the way we currently set each interface back to altsetting
0 when a driver unbinds.

Alan Stern

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

[Alan Stern]

On Wed, 2 Oct 2013, Alan Stern wrote:

> > Ok, so it sounds like we want to keep the changes the endpoints across
> > alt setting changes.
> 
> No, just the opposite.  That was the point I was trying to make.  Any
> changes to ep5 in altsetting 0 (for example) will have no effect on ep1
----------------------------------------------------------------------^^^

Typo: this should have been ep5 as well.

> in altsetting 1, because the two altsettings reference the endpoint by
> two separate usb_host_endpoint structures.

Alan Stern

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

[Hans de Goede]

Hi,

On 10/02/2013 08:39 PM, Sarah Sharp wrote:
> On Wed, Oct 02, 2013 at 10:22:52AM -0400, Alan Stern wrote:

<snip>

>> 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?

Allow me to jump in here. As you may remember I pointed out the media
side of this in the ubs-mini summit during the 2nd plumbers conference
in Portland.

I don't know about any media drivers which want to change interval, but
it make sense that there are those who will want to do that too, esp in
the hid area.

I can give 2 examples who want to change the maxpacketsize in media
land. Both for quite old usb-1 webcam chipsets. The manufacturers of
these chip-sets decided that having X alt-settings was too limiting, so
they have only 2. Alt 0 with no isoc endpoints, and alt 1 with 1 isoc
endpoint with a maxpacketsize of 1023. This means that if these device
somehow end up sharing usb-1 bandwidth with anything else (through
say 1 single tt usb-2 hub) they eat up all the periodic bandwidth, or
try to and fail.

Technically these devices actually have an almost unlimited number of
alt-settings, as they have a register through which the maxpacketsize
they will actually use can be configured.

Since we don't always need the full 1000 (interval 1) * 1023 bytes /
second bandwidth their alt-setting 1 gives us, currently their drivers
employ the following hack:

alt = &gspca_dev->dev->actconfig->intf_cache[0]->altsetting[1];
alt->endpoint[0].desc.wMaxPacketSize = cpu_to_le16(packet_size);

Before calling usb_set_interface(), so that they can start streaming
successfully even though the full usb-1 bandwidth which their alt1
descriptor claims it needs is not available, at least on ohci and ehci
this hack has that result, not sure if it works for xhci too (I can test
if people are interested).

For people who want to actually see the code for this admittedly ugly
hack, see:

drivers/media/usb/gspca/xirlink_cit.c

and:

drivers/media/usb/gspca/stv06xx/stv06xx.c

Note that their are several other chipsets which have a maxpacketsize
register too. But their descriptors do properly contain multiple
alt-settings with different maxpacketsizes (for the benefit of the OS
I assume), so all we do their is simply write the current wMaxPacketSize
to the register see ie: drivers/media/usb/gspca/ov519.c at line 3500 ,
technically we could use the discussed functionality there too, to do
finer grained bandwidth management but I don't really see a need for 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?

What the 2 media drivers in question currently do is:

1) Determine an optimal wMaxPacketSize for the resolution + framerate
userspace has selected (so one where we can actually reach the framerate
with no framedrop or image degradation due to over aggressive compression).
Also determine a minimum wMaxPacketSize, below which streaming simply
won't work reliabe, even with frame drop, etc.
2) Put the wMaxPacketSize in the intf_cache for the alt1 alt-setting
3) Try a usb_set_interface(dev, iface, 1)
4) If 3). fails, reduce wMaxPacketSize in the intf_cache for the alt1 alt-setting
by 100 bytes, until we hit the minimum, then goto 3, or bail if the last
try was with the minimum already.

They are basically doing the same as other webcam drivers, except that
other drivers determine an optimal and minimum alt-setting with which
to work and then try set_interface with alt-settings with decreasing
wMaxPacketSize, where as these drivers keep retrying with the same
alt-setting, while changing the wMaxPacketSize of that alt-setting.

So from a media (specifically webcam driver) pov, the suggested approach
of first calling usb_change_ep_bandwidth 1 or more times, and then calling
usb_set_interface makes a lot of sense.

Note that in the hid case however where we are likely talking about
cases where the descriptors say something stupid like "poll this mouse
8000 times / second", or "poll this joystick 1 / second", normally the
driver will not call usb_set_interface() at all, because these
devices normally have only 1 alt-setting (only alt0).

Still since this is a special case for quirk handling, I think it is
reasonable that drivers have to call usb_set_interface() after calling
usb_change_ep_bandwidth(), for the changes to go into effect.

>> 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.

At a minimum we want to keep them until the first usb_set_interface call,
which comes automatically with the transaction like interface Alan suggested
(so the usb_change_ep_bandwidth() calls just store a value some where and are
otherwise nops).

This makes me realize that the original suggestion to just have a
usb_change_ep_bandwidth() call which would change just that one ep and
immediately apply the changes, is not usable for the webcam case, since
in some cases the usb_set_interface call can only succeed with the
modified wMaxPacketSize and/or interval, so we would never be able to
move cur_altsetting to alt-setting 1, and thus would never be able to
change the wMaxPacketSize in there.

TBH I don't see much reason to keep the changes beyond the first set_interface
call, nor do I see much reason not too (except on driver unbind). Implementation
wise it might be easiest to just keep the override values until the first
call. This could be as simple as an array with 30 wMaxPacketSize override
values, and 30 interval overrides to be applied at the first set_interface
call.

 > But we still want to reset the values after the driver unbinds, correct?

Yes we definitely want to reset the values after an unbind.

Regards,

Hans