[PATCH] iso pipe support for usbatm

[matthieu castet]

Hi,

Roman Kagan wrote:
> On Sat, Mar 19, 2005 at 03:37:40PM +0100, matthieu castet wrote:
> 
>>Roman Kagan wrote:
>>
>>>On Sat, Mar 19, 2005 at 12:38:19AM +0100, matthieu castet wrote:
>>>
>>>
>>>>@@ -472,17 +510,44 @@
>>>>	instance = rcv->instance;
>>>>	buf = rcv->buffer;
>>>>
>>>>-	buf->filled_cells = urb->actual_length / (ATM_CELL_SIZE + 
>>>>instance->rx_padding);
>>>>+	if ( urb->actual_length <= 0 && !urb->status) {
>>>>+		int err;
>>>>+		if ((err = usb_submit_urb(rcv->urb, GFP_ATOMIC)) < 0) {
>>>
>>>
>>>Why do you resubmit the failed urb here?  It used to go in the normal
>>>codepath and get resubmitted in receive_tasklet.
>>>
>>
>>the buffer is empty and the urb didn't failed, so we could reuse the 
>>same urb with the same buffer : it avoids to go through receive_tasklet 
>>and locking for releasing/taking buffer.
>>This is needed for performance issue with iso pipe because the urb are 
>>returned every 1ms (or something like that), even if there is nothing 
>>received in order to keep the allocated bandwidth.
> 
> 
> So for bulk this should never happen?
> 
> Anyway it may make more sense to use more urbs in this case, so that
> some urbs are always submitted.  It's for that reason that I've
> increased the default number of urbs in my patch.
> 
Increasing the number of urb won't solve the extra cost of locking 
unlocking : in order to keep the allocated bandwidth, you need to 
resumit it as soon as possible. If you look in the driver/usb driver 
that use iso pipe, you will see that's lot's of them resumit the urb in 
the complete handler (I know this isn't a good argument).
Also the number is limited in iso mode.

> 
>>>>-	if (likely(!urb->status))
>>>>+	if (likely(!urb->status) ||
>>>>+			/* via chipset produce lot's of crc error...*/
>>>>+			(usb_pipeisoc(instance->rx_endpoint) && urb->status 
>>>>== -EILSEQ))
>>>>		tasklet_schedule(&instance->receive_tasklet);
>>>
>>>
>>>Sounds like iso occasionally misses data too?  BTW the condition you add
>>>here is ignored because it's masked by the other one.
>>
>>I don't think so : if urb->status == -EILSEQ and iso pipe, we have "if 
>>(0 || 1 && 1)" which is true
> 
> 
> Ups, you're certainly right.
> 
> BTW do you get -EILSEQ often?  Documentation/usb/error-codes.txt reads:
> 
> (*) Error codes like -EPROTO, -EILSEQ and -EOVERFLOW normally indicate
> hardware problems such as bad devices (including firmware) or cables.
> 
I think this is specific to via controler (someone told me that there 
windows driver have some trick for -EILSEQ error)

> 
>>>What is the typical ->iso_frame_size you're using? 
>>
>>This is given by the modem, I use the maximum : 1007, but others are 
>>also available :
> 
> 
> So this means the buffer size per urb is 1007 * 16 / 53 = 304 ATM cells,
> which is almost 5 times the default 64 for bulk.  Maybe simply
> increasing the buffer size in bulk mode will solve the problem of
> dropped cells?
It make it worse : I increase the size to 304, and I have lot's of 
unknown vpi/vci error and my download rate is about 4Ko/s ...

I thinks that's normal because before we detected errors only after 64 
cells, now we wait 304 cells....

I was using your new algorithm, with 4 urb.


Matthieu