Re: New USB core API to change interval and max packet size
Hi, On 10/02/2013 08:39 PM, Sarah Sharp wrote: On Wed, Oct 02, 2013 at 10:22:52AM -0400, Alan Stern wrote: 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 t
Re: New USB core API to change interval and max packet size
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 -- 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
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 -- 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
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
Re: New USB core API to change interval and max packet size
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
Em Wed, 02 Oct 2013 12:38:14 -0400 (EDT) Alan Stern 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 -- 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
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 -- 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
Em Wed, 02 Oct 2013 11:00:13 -0400 (EDT) Alan Stern 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 > > bmAttributes3 > > Transfer TypeInterrupt > > 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 -- 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
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 > bmAttributes3 > Transfer TypeInterrupt > 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 -- 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
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 -- 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
Hi Sarah,
Em Tue, 1 Oct 2013 13:45:54 -0700
Sarah Sharp 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:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
idVendor 0x2040 Hauppauge
idProduct 0x6600
bcdDevice0.69
iManufacturer 16
iProduct 32 HVR900H
iSerial64 4031932745
...
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes3
Transfer TypeInterrupt
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:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass9 Hub
bDeviceSubClass 0 Unused
bDeviceProtocol 1 Single TT
bMaxPacketSize064
idVendor 0x1d6b Linux Foundation
idProduct 0x0002 2.0 root hub
bcdDevice3.11
iManufacturer 3 Linux 3.11.2-201.fc19.x86_64 xhci_hcd
iProduct2 xHCI Host Controller
iSerial 1 :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
--
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
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
--
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
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
--
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
